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

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

[Liran Alon]

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;
+  }
+
   //
   // 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:
-- 
2.20.1

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

[Laszlo Ersek]

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:
> 

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

[Liran Alon]

On 01/04/2020 13:41, Laszlo Ersek wrote:
> On 04/01/20 00:56, Liran Alon wrote:
>
>   
> +  //
> +  // 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).

Thanks. Will note this for next time.

-Liran

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

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

[Laszlo Ersek]

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(-)

Pushed as commit e210fc130e5c9738909dca432bbf8bf277ba6e37, via
<https://github.com/tianocore/edk2/pull/491>.

Thanks!
Laszlo