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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.11.16 at 17:58, <boris.ostrovsky@xxxxxxxxxx> 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__?

Yes.

Jan


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