From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-40134.protonmail.ch (mail-40134.protonmail.ch [185.70.40.134]) by mx.groups.io with SMTP id smtpd.web12.9293.1578998083029173060 for ; Tue, 14 Jan 2020 02:34:43 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@protonmail.com header.s=default header.b=iLB07pjq; spf=pass (domain: protonmail.com, ip: 185.70.40.134, mailfrom: vit9696@protonmail.com) Date: Tue, 14 Jan 2020 10:34:36 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=default; t=1578998080; bh=yy9QiRGgR/CDwRC8LMyHsnweE6k5h5Pq8JmxVZQzfB0=; h=Date:To:From:Reply-To:Subject:In-Reply-To:References:Feedback-ID: From; b=iLB07pjqt89CEpC0+w2HZ8DN65Br4lz35/wRxe8+6kRyRxrUooVkqstcehlueqb9D M5DFM3MmGTSKzPmx4BfgFqMg3PvlJr2Xdv42lHRWXhOgnMvL6Z2RwQvGMhi7/gSble Kwbk1QZd1CBzdurnHrbTYT7T7bKpBXN4frdNo1Qw= To: "Ni, Ray" , "devel@edk2.groups.io" , Laszlo Ersek , "Gao, Zhichao" From: "Vitaly Cheptsov" Reply-To: vit9696 Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths Message-ID: In-Reply-To: <6D0IjdPY_gnW3iTK9Z145_tWcbCfqel3hQ0-6ENStUfchVtOe9pdu9r5ewViA8ICrkuDIQnHC8Ds1XBNfCLCP2F_hSN2-Sx9j8qqM6mPQF4=@protonmail.com> References: <20200112213845.96853-1-vit9696@protonmail.com> <20200112213845.96853-2-vit9696@protonmail.com> <483a200323c14afb95bdb94ad447868d@intel.com> <9A165163-4BB4-434A-8C65-A84CDCB3C3C2@protonmail.com> <734D49CCEBEEF84792F5B80ED585239D5C3F2C88@SHSMSX104.ccr.corp.intel.com> <0f80880e-417c-6aaf-4dfe-16158dd799c0@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5C3F3905@SHSMSX104.ccr.corp.intel.com> <6D0IjdPY_gnW3iTK9Z145_tWcbCfqel3hQ0-6ENStUfchVtOe9pdu9r5ewViA8ICrkuDIQnHC8Ds1XBNfCLCP2F_hSN2-Sx9j8qqM6mPQF4=@protonmail.com> Feedback-ID: p9QuX-L1wMgUm6nrSvNrf8juLupNs0VSnzXGVXuYDxlEahFdWtaedWDMB9zpwGDklGt7kzs1-RBc0cqz327Gcg==:Ext:ProtonMail MIME-Version: 1.0 X-Spam-Status: No, score=3.3 required=7.0 tests=ALL_TRUSTED,BAYES_50, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,FREEMAIL_REPLYTO_END_DIGIT, HTML_FONT_LOW_CONTRAST,HTML_MESSAGE shortcircuit=no autolearn=no autolearn_force=no version=3.4.2 X-Spam-Level: *** X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on mail.protonmail.ch X-Groupsio-MsgNum: 53214 Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg=pgp-sha256; boundary="---------------------50ca4ef21f5d03948a254ac69d6a3a44"; charset=UTF-8 -----------------------50ca4ef21f5d03948a254ac69d6a3a44 Content-Type: multipart/alternative; boundary="Apple-Mail=_19A2463F-D4DC-4F27-9105-5FB001402182" Date: Tue, 14 Jan 2020 13:33:33 +0300 From: vit9696 In-Reply-To: <6D0IjdPY_gnW3iTK9Z145_tWcbCfqel3hQ0-6ENStUfchVtOe9pdu9r5ewViA8ICrkuDIQnHC8Ds1XBNfCLCP2F_hSN2-Sx9j8qqM6mPQF4=@protonmail.com> Message-Id: Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.40.2.2.4\)) References: <20200112213845.96853-1-vit9696@protonmail.com> <20200112213845.96853-2-vit9696@protonmail.com> <483a200323c14afb95bdb94ad447868d@intel.com> <9A165163-4BB4-434A-8C65-A84CDCB3C3C2@protonmail.com> <734D49CCEBEEF84792F5B80ED585239D5C3F2C88@SHSMSX104.ccr.corp.intel.com> <0f80880e-417c-6aaf-4dfe-16158dd799c0@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5C3F3905@SHSMSX104.ccr.corp.intel.com> <6D0IjdPY_gnW3iTK9Z145_tWcbCfqel3hQ0-6ENStUfchVtOe9pdu9r5ewViA8ICrkuDIQnHC8Ds1XBNfCLCP2F_hSN2-Sx9j8qqM6mPQF4=@protonmail.com> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths To: "Ni, Ray" , "devel@edk2.groups.io" , Laszlo Ersek , "Gao, Zhichao" X-Mailer: Apple Mail (2.3608.40.2.2.4) --Apple-Mail=_19A2463F-D4DC-4F27-9105-5FB001402182 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 In addition to my previous letter I have to mention a couple more newly dis= covered details. 1. UEFI Shell (ShellPkg) actually has 3 functions dedicated to connecting = controllers and essentially doing the same thing: - ConnectAllEfi ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c This one locates all handles and runs connect on them. It is the one we me= ntioned in the bug. - LoadPciRomConnectAllDriversToAllControllers ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c This one is similar to ConnectAllEfi. The only difference is that it addit= ionally checks for Ctrl+C via ShellGetExecutionBreakFlag. - ConnectControllers ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c This one is more complex, as it supports explicitly connecting specified c= ontrollers, however, for connecting all controllers it locates handles with= gEfiDevicePathProtocolGuid. I.e. exactly what we ask. I believe that all t= hese functions should behave the same way in correspondence to gBS->Connect= Controller at the very least, and most likely there should be a library res= ponsible for connection and disconnection. 2. We also checked EFI 1.1 Shell, and confirmed that it consistently has c= hecks for device path presence before running connect on the handle[1][2]. = This makes us believe that our proposal is not really a firmware bug, but r= ather a limitation of the legacy specification. Considering all these discoveries, we believe our suggested change is legi= t depending on the minimum supported UEFI version. Therefore we propose sub= mitting a ControllerConnection library with 5 functions: - ConnectController - DisconnectController - GetAllControllers - ConnectAllControllers - DisconnectAllControllers Adopting it throughout the code will let the distribitor to be responsible= for choosing what is right for his applications (or firmwares). Best wishes, Vitaly [1] https://sourceforge.net/p/efi-shell/code/64/tree/trunk/Shell/LoadPciRo= m/LoadPciRom.c#l528 [2] https://sourceforge.net/p/efi-shell/code/64/tree/trunk/Shell/load/load= .c#l312 > 14 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 11:36, vit9696 =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 > Ray, >=20 > We are quite reluctant to have patches in EDK II for a large amount of w= idely adopted firmwares. Patches eventually break and require maintenance c= ost, and currently we are trying to get rid of them all. We believe that ED= K II Shell is supposed to work on real world platforms and not only the one= s that theoretically support the specification. It is always hard to adopt = changes based on third-party bugs, and we very well understand your concern= , yet it is something we have to do to stay beneficial to the end user. >=20 > Best wishes, > Vitaly >=20 > On Tue, Jan 14, 2020 at 05:53, Ni, Ray > wrote: >>=20 >> Vitaly, >>=20 >> I still have concern to modify the EDKII code to workaround a firmware = bug. >>=20 >> Can you just change in your local version? >>=20 >> >> Thanks, >>=20 >> Ray >>=20 >> >> From: devel@edk2.groups.io On Behalf Of Vitaly C= heptsov via Groups.Io >> Sent: Tuesday, January 14, 2020 4:47 AM >> To: Laszlo Ersek ; devel@edk2.groups.io; Ni, Ray ; Gao, Zhichao >> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles = without device paths >>=20 >> >> Thanks all for your input, >> >> These explanations seem sufficient to us that it is not a good idea to = change the behaviour for everyone. Even so, we still need this to be config= urable in some way, as having to patch EDK II is impracticable. >> >> We believe there are three possible routes to approach this problem: >> >> Introduce a separate ControllerConnectionLib library for this function.= While it is small, we found several places in our code that need to call i= t beyond UEFI Shell. This way different implementations could be used depen= ding on the chosen library. >> Introduce a ConnectRequiresDevicePath PCD, which will choose the prefer= red logic. >> Introduce a -dp Shell argument for affected commands the way Lazslo sug= gested. >> >> We believe either route or a combination of multiple routes have their = own benefits, and would suggest either going with 1+2 or with 3. Any approa= ch is fine for us. >> >> We will submit V2 of the patch after hearing the opinions. >> >> Best wishes, >> Vitaly >> >> On Mon, Jan 13, 2020 at 20:55, Laszlo Ersek > wrote: >>=20 >> On 01/13/20 12:56, Ni, Ray wrote: >> > We shouldn't assume that a DriverBindingStart() can only start on a h= andle with device path installed. DevicePath protocol is just a special pro= tocol. >> > It's possible that a bus driver starts on a host controller handle an= d creates multiple children, each with only a Specific_IO protocol installe= d. >> > Certain device driver can start on the children handle and open the S= pecific_IO protocol BY_DRIVER. >> > I am not sure if certain today's network drivers may work like this. = It's allowed per UEFI spec. >>=20 >> I agree. >>=20 >> Under "10.2 EFI Device Path Protocol", the spec says, "If the handle >> does not logically map to a physical device, the handle may not >> necessarily support the device path protocol." >>=20 >> I think gBS->ConnectController() and >> EFI_DRIVER_BINDING_PROTOCOL.Supported() should work on such handles. >>=20 >> If we'd like to work around related issues in drivers, then I'd suggest >> new command line options for the "load", "connect", "reconnect" shell >> commands (maybe more), for filtering out handles that do not carry >> device paths. Such command line options could be added as an extension, >> IIUC, such as "-_option". I.e., I believe it's not necessary to start >> with UEFI Shell Spec updates. >>=20 >> Thanks >> Laszlo >>=20 >> >> >>=20 >>=20 >=20 >=20 --Apple-Mail=_19A2463F-D4DC-4F27-9105-5FB001402182 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
In additi= on to my previous letter I have to mention a couple more newly discovered d= etails.

