Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

["Zeng, Star" <star.zeng@intel.com>]

Laszlo,

Summary the return status codes for OpenProtocol() as below.
EFI_SUCCESS
  Return the protocol interface in *Interface if Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
EFI_INVALID_PARAMETER
  Do not update *Interface because one of the parameters has a value that does not allow this API to function correctly.
EFI_UNSUPPORTED
  Return NULL in *Interface because UserHandle does not support Protocol if Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
EFI_ACCESS_DENIED
  Do not update *Interface because the protocol is already being used exclusively be someone else.  Returning the *Interface just makes it easy for the caller to use an interface they are not allowed to use.
EFI_ALREADY_STARTED
  Return the protocol interface in *Interface.  This allows bus drivers to use the interface they are already managing.

How about we clarify UEFI spec to only return the corresponding protocol interface in *Interface if the return status is EFI_SUCCESS or EFI_ALREADY_STARTED.
Then it preserves the exiting behavior for EFI_ALREADY_STARTED, but also updates OpenProtocol() to not modify Interface output parameter for all other error conditions except EFI_UNSUPPORTED (NULL will be returned in *Interface if Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL).

UEFI Spec (Current language)
==========================
There are a number of reasons that this function call can return an error. If an error is returned, then AgentHandle, ControllerHandle, and Attributes are not added to the list of agents consuming the protocol interface specified by Handle and Protocol, and Interface is returned unmodified. The following is the list of conditions that must be checked before this function can return EFI_SUCCESS.

UEFI Spec (Proposed new language)
==========================
There are a number of reasons that this function call can return an error. If an error is returned, then AgentHandle, ControllerHandle, and Attributes are not added to the list of agents consuming the protocol interface specified by Handle and Protocol. Interface is returned unmodified for all error conditions except EFI_UNSUPPORTED and EFI_ALREADY_STARTED, NULL will be returned in *Interface when EFI_UNSUPPORTED and Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL, the protocol interface will be returned in *Interface when EFI_ALREADY_STARTED. The following is the list of conditions that must be checked before this function can return EFI_SUCCESS.


Of course, the code still needs to be updated and the revised patch based on this V4 patch is attached.


Thanks,
Star

-----Original Message-----
From: Zeng, Star 
Sent: Tuesday, June 27, 2017 9:31 PM
To: Laszlo Ersek <lersek@redhat.com>; Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Gao, Liming <liming.gao@intel.com>; Fan, Jeff <jeff.fan@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

I also prefer to document it in UEFI spec personally.
And we are also having more discussion about it internally, nice to share more after that. :)


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, June 27, 2017 9:23 PM
To: Zeng, Star <star.zeng@intel.com>; Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Gao, Liming <liming.gao@intel.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

On 06/27/17 13:15, Laszlo Ersek wrote:
> On 06/27/17 11:59, Zeng, Star wrote:
>> Laszlo,
>>
>> I got the point and I like the idea of wrapper function personally.
>> Although we can clean up the UEFI drivers in EDK2 tree, but it is really hard to evaluate the UEFI drivers in OPROM on add-on card.
>> The patch did not just break OVMF, but also broke all real platforms we have in hand (it's my fault that I did not catch the failure when reviewing patch), the patch is so risky.
> 
> Can we perhaps add a FeaturePCD (default value: FALSE) and rework 
> Amit's patch to consider the PCD? I really like the idea of being true 
> to the UEFI spec.
> 
> In the edk2 tree we could rebase the affected drivers to the wrapper 
> function. And in OVMF (and in other in-tree emulation platforms), we 
> could set the FeaturePCD to TRUE.
> 
> It is possible to use OVMF with assigned physical PCI devices, but 
> people can easily rebuild OVMF themselves, with the FeaturePCD re-set 
> to FALSE. Users can also quickly report the exact cards / oproms that 
> break with the FeaturePCD set to TRUE.
> 
> 
> If you think it's simply not worth the effort, I'm OK with that, but 
> then we should specify this behavior in the UEFI spec -- we'll need a 
> Mantis bug for that. IMO it's not great that so many UEFI_DRIVERs in 
> the wild depend on behavior that is expressly forbidden by the UEFI 
> spec at the moment. If we can't modify all those drivers, then we 
> should adapt the spec, so that it reflects reality.
> 
> I'm not going to suggest specific language for the UEFI spec right now.
> But the wording could be based on the documentation of the wrapper 
> function that I'm working on now. I think I might post the patches 
> just for illustration.

OK, I'm giving up. It is too hard to track down every single crash -- the register dump written by UefiCpuPkg's exception handler to the serial port is not much help, even though it names a module to look at.

So I guess we have to accept that the dependency on EFI_ALREADY_STARTED setting Interface is just too wide-spread. :/ As I wrote above, I think this should be documented in the UEFI spec.

I filed the following mantis ticket:

  Permit OpenProtocol() to output the supported protocol interface even
  on error
  https://mantis.uefi.org/mantis/view.php?id=1815

Thanks
Laszlo