Issues in fadt check item?

[Jeffrey Hugo]

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.