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

[Lan Tianyu <tianyu.lan@intel.com>]

On 2017年09月01日 17:57, Roger Pau Monné wrote:
> 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@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  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.

There will be a problem how to get max apic id. Should we use the max
vcpu index to get max 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.

OK.

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

If apic_id < 255, still use xapic structure and use x2apic structure for
others. I think this is following your comment on last version.


> 
> Roger.
> 


-- 

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