| Message ID | 5444DE02.6030600@canonical.com |
|---|---|
| State | Rejected |
| Headers | show |
On 10/20/2014 04:03 AM, Colin Ian King wrote: > On 20/10/14 09:37, Fu Wei wrote: >> arm64: ignore load failed for FACS which is involved in old >> BIOS style suspend/hibernate and does not make sense on arm64. >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> --- >> src/lib/src/fwts_acpi_tables.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c >> index 56498e0..0641d93 100644 >> --- a/src/lib/src/fwts_acpi_tables.c >> +++ b/src/lib/src/fwts_acpi_tables.c >> @@ -375,7 +375,12 @@ static int fwts_acpi_handle_fadt( >> "FACS", "FIRMWARE_CTRL", "X_FIRMWARE_CTRL", >> &fadt->firmware_control, &fadt->x_firmware_ctrl, >> provenance) != FWTS_OK) { >> +#if defined(__aarch64__) >> + fwts_log_warning(fw, "Failed to load FACS: Cannot determine " >> + "address of FACS from FADT. IGNORE for AArch64 platform!"); >> +#else >> return FWTS_ERROR; >> +#endif >> } >> /* Determine DSDT addr and load it */ >> if (fwts_acpi_handle_fadt_tables(fw, fadt, >> > > I've re-read the relevant sections of the ACPI 5.1 specification and I > can't see any mention of this table being mandatory or not; so a failure > on it not existing may be wrong for any architecture. > > Does the Fixed ACPI Description Table Fixed Feature Flags > HW_REDUCED_ACPI bit being set to 1 imply that the FACS not existing is > OK? (See ACPI 5.1, able 5-35, bit 20). Ick. The spec is ambiguous. I could not find anything that explicitly said FACS is mandatory or optional. HW reduced mode does not affect the *existence* of the FACS table, though; both the FIRMWARE_CTRL and X_FIRMWARE_CTRL fields are allowed and usable in HW reduced (top of section 5.2.9) and at least one of them must be non-zero (per table 5-34). So, that would imply there must be an FACS, but it is not explicit. But, there's also things like the Global Lock in the FACS must not be used in HW reduced mode. And, the FACS fields for the waking vector probably don't even make sense on ARMv8. But, the Hardware Signature might make sense if a TPM is in use. So I guess -- based on what I'm seeing in the spec -- I would still leave this as an error if neither FIRMWARE_CTRL or X_FIRMWARE_CTRL is set. This seems to be a legitimate (if not weird) bug in the tables as currently defined. I'll try to put together an ECR to clarify this in the spec and make the table optional. > If that is a reasonable assumption, maybe that would be a better way of > checking. E.g. > > diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c > index 56498e0..97a7bd4 100644 > --- a/src/lib/src/fwts_acpi_tables.c > +++ b/src/lib/src/fwts_acpi_tables.c > @@ -311,6 +311,9 @@ static int fwts_acpi_handle_fadt_tables( > } > /* Is it sane? */ > if (addr == 0) { > + /* HW_REDUCED - address can be zero */ > + if (fadt->flags & (1 << 20)) > + return FWTS_OK; > fwts_log_error(fw, "Failed to load %s: Cannot > determine " > "address of %s from FADT, fields %s and > %s are zero.", > name, name, name_addr32, name_addr64); > @@ -320,6 +323,9 @@ static int fwts_acpi_handle_fadt_tables( > addr = (off_t)*addr32; > /* Is it sane? */ > if (addr == 0) { > + /* HW_REDUCED - address can be zero */ > + if (fadt->flags & (1 << 20)) > + return FWTS_OK; > fwts_log_error(fw, "Failed to load %s: Cannot > determine " > "address of %s from FADT, field %s is > zero.", > name, name, name_addr32); > > > Colin >
I saw Al had a discussion on awsg mailing list on this topic. Will it be better if we wait until his ECR merge to ACPI5.next and re-visit this topic? On 14-10-21 06:01 AM, Al Stone wrote: > On 10/20/2014 04:03 AM, Colin Ian King wrote: >> On 20/10/14 09:37, Fu Wei wrote: >>> arm64: ignore load failed for FACS which is involved in old >>> BIOS style suspend/hibernate and does not make sense on arm64. >>> >>> Signed-off-by: Fu Wei <fu.wei@linaro.org> >>> --- >>> src/lib/src/fwts_acpi_tables.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c >>> index 56498e0..0641d93 100644 >>> --- a/src/lib/src/fwts_acpi_tables.c >>> +++ b/src/lib/src/fwts_acpi_tables.c >>> @@ -375,7 +375,12 @@ static int fwts_acpi_handle_fadt( >>> "FACS", "FIRMWARE_CTRL", "X_FIRMWARE_CTRL", >>> &fadt->firmware_control, &fadt->x_firmware_ctrl, >>> provenance) != FWTS_OK) { >>> +#if defined(__aarch64__) >>> + fwts_log_warning(fw, "Failed to load FACS: Cannot determine " >>> + "address of FACS from FADT. IGNORE for AArch64 platform!"); >>> +#else >>> return FWTS_ERROR; >>> +#endif >>> } >>> /* Determine DSDT addr and load it */ >>> if (fwts_acpi_handle_fadt_tables(fw, fadt, >>> >> I've re-read the relevant sections of the ACPI 5.1 specification and I >> can't see any mention of this table being mandatory or not; so a failure >> on it not existing may be wrong for any architecture. >> >> Does the Fixed ACPI Description Table Fixed Feature Flags >> HW_REDUCED_ACPI bit being set to 1 imply that the FACS not existing is >> OK? (See ACPI 5.1, able 5-35, bit 20). > Ick. The spec is ambiguous. I could not find anything that explicitly > said FACS is mandatory or optional. HW reduced mode does not affect > the *existence* of the FACS table, though; both the FIRMWARE_CTRL and > X_FIRMWARE_CTRL fields are allowed and usable in HW reduced (top of > section 5.2.9) and at least one of them must be non-zero (per table > 5-34). So, that would imply there must be an FACS, but it is not explicit. > > But, there's also things like the Global Lock in the FACS must not be > used in HW reduced mode. And, the FACS fields for the waking vector > probably don't even make sense on ARMv8. But, the Hardware Signature > might make sense if a TPM is in use. > > So I guess -- based on what I'm seeing in the spec -- I would still > leave this as an error if neither FIRMWARE_CTRL or X_FIRMWARE_CTRL is > set. This seems to be a legitimate (if not weird) bug in the tables > as currently defined. > > I'll try to put together an ECR to clarify this in the spec and make > the table optional. > > >> If that is a reasonable assumption, maybe that would be a better way of >> checking. E.g. >> >> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c >> index 56498e0..97a7bd4 100644 >> --- a/src/lib/src/fwts_acpi_tables.c >> +++ b/src/lib/src/fwts_acpi_tables.c >> @@ -311,6 +311,9 @@ static int fwts_acpi_handle_fadt_tables( >> } >> /* Is it sane? */ >> if (addr == 0) { >> + /* HW_REDUCED - address can be zero */ >> + if (fadt->flags & (1 << 20)) >> + return FWTS_OK; >> fwts_log_error(fw, "Failed to load %s: Cannot >> determine " >> "address of %s from FADT, fields %s and >> %s are zero.", >> name, name, name_addr32, name_addr64); >> @@ -320,6 +323,9 @@ static int fwts_acpi_handle_fadt_tables( >> addr = (off_t)*addr32; >> /* Is it sane? */ >> if (addr == 0) { >> + /* HW_REDUCED - address can be zero */ >> + if (fadt->flags & (1 << 20)) >> + return FWTS_OK; >> fwts_log_error(fw, "Failed to load %s: Cannot >> determine " >> "address of %s from FADT, field %s is >> zero.", >> name, name, name_addr32); >> >> >> Colin >> >
diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c index 56498e0..97a7bd4 100644 --- a/src/lib/src/fwts_acpi_tables.c +++ b/src/lib/src/fwts_acpi_tables.c @@ -311,6 +311,9 @@ static int fwts_acpi_handle_fadt_tables( } /* Is it sane? */ if (addr == 0) { + /* HW_REDUCED - address can be zero */ + if (fadt->flags & (1 << 20)) + return FWTS_OK; fwts_log_error(fw, "Failed to load %s: Cannot determine " "address of %s from FADT, fields %s and