Issues in fadt check item?

[Dong, Eric]

Hi Alex & Jeffrey,

Thanks for your comments, I don’t aware my mark solution not work in this mail list. Attach my change code for you to reference. I think the old code logic not consistent.  PS: 12345678

I missed the latest 6.1 spec, I just check the 6.0 spec and not found this sentence. "If the X_PM1b_CNT_BLK field contains a non-zero value then this field must be zero." This is new added for 6.1 spec.

Thanks,
Eric
From: Alex Hung [mailto:alex.hung at canonical.com]
Sent: Thursday, December 08, 2016 1:38 AM
To: Jeffrey Hugo
Cc: Dong, Eric; fwts-devel at lists.ubuntu.com
Subject: Re: Issues in fadt check item?

I agree with Jeffrey that's actually not a fix: ACPI 6.1 explicitly states PM1b_CNT_BLK to meet "If the X_PM1b_CNT_BLK field contains a non-zero value then this field must be zero." The same also applies to PM2_CNT_BLK.
Many systems are shipped before ACPI 6.1's release, and this failure is odd for them.  I think it is a case this error can be ignored.  However, you should try to meet ACPI 6.1 spec if a target system is being developed.

On Wed, Dec 7, 2016 at 7:36 AM, Jeffrey Hugo <jhugo at codeaurora.org<mailto:jhugo at codeaurora.org>> wrote:
Hi Eric


On 12/7/2016 5:51 AM, Dong, Eric wrote:
Hi,



When I use fwts to check uefi bios code, it reports some issues related
to fadt check item. After check the uefi code and acpi spec, I didn’t
find error for uefi code. So I download fwts code (fwts_16.11.00) and i
think some bugs in the fadt check item.

One issue like below:

static void acpi_table_check_fadt_pm2_cnt_blk(fwts_framework *fw)

{

                if (fadt->pm2_cnt_blk == 0 && fadt->header.length < 208) {

                                fwts_skipped(fw, "FADT PM2_CNT_BLK not
being used.");

                                return;

                }



                if (fadt->pm2_cnt_blk == 0 &&
fadt->x_pm2_cnt_blk.address == 0) {

                                fwts_skipped(fw, "FADT PM2_CNT_BLK not
being used.");

                                return;

                }



                if ((fadt->pm2_cnt_blk != 0 &&
fadt->x_pm2_cnt_blk.address == 0) ||

                    (fadt->pm2_cnt_blk == 0 &&
fadt->x_pm2_cnt_blk.address != 0))

                                fwts_passed(fw,

                                                    "FADT only one of
the 32-bit or 64-bit "

                                                    "PM2_CNT_BLK fields
is being used.");

                else

                                fwts_failed(fw, LOG_LEVEL_MEDIUM,


"FADTPm2CntBlkOnlyOneField",

                                                    "FADT PM2_CNT_BLK
field has both the 32-bit "

                                                    "and the 64-bit
field set.");

                {

                if ((uint64_t)fadt->pm2_cnt_blk ==
fadt->x_pm2_cnt_blk.address) {

                                fwts_passed(fw,

                                                    "FADT 32- and 64-bit
PM2_CNT_BLK fields are "

                                                    "at least equal.");

                                fwts_advice(fw,

                                                    "Both FADT 32- and
64-bit PM2_CNT_BLK "

                                                    "fields are being
used, but only one should be "

                                                    "non-zero.  However,
they are at least equal so "

                                                    "the kernel will at
least have a usable value.");

                } else {

                                fwts_failed(fw, LOG_LEVEL_MEDIUM,

                                                    "FADTPm2CntBlkNotSet",

                                                    "FADT PM2_CNT_BLK is
a required field and must "

                                                    "have either a
32-bit or 64-bit address set.");

                                fwts_advice(fw,

                                                    "Both FADT 32- and
64-bit PM2_CNT_BLK "

                                                    "fields are being
used, but only one should be "

                                                    "non-zero.  Since
the fields value are not equal "

                                                    "the kernel cannot
unambiguously determine which "

                                                    "value is the
correct one.");

                }

}

}



I think the fix is: remove the red mark code and add green mark code.
what do you think?

I'm curious, can you describe the scenario or scenarios in which you feel the current code fails?

Having looked at the ACPI spec and the code you've identified in your mail, I don't yet see an issue with the current code that you've identified, and I actually think your "fix" is incorrect at this time.


Thanks,

Eric



--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

--
fwts-devel mailing list
fwts-devel at lists.ubuntu.com<mailto:fwts-devel at lists.ubuntu.com>
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel



--
Cheers,
Alex Hung
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/fwts-devel/attachments/20161208/a30f3e33/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fadt.zip
Type: application/x-zip-compressed
Size: 23859 bytes
Desc: fadt.zip
URL: <https://lists.ubuntu.com/archives/fwts-devel/attachments/20161208/a30f3e33/attachment-0001.bin>