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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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?

>
>>> 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.

-boris


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