Issues in fadt check item?
Dong, Eric
eric.dong at intel.com
Fri Dec 9 01:31:36 UTC 2016
Hi Jeffrey,
I agree with your point. My change is not correct for the 6.1 spec.
My proposal just based on the current code implementation, I think it's an obvious bug. It reports failure at first when both addresses set. Then it checks whether both addresses are same, if so, it reports pass again. The result is not consistent, so I think this is an obvious bug.
I can't find the git repository for the fwts code, so I use old/new format. Can you help to forward the link to me so I can propose new change for it.
Thanks,
Eric
> -----Original Message-----
> From: Jeffrey Hugo [mailto:jhugo at codeaurora.org]
> Sent: Friday, December 09, 2016 12:45 AM
> To: Dong, Eric; Alex Hung
> Cc: fwts-devel at lists.ubuntu.com
> Subject: Re: Issues in fadt check item?
>
> Yes, that is new language with 6.1, but it was added as a clarification.
> The original language indicated the X_ (extended) fields superseded
> the original fields. This is very common with the ACPI spec. A lot of
> the original functionality, particularly in FADT, is 32-bit specific and
> provides no mechanism to support 64-bit platforms. In-order to support
> 64-bit, new fields were added to the spec, with the intent that the new
> fields would replace the old fields on applicable targets, while the old
> fields would remain for backwards compatibility.
>
> This is aligned with the dictionary definition of superseded - "take the
> place of (a person or thing previously in authority or use); supplant."
>
> Apparently the original language was not clear enough to indicate only
> one set of fields should be used (how do you wedge a 64-bit value in a
> 32-bit field?) so it was clarified in 6.1.
>
> So while the language in 6.1 is new, it still says the same thing in
> regards to the use and intent of the fields.
>
> Concerning your proposed changes -
>
> FYI a patch file from "git format-patch" tends to be the most convenient
> way to review code changes. You can use "git send-email" to send that
> directly to the list without having to figure out attachments, etc.
>
> I'm not in favor of the proposed code change. The FADT test could be
> improved, but I don't think your current proposal is the right way to do
> so. You haven't stated explicitly, but I infer you are trying to get
> the test to pass on some FADT table that provides a 32-bit address in
> both fields. While that was never intended, it could be argued it could
> be valid in pre-6.1 specs.
>
> The problem I see with your code change is that it removes the
> conditions set by the 6.1 version of the spec. As Alex stated, new
> platforms should really conform to the latest spec version if possible,
> but I see some value in using FWTS to test older platform for long term
> support, etc.
>
> If you want to improve the test, my recommendation would be to have it
> look at the ACPI spec version in the FADT table, and apply the correct
> tests based on that. IE platforms advertising compliance with 6.0 or
> earlier are allowed to specify both values, so long as the values match,
> and advice is printed indicating that is not recommend. Platforms
> advertising compliance with 6.1 or newer will fail if they use both fields.
>
> If you go about proposing such changes, I would prefer to see updates to
> all applicable tests, and not just the pm_cnt_blk test.
>
> On 12/7/2016 5:44 PM, Dong, Eric wrote:
> > 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
> >
>
>
> --
> 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.
More information about the fwts-devel
mailing list