| Message ID | 1504155709-24276-4-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + 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.
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. >
On Mon, Sep 04, 2017 at 06:59:24PM +0800, Lan Tianyu wrote: > On 2017年09月01日 17:57, Roger Pau Monné wrote: > > On Thu, Aug 31, 2017 at 01:01:48AM -0400, Lan Tianyu wrote: > >> @@ -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? IMHO, this should become a loop that iterates over each vCPU getting it's APIC ID, and add either a lapic or x2apic struct size to sz. > >> - 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. I'm complaining about the (i < hvminfo->nr_vcpus) check, from the loop above i is bounded to hvminfo->nr_vpcu, so this check is useless AFAICT. Roger.
>>> On 04.09.17 at 13:12, <roger.pau@citrix.com> wrote: > On Mon, Sep 04, 2017 at 06:59:24PM +0800, Lan Tianyu wrote: >> On 2017年09月01日 17:57, Roger Pau Monné wrote: >> > On Thu, Aug 31, 2017 at 01:01:48AM -0400, Lan Tianyu wrote: >> >> @@ -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? > > IMHO, this should become a loop that iterates over each vCPU getting > it's APIC ID, and add either a lapic or x2apic struct size to sz. Indeed. Jan
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) + 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 ) - { + { 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; - 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) && + test_bit(i, hvminfo->vcpu_online) + ? ACPI_LOCAL_APIC_ENABLED : 0); + end = ++lapic; + } else { + struct acpi_20_madt_x2apic *lapic = end; + + 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; + 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 =