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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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

> 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)?

>
>> +/* Location of online VCPU bitmap. */
>> +#define ACPI_CPU_MAP                 0xaf00
>> +#define ACPI_CPU_MAP_LEN             ((HVM_MAX_VCPUS / 8) + \
>> +                                      ((HVM_MAX_VCPUS & 7) ? 1 : 0))
> ((HVM_MAX_VCPUS + 7) / 8)
>
> But as indicated elsewhere, this should be made less static anyway.

Right, we could size it by domain.max_vcpus.

-boris


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