Re: [edk2-devel] [PATCH 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices()

["Laszlo Ersek" <lersek@redhat.com>]

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.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@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 <lersek@redhat.com>

Thanks,
Laszlo