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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.11.16 at 15:39, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -244,7 +245,8 @@ int main(int argc, char **argv)
>  #endif
>  
>      /* Operation Region 'PRST': bitmask of online CPUs. */
> -    stmt("OperationRegion", "PRST, SystemIO, 0xaf00, 32");
> +    stmt("OperationRegion", "PRST, SystemIO, 0x%x, %d",

%#x

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

> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -87,6 +87,12 @@ struct hvm_domain {
>      } ioreq_server;
>      struct hvm_ioreq_server *default_ioreq_server;
>  
> +    /* PVH guests */
> +    struct {
> +        uint8_t pm1a[ACPI_PM1A_EVT_BLK_LEN];
> +        uint8_t gpe[ACPI_GPE0_BLK_LEN_V1];
> +    } acpi_io;

This is left unused in this patch - please add it once it gets used.

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

> +/* 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.

Also, no matter that there are (many, or should I say only) bad
examples in this file, please don't add new possible name space
conflicts: Any new constants should start with XEN_.

Jan


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