| Message ID | 1503629540-26053-5-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> Again, please provide spec. There are a few coding style issues in code btw.
>>> On 25.08.17 at 11:26, <wei.liu2@citrix.com> 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@intel.com> >> Signed-off-by: Chao Gao <chao.gao@intel.com> > > Again, please provide spec. I'd expect this to be the ACPI spec; I don't think links should be needed for such fundamental documentation. Jan
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.
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@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. 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. >
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 */ +}; + /* * 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; madt = ctxt->mem_ops.alloc(ctxt, sz, 16); if (!madt) return NULL; @@ -146,27 +150,48 @@ 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->nr_cpus = hvminfo->nr_vcpus; - 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++; + if (hvminfo->nr_vcpus < 256) { + struct acpi_20_madt_lapic *lapic = (struct acpi_20_madt_lapic *)end; + 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 = ((i < hvminfo->nr_vcpus) && + test_bit(i, hvminfo->vcpu_online) + ? ACPI_LOCAL_APIC_ENABLED : 0); + lapic++; + } + end = lapic; + } else { + struct acpi_20_madt_x2apic *lapic = (struct acpi_20_madt_x2apic *)end; + 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_X2APIC; + 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++; + } + end = lapic; } - madt->header.length = (unsigned char *)lapic - (unsigned char *)madt; + info->nr_cpus = hvminfo->nr_vcpus; + madt->header.length = (unsigned char *)end - (unsigned char *)madt; set_checksum(madt, offsetof(struct acpi_header, checksum), madt->header.length); info->madt_csum_addr =