[PATCH 0/1] MdeModulePkg/BdsDxe: Fix calling PlatformBootManagerWaitCallback on 0

[PATCH 0/1] MdeModulePkg/BdsDxe: Fix calling PlatformBootManagerWaitCallback on 0

[Pete Batard]

Independently of the OvmfPkg fix, I think Lazlo's fix is what we ultimately
want to have, as the logic of not wanting to get calls for progress updates
if the Timeout Pcd is zet to zero stands. Plus we want to handle the 0xFFFF
case.

Therefore I am submitting Lazlo's proposed patch as well, which, even if it
seems redundant, I don't think invalidates the OvmfPkg one, as a call that
may produce a division by zero should consider that case unless it can have
absolute certainty that it will never be called for the zero value, which I
don't think we can or should rely on here.

Laszlo Ersek (1):
  MdeModulePkg/BdsDxe: Fix calling PlatformBootManagerWaitCallback on 0

 MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

-- 
2.21.0.windows.1

[PATCH 1/1] MdeModulePkg/BdsDxe: Fix calling PlatformBootManagerWaitCallback on 0

[Pete Batard]

From: Laszlo Ersek <lersek@redhat.com>

Commit 2de1f611be06ded3a59726a4052a9039be7d459b introduced a regression
whereas platforms that did set PcdPlatformBootTimeOut to 0 are now getting
an unexpected call to PlatformBootManagerWaitCallback().

This patch also ensures that, if PcdPlatformBootTimeOut is 0xFFFF we don't
call PlatformBootManagerWaitCallback() with a zero argument as doing so
would produce an unwarranted jump to full progress completion which is
likely to throw off users.

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 7968a58f3454..d6ec31118c1f 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -341,7 +341,17 @@ BdsWait (
       TimeoutRemain--;
     }
   }
-  PlatformBootManagerWaitCallback (0);
+
+  //
+  // If the platform configured a nonzero and finite time-out, and we have
+  // actually reached that, report 100% completion to the platform.
+  //
+  // Note that the (TimeoutRemain == 0) condition excludes
+  // PcdPlatformBootTimeOut=0xFFFF, and that's deliberate.
+  //
+  if (PcdGet16 (PcdPlatformBootTimeOut) != 0 && TimeoutRemain == 0) {
+    PlatformBootManagerWaitCallback (0);
+  }
   DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
 }
 
-- 
2.21.0.windows.1

Re: [PATCH 1/1] MdeModulePkg/BdsDxe: Fix calling PlatformBootManagerWaitCallback on 0

[Liming Gao]

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: Pete Batard [mailto:pete@akeo.ie]
>Sent: Friday, October 11, 2019 11:44 PM
>To: devel@edk2.groups.io
>Cc: afish@apple.com; lersek@redhat.com; Gao, Liming <liming.gao@intel.com>
>Subject: [PATCH 1/1] MdeModulePkg/BdsDxe: Fix calling
>PlatformBootManagerWaitCallback on 0
>
>From: Laszlo Ersek <lersek@redhat.com>
>
>Commit 2de1f611be06ded3a59726a4052a9039be7d459b introduced a
>regression
>whereas platforms that did set PcdPlatformBootTimeOut to 0 are now getting
>an unexpected call to PlatformBootManagerWaitCallback().
>
>This patch also ensures that, if PcdPlatformBootTimeOut is 0xFFFF we don't
>call PlatformBootManagerWaitCallback() with a zero argument as doing so
>would produce an unwarranted jump to full progress completion which is
>likely to throw off users.
>
>Signed-off-by: Pete Batard <pete@akeo.ie>
>---
> MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
>diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>index 7968a58f3454..d6ec31118c1f 100644
>--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>@@ -341,7 +341,17 @@ BdsWait (
>       TimeoutRemain--;
>     }
>   }
>-  PlatformBootManagerWaitCallback (0);
>+
>+  //
>+  // If the platform configured a nonzero and finite time-out, and we have
>+  // actually reached that, report 100% completion to the platform.
>+  //
>+  // Note that the (TimeoutRemain == 0) condition excludes
>+  // PcdPlatformBootTimeOut=0xFFFF, and that's deliberate.
>+  //
>+  if (PcdGet16 (PcdPlatformBootTimeOut) != 0 && TimeoutRemain == 0) {
>+    PlatformBootManagerWaitCallback (0);
>+  }
>   DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
> }
>
>--
>2.21.0.windows.1

Re: [PATCH 1/1] MdeModulePkg/BdsDxe: Fix calling PlatformBootManagerWaitCallback on 0

[Laszlo Ersek]

Hi Liming,

On 10/12/19 03:44, Gao, Liming wrote:
> Reviewed-by: Liming Gao <liming.gao@intel.com>

can you please add

  Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2266

to the commit message, before pushing this?

(I'm not giving R-b or A-b myself, because I shouldn't approve code
whose authorship is attributed to me. I obviously agree with the patch.)

Thanks all,
Laszlo

