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

[Lan Tianyu <tianyu.lan@xxxxxxxxx>]

On 2017年08月25日 18:11, Roger Pau Monné wrote:
> 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@xxxxxxxxx>
>> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
>> ---
>>  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.

Sorry. We made APIC ID was vcpu id in our internal repo and didn't send
out. Will change this in next version.

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

OK. Will update.

> 
> Roger.
> 


-- 
Best regards
Tianyu Lan

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