Re: [Xen-devel] [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.11.16 at 17:23, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 11/15/2016 10:53 AM, Jan Beulich wrote:
>>>>> On 15.11.16 at 16:41, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 11/15/2016 10:13 AM, Jan Beulich wrote:
>>>>>>> On 15.11.16 at 15:47, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>> On 11/15/2016 03:47 AM, Jan Beulich wrote:
>>>>>>> --- a/tools/libacpi/static_tables.c
>>>>>>> +++ b/tools/libacpi/static_tables.c
>>>>>>> @@ -20,6 +20,8 @@
>>>>>>>   * Firmware ACPI Control Structure (FACS).
>>>>>>>   */
>>>>>>>  
>>>>>>> +#define ACPI_REG_BIT_OFFSET    0
>>>>>> Can you completely exclude us ever wanting to support something
>>>>>> that's not on a byte boundary? I think there was a good reason ...
>>>>>>
>>>>>>> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
>>>>>>>  /*
>>>>>>>   * Fixed ACPI Description Table (FADT).
>>>>>>>   */
>>>>>>> -
>>>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH         0x20
>>>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET        0x00
>>>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH         0x10
>>>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET        0x00
>>>>>>> -#define ACPI_PM_TMR_BLK_BIT_WIDTH           0x20
>>>>>>> -#define ACPI_PM_TMR_BLK_BIT_OFFSET          0x00
>>>>>> ... these specified both width and offset.
>>>>> Since OFFSET is not used anywhere I kept it local to static_tables.c. I
>>>>> can restore these macros per block and move them to public header but...
>>>>>
>>>>>>> --- a/xen/include/public/hvm/ioreq.h
>>>>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>  #ifndef _IOREQ_H_
>>>>>>>  #define _IOREQ_H_
>>>>>>>  
>>>>>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>>>>>> +
>>>>>>>  #define IOREQ_READ      1
>>>>>>>  #define IOREQ_WRITE     0
>>>>>>>  
>>>>>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>>>>>>  #define ACPI_GPE0_BLK_ADDRESS        ACPI_GPE0_BLK_ADDRESS_V0
>>>>>>>  #define ACPI_GPE0_BLK_LEN            ACPI_GPE0_BLK_LEN_V0
>>>>>>>  
>>>>>>> +#define ACPI_PM1A_EVT_BLK_LEN        0x04
>>>>>>> +#define ACPI_PM1A_CNT_BLK_LEN        0x02
>>>>>>> +#define ACPI_PM_TMR_BLK_LEN          0x04
>>>>>> Just like ACPI_GPE0_BLK_LEN these should really go next to their
>>>>>> address definitions. 
>>>>> ... together with this, it will make it rather messy/unsightly to go
>>>>> with Andrew's request to ifdef this with __XEN__/__XEN_TOOLS__.
>>>> Well, framing them that way is a good excuse for having them
>>>> separate from the others. In fact, however, the others also
>>>> should get hidden in the same way, just that we would need to
>>>> be more careful there (read: make the condition also check
>>>> __XEN_INTERFACE_VERSION__).
>>> Sorry, I don't follow this. How can interface version help here?
>> We can't outright remove existing definitions from the public interface,
>> but we can limit their exposure to old consumers.
> 
> But don't we need to support both V0 and V1 as long as qemu-trad is
> supported? In other words, checking interface version won't limit the
> scope at this point.

Doesn't qemu-trad set __XEN_TOOLS__?

>>>>>> Provided we really want to hard code further
>>>>>> values here in the first place, which I don't think we should. The
>>>>>> goal should rather be for all these hard coded values to go away
>>>>>> (which really should have happened when the V1 variants had
>>>>>> been added).
>>>>> How can we not hardcode this if the values should match those in FADT
>>>>> (i.e. static_tables.c)?
>>>> By having the loading entity obtain the dynamic values and adjust
>>>> the table(s) accordingly.
>>> And this. Which loading entity (ACPI builder?) and how would it adjust
>>> the addresses? It still needs those addresses defined somewhere. And the
>>> the hypervisor, which can't parse guest FADT, needs to get those addresses.
>> Didn't Andrew make quite clear that there needs to be a central
>> authority assigning guest resources? That's where the values
>> would come from, and they would need to be suitably propagated
>> to however is in need of knowing them.
> 
> Oh, but that is still (way?) off at this point. From what I understood
> about Andrew's proposal this will require fairly significant update of
> how regions are registered.

Well, perhaps. Yet I question whether it's a good idea to add another
fixed address right now, instead of switching over first.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel