| Message ID | 1453425260-21576-4-git-send-email-al.stone@linaro.org |
|---|---|
| State | Rejected |
| Headers | show |
On 2016-01-22 09:14 AM, Al Stone wrote: > If it is necessary to create an RSDP table because there is none that > can be read, add in only the RSDT or XSDT pointers as needed. For x86, > it can be either, but for arm64 it should only be the XSDT address that > is used in the RSDP. > > Signed-off-by: Al Stone <al.stone@linaro.org> > --- > src/lib/src/fwts_acpi_tables.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c > index 0191b6b..83bd1dd 100644 > --- a/src/lib/src/fwts_acpi_tables.c > +++ b/src/lib/src/fwts_acpi_tables.c > @@ -1101,14 +1101,17 @@ static int fwts_acpi_load_tables_fixup(fwts_framework *fw) > } > > /* Now we have all the tables, final fix up is required */ > - if (rsdp->rsdt_address != rsdt_fake_addr) { > - rsdp->rsdt_address = rsdt_fake_addr; > - redo_rsdp_checksum = true; > - } > - if ((rsdp->revision > 0) && (rsdp->length >= 36) && > - (rsdp->xsdt_address != xsdt_fake_addr)) { > - rsdp->xsdt_address = xsdt_fake_addr; > - redo_rsdp_checksum = true; > + if (fw->target_arch == FWTS_ARCH_ARM64) { > + if ((rsdp->revision > 0) && (rsdp->length >= 36) && > + (rsdp->xsdt_address != xsdt_fake_addr)) { > + rsdp->xsdt_address = xsdt_fake_addr; > + redo_rsdp_checksum = true; > + } > + } else { > + if (rsdp->rsdt_address != rsdt_fake_addr) { > + rsdp->rsdt_address = rsdt_fake_addr; > + redo_rsdp_checksum = true; > + } Since non-ARM (ex. x86) can have either RSDP or XSDP, should the below statements be included here as well? if ((rsdp->revision > 0) && (rsdp->length >= 36) && (rsdp->xsdt_address != xsdt_fake_addr)) { rsdp->xsdt_address = xsdt_fake_addr; redo_rsdp_checksum = true; } > } > /* And update checksum if we've updated the rsdp */ > if (redo_rsdp_checksum) { >
On 01/26/2016 08:01 PM, Alex Hung wrote: > On 2016-01-22 09:14 AM, Al Stone wrote: >> If it is necessary to create an RSDP table because there is none that >> can be read, add in only the RSDT or XSDT pointers as needed. For x86, >> it can be either, but for arm64 it should only be the XSDT address that >> is used in the RSDP. >> >> Signed-off-by: Al Stone <al.stone@linaro.org> >> --- >> src/lib/src/fwts_acpi_tables.c | 19 +++++++++++-------- >> 1 file changed, 11 insertions(+), 8 deletions(-) >> >> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c >> index 0191b6b..83bd1dd 100644 >> --- a/src/lib/src/fwts_acpi_tables.c >> +++ b/src/lib/src/fwts_acpi_tables.c >> @@ -1101,14 +1101,17 @@ static int fwts_acpi_load_tables_fixup(fwts_framework >> *fw) >> } >> >> /* Now we have all the tables, final fix up is required */ >> - if (rsdp->rsdt_address != rsdt_fake_addr) { >> - rsdp->rsdt_address = rsdt_fake_addr; >> - redo_rsdp_checksum = true; >> - } >> - if ((rsdp->revision > 0) && (rsdp->length >= 36) && >> - (rsdp->xsdt_address != xsdt_fake_addr)) { >> - rsdp->xsdt_address = xsdt_fake_addr; >> - redo_rsdp_checksum = true; >> + if (fw->target_arch == FWTS_ARCH_ARM64) { >> + if ((rsdp->revision > 0) && (rsdp->length >= 36) && >> + (rsdp->xsdt_address != xsdt_fake_addr)) { >> + rsdp->xsdt_address = xsdt_fake_addr; >> + redo_rsdp_checksum = true; >> + } >> + } else { >> + if (rsdp->rsdt_address != rsdt_fake_addr) { >> + rsdp->rsdt_address = rsdt_fake_addr; >> + redo_rsdp_checksum = true; >> + } > > Since non-ARM (ex. x86) can have either RSDP or XSDP, should the below I assume that's a typo and RSDT or XSDT was meant... > statements be included here as well? > > if ((rsdp->revision > 0) && (rsdp->length >= 36) && > (rsdp->xsdt_address != xsdt_fake_addr)) { > rsdp->xsdt_address = xsdt_fake_addr; > redo_rsdp_checksum = true; > } That's a good question. For 6.0 and later, either RSDT or XSDT can be used but only one is supposed to be used at a time. Unfortunately, the spec has been really bad about aligning table revisions with spec revisions. And as a practical matter, many vendors fill in both of the fields on x86, and a lot of them tend to use whatever table version they like, even if it's inconsistent with the length. Should the check be on the FADT major/minor numbers instead? I think that might be a more reliable check. >> } >> /* And update checksum if we've updated the rsdp */ >> if (redo_rsdp_checksum) { >> > >
On 01/28/2016 10:50 AM, Al Stone wrote: > On 01/26/2016 08:01 PM, Alex Hung wrote: >> On 2016-01-22 09:14 AM, Al Stone wrote: >>> If it is necessary to create an RSDP table because there is none that >>> can be read, add in only the RSDT or XSDT pointers as needed. For x86, >>> it can be either, but for arm64 it should only be the XSDT address that >>> is used in the RSDP. >>> >>> Signed-off-by: Al Stone <al.stone@linaro.org> >>> --- >>> src/lib/src/fwts_acpi_tables.c | 19 +++++++++++-------- >>> 1 file changed, 11 insertions(+), 8 deletions(-) >>> >>> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c >>> index 0191b6b..83bd1dd 100644 >>> --- a/src/lib/src/fwts_acpi_tables.c >>> +++ b/src/lib/src/fwts_acpi_tables.c >>> @@ -1101,14 +1101,17 @@ static int fwts_acpi_load_tables_fixup(fwts_framework >>> *fw) >>> } >>> >>> /* Now we have all the tables, final fix up is required */ >>> - if (rsdp->rsdt_address != rsdt_fake_addr) { >>> - rsdp->rsdt_address = rsdt_fake_addr; >>> - redo_rsdp_checksum = true; >>> - } >>> - if ((rsdp->revision > 0) && (rsdp->length >= 36) && >>> - (rsdp->xsdt_address != xsdt_fake_addr)) { >>> - rsdp->xsdt_address = xsdt_fake_addr; >>> - redo_rsdp_checksum = true; >>> + if (fw->target_arch == FWTS_ARCH_ARM64) { >>> + if ((rsdp->revision > 0) && (rsdp->length >= 36) && >>> + (rsdp->xsdt_address != xsdt_fake_addr)) { >>> + rsdp->xsdt_address = xsdt_fake_addr; >>> + redo_rsdp_checksum = true; >>> + } >>> + } else { >>> + if (rsdp->rsdt_address != rsdt_fake_addr) { >>> + rsdp->rsdt_address = rsdt_fake_addr; >>> + redo_rsdp_checksum = true; >>> + } >> >> Since non-ARM (ex. x86) can have either RSDP or XSDP, should the below > > I assume that's a typo and RSDT or XSDT was meant... > >> statements be included here as well? >> >> if ((rsdp->revision > 0) && (rsdp->length >= 36) && >> (rsdp->xsdt_address != xsdt_fake_addr)) { >> rsdp->xsdt_address = xsdt_fake_addr; >> redo_rsdp_checksum = true; >> } > > That's a good question. For 6.0 and later, either RSDT or XSDT can > be used but only one is supposed to be used at a time. Unfortunately, > the spec has been really bad about aligning table revisions with spec > revisions. And as a practical matter, many vendors fill in both of > the fields on x86, and a lot of them tend to use whatever table version > they like, even if it's inconsistent with the length. > > Should the check be on the FADT major/minor numbers instead? I think > that might be a more reliable check. D'oh. My bad. Once I paid closer attention, I realized what you're saying; I answered a different question, I think. On further thought, you're right. If we're bodging up the RSDP anyway on x86, we know we're going to have vendors doing odd things that require filling in both the RSDT and XSDT addresses. This will be reported as an error when checking for compliance but it will make sense under the circumstances. I'll add these lines in v2 of these patches. Thanks for catching that. >>> } >>> /* And update checksum if we've updated the rsdp */ >>> if (redo_rsdp_checksum) { >>> >> >> > >
diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c index 0191b6b..83bd1dd 100644 --- a/src/lib/src/fwts_acpi_tables.c +++ b/src/lib/src/fwts_acpi_tables.c @@ -1101,14 +1101,17 @@ static int fwts_acpi_load_tables_fixup(fwts_framework *fw) } /* Now we have all the tables, final fix up is required */ - if (rsdp->rsdt_address != rsdt_fake_addr) { - rsdp->rsdt_address = rsdt_fake_addr; - redo_rsdp_checksum = true; - } - if ((rsdp->revision > 0) && (rsdp->length >= 36) && - (rsdp->xsdt_address != xsdt_fake_addr)) { - rsdp->xsdt_address = xsdt_fake_addr; - redo_rsdp_checksum = true; + if (fw->target_arch == FWTS_ARCH_ARM64) { + if ((rsdp->revision > 0) && (rsdp->length >= 36) && + (rsdp->xsdt_address != xsdt_fake_addr)) { + rsdp->xsdt_address = xsdt_fake_addr; + redo_rsdp_checksum = true; + } + } else { + if (rsdp->rsdt_address != rsdt_fake_addr) { + rsdp->rsdt_address = rsdt_fake_addr; + redo_rsdp_checksum = true; + } } /* And update checksum if we've updated the rsdp */ if (redo_rsdp_checksum) {
If it is necessary to create an RSDP table because there is none that can be read, add in only the RSDT or XSDT pointers as needed. For x86, it can be either, but for arm64 it should only be the XSDT address that is used in the RSDP. Signed-off-by: Al Stone <al.stone@linaro.org> --- src/lib/src/fwts_acpi_tables.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)