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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 11/15/2016 11:33 AM, Jan Beulich wrote:
>>>> 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__?

Oh, so you meant that interface version would be an OR, in addition to
__XEN__ and __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.


I think getting that framework in order would be out of the scope of
what this series is trying to achieve.

-boris


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