Re: [RFC PATCH 4/5] hvmload: Add x2apic entry support in the MADT build

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Aug 24, 2017 at 10:52:19PM -0400, Lan Tianyu wrote:
> This patch is to add x2apic entry support for ACPI MADT table.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
>  tools/libacpi/acpi2_0.h | 10 ++++++++
>  tools/libacpi/build.c   | 61 ++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 53 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
> index 758a823..ff18b3e 100644
> --- a/tools/libacpi/acpi2_0.h
> +++ b/tools/libacpi/acpi2_0.h
> @@ -322,6 +322,7 @@ struct acpi_20_waet {
>  #define ACPI_IO_SAPIC                       0x06
>  #define ACPI_PROCESSOR_LOCAL_SAPIC          0x07
>  #define ACPI_PLATFORM_INTERRUPT_SOURCES     0x08
> +#define ACPI_PROCESSOR_LOCAL_X2APIC         0x09
>  
>  /*
>   * APIC Structure Definitions.
> @@ -338,6 +339,15 @@ struct acpi_20_madt_lapic {
>      uint32_t flags;
>  };
>  
> +struct acpi_20_madt_x2apic {
> +    uint8_t  type;
> +    uint8_t  length;
> +    uint16_t reserved;		    /* reserved - must be zero */
> +    uint32_t apic_id;           /* Processor x2APIC ID  */
> +    uint32_t flags;
> +    uint32_t acpi_processor_id;	/* ACPI processor UID */

There's a mix of tabs and spaces above.

> +};
> +
>  /*
>   * Local APIC Flags.  All other bits are reserved and must be 0.
>   */
> diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
> index c7cc784..36e582a 100644
> --- a/tools/libacpi/build.c
> +++ b/tools/libacpi/build.c
> @@ -82,9 +82,9 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
>      struct acpi_20_madt           *madt;
>      struct acpi_20_madt_intsrcovr *intsrcovr;
>      struct acpi_20_madt_ioapic    *io_apic;
> -    struct acpi_20_madt_lapic     *lapic;
>      const struct hvm_info_table   *hvminfo = config->hvminfo;
>      int i, sz;
> +    void *end;
>  
>      if ( config->lapic_id == NULL )
>          return NULL;
> @@ -92,7 +92,11 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
>      sz  = sizeof(struct acpi_20_madt);
>      sz += sizeof(struct acpi_20_madt_intsrcovr) * 16;
>      sz += sizeof(struct acpi_20_madt_ioapic);
> -    sz += sizeof(struct acpi_20_madt_lapic) * hvminfo->nr_vcpus;
> +
> +    if (hvminfo->nr_vcpus < 256)
> +        sz += sizeof(struct acpi_20_madt_lapic) * hvminfo->nr_vcpus;
> +    else
> +        sz += sizeof(struct acpi_20_madt_x2apic) * hvminfo->nr_vcpus;

This is wrong, APIC ID is cpu id * 2, so the limit here needs to be
128, not 256. Also this should be set as a constant somewhere.

Apart from that, although this is technically correct, I would rather
prefer the first 128 vCPUs to have xAPIC entries, and APIC IDs > 254
to use x2APIC entries. This will allow a guest without x2APIC support
to still boot on VMs > 128 vCPUs, although they won't be able to use
the extra CPUs. IIRC this is in line with what bare metal does.

Roger.

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