[MdeModulePkg/Library v1 0/1] Fix exception issue while UsbMass block io uninstalled

[MdeModulePkg/Library v1 0/1] Fix exception issue while UsbMass block io uninstalled

[Ming Huang]

Another exception issue while UsbMass block io uninstalled when boot to grub:
The system environment: virtual-CDROM(USB interface) via BMC, insert a
iso file to virtual-CDROM, like ubuntu-18.04.1-server-arm64.iso, change
virtaul-CDROM to first boot option.

Disconnecting virtual-CDROM when boot to grub menu
"Install Ubuntu Server"
then select "Install Ubuntu Server", system will also get exception.

The root cause is the EFI_BLOCK_IO_PROTOCOL for USBMass will be uninstalled
in this situation after print some transfer error(see blow), but grub will
still use the block io which had initialized by grub_efidisk_init() in efidisk.c.
When run m->io_align in grub_efidisk_open ():
  if (m->io_align & (m->io_align - 1))
grub will get exception for the EFI_BLOCK_IO_PROTOCOL had uninstalled and
the memory had set to 0xAF by PcdDebugClearMemoryValue.

This exception look like the matching problem grub and uefi. Is it need to
do something in uefi side or grub side?

The open source grub grub_efidisk_open function chunk:
  m = d->block_io->media;
  /* FIXME: Probably it is better to store the block size in the disk,
     and total sectors should be replaced with total blocks.  */
  grub_dprintf ("efidisk",
                "m = %p, last block = %llx, block size = %x, io align = %x\n",
                m, (unsigned long long) m->last_block, m->block_size,
                m->io_align);

  /* Ensure required buffer alignment is a power of two (or is zero). */
  if (m->io_align & (m->io_align - 1))

USB transfer error log:
UsbBootExecCmd: Device Error to Exec 0x0 Cmd (Result = 1)
EhcExecTransfer: transfer failed with 40
EhcBulkTransfer: error - Device Error, transfer - 40
UsbBotExecCommand: UsbBotSendCommand (Device Error)
UsbBootRequestSense: (Device Error) CmdResult=0x1
UsbBootDetectMedia: UsbBootIsUnitReady (Device Error)
-----------------------------------------------------------

Ming Huang (1):
  MdeModulePkg/UefiBootManangerLib: Fix exception issue

 MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.9.5

[MdeModulePkg/Library v1 1/1] MdeModulePkg/UefiBootManangerLib: Fix exception issue

[Ming Huang]

The system environment: virtual-CDROM(USB interface) via BMC, insert a
iso file to CDROM, like ubuntu-18.04.1-server-arm64.iso, change CDROM
to first boot option.
With release version bios, disconnecting CDROM when boot to
"1 seconds left, Press Esc or F2 to enter Setup"
then system will get a exception.

The root cause is the EFI_BLOCK_IO_PROTOCOL for UsbMass will be uninstalled
in this situation after print some transfer error. The status will be
invalid parameter. This line will get a exception for BlockIo not point
to right address:
AllocatePool (BlockIo->Media->BlockSize)
So, here need to judge the status not using ASSERT_EFI_ERROR.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ming Huang <ming.huang@linaro.org>
---
 MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index d5957db610d9..c2f1c651b02f 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -1068,7 +1068,9 @@ BmExpandMediaDevicePath (
   // Block IO read/write will success.
   //
   Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID **) &BlockIo);
-  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    return NULL;
+  }
   Buffer = AllocatePool (BlockIo->Media->BlockSize);
   if (Buffer != NULL) {
     BlockIo->ReadBlocks (
-- 
2.9.5

Re: [MdeModulePkg/Library v1 1/1] MdeModulePkg/UefiBootManangerLib: Fix exception issue

[Leif Lindholm]

+MdeModulePkg maintainers (you added MdePkg maintainers to cc)

This looks like an improvement to me.

Am I correct in guessing this behaviour refers to some specific corner
case of a USB CDROM emulated from a BMC?

On Mon, Feb 25, 2019 at 05:10:52PM +0800, Ming Huang wrote:
> The system environment: virtual-CDROM(USB interface) via BMC, insert a
> iso file to CDROM, like ubuntu-18.04.1-server-arm64.iso, change CDROM
> to first boot option.
> With release version bios, disconnecting CDROM when boot to
> "1 seconds left, Press Esc or F2 to enter Setup"
> then system will get a exception.
> 
> The root cause is the EFI_BLOCK_IO_PROTOCOL for UsbMass will be uninstalled
> in this situation after print some transfer error. The status will be
> invalid parameter. This line will get a exception for BlockIo not point
> to right address:
> AllocatePool (BlockIo->Media->BlockSize)
> So, here need to judge the status not using ASSERT_EFI_ERROR.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ming Huang <ming.huang@linaro.org>
> ---
>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> index d5957db610d9..c2f1c651b02f 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> @@ -1068,7 +1068,9 @@ BmExpandMediaDevicePath (
>    // Block IO read/write will success.
>    //
>    Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID **) &BlockIo);
> -  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {

It would still be worth including an ASSERT here, to let DEBUG builds
report on point of failure rather than several steps up the chain.

/
    Leif

> +    return NULL;
> +  }
>    Buffer = AllocatePool (BlockIo->Media->BlockSize);
>    if (Buffer != NULL) {
>      BlockIo->ReadBlocks (
> -- 
> 2.9.5
> 

Re: [MdeModulePkg/Library v1 1/1] MdeModulePkg/UefiBootManangerLib: Fix exception issue

[Wu, Hao A]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Monday, March 18, 2019 8:43 PM
> To: Ming Huang
> Cc: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org; Zeng, Star; Dong,
> Eric; Ni, Ray; dann.frazier@canonical.com; ard.biesheuvel@linaro.org; Kinney,
> Michael D; Gao, Liming; wanghuiqiang@huawei.com;
> huangming23@huawei.com; zhangjinsong2@huawei.com;
> huangdaode@hisilicon.com; waip23@126.com; Wang, Jian J; Wu, Hao A; Ni,
> Ray
> Subject: Re: [MdeModulePkg/Library v1 1/1]
> MdeModulePkg/UefiBootManangerLib: Fix exception issue
> 
> +MdeModulePkg maintainers (you added MdePkg maintainers to cc)
> 
> This looks like an improvement to me.
> 
> Am I correct in guessing this behaviour refers to some specific corner
> case of a USB CDROM emulated from a BMC?
> 
> On Mon, Feb 25, 2019 at 05:10:52PM +0800, Ming Huang wrote:
> > The system environment: virtual-CDROM(USB interface) via BMC, insert a
> > iso file to CDROM, like ubuntu-18.04.1-server-arm64.iso, change CDROM
> > to first boot option.
> > With release version bios, disconnecting CDROM when boot to
> > "1 seconds left, Press Esc or F2 to enter Setup"
> > then system will get a exception.
> >
> > The root cause is the EFI_BLOCK_IO_PROTOCOL for UsbMass will be
> uninstalled
> > in this situation after print some transfer error. The status will be
> > invalid parameter. This line will get a exception for BlockIo not point

Do you mean 'EFI_INVALID_PARAMETER' is returned from:
  Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID **) &BlockIo);

If so, my guess is that 'Handle' is NULL at this point. An improvement can
be adding a previous check for 'Status' after the ASSERT at: 

  Status = gBS->LocateDevicePath (&gEfiBlockIoProtocolGuid, &TempDevicePath, &Handle);
  ASSERT_EFI_ERROR (Status);

And leave:

  Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID **) &BlockIo);
  ASSERT_EFI_ERROR (Status);

unchanged.

Best Regards,
Hao Wu

> > to right address:
> > AllocatePool (BlockIo->Media->BlockSize)
> > So, here need to judge the status not using ASSERT_EFI_ERROR.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ming Huang <ming.huang@linaro.org>
> > ---
> >  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > index d5957db610d9..c2f1c651b02f 100644
> > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > @@ -1068,7 +1068,9 @@ BmExpandMediaDevicePath (
> >    // Block IO read/write will success.
> >    //
> >    Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID
> **) &BlockIo);
> > -  ASSERT_EFI_ERROR (Status);
> > +  if (EFI_ERROR (Status)) {
> 
> It would still be worth including an ASSERT here, to let DEBUG builds
> report on point of failure rather than several steps up the chain.
> 
> /
>     Leif
> 
> > +    return NULL;
> > +  }
> >    Buffer = AllocatePool (BlockIo->Media->BlockSize);
> >    if (Buffer != NULL) {
> >      BlockIo->ReadBlocks (
> > --
> > 2.9.5
> >

Re: [MdeModulePkg/Library v1 1/1] MdeModulePkg/UefiBootManangerLib: Fix exception issue

[Ming Huang]

On 3/19/2019 10:25 AM, Wu, Hao A wrote:
>> -----Original Message-----
>> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>> Sent: Monday, March 18, 2019 8:43 PM
>> To: Ming Huang
>> Cc: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org; Zeng, Star; Dong,
>> Eric; Ni, Ray; dann.frazier@canonical.com; ard.biesheuvel@linaro.org; Kinney,
>> Michael D; Gao, Liming; wanghuiqiang@huawei.com;
>> huangming23@huawei.com; zhangjinsong2@huawei.com;
>> huangdaode@hisilicon.com; waip23@126.com; Wang, Jian J; Wu, Hao A; Ni,
>> Ray
>> Subject: Re: [MdeModulePkg/Library v1 1/1]
>> MdeModulePkg/UefiBootManangerLib: Fix exception issue
>>
>> +MdeModulePkg maintainers (you added MdePkg maintainers to cc)
>>
>> This looks like an improvement to me.
>>
>> Am I correct in guessing this behaviour refers to some specific corner
>> case of a USB CDROM emulated from a BMC?
>>
>> On Mon, Feb 25, 2019 at 05:10:52PM +0800, Ming Huang wrote:
>>> The system environment: virtual-CDROM(USB interface) via BMC, insert a
>>> iso file to CDROM, like ubuntu-18.04.1-server-arm64.iso, change CDROM
>>> to first boot option.
>>> With release version bios, disconnecting CDROM when boot to
>>> "1 seconds left, Press Esc or F2 to enter Setup"
>>> then system will get a exception.
>>>
>>> The root cause is the EFI_BLOCK_IO_PROTOCOL for UsbMass will be
>> uninstalled
>>> in this situation after print some transfer error. The status will be
>>> invalid parameter. This line will get a exception for BlockIo not point
> 
> Do you mean 'EFI_INVALID_PARAMETER' is returned from:
>   Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID **) &BlockIo);

Yes.

> 
> If so, my guess is that 'Handle' is NULL at this point. An improvement can
> be adding a previous check for 'Status' after the ASSERT at: 
> 
>   Status = gBS->LocateDevicePath (&gEfiBlockIoProtocolGuid, &TempDevicePath, &Handle);
>   ASSERT_EFI_ERROR (Status);

As my debug output, this 'Status' is seccuss and Handle is not NULL, but
gBS->ConnectController return:Not Found

Debug output:
[BmExpandMediaDevicePath]:[1056L] Handle=3E3F3D18 BlockIo=3B2757B6 Media=AFAF6C617470AFAF Status=Success
EhcExecTransfer: transfer failed with 40
EhcBulkTransfer: error - Device Error, transfer - 40
.........
[UsbOnHubInterrupt]:[632L] SignalEvent (HubIf->HubNotify)
UsbBotExecCommand: UsbBotSendCommand (Device Error)
UsbBootExecCmd: Device Error to Exec 0x0 Cmd (Result = 1)
EhcExecTransfer: transfer failed with 40
...............
[USBMassDriverBindingStop]:[1010L] Uninstall USB block io, free: 3E44F218(F0)
[BmExpandMediaDevicePath]:[1064L] Connect Not Found
[BmExpandMediaDevicePath]:[1076L] Handle=3E3F3D18 BlockIo=3B2757B6 Media=AFAF6C617470AFAF Status=Invalid Parameter

Thanks

> 
> And leave:
> 
>   Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID **) &BlockIo);
>   ASSERT_EFI_ERROR (Status);
> 
> unchanged.



> 
> Best Regards,
> Hao Wu
> 
>>> to right address:
>>> AllocatePool (BlockIo->Media->BlockSize)
>>> So, here need to judge the status not using ASSERT_EFI_ERROR.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ming Huang <ming.huang@linaro.org>
>>> ---
>>>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>>> index d5957db610d9..c2f1c651b02f 100644
>>> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>>> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>>> @@ -1068,7 +1068,9 @@ BmExpandMediaDevicePath (
>>>    // Block IO read/write will success.
>>>    //
>>>    Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID
>> **) &BlockIo);
>>> -  ASSERT_EFI_ERROR (Status);
>>> +  if (EFI_ERROR (Status)) {
>>
>> It would still be worth including an ASSERT here, to let DEBUG builds
>> report on point of failure rather than several steps up the chain.
>>
>> /
>>     Leif
>>
>>> +    return NULL;
>>> +  }
>>>    Buffer = AllocatePool (BlockIo->Media->BlockSize);
>>>    if (Buffer != NULL) {
>>>      BlockIo->ReadBlocks (
>>> --
>>> 2.9.5
>>>

Re: [MdeModulePkg/Library v1 1/1] MdeModulePkg/UefiBootManangerLib: Fix exception issue

[Wu, Hao A]

> -----Original Message-----
> From: Ming Huang [mailto:ming.huang@linaro.org]
> Sent: Tuesday, March 19, 2019 12:14 PM
> To: Wu, Hao A; Leif Lindholm
> Cc: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org; Zeng, Star; Dong,
> Eric; Ni, Ray; dann.frazier@canonical.com; ard.biesheuvel@linaro.org; Kinney,
> Michael D; Gao, Liming; wanghuiqiang@huawei.com;
> huangming23@huawei.com; zhangjinsong2@huawei.com;
> huangdaode@hisilicon.com; waip23@126.com; Wang, Jian J
> Subject: Re: [MdeModulePkg/Library v1 1/1]
> MdeModulePkg/UefiBootManangerLib: Fix exception issue
> 
> 
> 
> On 3/19/2019 10:25 AM, Wu, Hao A wrote:
> >> -----Original Message-----
> >> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> >> Sent: Monday, March 18, 2019 8:43 PM
> >> To: Ming Huang
> >> Cc: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org; Zeng, Star; Dong,
> >> Eric; Ni, Ray; dann.frazier@canonical.com; ard.biesheuvel@linaro.org;
> Kinney,
> >> Michael D; Gao, Liming; wanghuiqiang@huawei.com;
> >> huangming23@huawei.com; zhangjinsong2@huawei.com;
> >> huangdaode@hisilicon.com; waip23@126.com; Wang, Jian J; Wu, Hao A;
> Ni,
> >> Ray
> >> Subject: Re: [MdeModulePkg/Library v1 1/1]
> >> MdeModulePkg/UefiBootManangerLib: Fix exception issue
> >>
> >> +MdeModulePkg maintainers (you added MdePkg maintainers to cc)
> >>
> >> This looks like an improvement to me.
> >>
> >> Am I correct in guessing this behaviour refers to some specific corner
> >> case of a USB CDROM emulated from a BMC?
> >>
> >> On Mon, Feb 25, 2019 at 05:10:52PM +0800, Ming Huang wrote:
> >>> The system environment: virtual-CDROM(USB interface) via BMC, insert
> a
> >>> iso file to CDROM, like ubuntu-18.04.1-server-arm64.iso, change CDROM
> >>> to first boot option.
> >>> With release version bios, disconnecting CDROM when boot to
> >>> "1 seconds left, Press Esc or F2 to enter Setup"
> >>> then system will get a exception.
> >>>
> >>> The root cause is the EFI_BLOCK_IO_PROTOCOL for UsbMass will be
> >> uninstalled
> >>> in this situation after print some transfer error. The status will be
> >>> invalid parameter. This line will get a exception for BlockIo not point
> >
> > Do you mean 'EFI_INVALID_PARAMETER' is returned from:
> >   Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID
> **) &BlockIo);
> 
> Yes.
> 
> >
> > If so, my guess is that 'Handle' is NULL at this point. An improvement can
> > be adding a previous check for 'Status' after the ASSERT at:
> >
> >   Status = gBS->LocateDevicePath (&gEfiBlockIoProtocolGuid,
> &TempDevicePath, &Handle);
> >   ASSERT_EFI_ERROR (Status);
> 
> As my debug output, this 'Status' is seccuss and Handle is not NULL, but
> gBS->ConnectController return:Not Found
> 
> Debug output:
> [BmExpandMediaDevicePath]:[1056L] Handle=3E3F3D18 BlockIo=3B2757B6
> Media=AFAF6C617470AFAF Status=Success
> EhcExecTransfer: transfer failed with 40
> EhcBulkTransfer: error - Device Error, transfer - 40
> .........
> [UsbOnHubInterrupt]:[632L] SignalEvent (HubIf->HubNotify)
> UsbBotExecCommand: UsbBotSendCommand (Device Error)
> UsbBootExecCmd: Device Error to Exec 0x0 Cmd (Result = 1)
> EhcExecTransfer: transfer failed with 40
> ...............
> [USBMassDriverBindingStop]:[1010L] Uninstall USB block io, free:
> 3E44F218(F0)
> [BmExpandMediaDevicePath]:[1064L] Connect Not Found
> [BmExpandMediaDevicePath]:[1076L] Handle=3E3F3D18 BlockIo=3B2757B6
> Media=AFAF6C617470AFAF Status=Invalid Parameter

Thanks for the debug information, I got it now.

The call to the gBS->ConnectController() leads to protocols being
uninstalled from 'Handle' and removing 'Handle' from the database. Then
within the call to gBS->HandleProtocol(), CoreValidateHandle() returns
EFI_INVALID_PARAMETER since the handle cannot be found.

I am good with this patch, please help to address Leif's previous comment
to keep the ASSERT.

Also, I have filed a Bugzilla tracker for this:
https://bugzilla.tianocore.org/show_bug.cgi?id=1631

Could you help to add the reference to the above BZ in the commit log
message? Thanks.


Best Regards,
Hao Wu

> 
> Thanks
> 
> >
> > And leave:
> >
> >   Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID
> **) &BlockIo);
> >   ASSERT_EFI_ERROR (Status);
> >
> > unchanged.
> 
> 
> 
> >
> > Best Regards,
> > Hao Wu
> >
> >>> to right address:
> >>> AllocatePool (BlockIo->Media->BlockSize)
> >>> So, here need to judge the status not using ASSERT_EFI_ERROR.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Ming Huang <ming.huang@linaro.org>
> >>> ---
> >>>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> >> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> >>> index d5957db610d9..c2f1c651b02f 100644
> >>> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> >>> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> >>> @@ -1068,7 +1068,9 @@ BmExpandMediaDevicePath (
> >>>    // Block IO read/write will success.
> >>>    //
> >>>    Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid,
> (VOID
> >> **) &BlockIo);
> >>> -  ASSERT_EFI_ERROR (Status);
> >>> +  if (EFI_ERROR (Status)) {
> >>
> >> It would still be worth including an ASSERT here, to let DEBUG builds
> >> report on point of failure rather than several steps up the chain.
> >>
> >> /
> >>     Leif
> >>
> >>> +    return NULL;
> >>> +  }
> >>>    Buffer = AllocatePool (BlockIo->Media->BlockSize);
> >>>    if (Buffer != NULL) {
> >>>      BlockIo->ReadBlocks (
> >>> --
> >>> 2.9.5
> >>>

Re: [MdeModulePkg/Library v1 1/1] MdeModulePkg/UefiBootManangerLib: Fix exception issue

[Ming Huang]

On 3/19/2019 1:56 PM, Wu, Hao A wrote:
>> -----Original Message-----
>> From: Ming Huang [mailto:ming.huang@linaro.org]
>> Sent: Tuesday, March 19, 2019 12:14 PM
>> To: Wu, Hao A; Leif Lindholm
>> Cc: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org; Zeng, Star; Dong,
>> Eric; Ni, Ray; dann.frazier@canonical.com; ard.biesheuvel@linaro.org; Kinney,
>> Michael D; Gao, Liming; wanghuiqiang@huawei.com;
>> huangming23@huawei.com; zhangjinsong2@huawei.com;
>> huangdaode@hisilicon.com; waip23@126.com; Wang, Jian J
>> Subject: Re: [MdeModulePkg/Library v1 1/1]
>> MdeModulePkg/UefiBootManangerLib: Fix exception issue
>>
>>
>>
>> On 3/19/2019 10:25 AM, Wu, Hao A wrote:
>>>> -----Original Message-----
>>>> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>>>> Sent: Monday, March 18, 2019 8:43 PM
>>>> To: Ming Huang
>>>> Cc: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org; Zeng, Star; Dong,
>>>> Eric; Ni, Ray; dann.frazier@canonical.com; ard.biesheuvel@linaro.org;
>> Kinney,
>>>> Michael D; Gao, Liming; wanghuiqiang@huawei.com;
>>>> huangming23@huawei.com; zhangjinsong2@huawei.com;
>>>> huangdaode@hisilicon.com; waip23@126.com; Wang, Jian J; Wu, Hao A;
>> Ni,
>>>> Ray
>>>> Subject: Re: [MdeModulePkg/Library v1 1/1]
>>>> MdeModulePkg/UefiBootManangerLib: Fix exception issue
>>>>
>>>> +MdeModulePkg maintainers (you added MdePkg maintainers to cc)
>>>>
>>>> This looks like an improvement to me.
>>>>
>>>> Am I correct in guessing this behaviour refers to some specific corner
>>>> case of a USB CDROM emulated from a BMC?
>>>>
>>>> On Mon, Feb 25, 2019 at 05:10:52PM +0800, Ming Huang wrote:
>>>>> The system environment: virtual-CDROM(USB interface) via BMC, insert
>> a
>>>>> iso file to CDROM, like ubuntu-18.04.1-server-arm64.iso, change CDROM
>>>>> to first boot option.
>>>>> With release version bios, disconnecting CDROM when boot to
>>>>> "1 seconds left, Press Esc or F2 to enter Setup"
>>>>> then system will get a exception.
>>>>>
>>>>> The root cause is the EFI_BLOCK_IO_PROTOCOL for UsbMass will be
>>>> uninstalled
>>>>> in this situation after print some transfer error. The status will be
>>>>> invalid parameter. This line will get a exception for BlockIo not point
>>>
>>> Do you mean 'EFI_INVALID_PARAMETER' is returned from:
>>>   Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID
>> **) &BlockIo);
>>
>> Yes.
>>
>>>
>>> If so, my guess is that 'Handle' is NULL at this point. An improvement can
>>> be adding a previous check for 'Status' after the ASSERT at:
>>>
>>>   Status = gBS->LocateDevicePath (&gEfiBlockIoProtocolGuid,
>> &TempDevicePath, &Handle);
>>>   ASSERT_EFI_ERROR (Status);
>>
>> As my debug output, this 'Status' is seccuss and Handle is not NULL, but
>> gBS->ConnectController return:Not Found
>>
>> Debug output:
>> [BmExpandMediaDevicePath]:[1056L] Handle=3E3F3D18 BlockIo=3B2757B6
>> Media=AFAF6C617470AFAF Status=Success
>> EhcExecTransfer: transfer failed with 40
>> EhcBulkTransfer: error - Device Error, transfer - 40
>> .........
>> [UsbOnHubInterrupt]:[632L] SignalEvent (HubIf->HubNotify)
>> UsbBotExecCommand: UsbBotSendCommand (Device Error)
>> UsbBootExecCmd: Device Error to Exec 0x0 Cmd (Result = 1)
>> EhcExecTransfer: transfer failed with 40
>> ...............
>> [USBMassDriverBindingStop]:[1010L] Uninstall USB block io, free:
>> 3E44F218(F0)
>> [BmExpandMediaDevicePath]:[1064L] Connect Not Found
>> [BmExpandMediaDevicePath]:[1076L] Handle=3E3F3D18 BlockIo=3B2757B6
>> Media=AFAF6C617470AFAF Status=Invalid Parameter
> 
> Thanks for the debug information, I got it now.
> 
> The call to the gBS->ConnectController() leads to protocols being
> uninstalled from 'Handle' and removing 'Handle' from the database. Then
> within the call to gBS->HandleProtocol(), CoreValidateHandle() returns
> EFI_INVALID_PARAMETER since the handle cannot be found.
> 
> I am good with this patch, please help to address Leif's previous comment
> to keep the ASSERT.

I will add ASSERT back in v2.

> 
> Also, I have filed a Bugzilla tracker for this:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1631
> 
> Could you help to add the reference to the above BZ in the commit log
> message? Thanks.

Sure, add it in v2.

Thanks

> 
> 
> Best Regards,
> Hao Wu
> 
>>
>> Thanks
>>
>>>
>>> And leave:
>>>
>>>   Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID
>> **) &BlockIo);
>>>   ASSERT_EFI_ERROR (Status);
>>>
>>> unchanged.
>>
>>
>>
>>>
>>> Best Regards,
>>> Hao Wu
>>>
>>>>> to right address:
>>>>> AllocatePool (BlockIo->Media->BlockSize)
>>>>> So, here need to judge the status not using ASSERT_EFI_ERROR.
>>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>> Signed-off-by: Ming Huang <ming.huang@linaro.org>
>>>>> ---
>>>>>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>>>> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>>>>> index d5957db610d9..c2f1c651b02f 100644
>>>>> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>>>>> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>>>>> @@ -1068,7 +1068,9 @@ BmExpandMediaDevicePath (
>>>>>    // Block IO read/write will success.
>>>>>    //
>>>>>    Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid,
>> (VOID
>>>> **) &BlockIo);
>>>>> -  ASSERT_EFI_ERROR (Status);
>>>>> +  if (EFI_ERROR (Status)) {
>>>>
>>>> It would still be worth including an ASSERT here, to let DEBUG builds
>>>> report on point of failure rather than several steps up the chain.
>>>>
>>>> /
>>>>     Leif
>>>>
>>>>> +    return NULL;
>>>>> +  }
>>>>>    Buffer = AllocatePool (BlockIo->Media->BlockSize);
>>>>>    if (Buffer != NULL) {
>>>>>      BlockIo->ReadBlocks (
>>>>> --
>>>>> 2.9.5
>>>>>

Re: [MdeModulePkg/Library v1 1/1] MdeModulePkg/UefiBootManangerLib: Fix exception issue

[Ming Huang]

On 3/18/2019 8:42 PM, Leif Lindholm wrote:
> +MdeModulePkg maintainers (you added MdePkg maintainers to cc)
> 
> This looks like an improvement to me.
> 
> Am I correct in guessing this behaviour refers to some specific corner
> case of a USB CDROM emulated from a BMC?

Yes, I found this issue with a USB CDROM emulated from a BMC.
I guess have the same symptom with physical USB CDROM.

Thanks

> 
> On Mon, Feb 25, 2019 at 05:10:52PM +0800, Ming Huang wrote:
>> The system environment: virtual-CDROM(USB interface) via BMC, insert a
>> iso file to CDROM, like ubuntu-18.04.1-server-arm64.iso, change CDROM
>> to first boot option.
>> With release version bios, disconnecting CDROM when boot to
>> "1 seconds left, Press Esc or F2 to enter Setup"
>> then system will get a exception.
>>
>> The root cause is the EFI_BLOCK_IO_PROTOCOL for UsbMass will be uninstalled
>> in this situation after print some transfer error. The status will be
>> invalid parameter. This line will get a exception for BlockIo not point
>> to right address:
>> AllocatePool (BlockIo->Media->BlockSize)
>> So, here need to judge the status not using ASSERT_EFI_ERROR.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ming Huang <ming.huang@linaro.org>
>> ---
>>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> index d5957db610d9..c2f1c651b02f 100644
>> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> @@ -1068,7 +1068,9 @@ BmExpandMediaDevicePath (
>>    // Block IO read/write will success.
>>    //
>>    Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID **) &BlockIo);
>> -  ASSERT_EFI_ERROR (Status);
>> +  if (EFI_ERROR (Status)) {
> 
> It would still be worth including an ASSERT here, to let DEBUG builds
> report on point of failure rather than several steps up the chain.
> 
> /
>     Leif
> 
>> +    return NULL;
>> +  }
>>    Buffer = AllocatePool (BlockIo->Media->BlockSize);
>>    if (Buffer != NULL) {
>>      BlockIo->ReadBlocks (
>> -- 
>> 2.9.5
>>

Re: [MdeModulePkg/Library v1 1/1] MdeModulePkg/UefiBootManangerLib: Fix exception issue

[Leif Lindholm]

On Tue, Mar 19, 2019 at 12:01:51PM +0800, Ming Huang wrote:
> On 3/18/2019 8:42 PM, Leif Lindholm wrote:
> > +MdeModulePkg maintainers (you added MdePkg maintainers to cc)
> > 
> > This looks like an improvement to me.
> > 
> > Am I correct in guessing this behaviour refers to some specific corner
> > case of a USB CDROM emulated from a BMC?
> 
> Yes, I found this issue with a USB CDROM emulated from a BMC.
> I guess have the same symptom with physical USB CDROM.

Yes, you would just be less likely to disconnect it :)

Thanks for the confirmation.

Best Regards,

Leif