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

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----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
> >>>