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

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

-boris


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