Re: [PATCH v2] OvmfPkg/PvScsiDxe: Refactor setup of rings to separate function

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

On 04/01/20 00:56, Liran Alon wrote:
> Previous to this change, PvScsiFreeRings() was not undoing all
> operations that was done by PvScsiInitRings().
> This is because PvScsiInitRings() was both preparing rings (Allocate
> memory and map it for device DMA) and setup the rings against device by
> issueing a device command. While PvScsiFreeRings() only unmaps the rings
> and free their memory.
> 
> Driver do not have a functional error as it makes sure to reset device
> before every call site to PvScsiFreeRings(). However, this is not
> intuitive.
> 
> Therefore, prefer to refactor the setup of the ring against device to a
> separate function than PvScsiInitRings().
> 
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/PvScsiDxe/PvScsi.c | 163 +++++++++++++++++++------------------
>  1 file changed, 85 insertions(+), 78 deletions(-)
> 
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 1ca50390c0e5..8c458ecceeb0 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -991,13 +991,6 @@ PvScsiInitRings (
>    )
>  {
>    EFI_STATUS Status;
> -  union {
> -    PVSCSI_CMD_DESC_SETUP_RINGS Cmd;
> -    UINT32                      Uint32;
> -  } AlignedCmd;
> -  PVSCSI_CMD_DESC_SETUP_RINGS *Cmd;
> -
> -  Cmd = &AlignedCmd.Cmd;
>  
>    Status = PvScsiAllocateSharedPages (
>               Dev,
> @@ -1032,6 +1025,69 @@ PvScsiInitRings (
>    }
>    ZeroMem (Dev->RingDesc.RingCmps, EFI_PAGE_SIZE);
>  
> +  return EFI_SUCCESS;
> +
> +FreeRingReqs:
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    Dev->RingDesc.RingReqs,
> +    &Dev->RingDesc.RingReqsDmaDesc
> +    );
> +
> +FreeRingState:
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    Dev->RingDesc.RingState,
> +    &Dev->RingDesc.RingStateDmaDesc
> +    );
> +
> +  return Status;
> +}
> +
> +STATIC
> +VOID
> +PvScsiFreeRings (
> +  IN OUT PVSCSI_DEV *Dev
> +  )
> +{
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    Dev->RingDesc.RingCmps,
> +    &Dev->RingDesc.RingCmpsDmaDesc
> +    );
> +
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    Dev->RingDesc.RingReqs,
> +    &Dev->RingDesc.RingReqsDmaDesc
> +    );
> +
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    Dev->RingDesc.RingState,
> +    &Dev->RingDesc.RingStateDmaDesc
> +    );
> +}
> +
> +STATIC
> +EFI_STATUS
> +PvScsiSetupRings (
> +  IN OUT PVSCSI_DEV *Dev
> +  )
> +{
> +  union {
> +    PVSCSI_CMD_DESC_SETUP_RINGS Cmd;
> +    UINT32                      Uint32;
> +  } AlignedCmd;
> +  PVSCSI_CMD_DESC_SETUP_RINGS *Cmd;
> +
> +  Cmd = &AlignedCmd.Cmd;
> +
>    ZeroMem (Cmd, sizeof (*Cmd));
>    Cmd->ReqRingNumPages = 1;
>    Cmd->CmpRingNumPages = 1;
> @@ -1052,71 +1108,12 @@ PvScsiInitRings (
>      sizeof (*Cmd) % sizeof (UINT32) == 0,
>      "Cmd must be multiple of 32-bit words"
>      );
> -  Status = PvScsiWriteCmdDesc (
> -             Dev,
> -             PvScsiCmdSetupRings,
> -             (UINT32 *)Cmd,
> -             sizeof (*Cmd) / sizeof (UINT32)
> -             );
> -  if (EFI_ERROR (Status)) {
> -    goto FreeRingCmps;
> -  }
> -
> -  return EFI_SUCCESS;
> -
> -FreeRingCmps:
> -  PvScsiFreeSharedPages (
> -    Dev,
> -    1,
> -    Dev->RingDesc.RingCmps,
> -    &Dev->RingDesc.RingCmpsDmaDesc
> -    );
> -
> -FreeRingReqs:
> -  PvScsiFreeSharedPages (
> -    Dev,
> -    1,
> -    Dev->RingDesc.RingReqs,
> -    &Dev->RingDesc.RingReqsDmaDesc
> -    );
> -
> -FreeRingState:
> -  PvScsiFreeSharedPages (
> -    Dev,
> -    1,
> -    Dev->RingDesc.RingState,
> -    &Dev->RingDesc.RingStateDmaDesc
> -    );
> -
> -  return Status;
> -}
> -
> -STATIC
> -VOID
> -PvScsiFreeRings (
> -  IN OUT PVSCSI_DEV *Dev
> -  )
> -{
> -  PvScsiFreeSharedPages (
> -    Dev,
> -    1,
> -    Dev->RingDesc.RingCmps,
> -    &Dev->RingDesc.RingCmpsDmaDesc
> -    );
> -
> -  PvScsiFreeSharedPages (
> -    Dev,
> -    1,
> -    Dev->RingDesc.RingReqs,
> -    &Dev->RingDesc.RingReqsDmaDesc
> -    );
> -
> -  PvScsiFreeSharedPages (
> -    Dev,
> -    1,
> -    Dev->RingDesc.RingState,
> -    &Dev->RingDesc.RingStateDmaDesc
> -    );
> +  return PvScsiWriteCmdDesc (
> +           Dev,
> +           PvScsiCmdSetupRings,
> +           (UINT32 *)Cmd,
> +           sizeof (*Cmd) / sizeof (UINT32)
> +           );
>  }
>  
>  STATIC
> @@ -1171,6 +1168,14 @@ PvScsiInit (
>      goto FreeRings;
>    }
>  
> +  //
> +  // Setup rings against device
> +  //
> +  Status = PvScsiSetupRings (Dev);
> +  if (EFI_ERROR (Status)) {
> +    goto FreeDMACommBuffer;

I'm going to rename this label to "FreeDmaCommBuffer" upon pushing (due
to de-capitalization of acronyms in edk2 CamelCase).

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thank you!
Laszlo

> +  }
> +
>    //
>    // Populate the exported interface's attributes
>    //
> @@ -1202,13 +1207,15 @@ PvScsiInit (
>  
>    return EFI_SUCCESS;
>  
> +FreeDMACommBuffer:
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
> +    Dev->DmaBuf,
> +    &Dev->DmaBufDmaDesc
> +    );
> +
>  FreeRings:
> -  //
> -  // Reset device to stop device usage of the rings.
> -  // This is required to safely free the rings.
> -  //
> -  PvScsiResetAdapter (Dev);
> -
>    PvScsiFreeRings (Dev);
>  
>  RestorePciAttributes:
>