1. UEFI S= hell (ShellPkg) actually has 3 functions dedicated to connecting controller= s and essentially doing the same thing:

- ConnectAllEfi
ShellPkg/Librar= y/UefiShellLevel2CommandsLib/Load.c

This one locates all handles and runs connect on them. I= t is the one we mentioned in the bug.

<= /div>
- LoadPciRomConnectAllDriversToAllControllers
ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c
=

This one is similar to= ConnectAllEfi. The only difference is that it additionally checks for Ctrl= +C via ShellGetExecutionBreakFlag.

- ConnectControllers
ShellPkg/Librar= y/UefiShellDriver1CommandsLib/Connect.c

This one is more complex, as it supports explicitly = connecting specified controllers, however, for connecting all controllers i= t locates handles with gEfiDevicePathProtocolGuid. I.e. exactly what we ask= . I believe that all these functions should behave the same way in correspo= ndence to gBS->ConnectController at the very least, and most likely ther= e should be a library responsible for connection and disconnection.

2. We also checked EFI 1= .1 Shell, and confirmed that it consistently has checks for device path pre= sence before running connect on the handle[1][2]. This makes us believe tha= t our proposal is not really a firmware bug, but rather a limitation of the= legacy specification.

Considering all these discoveries, we believe our suggested change i= s legit depending on the minimum supported UEFI version. Therefore we propo= se submitting a ControllerConnection library with 5 functions:

