From: Laszlo Ersek <lersek@redhat.com>
To: "Zeng, Star" <star.zeng@intel.com>,
Amit Kumar <amit.ak@samsung.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Tian, Feng" <feng.tian@intel.com>,
"Gao, Liming" <liming.gao@intel.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>,
"Gabriel L. Somlo (GMail)" <gsomlo@gmail.com>,
"Fan, Jeff" <jeff.fan@intel.com>
Subject: Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol
Date: Tue, 27 Jun 2017 13:15:07 +0200 [thread overview]
Message-ID: <dc350a86-6caa-8fc1-9a53-fa2ae963efd3@redhat.com> (raw)
In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B8EEED6@shsmsx102.ccr.corp.intel.com>
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.
Thanks,
Laszlo
>
>
> Thanks,
> Star
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, June 27, 2017 5:45 PM
> To: Zeng, Star <star.zeng@intel.com>; Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Fan, Jeff <jeff.fan@intel.com>
> Subject: Re: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol
>
> Hi Star,
>
> On 06/27/17 03:37, Zeng, Star wrote:
>> I have reverted this patch at fd220166c435479a81dfe8519c92abec9acf7d82 first.
>
> Thanks for that. More comments below:
>
>>
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Tuesday, June 27, 2017 8:53 AM
>> To: Laszlo Ersek <lersek@redhat.com>; Amit Kumar
>> <amit.ak@samsung.com>; edk2-devel@lists.01.org
>> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Gabriel L. Somlo (GMail)
>> <gsomlo@gmail.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
>>
>> Laszlo,
>>
>> I agree to revert this patch first since it breaks something.
>> Could you help do that with detailed revert log?
>>
>> Anyway, have you found any bug in this patch itself?
>
> No, as I said, I think the patch has the right goal (it improves compliance with the UEFI specification).
>
> And, while I didn't personally review the patch, I trust your and Liming's review on it.
>
> The problem is not with the patch per se. The problem is that the patch triggers an existing bug in many other drivers.
>
> So the actual bugs are in those drivers that incorrectly expect
> OpenProtocol() to set Interface even when OpenProtocol() returns EFI_ALREADY_STARTED.
>
> The end result is that all these drivers should be fixed first (with separate patches), before fixing OpenProtocol() itself.
>
> I'll try to work a bit more on those drivers and discover how many there are of them. Obviously I can't audit *all* drivers in the edk2 tree, just those that break the OVMF boot process.
>
> In fact, the newest idea I have is the following: I think I'll introduce an OpenProtocol() wrapper function to UefiLib. This function should work identically to OpenProtocol(), except when OpenProtocol() returns EFI_ALREADY_STARTED. In that case, the wrapper function will repeat the exact same OpenProtocol() call just with GET_PROTOCOL attribute, and set Interface on output. This will restore and legitimize the OpenProtocol() behavior expected by all those problematic drivers, without having to do intrusive error handling in them.
>
> Thanks,
> Laszlo
>
next prev parent reply other threads:[~2017-06-27 11:13 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20170623100913epcas5p3a10353593d6348b5bf3c890e2deaadb7@epcas5p3.samsung.com>
2017-06-23 10:09 ` [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol Amit Kumar
2017-06-26 1:19 ` Zeng, Star
2017-06-26 2:46 ` Zeng, Star
2017-06-27 0:47 ` Laszlo Ersek
2017-06-27 0:52 ` Zeng, Star
2017-06-27 1:37 ` Zeng, Star
2017-06-27 9:44 ` Laszlo Ersek
2017-06-27 9:59 ` Zeng, Star
2017-06-27 11:15 ` Laszlo Ersek [this message]
2017-06-27 13:23 ` Laszlo Ersek
2017-06-27 13:31 ` Zeng, Star
2017-06-28 9:34 ` Zeng, Star
2017-06-28 12:39 ` Laszlo Ersek
2017-06-28 13:00 ` Zeng, Star
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dc350a86-6caa-8fc1-9a53-fa2ae963efd3@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox