| Message ID | 1489075441-23745-3-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | ArmVirtQemu: make DT vs ACPI support mutually exclusive | expand |
On Thu, Mar 09, 2017 at 05:04:00PM +0100, Ard Biesheuvel wrote: > Wait for ReadyToBoot to install the FDT configuration table. This allows Semantic nitpicking: "Wait for" sounds a little like a delay. "Defer FDT configuration table installation until ReadyToBoot is signaled."? Fold in if you agree that's an improvement. > any driver to make modifications in the mean time, and will also allow us > to defer the decision of whether to install it in the first place to later > on in the boot. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 41 ++++++++++++++++---- > ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf | 1 + > 2 files changed, 34 insertions(+), 8 deletions(-) > > diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c > index 7cc0c44ca12a..0327af5739f2 100644 > --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c > +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c > @@ -303,6 +303,30 @@ STATIC FDT_CLIENT_PROTOCOL mFdtClientProtocol = { > GetOrInsertChosenNode, > }; > > +STATIC > +VOID > +EFIAPI > +OnReadyToBoot ( > + EFI_EVENT Event, > + VOID *Context > + ) > +{ > + EFI_STATUS Status; > + > + if (!FeaturePcdGet (PcdPureAcpiBoot)) { > + // > + // Only install the FDT as a configuration table if we want to leave it up > + // to the OS to decide whether it prefers ACPI over DT. > + // I was going to complain about how it felt pointless to introduce this only to delete it in 2/3, but I really like how it improves the clarity of 3/3. Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > + Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase); > + ASSERT_EFI_ERROR (Status); > + } > + > + gBS->CloseEvent (Event); > +} > + > +STATIC EFI_EVENT ReadyToBootEvent; > + > EFI_STATUS > EFIAPI > InitializeFdtClientDxe ( > @@ -330,14 +354,15 @@ InitializeFdtClientDxe ( > > DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, mDeviceTreeBase)); > > - if (!FeaturePcdGet (PcdPureAcpiBoot)) { > - // > - // Only install the FDT as a configuration table if we want to leave it up > - // to the OS to decide whether it prefers ACPI over DT. > - // > - Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DeviceTreeBase); > - ASSERT_EFI_ERROR (Status); > - } > + Status = gBS->CreateEventEx ( > + EVT_NOTIFY_SIGNAL, > + TPL_CALLBACK, > + OnReadyToBoot, > + NULL, > + &gEfiEventReadyToBootGuid, > + &ReadyToBootEvent > + ); > + ASSERT_EFI_ERROR (Status); > > return gBS->InstallProtocolInterface (&ImageHandle, &gFdtClientProtocolGuid, > EFI_NATIVE_INTERFACE, &mFdtClientProtocol); > diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf > index 3a0cd37040eb..00017727c32c 100644 > --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf > +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf > @@ -42,6 +42,7 @@ [Protocols] > gFdtClientProtocolGuid ## PRODUCES > > [Guids] > + gEfiEventReadyToBootGuid > gFdtHobGuid > gFdtTableGuid > > -- > 2.7.4 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/09/17 17:04, Ard Biesheuvel wrote: > Wait for ReadyToBoot to install the FDT configuration table. This allows > any driver to make modifications in the mean time, and will also allow us > to defer the decision of whether to install it in the first place to later > on in the boot. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 41 ++++++++++++++++---- > ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf | 1 + > 2 files changed, 34 insertions(+), 8 deletions(-) > > diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c > index 7cc0c44ca12a..0327af5739f2 100644 > --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c > +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c > @@ -303,6 +303,30 @@ STATIC FDT_CLIENT_PROTOCOL mFdtClientProtocol = { > GetOrInsertChosenNode, > }; > > +STATIC > +VOID > +EFIAPI > +OnReadyToBoot ( > + EFI_EVENT Event, > + VOID *Context > + ) > +{ > + EFI_STATUS Status; > + > + if (!FeaturePcdGet (PcdPureAcpiBoot)) { > + // > + // Only install the FDT as a configuration table if we want to leave it up > + // to the OS to decide whether it prefers ACPI over DT. > + // > + Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase); > + ASSERT_EFI_ERROR (Status); > + } > + > + gBS->CloseEvent (Event); OK, you didn't forget to close the event. :) > +} > + > +STATIC EFI_EVENT ReadyToBootEvent; This should be called "mReadyToBootEvent". > + > EFI_STATUS > EFIAPI > InitializeFdtClientDxe ( > @@ -330,14 +354,15 @@ InitializeFdtClientDxe ( > > DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, mDeviceTreeBase)); > > - if (!FeaturePcdGet (PcdPureAcpiBoot)) { > - // > - // Only install the FDT as a configuration table if we want to leave it up > - // to the OS to decide whether it prefers ACPI over DT. > - // > - Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DeviceTreeBase); > - ASSERT_EFI_ERROR (Status); > - } > + Status = gBS->CreateEventEx ( > + EVT_NOTIFY_SIGNAL, > + TPL_CALLBACK, > + OnReadyToBoot, > + NULL, > + &gEfiEventReadyToBootGuid, > + &ReadyToBootEvent > + ); > + ASSERT_EFI_ERROR (Status); I guess it's OK to insist that this succeed. Also, this is done after the assignment to mDeviceTreeBase, so that's good. > > return gBS->InstallProtocolInterface (&ImageHandle, &gFdtClientProtocolGuid, > EFI_NATIVE_INTERFACE, &mFdtClientProtocol); However, if the protocol installation fails for any reason, we exit the driver with an error code, and the driver will be unloaded. We'll have a dangling callback pointer In case of failure, the event should be closed (== CreateEventEx() should be undone). > diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf > index 3a0cd37040eb..00017727c32c 100644 > --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf > +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf > @@ -42,6 +42,7 @@ [Protocols] > gFdtClientProtocolGuid ## PRODUCES > > [Guids] > + gEfiEventReadyToBootGuid > gFdtHobGuid > gFdtTableGuid > > For completeness, can you please also #include <Guid/EventGroup.h> for gEfiEventReadyToBootGuid's sake, in the C file? (It is already pulled in by something else, but, again, for completenes...) Looks okay to me otherwise. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c index 7cc0c44ca12a..0327af5739f2 100644 --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c @@ -303,6 +303,30 @@ STATIC FDT_CLIENT_PROTOCOL mFdtClientProtocol = { GetOrInsertChosenNode, }; +STATIC +VOID +EFIAPI +OnReadyToBoot ( + EFI_EVENT Event, + VOID *Context + ) +{ + EFI_STATUS Status; + + if (!FeaturePcdGet (PcdPureAcpiBoot)) { + // + // Only install the FDT as a configuration table if we want to leave it up + // to the OS to decide whether it prefers ACPI over DT. + // + Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase); + ASSERT_EFI_ERROR (Status); + } + + gBS->CloseEvent (Event); +} + +STATIC EFI_EVENT ReadyToBootEvent; + EFI_STATUS EFIAPI InitializeFdtClientDxe ( @@ -330,14 +354,15 @@ InitializeFdtClientDxe ( DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, mDeviceTreeBase)); - if (!FeaturePcdGet (PcdPureAcpiBoot)) { - // - // Only install the FDT as a configuration table if we want to leave it up - // to the OS to decide whether it prefers ACPI over DT. - // - Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DeviceTreeBase); - ASSERT_EFI_ERROR (Status); - } + Status = gBS->CreateEventEx ( + EVT_NOTIFY_SIGNAL, + TPL_CALLBACK, + OnReadyToBoot, + NULL, + &gEfiEventReadyToBootGuid, + &ReadyToBootEvent + ); + ASSERT_EFI_ERROR (Status); return gBS->InstallProtocolInterface (&ImageHandle, &gFdtClientProtocolGuid, EFI_NATIVE_INTERFACE, &mFdtClientProtocol); diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf index 3a0cd37040eb..00017727c32c 100644 --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf @@ -42,6 +42,7 @@ [Protocols] gFdtClientProtocolGuid ## PRODUCES [Guids] + gEfiEventReadyToBootGuid gFdtHobGuid gFdtTableGuid
Wait for ReadyToBoot to install the FDT configuration table. This allows any driver to make modifications in the mean time, and will also allow us to defer the decision of whether to install it in the first place to later on in the boot. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 41 ++++++++++++++++---- ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf | 1 + 2 files changed, 34 insertions(+), 8 deletions(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel