| Message ID | 1468343414-20254-1-git-send-email-prarit@redhat.com |
|---|---|
| State | Rejected |
| Headers | show |
On 2016-07-13 01:10 AM, Prarit Bhargava wrote: > If both I/O APIC and I/O SAPIC subtables exist, there are two checks that > should be run. The first is to confirm that there are at least as many > I/O SAPIC tables as there are I/O APIC tables. The second is to check that > every I/O APIC ID has a corresponding I/O SAPIC ID table. > > This patchset implements the tests. > > Additionally note a fixed typo for "MADIOSPAICAddrZero". > > Signed-off-by: Prarit Bhargava <prarit@redhat.com> > --- > src/acpi/madt/madt.c | 76 +++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 69 insertions(+), 7 deletions(-) > > diff --git a/src/acpi/madt/madt.c b/src/acpi/madt/madt.c > index 8ee231ee16be..d4fcc195b134 100644 > --- a/src/acpi/madt/madt.c > +++ b/src/acpi/madt/madt.c > @@ -121,6 +121,7 @@ > #define SUBTABLE_UNDEFINED 0x00 > #define SUBTABLE_VARIABLE 0xff > #define NUM_SUBTABLE_TYPES 16 > +#define MAX_IO_APIC_ID 256 /* IO APIC ID field is 1 byte */ > > struct acpi_madt_subtable_lengths { > unsigned short major_version; /* from revision in FADT header */ > @@ -434,6 +435,7 @@ static int madt_local_apic(fwts_framework *fw, > return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); > } > > +static unsigned char madt_io_apics[MAX_IO_APIC_ID]; > static int madt_io_apic(fwts_framework *fw, > fwts_acpi_madt_sub_table_header *hdr, > const uint8_t *data) > @@ -461,6 +463,14 @@ static int madt_io_apic(fwts_framework *fw, > fwts_passed(fw, "MADT %s address in non-zero.", > madt_sub_names[hdr->type]); > > + if (!madt_io_apics[ioapic->io_apic_id]) > + madt_io_apics[ioapic->io_apic_id] = 1; > + else > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "MADIOSAPICNotUnique", > + "There are multiple entries for id %d.", > + ioapic->io_apic_id); > + > return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); > } > > @@ -588,6 +598,7 @@ static int madt_lapic_addr_override(fwts_framework *fw, > return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); > } > > +static unsigned char madt_io_sapics[MAX_IO_APIC_ID]; > static int madt_io_sapic(fwts_framework *fw, > fwts_acpi_madt_sub_table_header *hdr, > const uint8_t *data) > @@ -608,7 +619,7 @@ static int madt_io_sapic(fwts_framework *fw, > > if (sapic->address == 0) > fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "MADIOSPAICAddrZero", > + "MADIOSAPICAddrZero", > "MADT %s address is zero, needs to be defined.", > madt_sub_names[hdr->type]); > else > @@ -616,12 +627,13 @@ static int madt_io_sapic(fwts_framework *fw, > "MADT %s address set to non-zero value.", > madt_sub_names[hdr->type]); > > - /* > - * TODO: if both I/O APIC and I/O SAPIC subtables exist, there > - * must be at least as many I/O SAPIC subtables as I/O APIC subtables, > - * and that every I/O APIC has a corresponding I/O SAPIC with the > - * same APIC ID. > - */ > + if (!madt_io_sapics[sapic->io_sapic_id]) > + madt_io_sapics[sapic->io_sapic_id] = 1; > + else > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "MADIOSAPICNotUnique", > + "There are multiple entries for id %d.", > + sapic->io_sapic_id); > > return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); > } > @@ -1178,6 +1190,49 @@ static int madt_gic_its(fwts_framework *fw, > return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); > } > > +static void madt_ioapic_sapic_compare(fwts_framework *fw, > + int num_ioapics, > + int num_iosapics) > +{ > + bool failed; > + int ioapic; > + > + if (!num_iosapics) > + return; /* Nothing to do */ > + > + /* > + * If both I/O APIC and I/O SAPIC subtables exist, there must be at > + * least as many I/O SAPIC subtables as I/O APIC subtables. > + */ > + if (num_iosapics < num_ioapics) > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "MADTIOAPICSAPICCOMPARELessThan ", > + "The number of IO SAPICS (%d) is less than " > + "the number of IO APICS (%d).", > + num_iosapics, num_ioapics); > + else > + fwts_passed(fw, "The number of IO SAPICS (%d) is at least" > + "equal to the number of IO APICS (%d)", > + num_iosapics, num_ioapics); > + > + /* Every I/O APIC must have a corresponding I/O SAPIC. */ > + failed = false; > + for (ioapic = 0; ioapic <= MAX_IO_APIC_ID; ioapic++) > + if (madt_io_apics[ioapic] && > + (madt_io_apics[ioapic] != madt_io_sapics[ioapic])) { > + failed = true; > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "MADTIOAPICSAPICCOMPARENoSAPIC ", > + "IO APIC %d does not have a IO " > + "SAPIC entry", > + ioapic); > + } When I compile, gcc complains acpi/madt/madt.c:1221:20: error: iteration 256u invokes undefined behavior [-Werror=aggressive-loop-optimizations] if (madt_io_apics[ioapic] && ^ acpi/madt/madt.c:1220:2: note: containing loop for (ioapic = 0; ioapic <= MAX_IO_APIC_ID; ioapic++) ^ cc1: all warnings being treated as errors This can be fixed as below (1. fix out-of-bound array, and 2) add "{}} to for statement) - for (ioapic = 0; ioapic <= MAX_IO_APIC_ID; ioapic++) + for (ioapic = 0; ioapic < MAX_IO_APIC_ID; ioapic++) { if (madt_io_apics[ioapic] && (madt_io_apics[ioapic] != madt_io_sapics[ioapic])) { failed = true; .... } + } > + > + if (!failed) > + fwts_passed(fw, "Each IO APIC entry has a corresponding" > + "SAPIC entry."); > +} > + > static int madt_subtables(fwts_framework *fw) > { > fwts_acpi_table_madt *madt = (fwts_acpi_table_madt *)mtable->data; > @@ -1188,6 +1243,8 @@ static int madt_subtables(fwts_framework *fw) > ssize_t length = mtable->length; > int ii = 0; > int proper_len; > + int num_ioapics = 0; > + int num_iosapics = 0; > > /* > * check the correctness of each subtable type, and whether or > @@ -1287,6 +1344,7 @@ static int madt_subtables(fwts_framework *fw) > > case FWTS_ACPI_MADT_IO_APIC: > skip = madt_io_apic(fw, hdr, data); > + num_ioapics++; > break; > > case FWTS_ACPI_MADT_INTERRUPT_OVERRIDE: > @@ -1307,6 +1365,7 @@ static int madt_subtables(fwts_framework *fw) > > case FWTS_ACPI_MADT_IO_SAPIC: > skip = madt_io_sapic(fw, hdr, data); > + num_iosapics++; > break; > > case FWTS_ACPI_MADT_LOCAL_SAPIC: > @@ -1385,6 +1444,9 @@ static int madt_subtables(fwts_framework *fw) > length -= skip; > } > > + /* run comparison tests */ > + madt_ioapic_sapic_compare(fw, num_ioapics, num_iosapics); > + > return FWTS_OK; > } > >
On 12/07/16 18:10, Prarit Bhargava wrote: > If both I/O APIC and I/O SAPIC subtables exist, there are two checks that > should be run. The first is to confirm that there are at least as many > I/O SAPIC tables as there are I/O APIC tables. The second is to check that > every I/O APIC ID has a corresponding I/O SAPIC ID table. > > This patchset implements the tests. > > Additionally note a fixed typo for "MADIOSPAICAddrZero". > > Signed-off-by: Prarit Bhargava <prarit@redhat.com> > --- > src/acpi/madt/madt.c | 76 +++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 69 insertions(+), 7 deletions(-) > > diff --git a/src/acpi/madt/madt.c b/src/acpi/madt/madt.c > index 8ee231ee16be..d4fcc195b134 100644 > --- a/src/acpi/madt/madt.c > +++ b/src/acpi/madt/madt.c > @@ -121,6 +121,7 @@ > #define SUBTABLE_UNDEFINED 0x00 > #define SUBTABLE_VARIABLE 0xff > #define NUM_SUBTABLE_TYPES 16 > +#define MAX_IO_APIC_ID 256 /* IO APIC ID field is 1 byte */ > > struct acpi_madt_subtable_lengths { > unsigned short major_version; /* from revision in FADT header */ > @@ -434,6 +435,7 @@ static int madt_local_apic(fwts_framework *fw, > return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); > } > > +static unsigned char madt_io_apics[MAX_IO_APIC_ID]; > static int madt_io_apic(fwts_framework *fw, > fwts_acpi_madt_sub_table_header *hdr, > const uint8_t *data) > @@ -461,6 +463,14 @@ static int madt_io_apic(fwts_framework *fw, > fwts_passed(fw, "MADT %s address in non-zero.", > madt_sub_names[hdr->type]); > > + if (!madt_io_apics[ioapic->io_apic_id]) > + madt_io_apics[ioapic->io_apic_id] = 1; > + else > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "MADIOSAPICNotUnique", > + "There are multiple entries for id %d.", > + ioapic->io_apic_id); > + > return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); > } > > @@ -588,6 +598,7 @@ static int madt_lapic_addr_override(fwts_framework *fw, > return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); > } > > +static unsigned char madt_io_sapics[MAX_IO_APIC_ID]; > static int madt_io_sapic(fwts_framework *fw, > fwts_acpi_madt_sub_table_header *hdr, > const uint8_t *data) > @@ -608,7 +619,7 @@ static int madt_io_sapic(fwts_framework *fw, > > if (sapic->address == 0) > fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "MADIOSPAICAddrZero", > + "MADIOSAPICAddrZero", > "MADT %s address is zero, needs to be defined.", > madt_sub_names[hdr->type]); > else > @@ -616,12 +627,13 @@ static int madt_io_sapic(fwts_framework *fw, > "MADT %s address set to non-zero value.", > madt_sub_names[hdr->type]); > > - /* > - * TODO: if both I/O APIC and I/O SAPIC subtables exist, there > - * must be at least as many I/O SAPIC subtables as I/O APIC subtables, > - * and that every I/O APIC has a corresponding I/O SAPIC with the > - * same APIC ID. > - */ > + if (!madt_io_sapics[sapic->io_sapic_id]) > + madt_io_sapics[sapic->io_sapic_id] = 1; > + else > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "MADIOSAPICNotUnique", > + "There are multiple entries for id %d.", > + sapic->io_sapic_id); > > return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); > } > @@ -1178,6 +1190,49 @@ static int madt_gic_its(fwts_framework *fw, > return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); > } > > +static void madt_ioapic_sapic_compare(fwts_framework *fw, > + int num_ioapics, > + int num_iosapics) > +{ > + bool failed; > + int ioapic; > + > + if (!num_iosapics) > + return; /* Nothing to do */ > + > + /* > + * If both I/O APIC and I/O SAPIC subtables exist, there must be at > + * least as many I/O SAPIC subtables as I/O APIC subtables. > + */ > + if (num_iosapics < num_ioapics) > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "MADTIOAPICSAPICCOMPARELessThan ", > + "The number of IO SAPICS (%d) is less than " > + "the number of IO APICS (%d).", > + num_iosapics, num_ioapics); > + else > + fwts_passed(fw, "The number of IO SAPICS (%d) is at least" > + "equal to the number of IO APICS (%d)", > + num_iosapics, num_ioapics); > + > + /* Every I/O APIC must have a corresponding I/O SAPIC. */ > + failed = false; > + for (ioapic = 0; ioapic <= MAX_IO_APIC_ID; ioapic++) Should that be: for (ioapic = 0; ioapic < MAX_IO_APIC_ID; ioapic++) > + if (madt_io_apics[ioapic] && > + (madt_io_apics[ioapic] != madt_io_sapics[ioapic])) { > + failed = true; > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "MADTIOAPICSAPICCOMPARENoSAPIC ", > + "IO APIC %d does not have a IO " > + "SAPIC entry", > + ioapic); > + } > + > + if (!failed) > + fwts_passed(fw, "Each IO APIC entry has a corresponding" > + "SAPIC entry."); > +} > + > static int madt_subtables(fwts_framework *fw) > { > fwts_acpi_table_madt *madt = (fwts_acpi_table_madt *)mtable->data; > @@ -1188,6 +1243,8 @@ static int madt_subtables(fwts_framework *fw) > ssize_t length = mtable->length; > int ii = 0; > int proper_len; > + int num_ioapics = 0; > + int num_iosapics = 0; > > /* > * check the correctness of each subtable type, and whether or > @@ -1287,6 +1344,7 @@ static int madt_subtables(fwts_framework *fw) > > case FWTS_ACPI_MADT_IO_APIC: > skip = madt_io_apic(fw, hdr, data); > + num_ioapics++; > break; > > case FWTS_ACPI_MADT_INTERRUPT_OVERRIDE: > @@ -1307,6 +1365,7 @@ static int madt_subtables(fwts_framework *fw) > > case FWTS_ACPI_MADT_IO_SAPIC: > skip = madt_io_sapic(fw, hdr, data); > + num_iosapics++; > break; > > case FWTS_ACPI_MADT_LOCAL_SAPIC: > @@ -1385,6 +1444,9 @@ static int madt_subtables(fwts_framework *fw) > length -= skip; > } > > + /* run comparison tests */ > + madt_ioapic_sapic_compare(fw, num_ioapics, num_iosapics); > + > return FWTS_OK; > } > > Apart from the one issue, the rest looks good. Colin
diff --git a/src/acpi/madt/madt.c b/src/acpi/madt/madt.c index 8ee231ee16be..d4fcc195b134 100644 --- a/src/acpi/madt/madt.c +++ b/src/acpi/madt/madt.c @@ -121,6 +121,7 @@ #define SUBTABLE_UNDEFINED 0x00 #define SUBTABLE_VARIABLE 0xff #define NUM_SUBTABLE_TYPES 16 +#define MAX_IO_APIC_ID 256 /* IO APIC ID field is 1 byte */ struct acpi_madt_subtable_lengths { unsigned short major_version; /* from revision in FADT header */ @@ -434,6 +435,7 @@ static int madt_local_apic(fwts_framework *fw, return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); } +static unsigned char madt_io_apics[MAX_IO_APIC_ID]; static int madt_io_apic(fwts_framework *fw, fwts_acpi_madt_sub_table_header *hdr, const uint8_t *data) @@ -461,6 +463,14 @@ static int madt_io_apic(fwts_framework *fw, fwts_passed(fw, "MADT %s address in non-zero.", madt_sub_names[hdr->type]); + if (!madt_io_apics[ioapic->io_apic_id]) + madt_io_apics[ioapic->io_apic_id] = 1; + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "MADIOSAPICNotUnique", + "There are multiple entries for id %d.", + ioapic->io_apic_id); + return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); } @@ -588,6 +598,7 @@ static int madt_lapic_addr_override(fwts_framework *fw, return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); } +static unsigned char madt_io_sapics[MAX_IO_APIC_ID]; static int madt_io_sapic(fwts_framework *fw, fwts_acpi_madt_sub_table_header *hdr, const uint8_t *data) @@ -608,7 +619,7 @@ static int madt_io_sapic(fwts_framework *fw, if (sapic->address == 0) fwts_failed(fw, LOG_LEVEL_MEDIUM, - "MADIOSPAICAddrZero", + "MADIOSAPICAddrZero", "MADT %s address is zero, needs to be defined.", madt_sub_names[hdr->type]); else @@ -616,12 +627,13 @@ static int madt_io_sapic(fwts_framework *fw, "MADT %s address set to non-zero value.", madt_sub_names[hdr->type]); - /* - * TODO: if both I/O APIC and I/O SAPIC subtables exist, there - * must be at least as many I/O SAPIC subtables as I/O APIC subtables, - * and that every I/O APIC has a corresponding I/O SAPIC with the - * same APIC ID. - */ + if (!madt_io_sapics[sapic->io_sapic_id]) + madt_io_sapics[sapic->io_sapic_id] = 1; + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "MADIOSAPICNotUnique", + "There are multiple entries for id %d.", + sapic->io_sapic_id); return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); } @@ -1178,6 +1190,49 @@ static int madt_gic_its(fwts_framework *fw, return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header)); } +static void madt_ioapic_sapic_compare(fwts_framework *fw, + int num_ioapics, + int num_iosapics) +{ + bool failed; + int ioapic; + + if (!num_iosapics) + return; /* Nothing to do */ + + /* + * If both I/O APIC and I/O SAPIC subtables exist, there must be at + * least as many I/O SAPIC subtables as I/O APIC subtables. + */ + if (num_iosapics < num_ioapics) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "MADTIOAPICSAPICCOMPARELessThan ", + "The number of IO SAPICS (%d) is less than " + "the number of IO APICS (%d).", + num_iosapics, num_ioapics); + else + fwts_passed(fw, "The number of IO SAPICS (%d) is at least" + "equal to the number of IO APICS (%d)", + num_iosapics, num_ioapics); + + /* Every I/O APIC must have a corresponding I/O SAPIC. */ + failed = false; + for (ioapic = 0; ioapic <= MAX_IO_APIC_ID; ioapic++) + if (madt_io_apics[ioapic] && + (madt_io_apics[ioapic] != madt_io_sapics[ioapic])) { + failed = true; + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "MADTIOAPICSAPICCOMPARENoSAPIC ", + "IO APIC %d does not have a IO " + "SAPIC entry", + ioapic); + } + + if (!failed) + fwts_passed(fw, "Each IO APIC entry has a corresponding" + "SAPIC entry."); +} + static int madt_subtables(fwts_framework *fw) { fwts_acpi_table_madt *madt = (fwts_acpi_table_madt *)mtable->data; @@ -1188,6 +1243,8 @@ static int madt_subtables(fwts_framework *fw) ssize_t length = mtable->length; int ii = 0; int proper_len; + int num_ioapics = 0; + int num_iosapics = 0; /* * check the correctness of each subtable type, and whether or @@ -1287,6 +1344,7 @@ static int madt_subtables(fwts_framework *fw) case FWTS_ACPI_MADT_IO_APIC: skip = madt_io_apic(fw, hdr, data); + num_ioapics++; break; case FWTS_ACPI_MADT_INTERRUPT_OVERRIDE: @@ -1307,6 +1365,7 @@ static int madt_subtables(fwts_framework *fw) case FWTS_ACPI_MADT_IO_SAPIC: skip = madt_io_sapic(fw, hdr, data); + num_iosapics++; break; case FWTS_ACPI_MADT_LOCAL_SAPIC: @@ -1385,6 +1444,9 @@ static int madt_subtables(fwts_framework *fw) length -= skip; } + /* run comparison tests */ + madt_ioapic_sapic_compare(fw, num_ioapics, num_iosapics); + return FWTS_OK; }
If both I/O APIC and I/O SAPIC subtables exist, there are two checks that should be run. The first is to confirm that there are at least as many I/O SAPIC tables as there are I/O APIC tables. The second is to check that every I/O APIC ID has a corresponding I/O SAPIC ID table. This patchset implements the tests. Additionally note a fixed typo for "MADIOSPAICAddrZero". Signed-off-by: Prarit Bhargava <prarit@redhat.com> --- src/acpi/madt/madt.c | 76 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 69 insertions(+), 7 deletions(-)