[prev in list] [next in list] [prev in thread] [next in thread]
List: fwts-devel
Subject: Re: Issues in fadt check item?
From: Alex Hung <alex.hung () canonical ! com>
Date: 2016-12-09 5:42:31
Message-ID: CAJ=jquZge3OSgSwGwxzgi1DcxJ6B3ERTeJFvmyn9BUppCgC3PQ () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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@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@codeaurora.org]
> > Sent: Friday, December 09, 2016 12:45 AM
> > To: Dong, Eric; Alex Hung
> > Cc: fwts-devel@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@canonical.com]
> > > *Sent:* Thursday, December 08, 2016 1:38 AM
> > > *To:* Jeffrey Hugo
> > > *Cc:* Dong, Eric; fwts-devel@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@codeaurora.org
> > > <mailto:jhugo@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@lists.ubuntu.com <mailto:fwts-devel@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
[Attachment #5 (text/html)]
<div dir="ltr"><div class="gmail_default" style="font-family:verdana,sans-serif">FWTS \
source code is available @ git://<a \
href="http://kernel.ubuntu.com/hwe/fwts.git">kernel.ubuntu.com/hwe/fwts.git</a></div></div><div \
class="gmail_extra"><br><div class="gmail_quote">On Thu, Dec 8, 2016 at 5:31 PM, \
Dong, Eric <span dir="ltr"><<a href="mailto:eric.dong@intel.com" \
target="_blank">eric.dong@intel.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">Hi Jeffrey,<br> <br>
I agree with your point. My change is not correct for the 6.1 spec.<br>
<br>
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.<br> <br>
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.<br> <br>
Thanks,<br>
Eric<br>
<div class="HOEnZb"><div class="h5"><br>
> -----Original Message-----<br>
> From: Jeffrey Hugo [mailto:<a \
href="mailto:jhugo@codeaurora.org">jhugo@codeaurora.org</a>]<br> > Sent: Friday, \
December 09, 2016 12:45 AM<br> > To: Dong, Eric; Alex Hung<br>
> Cc: <a href="mailto:fwts-devel@lists.ubuntu.com">fwts-devel@lists.ubuntu.com</a><br>
> Subject: Re: Issues in fadt check item?<br>
><br>
> Yes, that is new language with 6.1, but it was added as a clarification.<br>
> The original language indicated the X_ (extended) fields superseded<br>
> the original fields. This is very common with the ACPI spec. A lot of<br>
> the original functionality, particularly in FADT, is 32-bit specific and<br>
> provides no mechanism to support 64-bit platforms. In-order to support<br>
> 64-bit, new fields were added to the spec, with the intent that the new<br>
> fields would replace the old fields on applicable targets, while the old<br>
> fields would remain for backwards compatibility.<br>
><br>
> This is aligned with the dictionary definition of superseded - "take \
the<br> > place of (a person or thing previously in authority or use); \
supplant."<br> ><br>
> Apparently the original language was not clear enough to indicate only<br>
> one set of fields should be used (how do you wedge a 64-bit value in a<br>
> 32-bit field?) so it was clarified in 6.1.<br>
><br>
> So while the language in 6.1 is new, it still says the same thing in<br>
> regards to the use and intent of the fields.<br>
><br>
> Concerning your proposed changes -<br>
><br>
> FYI a patch file from "git format-patch" tends to be the most \
convenient<br> > way to review code changes. You can use "git \
send-email" to send that<br> > directly to the list without having to figure \
out attachments, etc.<br> ><br>
> I'm not in favor of the proposed code change. The FADT test could be<br>
> improved, but I don't think your current proposal is the right way to do<br>
> so. You haven't stated explicitly, but I infer you are trying to get<br>
> the test to pass on some FADT table that provides a 32-bit address in<br>
> both fields. While that was never intended, it could be argued it could<br>
> be valid in pre-6.1 specs.<br>
><br>
> The problem I see with your code change is that it removes the<br>
> conditions set by the 6.1 version of the spec. As Alex stated, new<br>
> platforms should really conform to the latest spec version if possible,<br>
> but I see some value in using FWTS to test older platform for long term<br>
> support, etc.<br>
><br>
> If you want to improve the test, my recommendation would be to have it<br>
> look at the ACPI spec version in the FADT table, and apply the correct<br>
> tests based on that. IE platforms advertising compliance with 6.0 or<br>
> earlier are allowed to specify both values, so long as the values match,<br>
> and advice is printed indicating that is not recommend. Platforms<br>
> advertising compliance with 6.1 or newer will fail if they use both fields.<br>
><br>
> If you go about proposing such changes, I would prefer to see updates to<br>
> all applicable tests, and not just the pm_cnt_blk test.<br>
><br>
> On 12/7/2016 5:44 PM, Dong, Eric wrote:<br>
> > Hi Alex & Jeffrey,<br>
> ><br>
> ><br>
> ><br>
> > Thanks for your comments, I don't aware my mark solution not work in<br>
> > this mail list. Attach my change code for you to reference. I think the<br>
> > old code logic not consistent. PS: 12345678<br>
> ><br>
> ><br>
> ><br>
> > I missed the latest 6.1 spec, I just check the 6.0 spec and not found<br>
> > this sentence. "If the X_PM1b_CNT_BLK field contains a non-zero \
value<br> > > then this field must be zero." This is new added for 6.1 \
spec.<br> > ><br>
> ><br>
> ><br>
> > Thanks,<br>
> ><br>
> > Eric<br>
> ><br>
> > *From:*Alex Hung [mailto:<a \
href="mailto:alex.hung@canonical.com">alex.hung@canonical.<wbr>com</a>]<br> > > \
*Sent:* Thursday, December 08, 2016 1:38 AM<br> > > *To:* Jeffrey Hugo<br>
> > *Cc:* Dong, Eric; <a \
href="mailto:fwts-devel@lists.ubuntu.com">fwts-devel@lists.ubuntu.com</a><br> > \
> *Subject:* Re: Issues in fadt check item?<br> > ><br>
> ><br>
> ><br>
> > I agree with Jeffrey that's actually not a fix: ACPI 6.1 explicitly<br>
> > states PM1b_CNT_BLK to meet "If the X_PM1b_CNT_BLK field contains \
a<br> > > non-zero value then this field must be zero." The same also \
applies to<br> > > PM2_CNT_BLK.<br>
> ><br>
> > Many systems are shipped before ACPI 6.1's release, and this failure \
is<br> > > odd for them. I think it is a case this error can be ignored. \
However,<br> > > you should try to meet ACPI 6.1 spec if a target system is \
being developed.<br> > ><br>
> ><br>
> ><br>
> > On Wed, Dec 7, 2016 at 7:36 AM, Jeffrey Hugo <<a \
href="mailto:jhugo@codeaurora.org">jhugo@codeaurora.org</a><br> > > \
<mailto:<a href="mailto:jhugo@codeaurora.org">jhugo@codeaurora.org</a>>> \
wrote:<br> > ><br>
> > Hi Eric<br>
> ><br>
> ><br>
> ><br>
> > On 12/7/2016 5:51 AM, Dong, Eric wrote:<br>
> ><br>
> > Hi,<br>
> ><br>
> ><br>
> ><br>
> > When I use fwts to check uefi bios code, it reports some issues related<br>
> > to fadt check item. After check the uefi code and acpi spec, I didn't<br>
> > find error for uefi code. So I download fwts code (fwts_16.11.00) and i<br>
> > think some bugs in the fadt check item.<br>
> ><br>
> > One issue like below:<br>
> ><br>
> > static void acpi_table_check_fadt_pm2_cnt_<wbr>blk(fwts_framework *fw)<br>
> ><br>
> > {<br>
> ><br>
> > if (fadt->pm2_cnt_blk == 0 && \
fadt->header.length < 208) {<br> > ><br>
> > fwts_skipped(fw, \
"FADT PM2_CNT_BLK not<br> > > being used.");<br>
> ><br>
> > return;<br>
> ><br>
> > }<br>
> ><br>
> ><br>
> ><br>
> > if (fadt->pm2_cnt_blk == 0 &&<br>
> > fadt->x_pm2_cnt_blk.address == 0) {<br>
> ><br>
> > fwts_skipped(fw, \
"FADT PM2_CNT_BLK not<br> > > being used.");<br>
> ><br>
> > return;<br>
> ><br>
> > }<br>
> ><br>
> ><br>
> ><br>
> > if ((fadt->pm2_cnt_blk != 0 &&<br>
> > fadt->x_pm2_cnt_blk.address == 0) ||<br>
> ><br>
> > (fadt->pm2_cnt_blk == 0 &&<br>
> > fadt->x_pm2_cnt_blk.address != 0))<br>
> ><br>
> > fwts_passed(fw,<br>
> ><br>
> > \
"FADT only one of<br> > > the 32-bit or 64-bit "<br>
> ><br>
> > \
"PM2_CNT_BLK fields<br> > > is being used.");<br>
> ><br>
> > else<br>
> ><br>
> > fwts_failed(fw, \
LOG_LEVEL_MEDIUM,<br> > ><br>
> ><br>
> > "FADTPm2CntBlkOnlyOneField",<br>
> ><br>
> > \
"FADT PM2_CNT_BLK<br> > > field has both the 32-bit "<br>
> ><br>
> > \
"and the 64-bit<br> > > field set.");<br>
> ><br>
> > {<br>
> ><br>
> > if ((uint64_t)fadt->pm2_cnt_blk ==<br>
> > fadt->x_pm2_cnt_blk.address) {<br>
> ><br>
> > fwts_passed(fw,<br>
> ><br>
> > \
"FADT 32- and 64-bit<br> > > PM2_CNT_BLK fields are "<br>
> ><br>
> > \
"at least equal.");<br> > ><br>
> > fwts_advice(fw,<br>
> ><br>
> > \
"Both FADT 32- and<br> > > 64-bit PM2_CNT_BLK "<br>
> ><br>
> > \
"fields are being<br> > > used, but only one should be "<br>
> ><br>
> > \
"non-zero. However,<br> > > they are at least equal so "<br>
> ><br>
> > \
"the kernel will at<br> > > least have a usable value.");<br>
> ><br>
> > } else {<br>
> ><br>
> > fwts_failed(fw, \
LOG_LEVEL_MEDIUM,<br> > ><br>
> > \
"FADTPm2CntBlkNotSet",<br> > ><br>
> > \
"FADT PM2_CNT_BLK is<br> > > a required field and must "<br>
> ><br>
> > \
"have either a<br> > > 32-bit or 64-bit address set.");<br>
> ><br>
> > fwts_advice(fw,<br>
> ><br>
> > \
"Both FADT 32- and<br> > > 64-bit PM2_CNT_BLK "<br>
> ><br>
> > \
"fields are being<br> > > used, but only one should be "<br>
> ><br>
> > \
"non-zero. Since<br> > > the fields value are not equal "<br>
> ><br>
> > \
"the kernel cannot<br> > > unambiguously determine which "<br>
> ><br>
> > \
"value is the<br> > > correct one.");<br>
> ><br>
> > }<br>
> ><br>
> > }<br>
> ><br>
> > }<br>
> ><br>
> ><br>
> ><br>
> > I think the fix is: remove the red mark code and add green mark code.<br>
> > what do you think?<br>
> ><br>
> ><br>
> ><br>
> > I'm curious, can you describe the scenario or scenarios in which \
you<br> > > feel the current code fails?<br>
> ><br>
> > Having looked at the ACPI spec and the code you've identified in \
your<br> > > mail, I don't yet see an issue with the current code that \
you've<br> > > identified, and I actually think your "fix" is \
incorrect at this time.<br> > ><br>
> ><br>
> ><br>
> > Thanks,<br>
> ><br>
> > Eric<br>
> ><br>
> ><br>
> ><br>
> > --<br>
> > Jeffrey Hugo<br>
> > Qualcomm Datacenter Technologies as an affiliate of Qualcomm<br>
> > Technologies, Inc.<br>
> > Qualcomm Technologies, Inc. is a member of the<br>
> > Code Aurora Forum, a Linux Foundation Collaborative Project.<br>
> ><br>
> > --<br>
> > fwts-devel mailing list<br>
> > <a href="mailto:fwts-devel@lists.ubuntu.com">fwts-devel@lists.ubuntu.com</a> \
<mailto:<a href="mailto:fwts-devel@lists.ubuntu.com">fwts-devel@lists.<wbr>ubuntu.com</a>><br>
> > Modify settings or unsubscribe at:<br>
> > <a href="https://lists.ubuntu.com/mailman/listinfo/fwts-devel" \
rel="noreferrer" target="_blank">https://lists.ubuntu.com/<wbr>mailman/listinfo/fwts-devel</a><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > --<br>
> ><br>
> > Cheers,<br>
> > Alex Hung<br>
> ><br>
><br>
><br>
> --<br>
> Jeffrey Hugo<br>
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm<br>
> Technologies, Inc.<br>
> Qualcomm Technologies, Inc. is a member of the<br>
> Code Aurora Forum, a Linux Foundation Collaborative Project.<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div \
class="gmail_signature" data-smartmail="gmail_signature"><div \
dir="ltr">Cheers,<br>Alex Hung<br></div></div> </div>
--
fwts-devel mailing list
fwts-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic