[V2][PATCH 2/2] acpi: acpitables: FADT: Ignore fields at offset 46 through 108 for HW_REDUCED_ACPI
Colin Ian King
colin.king at canonical.com
Tue Apr 7 10:20:10 UTC 2015
On 07/04/15 11:12, Hanjun Guo wrote:
> On 2015年04月07日 13:44, Heyi Guo wrote:
>> According to ACPI spec 5.1, section 5.2.9, "If the HW_REDUCED_ACPI
>> flag in the table is set, OSPM will ignore fields related to the ACPI
>> HW register interface: Fields at offsets 46 through 108 and 148
>> through 232, as well as FADT Flag bits 1, 2, 3,7,8,12,13, 14, 16 and
>> 17).", add precondition of checking SMI_CMD, PM_TMR_LEN, etc. As the
>> code becomes a little complex, split it into small functions for each
>> important field check.
>>
>> Signed-off-by: Heyi Guo <heyi.guo at linaro.org>
>> ---
>> src/acpi/acpitables/acpitables.c | 90
>> +++++++++++++++++++++++++++++++---------
>> 1 file changed, 70 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/acpi/acpitables/acpitables.c
>> b/src/acpi/acpitables/acpitables.c
>> index e5fffff..52b6659 100644
>> --- a/src/acpi/acpitables/acpitables.c
>> +++ b/src/acpi/acpitables/acpitables.c
>> @@ -69,12 +69,10 @@ static void acpi_table_check_hpet(fwts_framework
>> *fw, fwts_acpi_table_info *tabl
>>
>> }
>>
>> -static void acpi_table_check_fadt(fwts_framework *fw,
>> fwts_acpi_table_info *table)
>> +static void acpi_table_check_fadt_firmware_control(fwts_framework
>> *fw, fwts_acpi_table_fadt *fadt)
>> {
>> - fwts_acpi_table_fadt *fadt = (fwts_acpi_table_fadt*)table->data;
>> -
>> if (fadt->firmware_control == 0) {
>> - if (table->length >= 140) {
>> + if (fadt->header.length >= 140) {
>> if ((fadt->x_firmware_ctrl == 0) &&
>> !(fwts_acpi_is_reduced_hardware(fadt))) {
>> fwts_failed(fw, LOG_LEVEL_CRITICAL, "FADTFACSZero",
>> "FADT 32 bit FIRMWARE_CONTROL and 64 bit X_FIRMWARE_CONTROL (FACS
>> address) are null.");
>> fwts_advice(fw, "The 32 bit FIRMWARE_CTRL or 64 bit
>> X_FIRMWARE_CTRL should point to a valid "
>> @@ -88,7 +86,7 @@ static void acpi_table_check_fadt(fwts_framework
>> *fw, fwts_acpi_table_info *tabl
>> "bug and needs to be fixed.");
>> }
>> } else {
>> - if (table->length >= 140) {
>> + if (fadt->header.length >= 140) {
>> if (fadt->x_firmware_ctrl != 0) {
>> if (((uint64_t)fadt->firmware_control !=
>> fadt->x_firmware_ctrl)) {
>> fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> "FwCtrl32and64Differ",
>> @@ -103,10 +101,14 @@ static void acpi_table_check_fadt(fwts_framework
>> *fw, fwts_acpi_table_info *tabl
>> }
>> }
>> }
>> +}
>>
>> +static void acpi_table_check_fadt_dsdt(fwts_framework *fw,
>> fwts_acpi_table_fadt *fadt)
>> +{
>> if (fadt->dsdt == 0)
>> fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTDSTNull", "FADT DSDT
>> address is null.");
>> - if (table->length >= 148) {
>> +
>> + if (fadt->header.length >= 148) {
>> if (fadt->x_dsdt == 0) {
>> fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTXDSTDNull", "FADT
>> X_DSDT address is null.");
>> fwts_advice(fw, "An ACPI 2.0 FADT is being used however
>> the 64 bit X_DSDT is null."
>> @@ -121,6 +123,16 @@ static void acpi_table_check_fadt(fwts_framework
>> *fw, fwts_acpi_table_info *tabl
>> "The kernel works around this by using the 64
>> bit X_DSDT pointer to the DSDT. ");
>> }
>> }
>> +}
>> +
>> +
>> +static void acpi_table_check_fadt_smi(fwts_framework *fw,
>> fwts_acpi_table_fadt *fadt)
>> +{
>> + if (fwts_acpi_is_reduced_hardware(fadt)) {
>> + if (fadt->smi_cmd != 0)
>> + fwts_warning(fw, "FADT SMI_CMD is not zero but should be
>> in reduced hardware mode.");
>> + return;
>> + }
>>
>> /*
>> * Section 5.2.9 (Fixed ACPI Description Table) of the ACPI 5.0
>> @@ -153,6 +165,15 @@ static void acpi_table_check_fadt(fwts_framework
>> *fw, fwts_acpi_table_info *tabl
>> "specific functions will not work.");
>> }
>> }
>> +}
>> +
>> +static void acpi_table_check_fadt_pm_tmr(fwts_framework *fw,
>> fwts_acpi_table_fadt *fadt)
>> +{
>> + if (fwts_acpi_is_reduced_hardware(fadt)) {
>> + if (fadt->pm_tmr_len != 0)
>> + fwts_warning(fw, "FADT PM_TMR_LEN is not zero but should
>> be in reduced hardware mode.");
>> + return;
>> + }
>>
>> if (fadt->pm_tmr_len != 4) {
>> fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadPMTMRLEN",
>> @@ -161,6 +182,18 @@ static void acpi_table_check_fadt(fwts_framework
>> *fw, fwts_acpi_table_info *tabl
>> "This fields value must be 4. If it is not the
>> correct size then the kernel "
>> "will not request a region for the pm timer block. ");
>> }
>> +}
>> +
>> +static void acpi_table_check_fadt_gpe(fwts_framework *fw,
>> fwts_acpi_table_fadt *fadt)
>> +{
>> + if (fwts_acpi_is_reduced_hardware(fadt)) {
>> + if (fadt->gpe0_blk_len != 0)
>> + fwts_warning(fw, "FADT GPE0_BLK_LEN is not zero but
>> should be in reduced hardware mode.");
>> + if (fadt->gpe1_blk_len != 0)
>> + fwts_warning(fw, "FADT GPE1_BLK_LEN is not zero but
>> should be in reduced hardware mode.");
>
> I prefer to print a single message:
>
> if ((fadt->gpe0_blk_len != 0) || if (fadt->gpe1_blk_len != 0))
> fwts_warningfw, "FADT GPE0_BLK_LEN should be zero in reduced
> hardware mode.");
>
I'd prefer two distinct warning messages because the more exact
information we have the less work an engineer needs to do to check
exactly what is wrong when they get the message. I prefer clarity over
bloat.
Colin
> other that that,
>
> Reviewed-by: Hanjun Guo <hanjun.guo at linaro.org>
>
> Thanks
> Hanjun
>
>> + return;
>> + }
>> +
>> if (fadt->gpe0_blk_len & 1) {
>> fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadGPEBLKLEN", "FADT
>> GPE0_BLK_LEN is %" PRIu8
>> ", should a multiple of 2.", fadt->gpe0_blk_len);
>> @@ -175,6 +208,36 @@ static void acpi_table_check_fadt(fwts_framework
>> *fw, fwts_acpi_table_info *tabl
>> "not map in the GPE1 region. This could mean that
>> General Purpose Events will not "
>> "function correctly (for example lid or ac-power
>> events).");
>> }
>> +}
>> +
>> +static void acpi_table_check_fadt_reset(fwts_framework *fw,
>> fwts_acpi_table_fadt *fadt)
>> +{
>> + if (fadt->header.length>=129) {
>> + if ((fadt->reset_reg.address_space_id != 0) &&
>> + (fadt->reset_reg.address_space_id != 1) &&
>> + (fadt->reset_reg.address_space_id != 2)) {
>> + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadRESETREG",
>> + "FADT RESET_REG address space ID was %" PRIu8 ", must
>> be System Memory space (0), "
>> + "System I/O space (1), or PCI configuration space (2).",
>> + fadt->reset_reg.address_space_id);
>> + fwts_advice(fw, "If the FADT RESET_REG address space ID
>> is not set correctly then ACPI writes "
>> + "to this register *may* nor work correctly,
>> meaning a reboot via this mechanism may not work.");
>> + }
>> + if ((fadt->reset_value == 0) && (fadt->reset_reg.address != 0))
>> + fwts_warning(fw, "FADT RESET_VALUE is zero, which may be
>> incorrect, it is usually non-zero.");
>> + }
>> +}
>> +
>> +static void acpi_table_check_fadt(fwts_framework *fw,
>> fwts_acpi_table_info *table)
>> +{
>> + fwts_acpi_table_fadt *fadt = (fwts_acpi_table_fadt*)table->data;
>> +
>> + acpi_table_check_fadt_firmware_control(fw, fadt);
>> + acpi_table_check_fadt_dsdt(fw, fadt);
>> + acpi_table_check_fadt_smi(fw, fadt);
>> + acpi_table_check_fadt_pm_tmr(fw, fadt);
>> + acpi_table_check_fadt_gpe(fw, fadt);
>> +
>> /*
>> * Bug LP: /833644
>> *
>> @@ -208,20 +271,7 @@ static void acpi_table_check_fadt(fwts_framework
>> *fw, fwts_acpi_table_info *tabl
>> fwts_warning(fw, "FADT CENTURY is zero, RTC does not support
>> centenary feature is not supported.");
>> */
>>
>> - if (table->length>=129) {
>> - if ((fadt->reset_reg.address_space_id != 0) &&
>> - (fadt->reset_reg.address_space_id != 1) &&
>> - (fadt->reset_reg.address_space_id != 2)) {
>> - fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadRESETREG",
>> - "FADT RESET_REG address space ID was %" PRIu8 ", must
>> be System Memory space (0), "
>> - "System I/O space (1), or PCI configuration space (2).",
>> - fadt->reset_reg.address_space_id);
>> - fwts_advice(fw, "If the FADT RESET_REG address space ID
>> is not set correctly then ACPI writes "
>> - "to this register *may* nor work correctly,
>> meaning a reboot via this mechanism may not work.");
>> - }
>> - if ((fadt->reset_value == 0) && (fadt->reset_reg.address != 0))
>> - fwts_warning(fw, "FADT RESET_VALUE is zero, which may be
>> incorrect, it is usually non-zero.");
>> - }
>> + acpi_table_check_fadt_reset(fw, fadt);
>> }
>>
>> static void acpi_table_check_rsdp(fwts_framework *fw,
>> fwts_acpi_table_info *table)
>>
>
More information about the fwts-devel
mailing list