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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/11/16 16:58, Boris Ostrovsky wrote:
> 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.

Yes - adding the central authority framework is out of scope, because it
is a lot of work on its own to do.

All I want to ensure is that no hardcoded numbers get published in a
stable place in the API.  i.e. once the central management work is done,
I want these defines to disappear completely from the header file. 
Stuff behind __TOOLS__ is safe to modify later.

~Andrew

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