Re: [Xen-devel] [RFC PATCH V2 3/4] hvmload: Add x2apic entry support in the MADT build

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Aug 31, 2017 at 01:01:48AM -0400, Lan Tianyu wrote:
> This patch is to add x2apic entry support for ACPI MADT table
> according to ACPI spec 5.2.12.12 Processor Local x2APIC Structure
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
>  tools/libacpi/acpi2_0.h | 10 +++++++++
>  tools/libacpi/build.c   | 59 
> +++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
> index 758a823..caa3682 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 */
> +};
> +
>  /*
>   * 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..0c95850 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,12 @@ 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 < 128)

This should be done based on APIC ID.

> +        sz += sizeof(struct acpi_20_madt_lapic) * hvminfo->nr_vcpus;
> +    else
> +        sz += sizeof(struct acpi_20_madt_lapic) * 128 +
> +              sizeof(struct acpi_20_madt_x2apic) * (hvminfo->nr_vcpus - 128);
>  
>      madt = ctxt->mem_ops.alloc(ctxt, sz, 16);
>      if (!madt) return NULL;
> @@ -109,7 +114,7 @@ static struct acpi_20_madt *construct_madt(struct 
> acpi_ctxt *ctxt,
>      madt->flags      = ACPI_PCAT_COMPAT;
>  
>      if ( config->table_flags & ACPI_HAS_IOAPIC )
> -    {     
> +    {

Spurious cleanup?

>          intsrcovr = (struct acpi_20_madt_intsrcovr *)(madt + 1);
>          for ( i = 0; i < 16; i++ )
>          {
> @@ -146,27 +151,47 @@ static struct acpi_20_madt *construct_madt(struct 
> acpi_ctxt *ctxt,
>          io_apic->ioapic_id   = config->ioapic_id;
>          io_apic->ioapic_addr = config->ioapic_base_address;
>  
> -        lapic = (struct acpi_20_madt_lapic *)(io_apic + 1);
> +        end = (struct acpi_20_madt_lapic *)(io_apic + 1);
>      }
>      else
> -        lapic = (struct acpi_20_madt_lapic *)(madt + 1);
> +        end = (struct acpi_20_madt_lapic *)(madt + 1);
> +
> +    info->madt_lapic0_addr = ctxt->mem_ops.v2p(ctxt, end);
>  
> -    info->nr_cpus = hvminfo->nr_vcpus;

Why are you moving this? AFAICT the value of nr_vpcus is not changed,
so you might as well leave it as-is.

> -    info->madt_lapic0_addr = ctxt->mem_ops.v2p(ctxt, lapic);
>      for ( i = 0; i < hvminfo->nr_vcpus; i++ )
>      {
> -        memset(lapic, 0, sizeof(*lapic));
> -        lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
> -        lapic->length  = sizeof(*lapic);
> -        /* Processor ID must match processor-object IDs in the DSDT. */
> -        lapic->acpi_processor_id = i;
> -        lapic->apic_id = config->lapic_id(i);
> -        lapic->flags = (test_bit(i, hvminfo->vcpu_online)
> -                        ? ACPI_LOCAL_APIC_ENABLED : 0);
> -        lapic++;
> +        unsigned int apic_id = config->lapic_id(i);
> +
> +        if ( apic_id < 255 ) {
> +            struct acpi_20_madt_lapic *lapic = end;
> +
> +            memset(lapic, 0, sizeof(*lapic));
> +            lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
> +            lapic->length  = sizeof(*lapic);
> +            /* Processor ID must match processor-object IDs in the DSDT. */
> +            lapic->acpi_processor_id = i;
> +            lapic->apic_id = config->lapic_id(i);
> +            lapic->flags = ((i < hvminfo->nr_vcpus) &&

I don't understand why you have added this.

Roger.

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