[PATCH edk2-platforms 1/4] Ext4Pkg: Use depex for unicode collation protocols

[PATCH edk2-platforms 1/4] Ext4Pkg: Use depex for unicode collation protocols

[Ard Biesheuvel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]

The EXT4 driver implements the UEFI driver model, which means it can
consume one protocol (marked with a TO_START comment in the .INF) and
produce another (marked as BY_START). The TO_START protocols are not
prerequisites for loading and starting the module, they are simply
protocols the driver may consume when taking part in the driver model
dance.

The Unicode collation protocols, however, are different: loading the
driver will fail if neither of those are present. So they are not
TO_START protocols, and they need to be mentioned in the DEPEX so that
the DXE core will not dispatch the driver before the producers of the
prerequisite protocols have been dispatched. Also, mark them as
SOMETIMES_CONSUMES, as only one of the two is required.

Note that this means the driver is not a UEFI_DRIVER but a DXE_DRIVER,
as UEFI drivers have a default DEPEX on the architectural protocols
only.

Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Marvin Häuser <mhaeuser@posteo.de>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
This fixes an observed failure when attempting to use this driver
Raspberry Pi4 as a builtin.

 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf
index a153fc41ccd6..1514020fa6a6 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf
@@ -86,7 +86,7 @@ [Defines]
   BASE_NAME                      = Ext4Dxe
   MODULE_UNI_FILE                = Ext4Dxe.uni
   FILE_GUID                      = 75F2B676-D73B-45CB-B7C1-303C7F4E6FD6
-  MODULE_TYPE                    = UEFI_DRIVER
+  MODULE_TYPE                    = DXE_DRIVER
   VERSION_STRING                 = 1.0
 
   ENTRY_POINT                    = Ext4EntryPoint
@@ -141,9 +141,12 @@ [Protocols]
   gEfiDiskIo2ProtocolGuid               ## TO_START
   gEfiBlockIoProtocolGuid               ## TO_START
   gEfiSimpleFileSystemProtocolGuid      ## BY_START
-  gEfiUnicodeCollationProtocolGuid      ## TO_START
-  gEfiUnicodeCollation2ProtocolGuid     ## TO_START
+  gEfiUnicodeCollationProtocolGuid      ## SOMETIMES_CONSUMES
+  gEfiUnicodeCollation2ProtocolGuid     ## SOMETIMES_CONSUMES
 
 [Pcd]
   gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLang           ## SOMETIMES_CONSUMES
   gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLang   ## SOMETIMES_CONSUMES
+
+[Depex]
+  gEfiUnicodeCollationProtocolGuid OR gEfiUnicodeCollation2ProtocolGuid
-- 
2.39.1

Re: [PATCH edk2-platforms 1/4] Ext4Pkg: Use depex for unicode collation protocols

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 1659 bytes --]

Hi Ard,

Thank you! Is "1/4" a mistake or did I miss the other 3? :)
Comments inline.

> On 17. Feb 2023, at 12:12, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> The Unicode collation protocols, however, are different: loading the
> driver will fail if neither of those are present. So they are not
> TO_START protocols, and they need to be mentioned in the DEPEX so that
> the DXE core will not dispatch the driver before the producers of the
> prerequisite protocols have been dispatched. Also, mark them as
> SOMETIMES_CONSUMES, as only one of the two is required.

Right. FatPkg solves this by probing for the protocol in Start() [1], which should guarantee that all entry points have been executed first, right? I'd prefer a universal and consistent solution to the issue and this looks fine, honestly.

[1]
https://github.com/tianocore/edk2/blob/02fcfdce1e5ce86f1951191883e7e30de5aa08be/FatPkg/EnhancedFatDxe/Fat.c#L381
https://github.com/tianocore/edk2/blob/02fcfdce1e5ce86f1951191883e7e30de5aa08be/FatPkg/EnhancedFatDxe/Fat.inf#L19

> -  MODULE_TYPE                    = UEFI_DRIVER
> +  MODULE_TYPE                    = DXE_DRIVER

Is it not unidiomatic to use the UEFI Driver Binding model (UEFI) in a DXE driver (UEFI PI)?

> +[Depex]
> +  gEfiUnicodeCollationProtocolGuid OR gEfiUnicodeCollation2ProtocolGuid

Hmm, this will have the side effect that Ext4Dxe may load before (some of) the architectural protocols, right (modulo implicit dependencies via the UC protocols)? This would need some careful analysis, or we need to add all of the architectural protocols to preserve the old behaviour.

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 2723 bytes --]

Re: [edk2-devel] [PATCH edk2-platforms 1/4] Ext4Pkg: Use depex for unicode collation protocols

[Ard Biesheuvel]

On Fri, 17 Feb 2023 at 15:05, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hi Ard,
>
> Thank you! Is "1/4" a mistake or did I miss the other 3? :)

Oops.

It was part of some RPi4 patches but I decided to send it out by itself.


> Comments inline.
>
> On 17. Feb 2023, at 12:12, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> The Unicode collation protocols, however, are different: loading the
> driver will fail if neither of those are present. So they are not
> TO_START protocols, and they need to be mentioned in the DEPEX so that
> the DXE core will not dispatch the driver before the producers of the
> prerequisite protocols have been dispatched. Also, mark them as
> SOMETIMES_CONSUMES, as only one of the two is required.
>
>
> Right. FatPkg solves this by probing for the protocol in Start() [1], which should guarantee that all entry points have been executed first, right? I'd prefer a universal and consistent solution to the issue and this looks fine, honestly.
>
> [1]
> https://github.com/tianocore/edk2/blob/02fcfdce1e5ce86f1951191883e7e30de5aa08be/FatPkg/EnhancedFatDxe/Fat.c#L381
> https://github.com/tianocore/edk2/blob/02fcfdce1e5ce86f1951191883e7e30de5aa08be/FatPkg/EnhancedFatDxe/Fat.inf#L19
>

I'd prefer using existing features for this, rather than open coding it.


> -  MODULE_TYPE                    = UEFI_DRIVER
> +  MODULE_TYPE                    = DXE_DRIVER
>
>
> Is it not unidiomatic to use the UEFI Driver Binding model (UEFI) in a DXE driver (UEFI PI)?
>

Perhaps. But having a hard dependency on protocols beyond the default
set defined for UEFI drivers is arguably even worse.


> +[Depex]
> +  gEfiUnicodeCollationProtocolGuid OR gEfiUnicodeCollation2ProtocolGuid
>
>
> Hmm, this will have the side effect that Ext4Dxe may load before (some of) the architectural protocols, right (modulo implicit dependencies via the UC protocols)? This would need some careful analysis, or we need to add all of the architectural protocols to preserve the old behaviour.
>

The ext4 driver does nothing except install protocols in its
entrypoint, and everything else that happens is under the control of
the UEFI driver model, afaict.

Re: [edk2-devel] [PATCH edk2-platforms 1/4] Ext4Pkg: Use depex for unicode collation protocols

[Marvin Häuser]

> On 17. Feb 2023, at 15:29, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Fri, 17 Feb 2023 at 15:05, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> Hi Ard,
>> 
>> Thank you! Is "1/4" a mistake or did I miss the other 3? :)
> 
> Oops.
> 
> It was part of some RPi4 patches but I decided to send it out by itself.
> 
> 
>> Comments inline.
>> 
>> On 17. Feb 2023, at 12:12, Ard Biesheuvel <ardb@kernel.org> wrote:
>> 
>> The Unicode collation protocols, however, are different: loading the
>> driver will fail if neither of those are present. So they are not
>> TO_START protocols, and they need to be mentioned in the DEPEX so that
>> the DXE core will not dispatch the driver before the producers of the
>> prerequisite protocols have been dispatched. Also, mark them as
>> SOMETIMES_CONSUMES, as only one of the two is required.
>> 
>> 
>> Right. FatPkg solves this by probing for the protocol in Start() [1], which should guarantee that all entry points have been executed first, right? I'd prefer a universal and consistent solution to the issue and this looks fine, honestly.
>> 
>> [1]
>> https://github.com/tianocore/edk2/blob/02fcfdce1e5ce86f1951191883e7e30de5aa08be/FatPkg/EnhancedFatDxe/Fat.c#L381
>> https://github.com/tianocore/edk2/blob/02fcfdce1e5ce86f1951191883e7e30de5aa08be/FatPkg/EnhancedFatDxe/Fat.inf#L19
>> 
> 
> I'd prefer using existing features for this, rather than open coding it.

It‘s not like we replicate a feature, we just move a function call to suit the control flow better. I‘d like a consistent solution and the FatPkg one looks fine to me.

> 
> 
>> -  MODULE_TYPE                    = UEFI_DRIVER
>> +  MODULE_TYPE                    = DXE_DRIVER
>> 
>> 
>> Is it not unidiomatic to use the UEFI Driver Binding model (UEFI) in a DXE driver (UEFI PI)?
>> 
> 
> Perhaps. But having a hard dependency on protocols beyond the default
> set defined for UEFI drivers is arguably even worse.

This is still very much a UEFI driver in a logical sense, this is just abusing the types to utilise DXE concepts. I‘m not opposed to such things in general, but here it looks unnecessary. It doesn’t help I’m not a big fan of the DXE dispatcher to begin with. :)

I agree the dependency is awkward, but I have to check the reason and alternatives later. In the end, it‘s Pedro‘s call anyway.

> 
> 
>> +[Depex]
>> +  gEfiUnicodeCollationProtocolGuid OR gEfiUnicodeCollation2ProtocolGuid
>> 
>> 
>> Hmm, this will have the side effect that Ext4Dxe may load before (some of) the architectural protocols, right (modulo implicit dependencies via the UC protocols)? This would need some careful analysis, or we need to add all of the architectural protocols to preserve the old behaviour.
>> 
> 
> The ext4 driver does nothing except install protocols in its
> entrypoint, and everything else that happens is under the control of
> the UEFI driver model, afaict.

I guess. There also is a chance that libraries like DebugLib enable advanced features only as core services become available. But probably not a big deal.

Best regards,
Marvin

Re: [edk2-devel] [PATCH edk2-platforms 1/4] Ext4Pkg: Use depex for unicode collation protocols

[Ard Biesheuvel]

On Fri, 17 Feb 2023 at 15:55, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> > On 17. Feb 2023, at 15:29, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Fri, 17 Feb 2023 at 15:05, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >>
> >> Hi Ard,
> >>
> >> Thank you! Is "1/4" a mistake or did I miss the other 3? :)
> >
> > Oops.
> >
> > It was part of some RPi4 patches but I decided to send it out by itself.
> >
> >
> >> Comments inline.
> >>
> >> On 17. Feb 2023, at 12:12, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> The Unicode collation protocols, however, are different: loading the
> >> driver will fail if neither of those are present. So they are not
> >> TO_START protocols, and they need to be mentioned in the DEPEX so that
> >> the DXE core will not dispatch the driver before the producers of the
> >> prerequisite protocols have been dispatched. Also, mark them as
> >> SOMETIMES_CONSUMES, as only one of the two is required.
> >>
> >>
> >> Right. FatPkg solves this by probing for the protocol in Start() [1], which should guarantee that all entry points have been executed first, right? I'd prefer a universal and consistent solution to the issue and this looks fine, honestly.
> >>
> >> [1]
> >> https://github.com/tianocore/edk2/blob/02fcfdce1e5ce86f1951191883e7e30de5aa08be/FatPkg/EnhancedFatDxe/Fat.c#L381
> >> https://github.com/tianocore/edk2/blob/02fcfdce1e5ce86f1951191883e7e30de5aa08be/FatPkg/EnhancedFatDxe/Fat.inf#L19
> >>
> >
> > I'd prefer using existing features for this, rather than open coding it.
>
> It‘s not like we replicate a feature, we just move a function call to suit the control flow better. I‘d like a consistent solution and the FatPkg one looks fine to me.
>

So the FAT driver will happily load, but then fail in an obscure
manner when being started on a controller handle, in a way that is
indistinguishable (afict) from a partition that has not FAT file
system in the first place.

So it doesn't look fine to me at all, tbh. If the collation protocols
are required, we should check for them in supported(), and make a lot
of noise in a DEBUG build if the failure condition is an unexpected
one, i.e., some additional protocol we dependent on (even though we
pretend to be a UEFI driver)  does not exist.

Alternatively, we might defer installation of the driver binding
protocol implementation to a callback on the collation protocol
installation, but that seems like more work for the same result.


> >
> >
> >> -  MODULE_TYPE                    = UEFI_DRIVER
> >> +  MODULE_TYPE                    = DXE_DRIVER
> >>
> >>
> >> Is it not unidiomatic to use the UEFI Driver Binding model (UEFI) in a DXE driver (UEFI PI)?
> >>
> >
> > Perhaps. But having a hard dependency on protocols beyond the default
> > set defined for UEFI drivers is arguably even worse.
>
> This is still very much a UEFI driver in a logical sense, this is just abusing the types to utilise DXE concepts. I‘m not opposed to such things in general, but here it looks unnecessary. It doesn’t help I’m not a big fan of the DXE dispatcher to begin with. :)
>

I will +1 any solution that removes the DXE dispatcher entirely as a
side effect, but unfortunately, that is not going to happen :-)

> I agree the dependency is awkward, but I have to check the reason and alternatives later. In the end, it‘s Pedro‘s call anyway.
>

Indeed.

> >
> >
> >> +[Depex]
> >> +  gEfiUnicodeCollationProtocolGuid OR gEfiUnicodeCollation2ProtocolGuid
> >>
> >>
> >> Hmm, this will have the side effect that Ext4Dxe may load before (some of) the architectural protocols, right (modulo implicit dependencies via the UC protocols)? This would need some careful analysis, or we need to add all of the architectural protocols to preserve the old behaviour.
> >>
> >
> > The ext4 driver does nothing except install protocols in its
> > entrypoint, and everything else that happens is under the control of
> > the UEFI driver model, afaict.
>
> I guess. There also is a chance that libraries like DebugLib enable advanced features only as core services become available. But probably not a big deal.
>

We could depex on the UEFI driver prerequisites as well as the
collation protocol ones, arguably retaining the UEFI_DRIVER status.

Re: [edk2-devel] [PATCH edk2-platforms 1/4] Ext4Pkg: Use depex for unicode collation protocols

[Marvin Häuser]

> On 17. Feb 2023, at 16:17, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> So the FAT driver will happily load, but then fail in an obscure
> manner when being started on a controller handle, in a way that is
> indistinguishable (afict) from a partition that has not FAT file
> system in the first place.
> 
> So it doesn't look fine to me at all, tbh. If the collation protocols
> are required, we should check for them in supported(), and make a lot
> of noise in a DEBUG build if the failure condition is an unexpected
> one, i.e., some additional protocol we dependent on (even though we
> pretend to be a UEFI driver)  does not exist.

Right, I meant the approach to rely on the UEFI Driver Binding handling to defer locating UC, not the exact implementation. Sounds good.

@Pedro What do you think?

> 
> Alternatively, we might defer installation of the driver binding
> protocol implementation to a callback on the collation protocol
> installation, but that seems like more work for the same result.

Agreed.

> I will +1 any solution that removes the DXE dispatcher entirely as a
> side effect, but unfortunately, that is not going to happen :-)

We will probably attempt something like this downstream at some point (we do not care about UEFI PI), so even more reason I dislike dependencies on its workings. In a lot of cases (like the architectural protocols), the dependency situation is much more simple than what Depex supports. I.e., the protocols often are unconditionally installed in the entry point. Then we can just re-order the modules at FD build-time rather than relying on runtime code in the dispatcher. Complex dependencies, if they cannot be avoided, may be rewritten with events, not sure yet.

> We could depex on the UEFI driver prerequisites as well as the
> collation protocol ones, arguably retaining the UEFI_DRIVER status.

That's what I meant, yes, but I prefer the above solution myself.

Best regards,
Marvin

Re: [edk2-devel] [PATCH edk2-platforms 1/4] Ext4Pkg: Use depex for unicode collation protocols

[Pedro Falcato]

On Fri, Feb 17, 2023 at 3:38 PM Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> > On 17. Feb 2023, at 16:17, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > So the FAT driver will happily load, but then fail in an obscure
> > manner when being started on a controller handle, in a way that is
> > indistinguishable (afict) from a partition that has not FAT file
> > system in the first place.
> >
> > So it doesn't look fine to me at all, tbh. If the collation protocols
> > are required, we should check for them in supported(), and make a lot
> > of noise in a DEBUG build if the failure condition is an unexpected
> > one, i.e., some additional protocol we dependent on (even though we
> > pretend to be a UEFI driver)  does not exist.

Hi Ard, Marvin,

First of all, thank you for raising this issue and sending a patch. I
had been quietly following the thread while trying to get my ideas
straight.

>
> Right, I meant the approach to rely on the UEFI Driver Binding handling to defer locating UC, not the exact implementation. Sounds good.
>
> @Pedro What do you think?
>

I think deferring the Unicode collation init to Start()/Supported() is
a good idea. I believe there's good value in keeping Ext4Dxe a
UEFI_DRIVER.
I was trying to come up with reasons why this would not work (in a
general UEFI-spec sense, not only in Tiano, but for instance if you
would ever want to plug this into i.e u-boot's EFI implementation).
As far as I can tell, there doesn't seem to be any? At least from a PI
PoV things should only get Start()'d in BDS right? And since other
PI-unaware implementations don't know what DXE or DXE_DRIVERs are,
it's not like that would fix those use cases.

> >
> > Alternatively, we might defer installation of the driver binding
> > protocol implementation to a callback on the collation protocol
> > installation, but that seems like more work for the same result.
>
> Agreed.

+1

-- 
Pedro

Re: [edk2-devel] [PATCH edk2-platforms 1/4] Ext4Pkg: Use depex for unicode collation protocols

[Ard Biesheuvel]

On Fri, 17 Feb 2023 at 18:31, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Fri, Feb 17, 2023 at 3:38 PM Marvin Häuser <mhaeuser@posteo.de> wrote:
> >
> >
> > > On 17. Feb 2023, at 16:17, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > So the FAT driver will happily load, but then fail in an obscure
> > > manner when being started on a controller handle, in a way that is
> > > indistinguishable (afict) from a partition that has not FAT file
> > > system in the first place.
> > >
> > > So it doesn't look fine to me at all, tbh. If the collation protocols
> > > are required, we should check for them in supported(), and make a lot
> > > of noise in a DEBUG build if the failure condition is an unexpected
> > > one, i.e., some additional protocol we dependent on (even though we
> > > pretend to be a UEFI driver)  does not exist.
>
> Hi Ard, Marvin,
>
> First of all, thank you for raising this issue and sending a patch. I
> had been quietly following the thread while trying to get my ideas
> straight.
>
> >
> > Right, I meant the approach to rely on the UEFI Driver Binding handling to defer locating UC, not the exact implementation. Sounds good.
> >
> > @Pedro What do you think?
> >
>
> I think deferring the Unicode collation init to Start()/Supported() is
> a good idea. I believe there's good value in keeping Ext4Dxe a
> UEFI_DRIVER.
> I was trying to come up with reasons why this would not work (in a
> general UEFI-spec sense, not only in Tiano, but for instance if you
> would ever want to plug this into i.e u-boot's EFI implementation).
> As far as I can tell, there doesn't seem to be any? At least from a PI
> PoV things should only get Start()'d in BDS right? And since other
> PI-unaware implementations don't know what DXE or DXE_DRIVERs are,
> it's not like that would fix those use cases.
>

Indeed. I suspect this was moved into Start() rather than Supported()
because the latter is called much more often by the driver model
logic. However, given that we populate a global variable with a
protocol pointer, we can simply skip this logic once we have managed
to set it once.