- ConnectController
- DisconnectController
- GetAllControllers=
- ConnectAllControllers
- Discon= nectAllControllers

Adopting it throughout the code will let the distribitor to be responsibl= e for choosing what is right for his applications (or firmwares).

Best wishes,
Vitaly

[1= ] https://sourceforge.net/p/efi-shell= /code/64/tree/trunk/Shell/LoadPciRom/LoadPciRom.c#l528
[2] https://sourceforge.net/p/efi-shell/co= de/64/tree/trunk/Shell/load/load.c#l312


14 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 11:36, vit9696 <<= a href=3D"mailto:vit9696@protonmail.com" class=3D"">vit9696@protonmail.com<= /a>> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):

Ray,

We are quite reluctant to have patches in EDK II for a large = amount of widely adopted firmwares. Patches eventually break and = require maintenance cost, and currently we are trying to get rid of them al= l. We believe that= EDK II Shell is supposed to work on real world platforms and not only the = ones that theoretically support the specification. It is always hard to ado= pt changes based on third-party bugs, and we very well understand your conc= ern, yet it is something we have to do to stay beneficial to the end u= ser.

Best wishes,=
Vitaly

On T= ue, Jan 14, 2020 at 05:53, Ni, Ray <ray.ni@intel.com> wrote:

Vitaly,

I still have concern to modify the EDKII code to workaround a fi= rmware bug.

Can you just change in your local ver= sion?

 

Thanks,

Ray

 

From: devel@edk2.groups.io <<= a href=3D"mailto:devel@edk2.groups.io" class=3D"">devel@edk2.groups.io&= gt; On Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Tuesday, January 14, 2020 4:47 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>=
Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not c= onnect handles without device paths

 
Thanks all for you= r input,

 

These expla= nations seem sufficient to us that it is not a good idea to change the beha= viour for everyone. Even so, we still need this to be configurable in some = way, as having to patch EDK II is impracticable.

 

We believe = there are three possible routes to approach this problem:

 

  1. Introduce a= separate ControllerConnectionLib library for this function. While it is sm= all, we found several places in our code that need to call it beyond UEFI S= hell. This way different implementations could be used depending on the chosen library.
  2. Introduce a= ConnectRequiresDevicePath PCD, which will choose the preferred logic.
  3. Introduce a= -dp Shell argument for affected commands the way Lazslo suggested.<= /span>

 

We believe = either route or a combination of multiple routes have their own benefits, a= nd would suggest either going with 1+2 or with 3. Any approach is fine for = us.
<= p class=3D"p2" style=3D"margin:0in;margin-bottom:.0001pt;font-stretch: norm= al;min-height: 20.3px;caret-color: rgb(0, 0, 0);-webkit-text-size-adjust: a= uto">  

We will sub= mit V2 of the patch after hearing the opinions.

 

Best wishes= ,
Vitaly
 

On Mon, Jan 13, 2020 at 20:55, Laszlo Ersek &= lt;lersek@redhat.com>= ; wrote:

On 01/13/20 12:56, Ni, Ra= y wrote:
> We shouldn't assume that a DriverBindingStart() can only start on a h= andle with device path installed. DevicePath protocol is just a special pro= tocol.
> It's possible that a bus driver starts on a host controller handle an= d creates multiple children, each with only a Specific_IO protocol installe= d.
> Certain device driver can start on the children handle and open the S= pecific_IO protocol BY_DRIVER.
> I am not sure if certain today's network drivers may work like this. = It's allowed per UEFI spec.

I agree.

Under "10.2 EFI Device Path Protocol", the spec says, "If the handle
does not logically map to a physical device, the handle may not
necessarily support the device path protocol."

I think gBS->ConnectController() and
EFI_DRIVER_BINDING_PROTOCOL.Supported() should work on such handles.

If we'd like to work around related issues in drivers, then I'd suggest new command line options for the "load", "connect", "reconnect" shell
commands (maybe more), for filtering out handles that do not carry
device paths. Such command line options could be added as an extension, IIUC, such as "-_option". I.e., I believe it's not necessary to start
with UEFI Shell Spec updates.

Thanks
Laszlo

 
 




--Apple-Mail=_19A2463F-D4DC-4F27-9105-5FB001402182-- -----------------------50ca4ef21f5d03948a254ac69d6a3a44 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: ProtonMail wsBmBAEBCAAQBQJeHZk6CRBPsoxt7Hy0xQAKCRBPsoxt7Hy0xd7lB/9YJy1h ely0ysA9Q0vdCJrhNy3LDLaOG9hVd8hnPHMAOOPrA8sez1qCFgPrtaoNXNkU WA5Clt6Va8velnkk3A/NvIe23knBWBbF3bgCEjJ0O5D1vW6RZrUdgaF50r/o y4kjM1CpJD/BjdGUkXy60jx5FXIB7cglE/4olAGNZdFMjw0LLc+RLlNeIgWI 2W/iH9iTFPEvU2QC1CHYlReltwntwj6yYRCrsx3OyHmNAsZKqxaBHsOCq7sm /b9igTyW7Y60pM9d10bV1NAGon1zq9vzdSz0lBMAK0ZY85Y0VQcFlpBdS+4l DDGt/phRg2oJ+9t/sFATWtMRge7WraJ/mVIv =Lu6l -----END PGP SIGNATURE----- -----------------------50ca4ef21f5d03948a254ac69d6a3a44--