These rings are shared memory buffers between host and device in which
a cyclic buffer is managed to send request descriptors from host to
device and receive completion descriptors from device to host.
Note that because device may be constrained by IOMMU or guest may be run
under AMD SEV, we make sure to map these rings to device by using
PciIo->Map().
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 219 ++++++++++++++++++++++++++++++++
OvmfPkg/PvScsiDxe/PvScsi.h | 17 +++
OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 1 +
3 files changed, 237 insertions(+)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index cf75884350ee..c7d367e83a2d 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -11,11 +11,13 @@
#include <IndustryStandard/Pci.h>
#include <IndustryStandard/PvScsi.h>
+#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
#include <Protocol/PciIo.h>
+#include <Protocol/PciRootBridgeIo.h>
#include <Uefi/UefiSpec.h>
#include "PvScsi.h"
@@ -436,6 +438,207 @@ PvScsiRestorePciAttributes (
);
}
+STATIC
+EFI_STATUS
+PvScsiAllocateSharedPages (
+ IN PVSCSI_DEV *Dev,
+ IN UINTN Pages,
+ OUT VOID **HostAddress,
+ OUT PVSCSI_DMA_DESC *DmaDesc
+ )
+{
+ EFI_STATUS Status;
+ UINTN NumberOfBytes;
+
+ Status = Dev->PciIo->AllocateBuffer (
+ Dev->PciIo,
+ AllocateAnyPages,
+ EfiBootServicesData,
+ Pages,
+ HostAddress,
+ EFI_PCI_ATTRIBUTE_MEMORY_CACHED
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ NumberOfBytes = EFI_PAGES_TO_SIZE (Pages);
+ Status = Dev->PciIo->Map (
+ Dev->PciIo,
+ EfiPciIoOperationBusMasterCommonBuffer,
+ *HostAddress,
+ &NumberOfBytes,
+ &DmaDesc->DeviceAddress,
+ &DmaDesc->Mapping
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeBuffer;
+ }
+
+ if (NumberOfBytes != EFI_PAGES_TO_SIZE (Pages)) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Unmap;
+ }
+
+ return EFI_SUCCESS;
+
+Unmap:
+ Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
+
+FreeBuffer:
+ Dev->PciIo->FreeBuffer (Dev->PciIo, Pages, *HostAddress);
+
+ return Status;
+}
+
+STATIC
+VOID
+PvScsiFreeSharedPages (
+ IN PVSCSI_DEV *Dev,
+ IN UINTN Pages,
+ IN VOID *HostAddress,
+ IN PVSCSI_DMA_DESC *DmaDesc
+ )
+{
+ Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
+ Dev->PciIo->FreeBuffer (Dev->PciIo, Pages, HostAddress);
+}
+
+STATIC
+EFI_STATUS
+PvScsiInitRings (
+ IN OUT PVSCSI_DEV *Dev
+ )
+{
+ 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,
+ 1,
+ (VOID **)&Dev->RingDesc.RingState,
+ &Dev->RingDesc.RingStateDmaDesc
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ ZeroMem (Dev->RingDesc.RingState, EFI_PAGE_SIZE);
+
+ Status = PvScsiAllocateSharedPages (
+ Dev,
+ 1,
+ (VOID **)&Dev->RingDesc.RingReqs,
+ &Dev->RingDesc.RingReqsDmaDesc
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeRingState;
+ }
+ ZeroMem (Dev->RingDesc.RingReqs, EFI_PAGE_SIZE);
+
+ Status = PvScsiAllocateSharedPages (
+ Dev,
+ 1,
+ (VOID **)&Dev->RingDesc.RingCmps,
+ &Dev->RingDesc.RingCmpsDmaDesc
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeRingReqs;
+ }
+ ZeroMem (Dev->RingDesc.RingCmps, EFI_PAGE_SIZE);
+
+ ZeroMem (Cmd, sizeof (*Cmd));
+ Cmd->ReqRingNumPages = 1;
+ Cmd->CmpRingNumPages = 1;
+ Cmd->RingsStatePPN = RShiftU64 (
+ Dev->RingDesc.RingStateDmaDesc.DeviceAddress,
+ EFI_PAGE_SHIFT
+ );
+ Cmd->ReqRingPPNs[0] = RShiftU64 (
+ Dev->RingDesc.RingReqsDmaDesc.DeviceAddress,
+ EFI_PAGE_SHIFT
+ );
+ Cmd->CmpRingPPNs[0] = RShiftU64 (
+ Dev->RingDesc.RingCmpsDmaDesc.DeviceAddress,
+ EFI_PAGE_SHIFT
+ );
+
+ STATIC_ASSERT (
+ 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
+ );
+}
+
STATIC
EFI_STATUS
PvScsiInit (
@@ -466,6 +669,14 @@ PvScsiInit (
goto RestorePciAttributes;
}
+ //
+ // Init PVSCSI rings
+ //
+ Status = PvScsiInitRings (Dev);
+ if (EFI_ERROR (Status)) {
+ goto RestorePciAttributes;
+ }
+
//
// Populate the exported interface's attributes
//
@@ -509,6 +720,14 @@ PvScsiUninit (
IN OUT PVSCSI_DEV *Dev
)
{
+ //
+ // Reset device to stop device usage of the rings.
+ // This is required to safely free the rings.
+ //
+ PvScsiResetAdapter (Dev);
+
+ PvScsiFreeRings (Dev);
+
PvScsiRestorePciAttributes (Dev);
}
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 5f611dbbc98c..6d23b6e1eccf 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -15,12 +15,29 @@
#include <Library/DebugLib.h>
#include <Protocol/ScsiPassThruExt.h>
+typedef struct {
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+ VOID *Mapping;
+} PVSCSI_DMA_DESC;
+
+typedef struct {
+ PVSCSI_RINGS_STATE *RingState;
+ PVSCSI_DMA_DESC RingStateDmaDesc;
+
+ PVSCSI_RING_REQ_DESC *RingReqs;
+ PVSCSI_DMA_DESC RingReqsDmaDesc;
+
+ PVSCSI_RING_CMP_DESC *RingCmps;
+ PVSCSI_DMA_DESC RingCmpsDmaDesc;
+} PVSCSI_RING_DESC;
+
#define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
typedef struct {
UINT32 Signature;
EFI_PCI_IO_PROTOCOL *PciIo;
UINT64 OriginalPciAttributes;
+ PVSCSI_RING_DESC RingDesc;
UINT8 MaxTarget;
UINT8 MaxLun;
EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
index fcffc90d46c8..6200533698fc 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -26,6 +26,7 @@
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ BaseLib
BaseMemoryLib
DebugLib
MemoryAllocationLib
--
2.20.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56546): https://edk2.groups.io/g/devel/message/56546
Mute This Topic: https://groups.io/mt/72617147/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 03/28/20 21:00, Liran Alon wrote:
> These rings are shared memory buffers between host and device in which
> a cyclic buffer is managed to send request descriptors from host to
> device and receive completion descriptors from device to host.
>
> Note that because device may be constrained by IOMMU or guest may be run
> under AMD SEV, we make sure to map these rings to device by using
> PciIo->Map().
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
> OvmfPkg/PvScsiDxe/PvScsi.c | 219 ++++++++++++++++++++++++++++++++
> OvmfPkg/PvScsiDxe/PvScsi.h | 17 +++
> OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 1 +
> 3 files changed, 237 insertions(+)
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index cf75884350ee..c7d367e83a2d 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -11,11 +11,13 @@
>
> #include <IndustryStandard/Pci.h>
> #include <IndustryStandard/PvScsi.h>
> +#include <Library/BaseLib.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiLib.h>
> #include <Protocol/PciIo.h>
> +#include <Protocol/PciRootBridgeIo.h>
> #include <Uefi/UefiSpec.h>
>
> #include "PvScsi.h"
> @@ -436,6 +438,207 @@ PvScsiRestorePciAttributes (
> );
> }
>
> +STATIC
> +EFI_STATUS
> +PvScsiAllocateSharedPages (
> + IN PVSCSI_DEV *Dev,
> + IN UINTN Pages,
> + OUT VOID **HostAddress,
> + OUT PVSCSI_DMA_DESC *DmaDesc
> + )
> +{
> + EFI_STATUS Status;
> + UINTN NumberOfBytes;
> +
> + Status = Dev->PciIo->AllocateBuffer (
> + Dev->PciIo,
> + AllocateAnyPages,
> + EfiBootServicesData,
> + Pages,
> + HostAddress,
> + EFI_PCI_ATTRIBUTE_MEMORY_CACHED
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + NumberOfBytes = EFI_PAGES_TO_SIZE (Pages);
> + Status = Dev->PciIo->Map (
> + Dev->PciIo,
> + EfiPciIoOperationBusMasterCommonBuffer,
> + *HostAddress,
> + &NumberOfBytes,
> + &DmaDesc->DeviceAddress,
> + &DmaDesc->Mapping
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeBuffer;
> + }
> +
> + if (NumberOfBytes != EFI_PAGES_TO_SIZE (Pages)) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto Unmap;
> + }
> +
> + return EFI_SUCCESS;
> +
> +Unmap:
> + Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
> +
> +FreeBuffer:
> + Dev->PciIo->FreeBuffer (Dev->PciIo, Pages, *HostAddress);
> +
> + return Status;
> +}
> +
> +STATIC
> +VOID
> +PvScsiFreeSharedPages (
> + IN PVSCSI_DEV *Dev,
> + IN UINTN Pages,
> + IN VOID *HostAddress,
> + IN PVSCSI_DMA_DESC *DmaDesc
> + )
> +{
> + Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
> + Dev->PciIo->FreeBuffer (Dev->PciIo, Pages, HostAddress);
> +}
> +
> +STATIC
> +EFI_STATUS
> +PvScsiInitRings (
> + IN OUT PVSCSI_DEV *Dev
> + )
> +{
> + 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,
> + 1,
> + (VOID **)&Dev->RingDesc.RingState,
> + &Dev->RingDesc.RingStateDmaDesc
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + ZeroMem (Dev->RingDesc.RingState, EFI_PAGE_SIZE);
> +
> + Status = PvScsiAllocateSharedPages (
> + Dev,
> + 1,
> + (VOID **)&Dev->RingDesc.RingReqs,
> + &Dev->RingDesc.RingReqsDmaDesc
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeRingState;
> + }
> + ZeroMem (Dev->RingDesc.RingReqs, EFI_PAGE_SIZE);
> +
> + Status = PvScsiAllocateSharedPages (
> + Dev,
> + 1,
> + (VOID **)&Dev->RingDesc.RingCmps,
> + &Dev->RingDesc.RingCmpsDmaDesc
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeRingReqs;
> + }
> + ZeroMem (Dev->RingDesc.RingCmps, EFI_PAGE_SIZE);
> +
> + ZeroMem (Cmd, sizeof (*Cmd));
> + Cmd->ReqRingNumPages = 1;
> + Cmd->CmpRingNumPages = 1;
> + Cmd->RingsStatePPN = RShiftU64 (
> + Dev->RingDesc.RingStateDmaDesc.DeviceAddress,
> + EFI_PAGE_SHIFT
> + );
> + Cmd->ReqRingPPNs[0] = RShiftU64 (
> + Dev->RingDesc.RingReqsDmaDesc.DeviceAddress,
> + EFI_PAGE_SHIFT
> + );
> + Cmd->CmpRingPPNs[0] = RShiftU64 (
> + Dev->RingDesc.RingCmpsDmaDesc.DeviceAddress,
> + EFI_PAGE_SHIFT
> + );
> +
> + STATIC_ASSERT (
> + 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
> + );
> +}
> +
> STATIC
> EFI_STATUS
> PvScsiInit (
> @@ -466,6 +669,14 @@ PvScsiInit (
> goto RestorePciAttributes;
> }
>
> + //
> + // Init PVSCSI rings
> + //
> + Status = PvScsiInitRings (Dev);
> + if (EFI_ERROR (Status)) {
> + goto RestorePciAttributes;
> + }
> +
> //
> // Populate the exported interface's attributes
> //
> @@ -509,6 +720,14 @@ PvScsiUninit (
> IN OUT PVSCSI_DEV *Dev
> )
> {
> + //
> + // Reset device to stop device usage of the rings.
> + // This is required to safely free the rings.
> + //
> + PvScsiResetAdapter (Dev);
If I understand correctly (at the end of the series):
in order to save one of the (ultimately) two reset calls in
PvScsiUninit(), namely
- one for releasing the DMA buf
- and the other (internal to PvScsiFreeRings()) for releasing the rings,
you unnested the latter reset call from PvScsiFreeRings(), and added it
manually to the error path of PvScsiInit().
To me, that's more brittle and more difficult to reason about than what
I proposed, because now PvScsiFreeRings() does not completely undo
PvScsiInitRings(). I specifically requested that we please not try to
save on the two (seemingly) redundant reset calls in PvScsiUninit() --
see the paragraph containing "bad idea" here:
http://mid.mail-archive.com/45e5950c-ec82-c24b-4c38-2912402747b2@redhat.com
https://edk2.groups.io/g/devel/message/56425
(Note: we could be tempted to somehow "centralize" all of these
Reset operations into a single spot. Bad idea. We are revoking the
device's access rights to different resources, so the revocation
operations will show up in different spots. It's a mere circumstance
that the revocations all happen to be Reset operations.)
I might be paranoid of course -- I just feel that maybe-superfluous
reset operations on error paths are much better than silently
corrupted guest memory and/or disk contents.
You reacted to that message of mine, but not this paragraph specifically
(it was snipped from your followup) -- so I thought you were OK with it.
I'm a bit disappointed that you disagreed with my request *silently*.
To summarize: technically, your solution is correct; from a code hygiene
and resource ownership question, I'm convinced it is wrong.
But, I'll live with it.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
(Also, you didn't set up the "diff.orderFile" knob the way I requested
in point (8):
http://mid.mail-archive.com/0739202a-9b8a-759d-5809-2f2df69e9352@redhat.com
https://edk2.groups.io/g/devel/message/56420
and so the header file changes are again at the end of the patch...
Another thing I'll have to live with, I guess.)
Laszlo
> +
> + PvScsiFreeRings (Dev);
> +
> PvScsiRestorePciAttributes (Dev);
> }
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 5f611dbbc98c..6d23b6e1eccf 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -15,12 +15,29 @@
> #include <Library/DebugLib.h>
> #include <Protocol/ScsiPassThruExt.h>
>
> +typedef struct {
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
> + VOID *Mapping;
> +} PVSCSI_DMA_DESC;
> +
> +typedef struct {
> + PVSCSI_RINGS_STATE *RingState;
> + PVSCSI_DMA_DESC RingStateDmaDesc;
> +
> + PVSCSI_RING_REQ_DESC *RingReqs;
> + PVSCSI_DMA_DESC RingReqsDmaDesc;
> +
> + PVSCSI_RING_CMP_DESC *RingCmps;
> + PVSCSI_DMA_DESC RingCmpsDmaDesc;
> +} PVSCSI_RING_DESC;
> +
> #define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
>
> typedef struct {
> UINT32 Signature;
> EFI_PCI_IO_PROTOCOL *PciIo;
> UINT64 OriginalPciAttributes;
> + PVSCSI_RING_DESC RingDesc;
> UINT8 MaxTarget;
> UINT8 MaxLun;
> EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
> diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> index fcffc90d46c8..6200533698fc 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> +++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> @@ -26,6 +26,7 @@
> OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> + BaseLib
> BaseMemoryLib
> DebugLib
> MemoryAllocationLib
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56659): https://edk2.groups.io/g/devel/message/56659
Mute This Topic: https://groups.io/mt/72617147/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 30/03/2020 18:54, Laszlo Ersek wrote:
> On 03/28/20 21:00, Liran Alon wrote:
>> STATIC
>> EFI_STATUS
>> PvScsiInit (
>> @@ -466,6 +669,14 @@ PvScsiInit (
>> goto RestorePciAttributes;
>> }
>>
>> + //
>> + // Init PVSCSI rings
>> + //
>> + Status = PvScsiInitRings (Dev);
>> + if (EFI_ERROR (Status)) {
>> + goto RestorePciAttributes;
>> + }
>> +
>> //
>> // Populate the exported interface's attributes
>> //
>> @@ -509,6 +720,14 @@ PvScsiUninit (
>> IN OUT PVSCSI_DEV *Dev
>> )
>> {
>> + //
>> + // Reset device to stop device usage of the rings.
>> + // This is required to safely free the rings.
>> + //
>> + PvScsiResetAdapter (Dev);
> If I understand correctly (at the end of the series):
>
> in order to save one of the (ultimately) two reset calls in
> PvScsiUninit(), namely
> - one for releasing the DMA buf
> - and the other (internal to PvScsiFreeRings()) for releasing the rings,
> you unnested the latter reset call from PvScsiFreeRings(), and added it
> manually to the error path of PvScsiInit().
Right.
>
> To me, that's more brittle and more difficult to reason about than what
> I proposed, because now PvScsiFreeRings() does not completely undo
> PvScsiInitRings().
Hmm right. Because PvScsiInitRings() also setup the ring to the device
with a command. Haven't thought about it.
> I specifically requested that we please not try to
> save on the two (seemingly) redundant reset calls in PvScsiUninit() --
> see the paragraph containing "bad idea" here:
>
> https://urldefense.com/v3/__http://mid.mail-archive.com/45e5950c-ec82-c24b-4c38-2912402747b2@redhat.com__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-Fn-EyPU$
> https://urldefense.com/v3/__https://edk2.groups.io/g/devel/message/56425__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-4bfLR8U$
>
> (Note: we could be tempted to somehow "centralize" all of these
> Reset operations into a single spot. Bad idea. We are revoking the
> device's access rights to different resources, so the revocation
> operations will show up in different spots. It's a mere circumstance
> that the revocations all happen to be Reset operations.)
>
> I might be paranoid of course -- I just feel that maybe-superfluous
> reset operations on error paths are much better than silently
> corrupted guest memory and/or disk contents.
>
> You reacted to that message of mine, but not this paragraph specifically
> (it was snipped from your followup) -- so I thought you were OK with it.
> I'm a bit disappointed that you disagreed with my request *silently*.
Oh now I got what you meant! I misinterpreted it. Not silently ignored
it of course!
I can change this in a v4 patch-series if you would like that. Sorry for
the misunderstanding.
I do agree with your statement that PvScsiFreeRings() is not completely
an undo of PvScsiInitRings().
And maybe for making code a little bit easier to reason about, it's
preferred to just do one additional unnecessary device reset.
>
> To summarize: technically, your solution is correct; from a code hygiene
> and resource ownership question, I'm convinced it is wrong.
I agree.
>
> But, I'll live with it.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
I can send a v4 patch-series just changing that if you would like.
Or submit a patch to change it after it will be merged.
Or that you will change it when applying. Or leave it as is.
I don't have any objection to any of the above.
>
> (Also, you didn't set up the "diff.orderFile" knob the way I requested
> in point (8):
I actually did set it up in my ~/.gitconfig. It seems that it doesn't
work from some reason...
>
> https://urldefense.com/v3/__http://mid.mail-archive.com/0739202a-9b8a-759d-5809-2f2df69e9352@redhat.com__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-bs34O4s$
> https://urldefense.com/v3/__https://edk2.groups.io/g/devel/message/56420__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-msgEmGo$
>
> and so the header file changes are again at the end of the patch...
> Another thing I'll have to live with, I guess.)
>
> Laszlo
Thanks for the understanding,
-Liran
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56675): https://edk2.groups.io/g/devel/message/56675
Mute This Topic: https://groups.io/mt/72617147/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 03/30/20 19:24, Liran Alon wrote:
>
> On 30/03/2020 18:54, Laszlo Ersek wrote:
>> On 03/28/20 21:00, Liran Alon wrote:
>>> @@ -509,6 +720,14 @@ PvScsiUninit (
>>> IN OUT PVSCSI_DEV *Dev
>>> )
>>> {
>>> + //
>>> + // Reset device to stop device usage of the rings.
>>> + // This is required to safely free the rings.
>>> + //
>>> + PvScsiResetAdapter (Dev);
>> If I understand correctly (at the end of the series):
>>
>> in order to save one of the (ultimately) two reset calls in
>> PvScsiUninit(), namely
>> - one for releasing the DMA buf
>> - and the other (internal to PvScsiFreeRings()) for releasing the
>> rings, you unnested the latter reset call from PvScsiFreeRings(), and
>> added it manually to the error path of PvScsiInit().
> Right.
>> To me, that's more brittle and more difficult to reason about than
>> what I proposed, because now PvScsiFreeRings() does not completely
>> undo PvScsiInitRings().
> Hmm right. Because PvScsiInitRings() also setup the ring to the device
> with a command. Haven't thought about it.
>> I specifically requested that we please not try to save on the two
>> (seemingly) redundant reset calls in PvScsiUninit() -- see the
>> paragraph containing "bad idea" here:
>>
>> [...]
>>
>> (Note: we could be tempted to somehow "centralize" all of these
>> Reset operations into a single spot. Bad idea. We are revoking
>> the device's access rights to different resources, so the
>> revocation operations will show up in different spots. It's a
>> mere circumstance that the revocations all happen to be Reset
>> operations.)
>>
>> I might be paranoid of course -- I just feel that
>> maybe-superfluous reset operations on error paths are much
>> better than silently corrupted guest memory and/or disk
>> contents.
>>
>> You reacted to that message of mine, but not this paragraph
>> specifically (it was snipped from your followup) -- so I thought you
>> were OK with it. I'm a bit disappointed that you disagreed with my
>> request *silently*.
> Oh now I got what you meant! I misinterpreted it. Not silently ignored
> it of course!
> I can change this in a v4 patch-series if you would like that. Sorry
> for the misunderstanding.
>
> I do agree with your statement that PvScsiFreeRings() is not
> completely an undo of PvScsiInitRings().
> And maybe for making code a little bit easier to reason about, it's
> preferred to just do one additional unnecessary device reset.
>> To summarize: technically, your solution is correct; from a code
>> hygiene and resource ownership question, I'm convinced it is wrong.
> I agree.
>> But, I'll live with it.
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> I can send a v4 patch-series just changing that if you would like.
> Or submit a patch to change it after it will be merged.
> Or that you will change it when applying. Or leave it as is.
>
> I don't have any objection to any of the above.
OK. That's a relief to me, thank you! And I apologize for insinuating
that you deliberately & silently ignored said feedback from me. I tend
to be pedantic about email (it's not natural for me to think that
someone simply missed a comment) -- sorry about that!
Given that I've already merged v3 -- can you please post a followup
patch? Something like:
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 0a66c98421a9..3066b83b96fc 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -1093,6 +1093,12 @@ PvScsiFreeRings (
> IN OUT PVSCSI_DEV *Dev
> )
> {
> + //
> + // Reset device to stop device usage of the rings.
> + // This is required to safely free the rings.
> + //
> + PvScsiResetAdapter (Dev);
> +
> PvScsiFreeSharedPages (
> Dev,
> 1,
> @@ -1199,12 +1205,6 @@ PvScsiInit (
> return EFI_SUCCESS;
>
> FreeRings:
> - //
> - // Reset device to stop device usage of the rings.
> - // This is required to safely free the rings.
> - //
> - PvScsiResetAdapter (Dev);
> -
> PvScsiFreeRings (Dev);
>
> RestorePciAttributes:
> @@ -1220,12 +1220,8 @@ PvScsiUninit (
> )
> {
> //
> - // Reset device to:
> - // - Make device stop processing all requests.
> - // - Stop device usage of the rings.
> - //
> - // This is required to safely free the DMA communication buffer
> - // and the rings.
> + // Reset device to make device stop processing all requests.
> + // This is required to safely free the DMA communication buffer.
> //
> PvScsiResetAdapter (Dev);
Thank you!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56683): https://edk2.groups.io/g/devel/message/56683
Mute This Topic: https://groups.io/mt/72617147/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.