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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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