>> -----Original Message-----
>> From: Pete Batard [mailto:pete@akeo.ie]
>> Sent: Friday, October 11, 2019 11:44 PM
>> To: devel@edk2.groups.io
>> Cc: afish@apple.com; lersek@redhat.com; Gao, Liming <liming.gao@intel.com>
>> Subject: [PATCH 1/1] MdeModulePkg/BdsDxe: Fix calling
>> PlatformBootManagerWaitCallback on 0
>>
>> From: Laszlo Ersek <lersek@redhat.com>
>>
>> Commit 2de1f611be06ded3a59726a4052a9039be7d459b introduced a
>> regression
>> whereas platforms that did set PcdPlatformBootTimeOut to 0 are now getting
>> an unexpected call to PlatformBootManagerWaitCallback().
>>
>> This patch also ensures that, if PcdPlatformBootTimeOut is 0xFFFF we don't
>> call PlatformBootManagerWaitCallback() with a zero argument as doing so
>> would produce an unwarranted jump to full progress completion which is
>> likely to throw off users.
>>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
>> MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> index 7968a58f3454..d6ec31118c1f 100644
>> --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> @@ -341,7 +341,17 @@ BdsWait (
>>       TimeoutRemain--;
>>     }
>>   }
>> -  PlatformBootManagerWaitCallback (0);
>> +
>> +  //
>> +  // If the platform configured a nonzero and finite time-out, and we have
>> +  // actually reached that, report 100% completion to the platform.
>> +  //
>> +  // Note that the (TimeoutRemain == 0) condition excludes
>> +  // PcdPlatformBootTimeOut=0xFFFF, and that's deliberate.
>> +  //
>> +  if (PcdGet16 (PcdPlatformBootTimeOut) != 0 && TimeoutRemain == 0) {
>> +    PlatformBootManagerWaitCallback (0);
>> +  }
>>   DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
>> }
>>

Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/BdsDxe: Fix calling PlatformBootManagerWaitCallback on 0

[Liming Gao]

Update, and push @4d05a4b709ce52d4649698f887a1358246fa4437

>-----Original Message-----
>From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>Laszlo Ersek
>Sent: Monday, October 14, 2019 10:45 PM
>To: Gao, Liming <liming.gao@intel.com>; Pete Batard <pete@akeo.ie>
>Cc: devel@edk2.groups.io; afish@apple.com
>Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/BdsDxe: Fix calling
>PlatformBootManagerWaitCallback on 0
>
>Hi Liming,
>
>On 10/12/19 03:44, Gao, Liming wrote:
>> Reviewed-by: Liming Gao <liming.gao@intel.com>
>
>can you please add
>
>  Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2266
>
>to the commit message, before pushing this?
>
>(I'm not giving R-b or A-b myself, because I shouldn't approve code
>whose authorship is attributed to me. I obviously agree with the patch.)
>
>Thanks all,
>Laszlo
>
>>> -----Original Message-----
>>> From: Pete Batard [mailto:pete@akeo.ie]
>>> Sent: Friday, October 11, 2019 11:44 PM
>>> To: devel@edk2.groups.io
>>> Cc: afish@apple.com; lersek@redhat.com; Gao, Liming
><liming.gao@intel.com>
>>> Subject: [PATCH 1/1] MdeModulePkg/BdsDxe: Fix calling
>>> PlatformBootManagerWaitCallback on 0
>>>
>>> From: Laszlo Ersek <lersek@redhat.com>
>>>
>>> Commit 2de1f611be06ded3a59726a4052a9039be7d459b introduced a
>>> regression
>>> whereas platforms that did set PcdPlatformBootTimeOut to 0 are now
>getting
>>> an unexpected call to PlatformBootManagerWaitCallback().
>>>
>>> This patch also ensures that, if PcdPlatformBootTimeOut is 0xFFFF we don't
>>> call PlatformBootManagerWaitCallback() with a zero argument as doing so
>>> would produce an unwarranted jump to full progress completion which is
>>> likely to throw off users.
>>>
>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>> ---
>>> MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 12 +++++++++++-
>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>>> b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>>> index 7968a58f3454..d6ec31118c1f 100644
>>> --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>>> +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>>> @@ -341,7 +341,17 @@ BdsWait (
>>>       TimeoutRemain--;
>>>     }
>>>   }
>>> -  PlatformBootManagerWaitCallback (0);
>>> +
>>> +  //
>>> +  // If the platform configured a nonzero and finite time-out, and we have
>>> +  // actually reached that, report 100% completion to the platform.
>>> +  //
>>> +  // Note that the (TimeoutRemain == 0) condition excludes
>>> +  // PcdPlatformBootTimeOut=0xFFFF, and that's deliberate.
>>> +  //
>>> +  if (PcdGet16 (PcdPlatformBootTimeOut) != 0 && TimeoutRemain == 0) {
>>> +    PlatformBootManagerWaitCallback (0);
>>> +  }
>>>   DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
>>> }
>>>
>
>