The patch uses newly introduced VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer()
to map system memory to device address and programs the vring descriptors
with device addresses.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/VirtioRngDxe/VirtioRng.h | 1 +
OvmfPkg/VirtioRngDxe/VirtioRng.c | 47 +++++++++++++++++---
2 files changed, 43 insertions(+), 5 deletions(-)
diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h b/OvmfPkg/VirtioRngDxe/VirtioRng.h
index 998f9fae48c2..b372d84c1ebc 100644
--- a/OvmfPkg/VirtioRngDxe/VirtioRng.h
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h
@@ -38,6 +38,7 @@ typedef struct {
EFI_EVENT ExitBoot; // DriverBindingStart 0
VRING Ring; // VirtioRingInit 2
EFI_RNG_PROTOCOL Rng; // VirtioRngInit 1
+ VOID *RingMap; // VirtioRngInit 1
} VIRTIO_RNG_DEV;
#define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
index e20602ac7225..5ff54867616a 100644
--- a/OvmfPkg/VirtioRngDxe/VirtioRng.c
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
@@ -140,11 +140,15 @@ VirtioRngGetRNG (
UINT32 Len;
UINT32 BufferSize;
EFI_STATUS Status;
+ UINTN NumPages;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+ VOID *Mapping;
if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
return EFI_INVALID_PARAMETER;
}
+ Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This);
//
// We only support the raw algorithm, so reject requests for anything else
//
@@ -153,12 +157,18 @@ VirtioRngGetRNG (
return EFI_UNSUPPORTED;
}
- Buffer = (volatile UINT8 *)AllocatePool (RNGValueLength);
- if (Buffer == NULL) {
+ NumPages = EFI_SIZE_TO_PAGES (RNGValueLength);
+ Status = VirtioAllocateSharedPages (Dev->VirtIo, NumPages, (VOID *)&Buffer);
+ if (EFI_ERROR (Status)) {
return EFI_DEVICE_ERROR;
}
- Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This);
+ Status = VirtioMapSharedBufferWrite (Dev->VirtIo, (VOID *)Buffer,
+ RNGValueLength, &DeviceAddress, &Mapping);
+ if (EFI_ERROR (Status)) {
+ VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer);
+ return EFI_DEVICE_ERROR;
+ }
//
// The Virtio RNG device may return less data than we asked it to, and can
@@ -170,7 +180,7 @@ VirtioRngGetRNG (
VirtioPrepare (&Dev->Ring, &Indices);
VirtioAppendDesc (&Dev->Ring,
- (UINTN)Buffer + Index,
+ (UINTN)DeviceAddress + Index,
BufferSize,
VRING_DESC_F_WRITE,
&Indices);
@@ -178,19 +188,22 @@ VirtioRngGetRNG (
if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) !=
EFI_SUCCESS) {
Status = EFI_DEVICE_ERROR;
+ VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping);
goto FreeBuffer;
}
ASSERT (Len > 0);
ASSERT (Len <= BufferSize);
}
+ VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping);
+
for (Index = 0; Index < RNGValueLength; Index++) {
RNGValue[Index] = Buffer[Index];
}
Status = EFI_SUCCESS;
FreeBuffer:
- FreePool ((VOID *)Buffer);
+ VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer);
return Status;
}
@@ -281,6 +294,11 @@ VirtioRngInit (
goto Failed;
}
+ Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &Dev->RingMap);
+ if (EFI_ERROR (Status)) {
+ goto ReleaseQueue;
+ }
+
//
// Additional steps for MMIO: align the queue appropriately, and set the
// size. If anything fails from here on, we must release the ring resources.
@@ -332,6 +350,11 @@ VirtioRngInit (
return EFI_SUCCESS;
ReleaseQueue:
+ if (Dev->RingMap != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap);
+ Dev->RingMap = NULL;
+ }
+
VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
Failed:
@@ -359,6 +382,11 @@ VirtioRngUninit (
// the old comms area.
//
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
+
+ if (Dev->RingMap != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap);
+ }
+
VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
}
@@ -385,6 +413,15 @@ VirtioRngExitBoot (
//
Dev = Context;
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
+
+ //
+ // If Ring mapping exist then umap it so that hypervisor will not able to
+ // get readable data after device reset.
+ //
+ if (Dev->RingMap != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap);
+ Dev->RingMap = NULL;
+ }
}
--
2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 08/07/17 13:58, Brijesh Singh wrote: > The patch uses newly introduced VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() > to map system memory to device address and programs the vring descriptors > with device addresses. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > OvmfPkg/VirtioRngDxe/VirtioRng.h | 1 + > OvmfPkg/VirtioRngDxe/VirtioRng.c | 47 +++++++++++++++++--- > 2 files changed, 43 insertions(+), 5 deletions(-) I'm skipping all the other device driver conversion patches for now (v1 8-13), because I think that this is the simplest driver, and we already have enough changes queued from this review. I suggest that for v2, you please update and test all of the drivers (so that we can be sure that my requests are feasible), but move this driver patch in front of the others. I'll say a number of generalities: * Please never roll-back earlier actions directly within an error-catching "if" -- instead, insert a new error handling label at the appropriate place, and jump to it with "goto". Sooner or later we'll grow yet more actions, and then we'll have to convert those "if" bodies to goto's anyway. * NULL for RingMap is a valid value () -- see also point (8) in my v1 03/14 feedback -- and device driver logic should not depend on it. Instead, use additional (precise) labels -- such as "UnmapQueue" and "UnmapBuffer" -- and matching goto statements. If a VirtIo->MapSharedBuffer() implementation gives you RingMap=NULL (on success), then VirtIo->UnmapSharedBuffer() will also take RingMap=NULL -- you simply don't have to be aware of the value. * In most cases, the fact that you have a live exit-boot-services callback implies that your device is "live" as well. So no need for additional checks before unmapping the queue. (There are exceptions of course, such as virtio-net -- IIRC the SimpleNetworkProtocol has an internal state flag that affects this question.) For example, in the virtio-rng case, the exit-boot-services notification function is installed in VirtioRngDriverBindingStart() *after* the call to VirtioRngInit(). In addition, it is uninstalled -- its associated event is closed -- in VirtioRngDriverBindingStop(), *before* the call to VirtioRngUninit(). So whenever the notification function can run, the device is guaranteed to be live. Thanks, Laszlo > diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h b/OvmfPkg/VirtioRngDxe/VirtioRng.h > index 998f9fae48c2..b372d84c1ebc 100644 > --- a/OvmfPkg/VirtioRngDxe/VirtioRng.h > +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h > @@ -38,6 +38,7 @@ typedef struct { > EFI_EVENT ExitBoot; // DriverBindingStart 0 > VRING Ring; // VirtioRingInit 2 > EFI_RNG_PROTOCOL Rng; // VirtioRngInit 1 > + VOID *RingMap; // VirtioRngInit 1 > } VIRTIO_RNG_DEV; > > #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \ > diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c > index e20602ac7225..5ff54867616a 100644 > --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c > +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c > @@ -140,11 +140,15 @@ VirtioRngGetRNG ( > UINT32 Len; > UINT32 BufferSize; > EFI_STATUS Status; > + UINTN NumPages; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > + VOID *Mapping; > > if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) { > return EFI_INVALID_PARAMETER; > } > > + Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This); > // > // We only support the raw algorithm, so reject requests for anything else > // > @@ -153,12 +157,18 @@ VirtioRngGetRNG ( > return EFI_UNSUPPORTED; > } > > - Buffer = (volatile UINT8 *)AllocatePool (RNGValueLength); > - if (Buffer == NULL) { > + NumPages = EFI_SIZE_TO_PAGES (RNGValueLength); > + Status = VirtioAllocateSharedPages (Dev->VirtIo, NumPages, (VOID *)&Buffer); > + if (EFI_ERROR (Status)) { > return EFI_DEVICE_ERROR; > } > > - Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This); > + Status = VirtioMapSharedBufferWrite (Dev->VirtIo, (VOID *)Buffer, > + RNGValueLength, &DeviceAddress, &Mapping); > + if (EFI_ERROR (Status)) { > + VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer); > + return EFI_DEVICE_ERROR; > + } > > // > // The Virtio RNG device may return less data than we asked it to, and can > @@ -170,7 +180,7 @@ VirtioRngGetRNG ( > > VirtioPrepare (&Dev->Ring, &Indices); > VirtioAppendDesc (&Dev->Ring, > - (UINTN)Buffer + Index, > + (UINTN)DeviceAddress + Index, > BufferSize, > VRING_DESC_F_WRITE, > &Indices); > @@ -178,19 +188,22 @@ VirtioRngGetRNG ( > if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) != > EFI_SUCCESS) { > Status = EFI_DEVICE_ERROR; > + VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping); > goto FreeBuffer; > } > ASSERT (Len > 0); > ASSERT (Len <= BufferSize); > } > > + VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping); > + > for (Index = 0; Index < RNGValueLength; Index++) { > RNGValue[Index] = Buffer[Index]; > } > Status = EFI_SUCCESS; > > FreeBuffer: > - FreePool ((VOID *)Buffer); > + VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer); > return Status; > } > > @@ -281,6 +294,11 @@ VirtioRngInit ( > goto Failed; > } > > + Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &Dev->RingMap); > + if (EFI_ERROR (Status)) { > + goto ReleaseQueue; > + } > + > // > // Additional steps for MMIO: align the queue appropriately, and set the > // size. If anything fails from here on, we must release the ring resources. > @@ -332,6 +350,11 @@ VirtioRngInit ( > return EFI_SUCCESS; > > ReleaseQueue: > + if (Dev->RingMap != NULL) { > + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap); > + Dev->RingMap = NULL; > + } > + > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > > Failed: > @@ -359,6 +382,11 @@ VirtioRngUninit ( > // the old comms area. > // > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > + > + if (Dev->RingMap != NULL) { > + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap); > + } > + > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > } > > @@ -385,6 +413,15 @@ VirtioRngExitBoot ( > // > Dev = Context; > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > + > + // > + // If Ring mapping exist then umap it so that hypervisor will not able to > + // get readable data after device reset. > + // > + if (Dev->RingMap != NULL) { > + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap); > + Dev->RingMap = NULL; > + } > } > > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 08/10/17 02:25, Laszlo Ersek wrote: > On 08/07/17 13:58, Brijesh Singh wrote: >> The patch uses newly introduced VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() >> to map system memory to device address and programs the vring descriptors >> with device addresses. >> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Tom Lendacky <thomas.lendacky@amd.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> OvmfPkg/VirtioRngDxe/VirtioRng.h | 1 + >> OvmfPkg/VirtioRngDxe/VirtioRng.c | 47 +++++++++++++++++--- >> 2 files changed, 43 insertions(+), 5 deletions(-) > > I'm skipping all the other device driver conversion patches for now (v1 > 8-13), because I think that this is the simplest driver, and we already > have enough changes queued from this review. > > I suggest that for v2, you please update and test all of the drivers (so > that we can be sure that my requests are feasible), but move this driver > patch in front of the others. > > I'll say a number of generalities: > > * Please never roll-back earlier actions directly within an > error-catching "if" -- instead, insert a new error handling label at > the appropriate place, and jump to it with "goto". Sooner or later > we'll grow yet more actions, and then we'll have to convert those "if" > bodies to goto's anyway. > > * NULL for RingMap is a valid value () -- see also point (8) in my v1 > 03/14 feedback -- and device driver logic should not depend on it. > Instead, use additional (precise) labels -- such as "UnmapQueue" and > "UnmapBuffer" -- and matching goto statements. > > If a VirtIo->MapSharedBuffer() implementation gives you RingMap=NULL > (on success), then VirtIo->UnmapSharedBuffer() will also take > RingMap=NULL -- you simply don't have to be aware of the value. > > * In most cases, the fact that you have a live exit-boot-services > callback implies that your device is "live" as well. So no need for > additional checks before unmapping the queue. (There are exceptions of > course, such as virtio-net -- IIRC the SimpleNetworkProtocol has an > internal state flag that affects this question.) > > For example, in the virtio-rng case, the exit-boot-services > notification function is installed in VirtioRngDriverBindingStart() > *after* the call to VirtioRngInit(). In addition, it is uninstalled -- > its associated event is closed -- in VirtioRngDriverBindingStop(), > *before* the call to VirtioRngUninit(). So whenever the notification > function can run, the device is guaranteed to be live. Something else: please don't forget about the VIRTIO_F_IOMMU_PLATFORM flag -- pls see point (5.3) in <http://mid.mail-archive.com/841bec5f-6f6e-8b1f-25ba-0fd37a915b72@redhat.com>. After this conversion is complete, we can claim that our virtio device drivers are "IOMMU aware", whenever we negotiate VIRTIO_F_VERSION_1. (It's a different question what IOMMU drivers we have.) Thanks, Laszlo >> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h b/OvmfPkg/VirtioRngDxe/VirtioRng.h >> index 998f9fae48c2..b372d84c1ebc 100644 >> --- a/OvmfPkg/VirtioRngDxe/VirtioRng.h >> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h >> @@ -38,6 +38,7 @@ typedef struct { >> EFI_EVENT ExitBoot; // DriverBindingStart 0 >> VRING Ring; // VirtioRingInit 2 >> EFI_RNG_PROTOCOL Rng; // VirtioRngInit 1 >> + VOID *RingMap; // VirtioRngInit 1 >> } VIRTIO_RNG_DEV; >> >> #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \ >> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c >> index e20602ac7225..5ff54867616a 100644 >> --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c >> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c >> @@ -140,11 +140,15 @@ VirtioRngGetRNG ( >> UINT32 Len; >> UINT32 BufferSize; >> EFI_STATUS Status; >> + UINTN NumPages; >> + EFI_PHYSICAL_ADDRESS DeviceAddress; >> + VOID *Mapping; >> >> if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) { >> return EFI_INVALID_PARAMETER; >> } >> >> + Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This); >> // >> // We only support the raw algorithm, so reject requests for anything else >> // >> @@ -153,12 +157,18 @@ VirtioRngGetRNG ( >> return EFI_UNSUPPORTED; >> } >> >> - Buffer = (volatile UINT8 *)AllocatePool (RNGValueLength); >> - if (Buffer == NULL) { >> + NumPages = EFI_SIZE_TO_PAGES (RNGValueLength); >> + Status = VirtioAllocateSharedPages (Dev->VirtIo, NumPages, (VOID *)&Buffer); >> + if (EFI_ERROR (Status)) { >> return EFI_DEVICE_ERROR; >> } >> >> - Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This); >> + Status = VirtioMapSharedBufferWrite (Dev->VirtIo, (VOID *)Buffer, >> + RNGValueLength, &DeviceAddress, &Mapping); >> + if (EFI_ERROR (Status)) { >> + VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer); >> + return EFI_DEVICE_ERROR; >> + } >> >> // >> // The Virtio RNG device may return less data than we asked it to, and can >> @@ -170,7 +180,7 @@ VirtioRngGetRNG ( >> >> VirtioPrepare (&Dev->Ring, &Indices); >> VirtioAppendDesc (&Dev->Ring, >> - (UINTN)Buffer + Index, >> + (UINTN)DeviceAddress + Index, >> BufferSize, >> VRING_DESC_F_WRITE, >> &Indices); >> @@ -178,19 +188,22 @@ VirtioRngGetRNG ( >> if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) != >> EFI_SUCCESS) { >> Status = EFI_DEVICE_ERROR; >> + VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping); >> goto FreeBuffer; >> } >> ASSERT (Len > 0); >> ASSERT (Len <= BufferSize); >> } >> >> + VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping); >> + >> for (Index = 0; Index < RNGValueLength; Index++) { >> RNGValue[Index] = Buffer[Index]; >> } >> Status = EFI_SUCCESS; >> >> FreeBuffer: >> - FreePool ((VOID *)Buffer); >> + VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer); >> return Status; >> } >> >> @@ -281,6 +294,11 @@ VirtioRngInit ( >> goto Failed; >> } >> >> + Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &Dev->RingMap); >> + if (EFI_ERROR (Status)) { >> + goto ReleaseQueue; >> + } >> + >> // >> // Additional steps for MMIO: align the queue appropriately, and set the >> // size. If anything fails from here on, we must release the ring resources. >> @@ -332,6 +350,11 @@ VirtioRngInit ( >> return EFI_SUCCESS; >> >> ReleaseQueue: >> + if (Dev->RingMap != NULL) { >> + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap); >> + Dev->RingMap = NULL; >> + } >> + >> VirtioRingUninit (Dev->VirtIo, &Dev->Ring); >> >> Failed: >> @@ -359,6 +382,11 @@ VirtioRngUninit ( >> // the old comms area. >> // >> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); >> + >> + if (Dev->RingMap != NULL) { >> + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap); >> + } >> + >> VirtioRingUninit (Dev->VirtIo, &Dev->Ring); >> } >> >> @@ -385,6 +413,15 @@ VirtioRngExitBoot ( >> // >> Dev = Context; >> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); >> + >> + // >> + // If Ring mapping exist then umap it so that hypervisor will not able to >> + // get readable data after device reset. >> + // >> + if (Dev->RingMap != NULL) { >> + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap); >> + Dev->RingMap = NULL; >> + } >> } >> >> >> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.