Issues in fadt check item?
Alex Hung
alex.hung at canonical.com
Fri Dec 9 05:42:31 UTC 2016
FWTS source code is available @ git://kernel.ubuntu.com/hwe/fwts.git
On Thu, Dec 8, 2016 at 5:31 PM, Dong, Eric <eric.dong at intel.com> wrote:
> 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.
>
--
Cheers,
Alex Hung
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/fwts-devel/attachments/20161208/1e51098a/attachment-0001.html>
More information about the fwts-devel
mailing list