public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ni, Ruiyu" <ruiyu.ni@Intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
	"Song, BinX" <binx.song@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Dong, Eric" <eric.dong@intel.com>
Subject: Re: [PATCH] UefiCpuPkg: Enhance CPU feature dependency check
Date: Thu, 1 Feb 2018 13:10:45 +0800	[thread overview]
Message-ID: <a7383cff-e42a-a9a5-83f8-2f057a5a4291@Intel.com> (raw)
In-Reply-To: <9ef96ab1-d50b-5f1a-14ff-43b07562a975@redhat.com>

On 1/31/2018 5:44 PM, Laszlo Ersek wrote:
> On 01/31/18 08:00, Song, BinX wrote:
>> Current CPU feature dependency check will hang on when meet below or
>> similar case:
>> if (IsCpuFeatureSupported (CPU_FEATURE_AESNI)) {
>>    Status = RegisterCpuFeature (
>>               "AESNI",
>>               AesniGetConfigData,
>>               AesniSupport,
>>               AesniInitialize,
>>               CPU_FEATURE_AESNI,
>>               CPU_FEATURE_MWAIT | CPU_FEATURE_BEFORE,
>>               CPU_FEATURE_END
>>               );
>>    ASSERT_EFI_ERROR (Status);
>> }
>> if (IsCpuFeatureSupported (CPU_FEATURE_MWAIT)) {
>>    Status = RegisterCpuFeature (
>>               "MWAIT",
>>               NULL,
>>               MonitorMwaitSupport,
>>               MonitorMwaitInitialize,
>>               CPU_FEATURE_MWAIT,
>>               CPU_FEATURE_AESNI | CPU_FEATURE_BEFORE,
>>               CPU_FEATURE_END
>>               );
>>    ASSERT_EFI_ERROR (Status);
>> }
>>
>> Solution is to separate current CPU feature dependency check into
>> sort and check two parts.
>>
>> Sort function:
>> According to CPU feature's dependency, sort all CPU features.
>> Later dependency will override previous dependency if they are conflicted.
>>
>> Check function:
>> Check sorted CPU features' relationship, ASSERT invalid relationship.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Bell Song <binx.song@intel.com>
>> ---
>>   .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 271 ++++++++++++++++++++-
>>   .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |   7 +
>>   .../RegisterCpuFeaturesLib.c                       | 130 +---------
>>   3 files changed, 278 insertions(+), 130 deletions(-)
>>
>> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
>> index 4d75c07..2fd0d5f 100644
>> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
>> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
>> @@ -423,6 +423,271 @@ DumpRegisterTableOnProcessor (
>>   }
>>   
>>   /**
>> +  From FeatureBitMask, find the right feature entry in CPU feature list.
>> +
>> +  @param[in]  FeatureList    The pointer to CPU feature list.
>> +  @param[in]  CurrentFeature The pointer to current CPU feature.
>> +  @param[in]  BeforeFlag     TRUE: BeforeFeatureBitMask; FALSE: AfterFeatureBitMask.
>> +
>> +  @return  The pointer to right CPU feature entry.
>> +**/
>> +LIST_ENTRY *
>> +FindFeatureInList(
>> +  IN LIST_ENTRY              *CpuFeatureList,
>> +  IN CPU_FEATURES_ENTRY      *CurrentCpuFeature,
>> +  IN BOOLEAN                  BeforeFlag
>> +  )
>> +{
>> +  LIST_ENTRY                 *TempEntry;
>> +  CPU_FEATURES_ENTRY         *TempFeature;
>> +  UINT8                      *FeatureBitMask;
>> +
>> +  FeatureBitMask = BeforeFlag ? CurrentCpuFeature->BeforeFeatureBitMask : CurrentCpuFeature->AfterFeatureBitMask;
>> +  TempEntry = GetFirstNode (CpuFeatureList);
>> +  while (!IsNull (CpuFeatureList, TempEntry)) {
>> +    TempFeature = CPU_FEATURE_ENTRY_FROM_LINK (TempEntry);
>> +    if (IsBitMaskMatchCheck (FeatureBitMask, TempFeature->FeatureMask)){
>> +      return TempEntry;
>> +    }
>> +    TempEntry = TempEntry->ForwardLink;
>> +  }
>> +
>> +  DEBUG ((DEBUG_ERROR, "Error: Feature %a ", CurrentCpuFeature->FeatureName, BeforeFlag ? "before ":"after ", "condition is invalid!\n"));
> 
> Hi, I skimmed this patch quickly -- I can tell that I can't really tell
> what's going on. I don't know how the feature dependencies are defined
> in the first place, and what the bug is.
> 
> However, I do see that the above DEBUG macro invocation is incorrect.
> The format string has one (1) %a conversion specification, but we pass
> three (3) arguments.
> 
> I think the last argument ("condition is invalid!\n") should actually be
> part of the format string. And then, the "before"/"after" string has to
> be printed somehow as well.
> 
> Another superficial observation below:
> 
>> +/**
>> +  Check sorted CPU features' relationship, ASSERT invalid one.
>> +
>> +  @param[in]  FeatureList  The pointer to CPU feature list.
>> +**/
>> +VOID
>> +CheckCpuFeaturesRelationShip (
> 
> I don't think we should capitalize "Ship" in this identifier.
> 
> Third comment: there are several ways to define "sorting", so I'm not
> sure my question applies, but: can we replace the manual sorting with
> SortLib?

Laszlo,
I haven't checked the patch in details.
But regarding to the SortLib suggestion, the feature entry is chained in
linked list, while SortLib can only perform sorting in array.

Bin,
Can we have a simpler fix to this issue?
If my understanding is correct, the patch tries to fix the infinite loop
in code.
If that's true, can we just firstly calculate how many loops are
expected before looping, then exit when the maximum loop is met?
Upon that, when the sort hasn't been finished, a wrong dependency
exists.


> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

-- 
Thanks,
Ray


  parent reply	other threads:[~2018-02-01  5:05 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-31  7:00 [PATCH] UefiCpuPkg: Enhance CPU feature dependency check Song, BinX
2018-01-31  7:41 ` Song, BinX
2018-01-31  9:44 ` Laszlo Ersek
2018-02-01  2:09   ` Song, BinX
2018-02-01 13:15     ` Laszlo Ersek
2018-02-02  1:34       ` Song, BinX
2018-02-01  5:10   ` Ni, Ruiyu [this message]
2018-02-01 13:25     ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a7383cff-e42a-a9a5-83f8-2f057a5a4291@Intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox