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

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

Hi Liran,

On 03/31/20 13:47, 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 | 150 +++++++++++++++++++------------------
>  1 file changed, 76 insertions(+), 74 deletions(-)

Thanks for the patch.

I didn't expect this approach -- hoisting the rings exposure from
SetupRings to Init --, but it turns out that I like it more than my own
previous suggestion -- pushing ResetAdapter into FreeRings :)

May I just trouble you with one further request?

> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 1ca50390c0e5..5b7fdcbda10b 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
> @@ -1157,6 +1154,10 @@ PvScsiInit (
>    if (EFI_ERROR (Status)) {
>      goto RestorePciAttributes;
>    }
> +  Status = PvScsiSetupRings (Dev);
> +  if (EFI_ERROR (Status)) {
> +    goto FreeRings;
> +  }

If you could move the PvScsiSetupRings() call *below* the DMA buffer
allocation (and rework the error path accordingly -- basically replacing
the "UnsetupRings" action with a "FreeDmaBuffer" action), then this
patch would be *perfect*.

Because then, the PvScsiUninit() order of actions would be a 100% mirror
for PvScsiInit()'s; namely with PvScsiResetAdapter() at the top of
PvScsiUninit() perfectly matching PvScsiSetupRings() at the end of
PvScsiInit()!

I don't want to annoy you too much, so if you really want to stick with
this version, I'll take it as well.

Thanks!
Laszlo

>  
>    //
>    // Allocate DMA communication buffer
> @@ -1168,7 +1169,7 @@ PvScsiInit (
>               &Dev->DmaBufDmaDesc
>               );
>    if (EFI_ERROR (Status)) {
> -    goto FreeRings;
> +    goto UnsetupRings;
>    }
>  
>    //
> @@ -1202,13 +1203,14 @@ PvScsiInit (
>  
>    return EFI_SUCCESS;
>  
> -FreeRings:
> +UnsetupRings:
>    //
>    // Reset device to stop device usage of the rings.
>    // This is required to safely free the rings.
>    //
>    PvScsiResetAdapter (Dev);
>  
> +FreeRings:
>    PvScsiFreeRings (Dev);
>  
>  RestorePciAttributes:
>