On 03/16/20 16:01, Liran Alon wrote:
> This causes the device to forget about the request/completion rings.
> We allocated said rings in EfiBootServicesData type memory, and code
> executing after ExitBootServices() is permitted to overwrite it.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Reviewed-by: Nikita Leshenko <nikita.leshche...@oracle.com>
> Signed-off-by: Liran Alon <liran.a...@oracle.com>
> ---
> OvmfPkg/PvScsiDxe/PvScsi.c | 42 +++++++++++++++++++++++++++++++++++++-
> OvmfPkg/PvScsiDxe/PvScsi.h | 1 +
> 2 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index e7d0a23db6ab..33167c177b42 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -1156,6 +1156,30 @@ PvScsiUninit (
> }
> }
>
> +//
> +// Event notification called by ExitBootServices()
> +//
(1) Comment style -- please use:
/**
Blah.
**/
> +STATIC
> +VOID
> +EFIAPI
> +PvScsiExitBoot (
> + IN EFI_EVENT Event,
> + IN VOID *Context
> + )
> +{
> + PVSCSI_DEV *Dev;
> +
> + Dev = Context;
> +
> + //
> + // Reset the device. This causes the device to forget about the
> + // request/completion rings. We allocated said rings in EfiBootServicesData
> + // type memory, and code executing after ExitBootServices() is permitted to
> + // overwrite it.
> + //
> + PvScsiWriteCmdDesc (Dev, PVSCSI_CMD_ADAPTER_RESET, NULL, 0);
> +}
> +
(2) Please log a DEBUG_VERBOSE message here, something like:
DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
Notification functions for EVT_SIGNAL_EXIT_BOOT_SERVICES are very
important, and sometimes obscure bugs can be tracked down somewhat more
easily if the firmware log captures the order in which the handlers are
queued / executed.
> //
> // Driver Binding
> //
> @@ -1249,6 +1273,17 @@ PvScsiDriverBindingStart (
> goto ClosePciIo;
> }
>
> + Status = gBS->CreateEvent (
> + EVT_SIGNAL_EXIT_BOOT_SERVICES,
> + TPL_CALLBACK,
> + &PvScsiExitBoot,
> + Dev,
> + &Dev->ExitBoot
> + );
> + if (EFI_ERROR (Status)) {
> + goto UninitDev;
> + }
> +
> //
> // Setup complete, attempt to export the driver instance's PassThru
> interface
> //
> @@ -1260,11 +1295,14 @@ PvScsiDriverBindingStart (
> &Dev->PassThru
> );
> if (EFI_ERROR (Status)) {
> - goto UninitDev;
> + goto CloseExitBoot;
> }
>
> return EFI_SUCCESS;
>
> +CloseExitBoot:
> + gBS->CloseEvent (Dev->ExitBoot);
> +
> UninitDev:
> PvScsiUninit (Dev);
>
> @@ -1319,6 +1357,8 @@ PvScsiDriverBindingStop (
> return Status;
> }
>
> + gBS->CloseEvent (Dev->ExitBoot);
> +
> PvScsiUninit (Dev);
>
> gBS->CloseProtocol (
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 08e876b75930..e68a7dedf71f 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -41,6 +41,7 @@ typedef struct {
> typedef struct {
> UINT32 Signature;
> EFI_PCI_IO_PROTOCOL *PciIo;
> + EFI_EVENT ExitBoot;
> UINT64 OriginalPciAttributes;
> PVSCSI_RING_DESC RingDesc;
> PVSCSI_DMA_BUFFER *DmaBuf;
>
These parts look fine.
With (1) and (2) addressed:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks,
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56167): https://edk2.groups.io/g/devel/message/56167
Mute This Topic: https://groups.io/mt/72001273/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
