Re: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move FindVariable after AutoUpdateLangVariable

["Guomin Jiang" <guomin.jiang@intel.com>]

Hi Ming,

Can you try the newest edk2?

Thanks
Guomin
> -----Original Message-----
> From: Huangming (Mark) <huangming23@huawei.com>
> Sent: Wednesday, July 22, 2020 2:44 PM
> To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; Wang,
> Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Gao,
> Liming <liming.gao@intel.com>
> Cc: Lidongzhan <lidongzhan@huawei.com>; Songdongkuang
> <songdongkuang@huawei.com>; wanghuiqiang
> <wanghuiqiang@huawei.com>; qiuliangen <qiuliangen@huawei.com>;
> Shenlimei <shenlimei@huawei.com>; xiewenyi (A)
> <xiewenyi2@huawei.com>; John Garry <john.garry@huawei.com>
> Subject: 答复: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
> Move FindVariable after AutoUpdateLangVariable
> 
> Hi GuoMin,
> 
> I don't think the new change can fix this issue(id=2667). We use older edk2
> version which don't have SynchronizeRuntimeVariableCache().
> 
> Thanks
> Ming
> 
> -----邮件原件-----
> 发件人: Jiang, Guomin [mailto:guomin.jiang@intel.com]
> 发送时间: 2020年7月22日 8:47
> 收件人: Huangming (Mark) <huangming23@huawei.com>;
> devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Gao, Liming <liming.gao@intel.com>
> 抄送: Lidongzhan <lidongzhan@huawei.com>; Songdongkuang
> <songdongkuang@huawei.com>; wanghuiqiang
> <wanghuiqiang@huawei.com>; qiuliangen <qiuliangen@huawei.com>;
> Shenlimei <shenlimei@huawei.com>; xiewenyi (A)
> <xiewenyi2@huawei.com>; John Garry <john.garry@huawei.com>
> 主题: RE: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move
> FindVariable after AutoUpdateLangVariable
> 
> Hi Ming,
> 
> The new posted change
> https://edk2.groups.io/g/devel/topic/75412007#62327 may be helpful for
> this issue.
> 
> Can you add the change in your code and verify it?
> 
> Thanks
> Guomin
> > -----Original Message-----
> > From: Ming Huang <huangming23@huawei.com>
> > Sent: Friday, July 3, 2020 8:49 PM
> > To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io;
> > Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> > Gao, Liming <liming.gao@intel.com>
> > Cc: lidongzhan@huawei.com; songdongkuang@huawei.com;
> > wanghuiqiang@huawei.com; qiuliangen@huawei.com;
> shenlimei@huawei.com;
> > xiewenyi2@huawei.com; john.garry@huawei.com
> > Subject: Re: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
> > Move FindVariable after AutoUpdateLangVariable
> >
> >
> >
> > 在 2020/7/1 8:22, Jiang, Guomin 写道:
> > > So I think the key point is why AutoUpdateLangVariable() return
> > > success
> > rather than fail, if is it reasonable for this case or we need other
> > error handing?
> >
> > I don't think AutoUpdateLangVariable() should return fail while occur
> > reclaim internal in AutoUpdateLangVariable () function. The problem is
> > the
> > Variable(VARIABLE_POINTER_TRACK) get by FindVariable is invald in this
> > situation and this Variable will be pass to UpdateVariable().
> >
> >   if (mVariableModuleGlobal->VariableGlobal.AuthSupport) {
> >     Status = AuthVariableLibProcessVariable (VariableName, VendorGuid,
> > Data, DataSize, Attributes);
> >   } else {
> > // This Variable is invald while occur reclaim internal in
> > AutoUpdateLangVariable ()
> >     Status = UpdateVariable (VariableName, VendorGuid, Data, DataSize,
> > Attributes, 0, 0, &Variable, NULL);
> >   }
> >
> > >
> > > I am glad to help you but I can't reproduce it until now, can you
> > > provide a
> > step to reproduce it in simulation platform.
> >
> > I am not familiar with simulation platform. We reproduct this issue in
> > our board once.
> > For accelerating reproduction this issue, Add Reclaim() to
> > AutoUpdateLangVariable() for test.
> >
> > Thanks,
> > Ming
> >
> > >
> > > If it is urgent, I suggest that discuss with your internal team
> > > first and explain
> > that we need consider the risk check it into edk2.
> > >
> > > Best Regards
> > > Guomin
> > >
> > >> -----Original Message-----
> > >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Ming
> > >> Huang via groups.io
> > >> Sent: Tuesday, June 30, 2020 8:26 PM
> > >> To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io;
> > >> Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > >> <hao.a.wu@intel.com>; Gao, Liming <liming.gao@intel.com>
> > >> Cc: lidongzhan@huawei.com; songdongkuang@huawei.com;
> > >> wanghuiqiang@huawei.com; qiuliangen@huawei.com;
> > shenlimei@huawei.com;
> > >> xiewenyi2@huawei.com
> > >> Subject: Re: [edk2-devel] [PATCH edk2 v1 1/1]
> MdeModulePkg/Variable:
> > >> Move FindVariable after AutoUpdateLangVariable
> > >>
> > >>
> > >>
> > >> 在 2020/6/30 8:58, Jiang, Guomin 写道:
> > >>> Hi Huang,
> > >>>
> > >>> >From issue statement, I guess that
> > >>> 1. AutoUpdateLangVariable() invoked, and it will invoke
> > >>> FindVariable() first, at the same time, reclaim occur and
> > >>> Variable.CurrPtr is invalid, it return
> > >> with success 2. UpdateVariable() is invoked when The old Lang's
> > >> State is valid and the new Lang's State is also valid.
> > >>> 3. In the situation, FindVariable() checked Lang's State and only
> > >>> enable one
> > >> Lang's State. But it didn't in fact.
> > >>> 4. BmForEachVariable() deadloop in the situation.
> > >>>
> > >>> Am I right?
> > >>
> > >> Yes, right.
> > >>
> > >> Thanks,
> > >> Ming
> > >>
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Ming
> > >>>> Huang via groups.io
> > >>>> Sent: Monday, June 29, 2020 2:06 PM
> > >>>> To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>;
> > >>>> Wu, Hao A <hao.a.wu@intel.com>; Gao, Liming
> > >>>> <liming.gao@intel.com>
> > >>>> Cc: lidongzhan@huawei.com; huangming23@huawei.com;
> > >>>> songdongkuang@huawei.com; wanghuiqiang@huawei.com;
> > >>>> qiuliangen@huawei.com; shenlimei@huawei.com;
> > >> xiewenyi2@huawei.com
> > >>>> Subject: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
> > >> Move
> > >>>> FindVariable after AutoUpdateLangVariable
> > >>>>
> > >>>> When occur reclaim in AutoUpdateLangVariable(), the CurrPtr of
> > >>>> Variable is invalid. The State will be update with wrong position
> > >>>> after UpdateVariable in this situation and two valid PlatformLang
> > >>>> or Lang
> > >> variables will exist.
> > >>>> BmForEachVariable() will enter endless loop while exist two valid
> > >>>> PlatformLang variables. So FindVariable() should be invoked atfer
> > >>>> AutoUpdateLangVariable().
> > >>>>
> > >>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2667
> > >>>>
> > >>>> Signed-off-by: Ming Huang <huangming23@huawei.com>
> > >>>> ---
> > >>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 26
> > >>>> ++++++++++----------
> > >>>>  1 file changed, 13 insertions(+), 13 deletions(-)
> > >>>>
> > >>>> diff --git
> > >>>> a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > >>>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > >>>> index 1e71fc6..0cec981 100644
> > >>>> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > >>>> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > >>>> @@ -2741,6 +2741,19 @@ VariableServiceSetVariable (
> > >>>>      mVariableModuleGlobal->NonVolatileLastVariableOffset =
> > >>>> (UINTN) NextVariable - (UINTN) Point;
> > >>>>    }
> > >>>>
> > >>>> +  if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
> > >>>> +    //
> > >>>> +    // Hook the operation of setting
> > >>>> + PlatformLangCodes/PlatformLang and
> > >>>> LangCodes/Lang.
> > >>>> +    //
> > >>>> +    Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
> > >>>> +    if (EFI_ERROR (Status)) {
> > >>>> +      //
> > >>>> +      // The auto update operation failed, directly return to
> > >>>> + avoid
> > >>>> inconsistency between PlatformLang and Lang.
> > >>>> +      //
> > >>>> +      goto Done;
> > >>>> +    }
> > >>>> +  }
> > >>>> +
> > >>>>    //
> > >>>>    // Check whether the input variable is already existed.
> > >>>>    //
> > >>>> @@ -2763,19 +2776,6 @@ VariableServiceSetVariable (
> > >>>>      }
> > >>>>    }
> > >>>>
> > >>>> -  if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
> > >>>> -    //
> > >>>> -    // Hook the operation of setting PlatformLangCodes/PlatformLang
> > and
> > >>>> LangCodes/Lang.
> > >>>> -    //
> > >>>> -    Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
> > >>>> -    if (EFI_ERROR (Status)) {
> > >>>> -      //
> > >>>> -      // The auto update operation failed, directly return to avoid
> > >> inconsistency
> > >>>> between PlatformLang and Lang.
> > >>>> -      //
> > >>>> -      goto Done;
> > >>>> -    }
> > >>>> -  }
> > >>>> -
> > >>>>    if (mVariableModuleGlobal->VariableGlobal.AuthSupport) {
> > >>>>      Status = AuthVariableLibProcessVariable (VariableName,
> > >>>> VendorGuid, Data, DataSize, Attributes);
> > >>>>    } else {
> > >>>> --
> > >>>> 2.8.1
> > >>>>
> > >>>>
> > >>>>
> > >>>
> > >>>
> > >>>
> > >>
> > >>
> > >> 
> > >
> > >
> > > .
> > >