* [PATCH v2 0/8] OvmfPkg/VirtioNetDxe: map host address to device address
@ 2017-09-11 12:16 Brijesh Singh
2017-09-11 12:16 ` [PATCH v2 1/8] OvmfPkg/VirtioNetDxe: add helper VirtioNetUninitRing() Brijesh Singh
` (7 more replies)
0 siblings, 8 replies; 17+ messages in thread
From: Brijesh Singh @ 2017-09-11 12:16 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
The patch updates VirtioNetDxe to use IOMMU-like member functions to map
the system physical address to device address for buffers (including vring,
device specific request and response pointed by vring descriptor, and any
furter memory reference by those request and response).
Repo: https://github.com/codomania/edk2
Branch: virtionet-2
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>
Brijesh Singh (8):
OvmfPkg/VirtioNetDxe: add helper VirtioNetUninitRing()
OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap()
OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages()
OvmfPkg/VirtioNetDxe: dynamically alloc transmit header
OvmfPkg/VirtioNetDxe: update TechNotes
OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions
OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address
OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM
OvmfPkg/VirtioNetDxe/VirtioNet.inf | 1 +
OvmfPkg/VirtioNetDxe/VirtioNet.h | 51 ++++-
OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 30 ++-
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 209 ++++++++++++++----
OvmfPkg/VirtioNetDxe/SnpReceive.c | 5 +-
OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 222 +++++++++++++++++++-
OvmfPkg/VirtioNetDxe/SnpShutdown.c | 4 +-
OvmfPkg/VirtioNetDxe/SnpTransmit.c | 34 ++-
OvmfPkg/VirtioNetDxe/TechNotes.txt | 28 ++-
9 files changed, 516 insertions(+), 68 deletions(-)
--
2.9.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/8] OvmfPkg/VirtioNetDxe: add helper VirtioNetUninitRing()
2017-09-11 12:16 [PATCH v2 0/8] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
@ 2017-09-11 12:16 ` Brijesh Singh
2017-09-13 7:26 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 2/8] OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap() Brijesh Singh
` (6 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Brijesh Singh @ 2017-09-11 12:16 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
Consolidate the virtio VRING resource cleanup into VirtioNetUninitRing().
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/VirtioNetDxe/VirtioNet.h | 7 +++++++
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 4 ++--
OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 16 ++++++++++++++++
OvmfPkg/VirtioNetDxe/SnpShutdown.c | 4 ++--
OvmfPkg/VirtioNetDxe/TechNotes.txt | 5 +++--
5 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 710859bc6115..87a0f06e01a4 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -263,6 +263,13 @@ VirtioNetShutdownTx (
IN OUT VNET_DEV *Dev
);
+VOID
+EFIAPI
+VirtioNetUninitRing (
+ IN OUT VNET_DEV *Dev,
+ IN OUT VRING *Ring
+ );
+
//
// event callbacks
//
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 0ecfe044a977..637c978709fd 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -510,10 +510,10 @@ AbortDevice:
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
ReleaseTxRing:
- VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
+ VirtioNetUninitRing (Dev, &Dev->TxRing);
ReleaseRxRing:
- VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
+ VirtioNetUninitRing (Dev, &Dev->RxRing);
DeviceFailed:
//
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 9fedb72fdbd4..5b75eabc7a6b 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -51,3 +51,19 @@ VirtioNetShutdownTx (
{
FreePool (Dev->TxFreeStack);
}
+
+/**
+ Release TX and RX VRING resources.
+
+ @param[in,out] Dev The VNET_DEV driver instance which was using the ring.
+ @param[in,out] Ring The virtio ring to clean up.
+*/
+VOID
+EFIAPI
+VirtioNetUninitRing (
+ IN OUT VNET_DEV *Dev,
+ IN OUT VRING *Ring
+ )
+{
+ VirtioRingUninit (Dev->VirtIo, Ring);
+}
diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
index 5e84191fbbdd..432e0691d457 100644
--- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
+++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
@@ -67,8 +67,8 @@ VirtioNetShutdown (
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
VirtioNetShutdownRx (Dev);
VirtioNetShutdownTx (Dev);
- VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
- VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
+ VirtioNetUninitRing (Dev, &Dev->TxRing);
+ VirtioNetUninitRing (Dev, &Dev->RxRing);
Dev->Snm.State = EfiSimpleNetworkStarted;
Status = EFI_SUCCESS;
diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
index 9c1dfe6a773e..86b91f561495 100644
--- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
+++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
@@ -70,8 +70,9 @@ faithfully indented) that implement the transition.
VirtioNetInitialize | | VirtioNetShutdown
VirtioNetInitRing {Rx, Tx} | | VirtioNetShutdownRx [SnpSharedHelpers.c]
VirtioRingInit | | VirtioNetShutdownTx [SnpSharedHelpers.c]
- VirtioNetInitTx | | VirtioRingUninit {Tx, Rx}
- VirtioNetInitRx | |
+ VirtioNetInitTx | | VirtioNetUninitRing [SnpSharedHelpers.c]
+ VirtioNetInitRx | | {Tx, Rx}
+ | | VirtioRingUninit
v |
+-----------------------------+
| EfiSimpleNetworkInitialized |
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/8] OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap()
2017-09-11 12:16 [PATCH v2 0/8] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
2017-09-11 12:16 ` [PATCH v2 1/8] OvmfPkg/VirtioNetDxe: add helper VirtioNetUninitRing() Brijesh Singh
@ 2017-09-11 12:16 ` Brijesh Singh
2017-09-13 8:07 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 3/8] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages() Brijesh Singh
` (5 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Brijesh Singh @ 2017-09-11 12:16 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
When device is behind the IOMMU then driver need to pass the device
address when programing the bus master. The patch uses VirtioRingMap() to
map the VRING system physical address[es] to device address[es].
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/VirtioNetDxe/VirtioNet.h | 7 ++-
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 50 +++++++++++++++-----
OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 10 ++--
OvmfPkg/VirtioNetDxe/SnpShutdown.c | 4 +-
OvmfPkg/VirtioNetDxe/TechNotes.txt | 1 +
5 files changed, 55 insertions(+), 17 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 87a0f06e01a4..6762fc9d1d6e 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -82,10 +82,14 @@ typedef struct {
EFI_HANDLE MacHandle; // VirtioNetDriverBindingStart
VRING RxRing; // VirtioNetInitRing
+ VOID *RxRingMap; // VirtioRingMap and
+ // VirtioNetInitRing
UINT8 *RxBuf; // VirtioNetInitRx
UINT16 RxLastUsed; // VirtioNetInitRx
VRING TxRing; // VirtioNetInitRing
+ VOID *TxRingMap; // VirtioRingMap and
+ // VirtioNetInitRing
UINT16 TxMaxPending; // VirtioNetInitTx
UINT16 TxCurPending; // VirtioNetInitTx
UINT16 *TxFreeStack; // VirtioNetInitTx
@@ -267,7 +271,8 @@ VOID
EFIAPI
VirtioNetUninitRing (
IN OUT VNET_DEV *Dev,
- IN OUT VRING *Ring
+ IN OUT VRING *Ring,
+ IN VOID *RingMap
);
//
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 637c978709fd..8eabdbff6f5e 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -35,11 +35,13 @@
the network device.
@param[out] Ring The virtio-ring inside the VNET_DEV structure,
corresponding to Selector.
+ @param[out] Mapping A resulting token to pass to VirtioNetUninitRing()
@retval EFI_UNSUPPORTED The queue size reported by the virtio-net device is
too small.
@return Status codes from VIRTIO_CFG_WRITE(),
- VIRTIO_CFG_READ() and VirtioRingInit().
+ VIRTIO_CFG_READ(), VirtioRingInit() and
+ VirtioRingMap().
@retval EFI_SUCCESS Ring initialized.
*/
@@ -49,11 +51,14 @@ EFIAPI
VirtioNetInitRing (
IN OUT VNET_DEV *Dev,
IN UINT16 Selector,
- OUT VRING *Ring
+ OUT VRING *Ring,
+ OUT VOID **Mapping
)
{
EFI_STATUS Status;
UINT16 QueueSize;
+ UINT64 RingBaseShift;
+ VOID *MapInfo;
//
// step 4b -- allocate selected queue
@@ -80,29 +85,42 @@ VirtioNetInitRing (
}
//
+ // If anything fails from here on, we must release the ring resources.
+ //
+ Status = VirtioRingMap (Dev->VirtIo, Ring, &RingBaseShift, &MapInfo);
+ 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.
+ // size. If anything fails from here on, we must unmap the ring resources.
//
Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
if (EFI_ERROR (Status)) {
- goto ReleaseQueue;
+ goto UnmapQueue;
}
Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
if (EFI_ERROR (Status)) {
- goto ReleaseQueue;
+ goto UnmapQueue;
}
//
// step 4c -- report GPFN (guest-physical frame number) of queue
//
- Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, 0);
+ Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, RingBaseShift);
if (EFI_ERROR (Status)) {
- goto ReleaseQueue;
+ goto UnmapQueue;
}
+ *Mapping = MapInfo;
+
return EFI_SUCCESS;
+UnmapQueue:
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, MapInfo);
+
ReleaseQueue:
VirtioRingUninit (Dev->VirtIo, Ring);
@@ -456,12 +474,22 @@ VirtioNetInitialize (
//
// step 4b, 4c -- allocate and report virtqueues
//
- Status = VirtioNetInitRing (Dev, VIRTIO_NET_Q_RX, &Dev->RxRing);
+ Status = VirtioNetInitRing (
+ Dev,
+ VIRTIO_NET_Q_RX,
+ &Dev->RxRing,
+ &Dev->RxRingMap
+ );
if (EFI_ERROR (Status)) {
goto DeviceFailed;
}
- Status = VirtioNetInitRing (Dev, VIRTIO_NET_Q_TX, &Dev->TxRing);
+ Status = VirtioNetInitRing (
+ Dev,
+ VIRTIO_NET_Q_TX,
+ &Dev->TxRing,
+ &Dev->TxRingMap
+ );
if (EFI_ERROR (Status)) {
goto ReleaseRxRing;
}
@@ -510,10 +538,10 @@ AbortDevice:
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
ReleaseTxRing:
- VirtioNetUninitRing (Dev, &Dev->TxRing);
+ VirtioNetUninitRing (Dev, &Dev->TxRing, Dev->TxRingMap);
ReleaseRxRing:
- VirtioNetUninitRing (Dev, &Dev->RxRing);
+ VirtioNetUninitRing (Dev, &Dev->RxRing, Dev->RxRingMap);
DeviceFailed:
//
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 5b75eabc7a6b..57c7395848bd 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -55,15 +55,19 @@ VirtioNetShutdownTx (
/**
Release TX and RX VRING resources.
- @param[in,out] Dev The VNET_DEV driver instance which was using the ring.
- @param[in,out] Ring The virtio ring to clean up.
+ @param[in,out] Dev The VNET_DEV driver instance which was using
+ the ring.
+ @param[in,out] Ring The virtio ring to clean up.
+ @param[in] RingMap A token return from the VirtioRingMap()
*/
VOID
EFIAPI
VirtioNetUninitRing (
IN OUT VNET_DEV *Dev,
- IN OUT VRING *Ring
+ IN OUT VRING *Ring,
+ IN VOID *RingMap
)
{
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RingMap);
VirtioRingUninit (Dev->VirtIo, Ring);
}
diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
index 432e0691d457..d8c11f20de61 100644
--- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
+++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
@@ -67,8 +67,8 @@ VirtioNetShutdown (
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
VirtioNetShutdownRx (Dev);
VirtioNetShutdownTx (Dev);
- VirtioNetUninitRing (Dev, &Dev->TxRing);
- VirtioNetUninitRing (Dev, &Dev->RxRing);
+ VirtioNetUninitRing (Dev, &Dev->TxRing, Dev->TxRingMap);
+ VirtioNetUninitRing (Dev, &Dev->RxRing, Dev->RxRingMap);
Dev->Snm.State = EfiSimpleNetworkStarted;
Status = EFI_SUCCESS;
diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
index 86b91f561495..0891e8210489 100644
--- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
+++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
@@ -72,6 +72,7 @@ faithfully indented) that implement the transition.
VirtioRingInit | | VirtioNetShutdownTx [SnpSharedHelpers.c]
VirtioNetInitTx | | VirtioNetUninitRing [SnpSharedHelpers.c]
VirtioNetInitRx | | {Tx, Rx}
+ | | VirtIo->UnmapSharedBuffer
| | VirtioRingUninit
v |
+-----------------------------+
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/8] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages()
2017-09-11 12:16 [PATCH v2 0/8] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
2017-09-11 12:16 ` [PATCH v2 1/8] OvmfPkg/VirtioNetDxe: add helper VirtioNetUninitRing() Brijesh Singh
2017-09-11 12:16 ` [PATCH v2 2/8] OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap() Brijesh Singh
@ 2017-09-11 12:16 ` Brijesh Singh
2017-09-13 9:14 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 4/8] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header Brijesh Singh
` (4 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Brijesh Singh @ 2017-09-11 12:16 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
When device is behind the IOMMU, VirtioNetDxe is required to use the
device address in bus master operations. RxBuf is allocated using
AllocatePool() which returns the system physical address.
The patch uses VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages() to allocate
the RxBuf and map with VirtioMapAllBytesInSharedBuffer() so that we can
obtain the device address for RxBuf.
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/VirtioNetDxe/VirtioNet.h | 4 +
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 77 +++++++++++++++-----
OvmfPkg/VirtioNetDxe/SnpReceive.c | 5 +-
OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 7 +-
OvmfPkg/VirtioNetDxe/TechNotes.txt | 2 +-
5 files changed, 74 insertions(+), 21 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 6762fc9d1d6e..7d5f33b01dc8 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -4,6 +4,7 @@
Protocol instances for virtio-net devices.
Copyright (C) 2013, Red Hat, Inc.
+ Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -86,6 +87,8 @@ typedef struct {
// VirtioNetInitRing
UINT8 *RxBuf; // VirtioNetInitRx
UINT16 RxLastUsed; // VirtioNetInitRx
+ UINTN RxBufNrPages; // VirtioNetInitRx
+ VOID *RxBufMap; // VirtioNetInitRx
VRING TxRing; // VirtioNetInitRing
VOID *TxRingMap; // VirtioRingMap and
@@ -95,6 +98,7 @@ typedef struct {
UINT16 *TxFreeStack; // VirtioNetInitTx
VIRTIO_1_0_NET_REQ TxSharedReq; // VirtioNetInitTx
UINT16 TxLastUsed; // VirtioNetInitTx
+ EFI_PHYSICAL_ADDRESS RxBufDeviceBase; // VirtioNetInitRx
} VNET_DEV;
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 8eabdbff6f5e..54c808c501bf 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -242,8 +242,10 @@ VirtioNetInitTx (
@param[in,out] Dev The VNET_DEV driver instance about to enter the
EfiSimpleNetworkInitialized state.
- @retval EFI_OUT_OF_RESOURCES Failed to allocate RX destination area.
- @return Status codes from VIRTIO_CFG_WRITE().
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate or map RX destination area.
+ @return Status codes from VIRTIO_CFG_WRITE() or
+ VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages or
+ VirtioMapAllBytesInSharedBuffer().
@retval EFI_SUCCESS RX setup successful. The device is live and may
already be writing to the receive area.
*/
@@ -255,13 +257,15 @@ VirtioNetInitRx (
IN OUT VNET_DEV *Dev
)
{
- EFI_STATUS Status;
- UINTN VirtioNetReqSize;
- UINTN RxBufSize;
- UINT16 RxAlwaysPending;
- UINTN PktIdx;
- UINT16 DescIdx;
- UINT8 *RxPtr;
+ EFI_STATUS Status;
+ UINTN VirtioNetReqSize;
+ UINTN RxBufSize;
+ UINT16 RxAlwaysPending;
+ UINTN PktIdx;
+ UINT16 DescIdx;
+ UINTN NumBytes;
+ EFI_PHYSICAL_ADDRESS RxBufDeviceAddress;
+ VOID *RxBuffer;
//
// In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on
@@ -286,11 +290,37 @@ VirtioNetInitRx (
//
RxAlwaysPending = (UINT16) MIN (Dev->RxRing.QueueSize / 2, VNET_MAX_PENDING);
- Dev->RxBuf = AllocatePool (RxAlwaysPending * RxBufSize);
- if (Dev->RxBuf == NULL) {
- return EFI_OUT_OF_RESOURCES;
+ //
+ // The RxBuf is shared between guest and hypervisor, use
+ // AllocateSharedPages() to allocate this memory region and map it with
+ // BusMasterCommonBuffer so that it can be accessed by both guest and
+ // hypervisor.
+ //
+ NumBytes = RxAlwaysPending * RxBufSize;
+ Dev->RxBufNrPages = EFI_SIZE_TO_PAGES (NumBytes);
+ Status = Dev->VirtIo->AllocateSharedPages (
+ Dev->VirtIo,
+ Dev->RxBufNrPages,
+ &RxBuffer
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
}
+ Status = VirtioMapAllBytesInSharedBuffer (
+ Dev->VirtIo,
+ VirtioOperationBusMasterCommonBuffer,
+ RxBuffer,
+ NumBytes,
+ &Dev->RxBufDeviceBase,
+ &Dev->RxBufMap
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeSharedBuffer;
+ }
+
+ Dev->RxBuf = RxBuffer;
+
//
// virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device
//
@@ -310,7 +340,7 @@ VirtioNetInitRx (
// link each chain into (from) the available ring as well
//
DescIdx = 0;
- RxPtr = Dev->RxBuf;
+ RxBufDeviceAddress = Dev->RxBufDeviceBase;
for (PktIdx = 0; PktIdx < RxAlwaysPending; ++PktIdx) {
//
// virtio-0.9.5, 2.4.1.2 Updating the Available Ring
@@ -321,16 +351,16 @@ VirtioNetInitRx (
//
// virtio-0.9.5, 2.4.1.1 Placing Buffers into the Descriptor Table
//
- Dev->RxRing.Desc[DescIdx].Addr = (UINTN) RxPtr;
+ Dev->RxRing.Desc[DescIdx].Addr = RxBufDeviceAddress;
Dev->RxRing.Desc[DescIdx].Len = (UINT32) VirtioNetReqSize;
Dev->RxRing.Desc[DescIdx].Flags = VRING_DESC_F_WRITE | VRING_DESC_F_NEXT;
Dev->RxRing.Desc[DescIdx].Next = (UINT16) (DescIdx + 1);
- RxPtr += Dev->RxRing.Desc[DescIdx++].Len;
+ RxBufDeviceAddress += Dev->RxRing.Desc[DescIdx++].Len;
- Dev->RxRing.Desc[DescIdx].Addr = (UINTN) RxPtr;
+ Dev->RxRing.Desc[DescIdx].Addr = RxBufDeviceAddress;
Dev->RxRing.Desc[DescIdx].Len = (UINT32) (RxBufSize - VirtioNetReqSize);
Dev->RxRing.Desc[DescIdx].Flags = VRING_DESC_F_WRITE;
- RxPtr += Dev->RxRing.Desc[DescIdx++].Len;
+ RxBufDeviceAddress += Dev->RxRing.Desc[DescIdx++].Len;
}
//
@@ -351,10 +381,21 @@ VirtioNetInitRx (
Status = Dev->VirtIo->SetQueueNotify (Dev->VirtIo, VIRTIO_NET_Q_RX);
if (EFI_ERROR (Status)) {
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
- FreePool (Dev->RxBuf);
+ goto UnmapSharedBuffer;
}
return Status;
+
+UnmapSharedBuffer:
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
+
+FreeSharedBuffer:
+ Dev->VirtIo->FreeSharedPages (
+ Dev->VirtIo,
+ Dev->RxBufNrPages,
+ RxBuffer
+ );
+ return Status;
}
diff --git a/OvmfPkg/VirtioNetDxe/SnpReceive.c b/OvmfPkg/VirtioNetDxe/SnpReceive.c
index 99abd7ebe454..c42489636ea0 100644
--- a/OvmfPkg/VirtioNetDxe/SnpReceive.c
+++ b/OvmfPkg/VirtioNetDxe/SnpReceive.c
@@ -82,6 +82,7 @@ VirtioNetReceive (
UINT8 *RxPtr;
UINT16 AvailIdx;
EFI_STATUS NotifyStatus;
+ UINTN RxBufOffset;
if (This == NULL || BufferSize == NULL || Buffer == NULL) {
return EFI_INVALID_PARAMETER;
@@ -143,7 +144,9 @@ VirtioNetReceive (
*HeaderSize = Dev->Snm.MediaHeaderSize;
}
- RxPtr = (UINT8 *)(UINTN) Dev->RxRing.Desc[DescIdx + 1].Addr;
+ RxBufOffset = (UINTN)(Dev->RxRing.Desc[DescIdx + 1].Addr -
+ Dev->RxBufDeviceBase);
+ RxPtr = Dev->RxBuf + RxBufOffset;
CopyMem (Buffer, RxPtr, RxLen);
if (DestAddr != NULL) {
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 57c7395848bd..ee4f9ed36ecd 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -39,7 +39,12 @@ VirtioNetShutdownRx (
IN OUT VNET_DEV *Dev
)
{
- FreePool (Dev->RxBuf);
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
+ Dev->VirtIo->FreeSharedPages (
+ Dev->VirtIo,
+ Dev->RxBufNrPages,
+ Dev->RxBuf
+ );
}
diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
index 0891e8210489..f39426fb13e4 100644
--- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
+++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
@@ -247,7 +247,7 @@ In VirtioNetInitRx, the guest allocates the fixed size Receive Destination
Area, which accommodates all packets delivered asynchronously by the host. To
each packet, a slice of this area is dedicated; each slice is further
subdivided into virtio-net request header and network packet data. The
-(guest-physical) addresses of these sub-slices are denoted with A2, A3, A4 and
+(device-physical) addresses of these sub-slices are denoted with A2, A3, A4 and
so on. Importantly, an even-subscript "A" always belongs to a virtio-net
request header, while an odd-subscript "A" always belongs to a packet
sub-slice.
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 4/8] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header
2017-09-11 12:16 [PATCH v2 0/8] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
` (2 preceding siblings ...)
2017-09-11 12:16 ` [PATCH v2 3/8] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages() Brijesh Singh
@ 2017-09-11 12:16 ` Brijesh Singh
2017-09-13 14:31 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 5/8] OvmfPkg/VirtioNetDxe: update TechNotes Brijesh Singh
` (3 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Brijesh Singh @ 2017-09-11 12:16 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
Each network packet is submitted for transmission by pushing the head
descriptor of a two-part descriptor chain to the Available Ring of the
TX queue. VirtioNetInitTx() sets up the the descriptor chains for all
queueable packets in advance, and points all the head descriptors to the
same shared, never modified, VIRTIO_1_0_NET_REQ header object (or its
initial VIRTIO_NET_REQ sub-object, dependent on virtio version).
VirtioNetInitTx() currently uses the header object's system physical
address for populating the head descriptors.
When device is behind the IOMMU, VirtioNet driver is required to provide
the device address of VIRTIO_1_0_NET_REQ header. In this patch we
dynamically allocate the header using AllocateSharedPages() and map with
BusMasterCommonBuffer so that header can be accessed by both processor
and the device.
We map the header object for CommonBuffer operation because, in order to
stick with the current code order, we populate the head descriptors with
the header's device address first, and fill in the header itself second.
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/VirtioNetDxe/VirtioNet.h | 3 +-
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 64 +++++++++++++++++---
OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 7 +++
3 files changed, 65 insertions(+), 9 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 7d5f33b01dc8..3f48bcc6b67c 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -96,7 +96,8 @@ typedef struct {
UINT16 TxMaxPending; // VirtioNetInitTx
UINT16 TxCurPending; // VirtioNetInitTx
UINT16 *TxFreeStack; // VirtioNetInitTx
- VIRTIO_1_0_NET_REQ TxSharedReq; // VirtioNetInitTx
+ VIRTIO_1_0_NET_REQ *TxSharedReq; // VirtioNetInitTx
+ VOID *TxSharedReqMap; // VirtioNetInitTx
UINT16 TxLastUsed; // VirtioNetInitTx
EFI_PHYSICAL_ADDRESS RxBufDeviceBase; // VirtioNetInitRx
} VNET_DEV;
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 54c808c501bf..6cedb406a172 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -18,6 +18,7 @@
**/
#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
@@ -147,6 +148,9 @@ ReleaseQueue:
@retval EFI_OUT_OF_RESOURCES Failed to allocate the stack to track the heads
of free descriptor chains.
+ @return Status codes from VIRTIO_DEVICE_PROTOCOL.
+ AllocateSharedPages() or
+ VirtioMapAllBytesInSharedBuffer()
@retval EFI_SUCCESS TX setup successful.
*/
@@ -157,8 +161,11 @@ VirtioNetInitTx (
IN OUT VNET_DEV *Dev
)
{
- UINTN TxSharedReqSize;
- UINTN PktIdx;
+ UINTN TxSharedReqSize;
+ UINTN PktIdx;
+ EFI_STATUS Status;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+ VOID *TxSharedReqBuffer;
Dev->TxMaxPending = (UINT16) MIN (Dev->TxRing.QueueSize / 2,
VNET_MAX_PENDING);
@@ -170,12 +177,42 @@ VirtioNetInitTx (
}
//
+ // Allocate TxSharedReq header and map with BusMasterCommonBuffer so that it
+ // can be accessed equally by both processor and device.
+ //
+ Status = Dev->VirtIo->AllocateSharedPages (
+ Dev->VirtIo,
+ EFI_SIZE_TO_PAGES (sizeof *Dev->TxSharedReq),
+ &TxSharedReqBuffer
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeTxFreeStack;
+ }
+
+ ZeroMem (TxSharedReqBuffer, sizeof *Dev->TxSharedReq);
+
+ Status = VirtioMapAllBytesInSharedBuffer (
+ Dev->VirtIo,
+ VirtioOperationBusMasterCommonBuffer,
+ TxSharedReqBuffer,
+ sizeof *(Dev->TxSharedReq),
+ &DeviceAddress,
+ &Dev->TxSharedReqMap
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeTxSharedReqBuffer;
+ }
+
+ Dev->TxSharedReq = TxSharedReqBuffer;
+
+
+ //
// In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on
// VIRTIO_NET_F_MRG_RXBUF, which we never negotiate.
//
TxSharedReqSize = (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) ?
- sizeof Dev->TxSharedReq.V0_9_5 :
- sizeof Dev->TxSharedReq;
+ sizeof (Dev->TxSharedReq->V0_9_5) :
+ sizeof *Dev->TxSharedReq;
for (PktIdx = 0; PktIdx < Dev->TxMaxPending; ++PktIdx) {
UINT16 DescIdx;
@@ -187,7 +224,7 @@ VirtioNetInitTx (
// For each possibly pending packet, lay out the descriptor for the common
// (unmodified by the host) virtio-net request header.
//
- Dev->TxRing.Desc[DescIdx].Addr = (UINTN) &Dev->TxSharedReq;
+ Dev->TxRing.Desc[DescIdx].Addr = DeviceAddress;
Dev->TxRing.Desc[DescIdx].Len = (UINT32) TxSharedReqSize;
Dev->TxRing.Desc[DescIdx].Flags = VRING_DESC_F_NEXT;
Dev->TxRing.Desc[DescIdx].Next = (UINT16) (DescIdx + 1);
@@ -202,13 +239,13 @@ VirtioNetInitTx (
//
// virtio-0.9.5, Appendix C, Packet Transmission
//
- Dev->TxSharedReq.V0_9_5.Flags = 0;
- Dev->TxSharedReq.V0_9_5.GsoType = VIRTIO_NET_HDR_GSO_NONE;
+ Dev->TxSharedReq->V0_9_5.Flags = 0;
+ Dev->TxSharedReq->V0_9_5.GsoType = VIRTIO_NET_HDR_GSO_NONE;
//
// For VirtIo 1.0 only -- the field exists, but it is unused
//
- Dev->TxSharedReq.NumBuffers = 0;
+ Dev->TxSharedReq->NumBuffers = 0;
//
// virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device
@@ -223,6 +260,17 @@ VirtioNetInitTx (
*Dev->TxRing.Avail.Flags = (UINT16) VRING_AVAIL_F_NO_INTERRUPT;
return EFI_SUCCESS;
+
+FreeTxSharedReqBuffer:
+ Dev->VirtIo->FreeSharedPages (
+ Dev->VirtIo,
+ EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
+ TxSharedReqBuffer
+ );
+FreeTxFreeStack:
+ FreePool (Dev->TxFreeStack);
+
+ return Status;
}
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index ee4f9ed36ecd..2ec3dc385a9f 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -54,6 +54,13 @@ VirtioNetShutdownTx (
IN OUT VNET_DEV *Dev
)
{
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap);
+ Dev->VirtIo->FreeSharedPages (
+ Dev->VirtIo,
+ EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
+ (VOID *) Dev->TxSharedReq
+ );
+
FreePool (Dev->TxFreeStack);
}
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 5/8] OvmfPkg/VirtioNetDxe: update TechNotes
2017-09-11 12:16 [PATCH v2 0/8] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
` (3 preceding siblings ...)
2017-09-11 12:16 ` [PATCH v2 4/8] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header Brijesh Singh
@ 2017-09-11 12:16 ` Brijesh Singh
2017-09-13 14:40 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions Brijesh Singh
` (2 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Brijesh Singh @ 2017-09-11 12:16 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
In next patches we will update Virtio transmit to use the device-mapped
address of the caller-supplied packet. The patch documents the new model.
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/VirtioNetDxe/TechNotes.txt | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
index f39426fb13e4..c6ca341ead15 100644
--- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
+++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
@@ -312,10 +312,14 @@ in the following:
that is shared by all of the head descriptors. This virtio-net request header
is never modified by the host.
-- Each tail descriptor is re-pointed to the caller-supplied packet buffer
- whenever VirtioNetTransmit places the corresponding head descriptor on the
- Available Ring. The caller is responsible to hang on to the unmodified buffer
- until it is reported transmitted by VirtioNetGetStatus.
+- Each tail descriptor is re-pointed to the device-mapped address of the
+ caller-supplied packet buffer whenever VirtioNetTransmit places the
+ corresponding head descriptor on the Available Ring. A reverse mapping, from
+ the device-mapped address to the caller-supplied packet address, is saved in
+ an associative data structure that belongs to the driver instance.
+
+- Per spec, the caller is responsible to hang on to the unmodified packet
+ buffer until it is reported transmitted by VirtioNetGetStatus.
Steps of packet transmission:
@@ -338,9 +342,11 @@ Steps of packet transmission:
- Client code calls VirtioNetGetStatus. In case the Used Ring is empty, the
function reports no Tx completion. Otherwise, a head descriptor's index is
consumed from the Used Ring and recycled to the private stack. The client
- code's original packet buffer address is fetched from the tail descriptor
- (where it has been stored at VirtioNetTransmit time) and returned to the
- caller.
+ code's original packet buffer address is calculated by fetching the
+ device-mapped address from the tail descriptor (where it has been stored at
+ VirtioNetTransmit time), and by looking up the device-mapped address in the
+ associative data structure. The reverse-mapped packet buffer address is
+ returned to the caller.
- The Len field of the Used Ring Element is not checked. The host is assumed to
have transmitted the entire packet -- VirtioNetTransmit had forced it below
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions
2017-09-11 12:16 [PATCH v2 0/8] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
` (4 preceding siblings ...)
2017-09-11 12:16 ` [PATCH v2 5/8] OvmfPkg/VirtioNetDxe: update TechNotes Brijesh Singh
@ 2017-09-11 12:16 ` Brijesh Singh
2017-09-13 18:07 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address Brijesh Singh
2017-09-11 12:16 ` [PATCH v2 8/8] OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh
7 siblings, 1 reply; 17+ messages in thread
From: Brijesh Singh @ 2017-09-11 12:16 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
When device is behind IOMMU, driver is require to pass the device address
of TxBuf in the Tx VRING. The patch adds helper functions and data
structure to map and unmap the TxBuf system physical address to a device
address.
Since the TxBuf is returned back to caller from VirtioNetGetStatus() hence
we use OrderedCollection interface to save the TxBuf system physical to
device address mapping. After the TxBuf is succesfully transmitted
VirtioNetUnmapTxBuf() does the reverse lookup in OrderedCollection data
structure to get the system physical address of TxBuf for a given device
address.
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/VirtioNetDxe/VirtioNet.inf | 1 +
OvmfPkg/VirtioNetDxe/VirtioNet.h | 32 ++++
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 15 +-
OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 188 ++++++++++++++++++++
4 files changed, 235 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.inf b/OvmfPkg/VirtioNetDxe/VirtioNet.inf
index a855ad4ac154..9ff6d87e6190 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.inf
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.inf
@@ -49,6 +49,7 @@
DebugLib
DevicePathLib
MemoryAllocationLib
+ OrderedCollectionLib
UefiBootServicesTableLib
UefiDriverEntryPoint
UefiLib
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 3f48bcc6b67c..906bec8e88f3 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -26,6 +26,7 @@
#include <Protocol/DevicePath.h>
#include <Protocol/DriverBinding.h>
#include <Protocol/SimpleNetwork.h>
+#include <Library/OrderedCollectionLib.h>
#define VNET_SIG SIGNATURE_32 ('V', 'N', 'E', 'T')
@@ -100,6 +101,7 @@ typedef struct {
VOID *TxSharedReqMap; // VirtioNetInitTx
UINT16 TxLastUsed; // VirtioNetInitTx
EFI_PHYSICAL_ADDRESS RxBufDeviceBase; // VirtioNetInitRx
+ ORDERED_COLLECTION *TxBufMapInfoCollection; // VirtioNetInitTx
} VNET_DEV;
@@ -281,6 +283,36 @@ VirtioNetUninitRing (
);
//
+// utility functions to map caller-supplied Tx buffer system physical address
+// to a device address and vice versa
+//
+EFI_STATUS
+EFIAPI
+VirtioNetMapTxBuf (
+ IN VNET_DEV *Dev,
+ IN UINT16 DescIdx,
+ IN VOID *Buffer,
+ IN UINTN NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress
+ );
+
+INTN
+EFIAPI
+VirtioNetTxMapInfoCompare (
+ IN CONST VOID *UserStruct1,
+ IN CONST VOID *UserStruct2
+ );
+
+EFI_STATUS
+EFIAPI
+VirtioNetUnmapTxBuf (
+ IN VNET_DEV *Dev,
+ IN UINT16 DescIdx,
+ OUT VOID **Buffer,
+ IN EFI_PHYSICAL_ADDRESS DeviceAddress
+ );
+
+//
// event callbacks
//
VOID
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 6cedb406a172..a8ffb9a8a7b1 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -176,6 +176,15 @@ VirtioNetInitTx (
return EFI_OUT_OF_RESOURCES;
}
+ Dev->TxBufMapInfoCollection = OrderedCollectionInit (
+ VirtioNetTxMapInfoCompare,
+ VirtioNetTxMapInfoCompare
+ );
+ if (Dev->TxBufMapInfoCollection == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto FreeTxFreeStack;
+ }
+
//
// Allocate TxSharedReq header and map with BusMasterCommonBuffer so that it
// can be accessed equally by both processor and device.
@@ -186,7 +195,7 @@ VirtioNetInitTx (
&TxSharedReqBuffer
);
if (EFI_ERROR (Status)) {
- goto FreeTxFreeStack;
+ goto UninitMapInfoCollection;
}
ZeroMem (TxSharedReqBuffer, sizeof *Dev->TxSharedReq);
@@ -267,6 +276,10 @@ FreeTxSharedReqBuffer:
EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
TxSharedReqBuffer
);
+
+UninitMapInfoCollection:
+ OrderedCollectionUninit (Dev->TxBufMapInfoCollection);
+
FreeTxFreeStack:
FreePool (Dev->TxFreeStack);
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 2ec3dc385a9f..dafb538b4b5a 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -18,6 +18,17 @@
#include "VirtioNet.h"
+//
+// The user structure for the ordered collection that will track the mapping
+// info of the packets queued in TxRing
+//
+typedef struct {
+ UINT16 DescIdx;
+ VOID *Buffer;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+ VOID *BufMap;
+} TX_BUF_MAP_INFO;
+
/**
Release RX and TX resources on the boundary of the
EfiSimpleNetworkInitialized state.
@@ -54,6 +65,20 @@ VirtioNetShutdownTx (
IN OUT VNET_DEV *Dev
)
{
+ ORDERED_COLLECTION_ENTRY *Entry, *Entry2;
+ TX_BUF_MAP_INFO *TxBufMapInfo;
+
+ for (Entry = OrderedCollectionMin (Dev->TxBufMapInfoCollection);
+ Entry != NULL;
+ Entry = Entry2) {
+ Entry2 = OrderedCollectionNext (Entry);
+ TxBufMapInfo = (TX_BUF_MAP_INFO *)Entry2;
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, TxBufMapInfo->BufMap);
+ FreePool (TxBufMapInfo);
+ OrderedCollectionDelete (Dev->TxBufMapInfoCollection, Entry, NULL);
+ }
+ OrderedCollectionUninit (Dev->TxBufMapInfoCollection);
+
Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap);
Dev->VirtIo->FreeSharedPages (
Dev->VirtIo,
@@ -83,3 +108,166 @@ VirtioNetUninitRing (
Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RingMap);
VirtioRingUninit (Dev->VirtIo, Ring);
}
+
+
+/**
+ Map Caller-supplied TxBuf buffer to the device-mapped address
+
+ @param[in] Dev The VNET_DEV driver instance which wants to
+ map the Tx packet.
+ @param[in] DescIdx VRING descriptor index which will point to
+ the device address
+ @param[in] Buffer The system physical address of TxBuf
+ @param[in] NumberOfBytes Number of bytes to map
+ @param[out] DeviceAddress The resulting device address for the bus
+ master access.
+
+ @retval EFI_OUT_OF_RESOURCES The request could not be completed due to
+ a lack of resources.
+ @retval EFI_INVALID_PARAMETER The VRING descriptor index is already mapped.
+*/
+EFI_STATUS
+EFIAPI
+VirtioNetMapTxBuf (
+ IN VNET_DEV *Dev,
+ IN UINT16 DescIdx,
+ IN VOID *Buffer,
+ IN UINTN NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress
+ )
+{
+ EFI_STATUS Status;
+ TX_BUF_MAP_INFO *TxBufMapInfo;
+ EFI_PHYSICAL_ADDRESS Address;
+ VOID *Mapping;
+ ORDERED_COLLECTION_ENTRY *Entry;
+
+ TxBufMapInfo = AllocatePool (sizeof (*TxBufMapInfo));
+ if (TxBufMapInfo == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ Status = VirtioMapAllBytesInSharedBuffer (
+ Dev->VirtIo,
+ VirtioOperationBusMasterRead,
+ Buffer,
+ NumberOfBytes,
+ &Address,
+ &Mapping
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeTxBufMapInfo;
+ }
+
+ TxBufMapInfo->DescIdx = DescIdx;
+ TxBufMapInfo->Buffer = Buffer;
+ TxBufMapInfo->DeviceAddress = Address;
+ TxBufMapInfo->BufMap = Mapping;
+
+ Status = OrderedCollectionInsert (
+ Dev->TxBufMapInfoCollection,
+ &Entry,
+ TxBufMapInfo
+ );
+ switch (Status) {
+ case RETURN_OUT_OF_RESOURCES:
+ Status = EFI_OUT_OF_RESOURCES;
+ goto UnmapTxBufBuffer;
+ case RETURN_ALREADY_STARTED:
+ Status = EFI_INVALID_PARAMETER;
+ goto UnmapTxBufBuffer;
+ default:
+ ASSERT (Status == RETURN_SUCCESS);
+ break;
+ }
+
+ ASSERT (OrderedCollectionUserStruct (Entry) == TxBufMapInfo);
+
+ *DeviceAddress = Address;
+
+ return EFI_SUCCESS;
+
+UnmapTxBufBuffer:
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
+
+FreeTxBufMapInfo:
+ FreePool (TxBufMapInfo);
+ return Status;
+}
+
+/**
+ Unmap (aka reverse mapping) device mapped TxBuf buffer to the system
+ physical address
+
+ @param[in] Dev The VNET_DEV driver instance which wants to
+ map the Tx packet.
+ @param[in] DescIdx VRING descriptor index which point to
+ the device address
+ @param[out] Buffer The system physical address of TxBuf
+ @param[out] DeviceAddress The device address for the TxBuf
+
+ @retval EFI_INVALID_PARAMETER The VRING descriptor index is not mapped
+*/
+EFI_STATUS
+EFIAPI
+VirtioNetUnmapTxBuf (
+ IN VNET_DEV *Dev,
+ IN UINT16 DescIdx,
+ OUT VOID **Buffer,
+ IN EFI_PHYSICAL_ADDRESS DeviceAddress
+ )
+{
+ TX_BUF_MAP_INFO StandaloneKey;
+ ORDERED_COLLECTION_ENTRY *Entry;
+ TX_BUF_MAP_INFO *UserStruct;
+ VOID *Ptr;
+ EFI_STATUS Status;
+
+ StandaloneKey.DescIdx = DescIdx;
+ Entry = OrderedCollectionFind (Dev->TxBufMapInfoCollection, &StandaloneKey);
+ if (Entry == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ OrderedCollectionDelete (Dev->TxBufMapInfoCollection, Entry, &Ptr);
+
+ UserStruct = Ptr;
+ ASSERT (UserStruct->DescIdx == DescIdx);
+
+ *Buffer = UserStruct->Buffer;
+ Status = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, UserStruct->BufMap);
+ FreePool (UserStruct);
+
+ return Status;
+}
+
+/**
+ Comparator function for two user structures.
+
+ @param[in] UserStruct1 Pointer to the first user structure.
+
+ @param[in] UserStruct2 Pointer to the second user structure.
+
+ @retval <0 If UserStruct1 compares less than UserStruct2.
+
+ @retval 0 If UserStruct1 compares equal to UserStruct2.
+
+ @retval >0 If UserStruct1 compares greater than UserStruct2.
+*/
+INTN
+EFIAPI
+VirtioNetTxMapInfoCompare (
+ IN CONST VOID *UserStruct1,
+ IN CONST VOID *UserStruct2
+ )
+{
+ CONST TX_BUF_MAP_INFO *MapInfo1;
+ CONST TX_BUF_MAP_INFO *MapInfo2;
+
+ MapInfo1 = UserStruct1;
+ MapInfo2 = UserStruct2;
+
+ return MapInfo1->DescIdx < MapInfo2->DescIdx ? -1 :
+ MapInfo1->DescIdx > MapInfo2->DescIdx ? 1 :
+ 0;
+}
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address
2017-09-11 12:16 [PATCH v2 0/8] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
` (5 preceding siblings ...)
2017-09-11 12:16 ` [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions Brijesh Singh
@ 2017-09-11 12:16 ` Brijesh Singh
2017-09-13 20:15 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 8/8] OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh
7 siblings, 1 reply; 17+ messages in thread
From: Brijesh Singh @ 2017-09-11 12:16 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
When device is behind the IOMMU, driver is require to pass the device
address of caller-supplied transmit buffer for the bus master operations.
The patch uses VirtioNetMapTxBuf() to map caller-supplied Tx packet to a
device-address and enqueue the device address in VRING for transfer and
perform the reverse mapping when transfer is completed so that we can
return the caller-supplied buffer.
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/VirtioNetDxe/SnpGetStatus.c | 30 +++++++++++++----
OvmfPkg/VirtioNetDxe/SnpTransmit.c | 34 ++++++++++++++++----
2 files changed, 50 insertions(+), 14 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
index 694940ea1d97..9bd62fbe8ec0 100644
--- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
+++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
@@ -61,11 +61,12 @@ VirtioNetGetStatus (
OUT VOID **TxBuf OPTIONAL
)
{
- VNET_DEV *Dev;
- EFI_TPL OldTpl;
- EFI_STATUS Status;
- UINT16 RxCurUsed;
- UINT16 TxCurUsed;
+ VNET_DEV *Dev;
+ EFI_TPL OldTpl;
+ EFI_STATUS Status;
+ UINT16 RxCurUsed;
+ UINT16 TxCurUsed;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
if (This == NULL) {
return EFI_INVALID_PARAMETER;
@@ -141,9 +142,24 @@ VirtioNetGetStatus (
ASSERT (DescIdx < (UINT32) (2 * Dev->TxMaxPending - 1));
//
- // report buffer address to caller that has been enqueued by caller
+ // get the device address to caller that has been enqueued by caller
//
- *TxBuf = (VOID *)(UINTN) Dev->TxRing.Desc[DescIdx + 1].Addr;
+ DeviceAddress = Dev->TxRing.Desc[DescIdx + 1].Addr;
+
+ //
+ // Unmap the device address and perform the reverse mapping to find the
+ // caller buffer address.
+ //
+ Status = VirtioNetUnmapTxBuf (
+ Dev,
+ DescIdx + 1,
+ TxBuf,
+ DeviceAddress
+ );
+ if (EFI_ERROR (Status)) {
+ Status = EFI_DEVICE_ERROR;
+ goto Exit;
+ }
//
// now this descriptor can be used again to enqueue a transmit buffer
diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
index 7ca40d5d0650..0be3243b4606 100644
--- a/OvmfPkg/VirtioNetDxe/SnpTransmit.c
+++ b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
@@ -73,11 +73,12 @@ VirtioNetTransmit (
IN UINT16 *Protocol OPTIONAL
)
{
- VNET_DEV *Dev;
- EFI_TPL OldTpl;
- EFI_STATUS Status;
- UINT16 DescIdx;
- UINT16 AvailIdx;
+ VNET_DEV *Dev;
+ EFI_TPL OldTpl;
+ EFI_STATUS Status;
+ UINT16 DescIdx;
+ UINT16 AvailIdx;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
if (This == NULL || BufferSize == 0 || Buffer == NULL) {
return EFI_INVALID_PARAMETER;
@@ -144,10 +145,29 @@ VirtioNetTransmit (
}
//
- // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
+ // Get the free VRING descriptor
//
DescIdx = Dev->TxFreeStack[Dev->TxCurPending++];
- Dev->TxRing.Desc[DescIdx + 1].Addr = (UINTN) Buffer;
+
+ //
+ // Map the transmit buffer system physical address to device address.
+ //
+ Status = VirtioNetMapTxBuf (
+ Dev,
+ DescIdx + 1,
+ Buffer,
+ BufferSize,
+ &DeviceAddress
+ );
+ if (EFI_ERROR (Status)) {
+ Status = EFI_DEVICE_ERROR;
+ goto Exit;
+ }
+
+ //
+ // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
+ //
+ Dev->TxRing.Desc[DescIdx + 1].Addr = DeviceAddress;
Dev->TxRing.Desc[DescIdx + 1].Len = (UINT32) BufferSize;
//
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 8/8] OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM
2017-09-11 12:16 [PATCH v2 0/8] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
` (6 preceding siblings ...)
2017-09-11 12:16 ` [PATCH v2 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address Brijesh Singh
@ 2017-09-11 12:16 ` Brijesh Singh
7 siblings, 0 replies; 17+ messages in thread
From: Brijesh Singh @ 2017-09-11 12:16 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
VirtioNetDxe driver has been updated to use IOMMU-like member functions
from VIRTIO_DEVICE_PROTOCOL to translate the system physical address to
device address. We do not need to do anything special when
VIRTIO_F_IOMMU_PLATFORM bit is present hence treat it in parallel with
VIRTIO_F_VERSION_1.
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index a8ffb9a8a7b1..4885aa73be15 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -560,7 +560,8 @@ VirtioNetInitialize (
ASSERT (Dev->Snm.MediaPresentSupported ==
!!(Features & VIRTIO_NET_F_STATUS));
- Features &= VIRTIO_NET_F_MAC | VIRTIO_NET_F_STATUS | VIRTIO_F_VERSION_1;
+ Features &= VIRTIO_NET_F_MAC | VIRTIO_NET_F_STATUS | VIRTIO_F_VERSION_1 |
+ VIRTIO_F_IOMMU_PLATFORM;
//
// In virtio-1.0, feature negotiation is expected to complete before queue
@@ -600,7 +601,7 @@ VirtioNetInitialize (
// step 5 -- keep only the features we want
//
if (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) {
- Features &= ~(UINT64)VIRTIO_F_VERSION_1;
+ Features &= ~(UINT64)(VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM);
Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
if (EFI_ERROR (Status)) {
goto ReleaseTxRing;
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/8] OvmfPkg/VirtioNetDxe: add helper VirtioNetUninitRing()
2017-09-11 12:16 ` [PATCH v2 1/8] OvmfPkg/VirtioNetDxe: add helper VirtioNetUninitRing() Brijesh Singh
@ 2017-09-13 7:26 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-09-13 7:26 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 09/11/17 14:16, Brijesh Singh wrote:
> Consolidate the virtio VRING resource cleanup into VirtioNetUninitRing().
>
> 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/VirtioNetDxe/VirtioNet.h | 7 +++++++
> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 4 ++--
> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 16 ++++++++++++++++
> OvmfPkg/VirtioNetDxe/SnpShutdown.c | 4 ++--
> OvmfPkg/VirtioNetDxe/TechNotes.txt | 5 +++--
> 5 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 710859bc6115..87a0f06e01a4 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -263,6 +263,13 @@ VirtioNetShutdownTx (
> IN OUT VNET_DEV *Dev
> );
>
> +VOID
> +EFIAPI
> +VirtioNetUninitRing (
> + IN OUT VNET_DEV *Dev,
> + IN OUT VRING *Ring
> + );
> +
> //
> // event callbacks
> //
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 0ecfe044a977..637c978709fd 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -510,10 +510,10 @@ AbortDevice:
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>
> ReleaseTxRing:
> - VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
> + VirtioNetUninitRing (Dev, &Dev->TxRing);
>
> ReleaseRxRing:
> - VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
> + VirtioNetUninitRing (Dev, &Dev->RxRing);
>
> DeviceFailed:
> //
> diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> index 9fedb72fdbd4..5b75eabc7a6b 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> @@ -51,3 +51,19 @@ VirtioNetShutdownTx (
> {
> FreePool (Dev->TxFreeStack);
> }
> +
> +/**
> + Release TX and RX VRING resources.
> +
> + @param[in,out] Dev The VNET_DEV driver instance which was using the ring.
> + @param[in,out] Ring The virtio ring to clean up.
> +*/
> +VOID
> +EFIAPI
> +VirtioNetUninitRing (
> + IN OUT VNET_DEV *Dev,
> + IN OUT VRING *Ring
> + )
> +{
> + VirtioRingUninit (Dev->VirtIo, Ring);
> +}
> diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> index 5e84191fbbdd..432e0691d457 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> @@ -67,8 +67,8 @@ VirtioNetShutdown (
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> VirtioNetShutdownRx (Dev);
> VirtioNetShutdownTx (Dev);
> - VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
> - VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
> + VirtioNetUninitRing (Dev, &Dev->TxRing);
> + VirtioNetUninitRing (Dev, &Dev->RxRing);
>
> Dev->Snm.State = EfiSimpleNetworkStarted;
> Status = EFI_SUCCESS;
> diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> index 9c1dfe6a773e..86b91f561495 100644
> --- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
> +++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> @@ -70,8 +70,9 @@ faithfully indented) that implement the transition.
> VirtioNetInitialize | | VirtioNetShutdown
> VirtioNetInitRing {Rx, Tx} | | VirtioNetShutdownRx [SnpSharedHelpers.c]
> VirtioRingInit | | VirtioNetShutdownTx [SnpSharedHelpers.c]
> - VirtioNetInitTx | | VirtioRingUninit {Tx, Rx}
> - VirtioNetInitRx | |
> + VirtioNetInitTx | | VirtioNetUninitRing [SnpSharedHelpers.c]
> + VirtioNetInitRx | | {Tx, Rx}
> + | | VirtioRingUninit
> v |
> +-----------------------------+
> | EfiSimpleNetworkInitialized |
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/8] OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap()
2017-09-11 12:16 ` [PATCH v2 2/8] OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap() Brijesh Singh
@ 2017-09-13 8:07 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-09-13 8:07 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 09/11/17 14:16, Brijesh Singh wrote:
> When device is behind the IOMMU then driver need to pass the device
> address when programing the bus master. The patch uses VirtioRingMap() to
> map the VRING system physical address[es] to device address[es].
>
> 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/VirtioNetDxe/VirtioNet.h | 7 ++-
> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 50 +++++++++++++++-----
> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 10 ++--
> OvmfPkg/VirtioNetDxe/SnpShutdown.c | 4 +-
> OvmfPkg/VirtioNetDxe/TechNotes.txt | 1 +
> 5 files changed, 55 insertions(+), 17 deletions(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 87a0f06e01a4..6762fc9d1d6e 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -82,10 +82,14 @@ typedef struct {
> EFI_HANDLE MacHandle; // VirtioNetDriverBindingStart
>
> VRING RxRing; // VirtioNetInitRing
> + VOID *RxRingMap; // VirtioRingMap and
> + // VirtioNetInitRing
> UINT8 *RxBuf; // VirtioNetInitRx
> UINT16 RxLastUsed; // VirtioNetInitRx
>
> VRING TxRing; // VirtioNetInitRing
> + VOID *TxRingMap; // VirtioRingMap and
> + // VirtioNetInitRing
> UINT16 TxMaxPending; // VirtioNetInitTx
> UINT16 TxCurPending; // VirtioNetInitTx
> UINT16 *TxFreeStack; // VirtioNetInitTx
> @@ -267,7 +271,8 @@ VOID
> EFIAPI
> VirtioNetUninitRing (
> IN OUT VNET_DEV *Dev,
> - IN OUT VRING *Ring
> + IN OUT VRING *Ring,
> + IN VOID *RingMap
> );
>
> //
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 637c978709fd..8eabdbff6f5e 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -35,11 +35,13 @@
> the network device.
> @param[out] Ring The virtio-ring inside the VNET_DEV structure,
> corresponding to Selector.
> + @param[out] Mapping A resulting token to pass to VirtioNetUninitRing()
>
> @retval EFI_UNSUPPORTED The queue size reported by the virtio-net device is
> too small.
> @return Status codes from VIRTIO_CFG_WRITE(),
> - VIRTIO_CFG_READ() and VirtioRingInit().
> + VIRTIO_CFG_READ(), VirtioRingInit() and
> + VirtioRingMap().
> @retval EFI_SUCCESS Ring initialized.
> */
>
> @@ -49,11 +51,14 @@ EFIAPI
> VirtioNetInitRing (
> IN OUT VNET_DEV *Dev,
> IN UINT16 Selector,
> - OUT VRING *Ring
> + OUT VRING *Ring,
> + OUT VOID **Mapping
> )
> {
> EFI_STATUS Status;
> UINT16 QueueSize;
> + UINT64 RingBaseShift;
> + VOID *MapInfo;
>
> //
> // step 4b -- allocate selected queue
> @@ -80,29 +85,42 @@ VirtioNetInitRing (
> }
>
> //
> + // If anything fails from here on, we must release the ring resources.
> + //
> + Status = VirtioRingMap (Dev->VirtIo, Ring, &RingBaseShift, &MapInfo);
> + 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.
> + // size. If anything fails from here on, we must unmap the ring resources.
> //
> Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
> if (EFI_ERROR (Status)) {
> - goto ReleaseQueue;
> + goto UnmapQueue;
> }
>
> Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
> if (EFI_ERROR (Status)) {
> - goto ReleaseQueue;
> + goto UnmapQueue;
> }
>
> //
> // step 4c -- report GPFN (guest-physical frame number) of queue
> //
> - Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, 0);
> + Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, RingBaseShift);
> if (EFI_ERROR (Status)) {
> - goto ReleaseQueue;
> + goto UnmapQueue;
> }
>
> + *Mapping = MapInfo;
> +
> return EFI_SUCCESS;
>
> +UnmapQueue:
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, MapInfo);
> +
> ReleaseQueue:
> VirtioRingUninit (Dev->VirtIo, Ring);
>
> @@ -456,12 +474,22 @@ VirtioNetInitialize (
> //
> // step 4b, 4c -- allocate and report virtqueues
> //
> - Status = VirtioNetInitRing (Dev, VIRTIO_NET_Q_RX, &Dev->RxRing);
> + Status = VirtioNetInitRing (
> + Dev,
> + VIRTIO_NET_Q_RX,
> + &Dev->RxRing,
> + &Dev->RxRingMap
> + );
> if (EFI_ERROR (Status)) {
> goto DeviceFailed;
> }
>
> - Status = VirtioNetInitRing (Dev, VIRTIO_NET_Q_TX, &Dev->TxRing);
> + Status = VirtioNetInitRing (
> + Dev,
> + VIRTIO_NET_Q_TX,
> + &Dev->TxRing,
> + &Dev->TxRingMap
> + );
> if (EFI_ERROR (Status)) {
> goto ReleaseRxRing;
> }
> @@ -510,10 +538,10 @@ AbortDevice:
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>
> ReleaseTxRing:
> - VirtioNetUninitRing (Dev, &Dev->TxRing);
> + VirtioNetUninitRing (Dev, &Dev->TxRing, Dev->TxRingMap);
>
> ReleaseRxRing:
> - VirtioNetUninitRing (Dev, &Dev->RxRing);
> + VirtioNetUninitRing (Dev, &Dev->RxRing, Dev->RxRingMap);
>
> DeviceFailed:
> //
> diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> index 5b75eabc7a6b..57c7395848bd 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> @@ -55,15 +55,19 @@ VirtioNetShutdownTx (
> /**
> Release TX and RX VRING resources.
>
> - @param[in,out] Dev The VNET_DEV driver instance which was using the ring.
> - @param[in,out] Ring The virtio ring to clean up.
> + @param[in,out] Dev The VNET_DEV driver instance which was using
> + the ring.
> + @param[in,out] Ring The virtio ring to clean up.
> + @param[in] RingMap A token return from the VirtioRingMap()
> */
> VOID
> EFIAPI
> VirtioNetUninitRing (
> IN OUT VNET_DEV *Dev,
> - IN OUT VRING *Ring
> + IN OUT VRING *Ring,
> + IN VOID *RingMap
> )
> {
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RingMap);
> VirtioRingUninit (Dev->VirtIo, Ring);
> }
> diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> index 432e0691d457..d8c11f20de61 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> @@ -67,8 +67,8 @@ VirtioNetShutdown (
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> VirtioNetShutdownRx (Dev);
> VirtioNetShutdownTx (Dev);
> - VirtioNetUninitRing (Dev, &Dev->TxRing);
> - VirtioNetUninitRing (Dev, &Dev->RxRing);
> + VirtioNetUninitRing (Dev, &Dev->TxRing, Dev->TxRingMap);
> + VirtioNetUninitRing (Dev, &Dev->RxRing, Dev->RxRingMap);
>
> Dev->Snm.State = EfiSimpleNetworkStarted;
> Status = EFI_SUCCESS;
> diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> index 86b91f561495..0891e8210489 100644
> --- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
> +++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> @@ -72,6 +72,7 @@ faithfully indented) that implement the transition.
> VirtioRingInit | | VirtioNetShutdownTx [SnpSharedHelpers.c]
> VirtioNetInitTx | | VirtioNetUninitRing [SnpSharedHelpers.c]
> VirtioNetInitRx | | {Tx, Rx}
> + | | VirtIo->UnmapSharedBuffer
> | | VirtioRingUninit
> v |
> +-----------------------------+
>
(1) The documentation udpate is not correct; please check my previous
review:
http://mid.mail-archive.com/04fefb16-23d5-f6df-0657-9d4c5e96ac57@redhat.com
In particular you forgot to add the VirtioRingMap function call on the
left side, under VirtioNetInitRing:
On 09/05/17 13:47, Laszlo Ersek wrote:
> (9d) modify the documentation again, to the following state:
>
>> +-------------------------+
>> | EfiSimpleNetworkStarted |
>> +-------------------------+
>> | ^
>> [SnpInitialize.c] | | [SnpShutdown.c]
>> VirtioNetInitialize | | VirtioNetShutdown
>> VirtioNetInitRing {Rx, Tx} | | VirtioNetShutdownRx [SnpSharedHelpers.c]
>> VirtioRingInit | | VirtioNetShutdownTx [SnpSharedHelpers.c]
>> VirtioRingMap | | VirtioNetUninitRing [SnpSharedHelpers.c]
>> VirtioNetInitTx | | {Tx, Rx}
>> VirtioNetInitRx | | VirtIo->UnmapSharedBuffer
>> | | VirtioRingUninit
>> v |
>> +-----------------------------+
>> | EfiSimpleNetworkInitialized |
>> +-----------------------------+
I think simply pasting the above into "TechNotes.txt" would be easiest.
The rest looks good!
Thanks
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/8] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages()
2017-09-11 12:16 ` [PATCH v2 3/8] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages() Brijesh Singh
@ 2017-09-13 9:14 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-09-13 9:14 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 09/11/17 14:16, Brijesh Singh wrote:
> When device is behind the IOMMU, VirtioNetDxe is required to use the
> device address in bus master operations. RxBuf is allocated using
> AllocatePool() which returns the system physical address.
>
> The patch uses VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages() to allocate
> the RxBuf and map with VirtioMapAllBytesInSharedBuffer() so that we can
> obtain the device address for RxBuf.
>
> 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/VirtioNetDxe/VirtioNet.h | 4 +
> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 77 +++++++++++++++-----
> OvmfPkg/VirtioNetDxe/SnpReceive.c | 5 +-
> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 7 +-
> OvmfPkg/VirtioNetDxe/TechNotes.txt | 2 +-
> 5 files changed, 74 insertions(+), 21 deletions(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 6762fc9d1d6e..7d5f33b01dc8 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -4,6 +4,7 @@
> Protocol instances for virtio-net devices.
>
> Copyright (C) 2013, Red Hat, Inc.
> + Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -86,6 +87,8 @@ typedef struct {
> // VirtioNetInitRing
> UINT8 *RxBuf; // VirtioNetInitRx
> UINT16 RxLastUsed; // VirtioNetInitRx
> + UINTN RxBufNrPages; // VirtioNetInitRx
> + VOID *RxBufMap; // VirtioNetInitRx
>
> VRING TxRing; // VirtioNetInitRing
> VOID *TxRingMap; // VirtioRingMap and
> @@ -95,6 +98,7 @@ typedef struct {
> UINT16 *TxFreeStack; // VirtioNetInitTx
> VIRTIO_1_0_NET_REQ TxSharedReq; // VirtioNetInitTx
> UINT16 TxLastUsed; // VirtioNetInitTx
> + EFI_PHYSICAL_ADDRESS RxBufDeviceBase; // VirtioNetInitRx
(1) For clarity, please move the new "RxBufDeviceBase" field between
"RxBufNrPages" and "RxBufMap".
> } VNET_DEV;
>
>
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 8eabdbff6f5e..54c808c501bf 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -242,8 +242,10 @@ VirtioNetInitTx (
> @param[in,out] Dev The VNET_DEV driver instance about to enter the
> EfiSimpleNetworkInitialized state.
>
> - @retval EFI_OUT_OF_RESOURCES Failed to allocate RX destination area.
> - @return Status codes from VIRTIO_CFG_WRITE().
> + @retval EFI_OUT_OF_RESOURCES Failed to allocate or map RX destination area.
> + @return Status codes from VIRTIO_CFG_WRITE() or
> + VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages or
> + VirtioMapAllBytesInSharedBuffer().
> @retval EFI_SUCCESS RX setup successful. The device is live and may
> already be writing to the receive area.
> */
The update to "@return ..." is fine.
(2) But, please remove the "@retval EFI_OUT_OF_RESOURCES" line entirely;
we no longer return this status code explicitly.
> @@ -255,13 +257,15 @@ VirtioNetInitRx (
> IN OUT VNET_DEV *Dev
> )
> {
> - EFI_STATUS Status;
> - UINTN VirtioNetReqSize;
> - UINTN RxBufSize;
> - UINT16 RxAlwaysPending;
> - UINTN PktIdx;
> - UINT16 DescIdx;
> - UINT8 *RxPtr;
> + EFI_STATUS Status;
> + UINTN VirtioNetReqSize;
> + UINTN RxBufSize;
> + UINT16 RxAlwaysPending;
> + UINTN PktIdx;
> + UINT16 DescIdx;
> + UINTN NumBytes;
> + EFI_PHYSICAL_ADDRESS RxBufDeviceAddress;
> + VOID *RxBuffer;
>
> //
> // In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on
> @@ -286,11 +290,37 @@ VirtioNetInitRx (
> //
> RxAlwaysPending = (UINT16) MIN (Dev->RxRing.QueueSize / 2, VNET_MAX_PENDING);
>
> - Dev->RxBuf = AllocatePool (RxAlwaysPending * RxBufSize);
> - if (Dev->RxBuf == NULL) {
> - return EFI_OUT_OF_RESOURCES;
> + //
> + // The RxBuf is shared between guest and hypervisor, use
> + // AllocateSharedPages() to allocate this memory region and map it with
> + // BusMasterCommonBuffer so that it can be accessed by both guest and
> + // hypervisor.
> + //
> + NumBytes = RxAlwaysPending * RxBufSize;
> + Dev->RxBufNrPages = EFI_SIZE_TO_PAGES (NumBytes);
> + Status = Dev->VirtIo->AllocateSharedPages (
> + Dev->VirtIo,
> + Dev->RxBufNrPages,
> + &RxBuffer
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> }
>
(3) In this spot, please add:
ZeroMem (RxBuffer, NumBytes);
The rest looks good.
Thank you,
Laszlo
> + Status = VirtioMapAllBytesInSharedBuffer (
> + Dev->VirtIo,
> + VirtioOperationBusMasterCommonBuffer,
> + RxBuffer,
> + NumBytes,
> + &Dev->RxBufDeviceBase,
> + &Dev->RxBufMap
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeSharedBuffer;
> + }
> +
> + Dev->RxBuf = RxBuffer;
> +
> //
> // virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device
> //
> @@ -310,7 +340,7 @@ VirtioNetInitRx (
> // link each chain into (from) the available ring as well
> //
> DescIdx = 0;
> - RxPtr = Dev->RxBuf;
> + RxBufDeviceAddress = Dev->RxBufDeviceBase;
> for (PktIdx = 0; PktIdx < RxAlwaysPending; ++PktIdx) {
> //
> // virtio-0.9.5, 2.4.1.2 Updating the Available Ring
> @@ -321,16 +351,16 @@ VirtioNetInitRx (
> //
> // virtio-0.9.5, 2.4.1.1 Placing Buffers into the Descriptor Table
> //
> - Dev->RxRing.Desc[DescIdx].Addr = (UINTN) RxPtr;
> + Dev->RxRing.Desc[DescIdx].Addr = RxBufDeviceAddress;
> Dev->RxRing.Desc[DescIdx].Len = (UINT32) VirtioNetReqSize;
> Dev->RxRing.Desc[DescIdx].Flags = VRING_DESC_F_WRITE | VRING_DESC_F_NEXT;
> Dev->RxRing.Desc[DescIdx].Next = (UINT16) (DescIdx + 1);
> - RxPtr += Dev->RxRing.Desc[DescIdx++].Len;
> + RxBufDeviceAddress += Dev->RxRing.Desc[DescIdx++].Len;
>
> - Dev->RxRing.Desc[DescIdx].Addr = (UINTN) RxPtr;
> + Dev->RxRing.Desc[DescIdx].Addr = RxBufDeviceAddress;
> Dev->RxRing.Desc[DescIdx].Len = (UINT32) (RxBufSize - VirtioNetReqSize);
> Dev->RxRing.Desc[DescIdx].Flags = VRING_DESC_F_WRITE;
> - RxPtr += Dev->RxRing.Desc[DescIdx++].Len;
> + RxBufDeviceAddress += Dev->RxRing.Desc[DescIdx++].Len;
> }
>
> //
> @@ -351,10 +381,21 @@ VirtioNetInitRx (
> Status = Dev->VirtIo->SetQueueNotify (Dev->VirtIo, VIRTIO_NET_Q_RX);
> if (EFI_ERROR (Status)) {
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> - FreePool (Dev->RxBuf);
> + goto UnmapSharedBuffer;
> }
>
> return Status;
> +
> +UnmapSharedBuffer:
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
> +
> +FreeSharedBuffer:
> + Dev->VirtIo->FreeSharedPages (
> + Dev->VirtIo,
> + Dev->RxBufNrPages,
> + RxBuffer
> + );
> + return Status;
> }
>
>
> diff --git a/OvmfPkg/VirtioNetDxe/SnpReceive.c b/OvmfPkg/VirtioNetDxe/SnpReceive.c
> index 99abd7ebe454..c42489636ea0 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpReceive.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpReceive.c
> @@ -82,6 +82,7 @@ VirtioNetReceive (
> UINT8 *RxPtr;
> UINT16 AvailIdx;
> EFI_STATUS NotifyStatus;
> + UINTN RxBufOffset;
>
> if (This == NULL || BufferSize == NULL || Buffer == NULL) {
> return EFI_INVALID_PARAMETER;
> @@ -143,7 +144,9 @@ VirtioNetReceive (
> *HeaderSize = Dev->Snm.MediaHeaderSize;
> }
>
> - RxPtr = (UINT8 *)(UINTN) Dev->RxRing.Desc[DescIdx + 1].Addr;
> + RxBufOffset = (UINTN)(Dev->RxRing.Desc[DescIdx + 1].Addr -
> + Dev->RxBufDeviceBase);
> + RxPtr = Dev->RxBuf + RxBufOffset;
> CopyMem (Buffer, RxPtr, RxLen);
>
> if (DestAddr != NULL) {
> diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> index 57c7395848bd..ee4f9ed36ecd 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> @@ -39,7 +39,12 @@ VirtioNetShutdownRx (
> IN OUT VNET_DEV *Dev
> )
> {
> - FreePool (Dev->RxBuf);
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
> + Dev->VirtIo->FreeSharedPages (
> + Dev->VirtIo,
> + Dev->RxBufNrPages,
> + Dev->RxBuf
> + );
> }
>
>
> diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> index 0891e8210489..f39426fb13e4 100644
> --- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
> +++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> @@ -247,7 +247,7 @@ In VirtioNetInitRx, the guest allocates the fixed size Receive Destination
> Area, which accommodates all packets delivered asynchronously by the host. To
> each packet, a slice of this area is dedicated; each slice is further
> subdivided into virtio-net request header and network packet data. The
> -(guest-physical) addresses of these sub-slices are denoted with A2, A3, A4 and
> +(device-physical) addresses of these sub-slices are denoted with A2, A3, A4 and
> so on. Importantly, an even-subscript "A" always belongs to a virtio-net
> request header, while an odd-subscript "A" always belongs to a packet
> sub-slice.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/8] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header
2017-09-11 12:16 ` [PATCH v2 4/8] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header Brijesh Singh
@ 2017-09-13 14:31 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-09-13 14:31 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 09/11/17 14:16, Brijesh Singh wrote:
> Each network packet is submitted for transmission by pushing the head
> descriptor of a two-part descriptor chain to the Available Ring of the
> TX queue. VirtioNetInitTx() sets up the the descriptor chains for all
> queueable packets in advance, and points all the head descriptors to the
> same shared, never modified, VIRTIO_1_0_NET_REQ header object (or its
> initial VIRTIO_NET_REQ sub-object, dependent on virtio version).
> VirtioNetInitTx() currently uses the header object's system physical
> address for populating the head descriptors.
>
> When device is behind the IOMMU, VirtioNet driver is required to provide
> the device address of VIRTIO_1_0_NET_REQ header. In this patch we
> dynamically allocate the header using AllocateSharedPages() and map with
> BusMasterCommonBuffer so that header can be accessed by both processor
> and the device.
>
> We map the header object for CommonBuffer operation because, in order to
> stick with the current code order, we populate the head descriptors with
> the header's device address first, and fill in the header itself second.
>
> 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/VirtioNetDxe/VirtioNet.h | 3 +-
> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 64 +++++++++++++++++---
> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 7 +++
> 3 files changed, 65 insertions(+), 9 deletions(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 7d5f33b01dc8..3f48bcc6b67c 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -96,7 +96,8 @@ typedef struct {
> UINT16 TxMaxPending; // VirtioNetInitTx
> UINT16 TxCurPending; // VirtioNetInitTx
> UINT16 *TxFreeStack; // VirtioNetInitTx
> - VIRTIO_1_0_NET_REQ TxSharedReq; // VirtioNetInitTx
> + VIRTIO_1_0_NET_REQ *TxSharedReq; // VirtioNetInitTx
> + VOID *TxSharedReqMap; // VirtioNetInitTx
> UINT16 TxLastUsed; // VirtioNetInitTx
> EFI_PHYSICAL_ADDRESS RxBufDeviceBase; // VirtioNetInitRx
> } VNET_DEV;
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 54c808c501bf..6cedb406a172 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -18,6 +18,7 @@
> **/
>
> #include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/UefiBootServicesTableLib.h>
>
> @@ -147,6 +148,9 @@ ReleaseQueue:
>
> @retval EFI_OUT_OF_RESOURCES Failed to allocate the stack to track the heads
> of free descriptor chains.
> + @return Status codes from VIRTIO_DEVICE_PROTOCOL.
> + AllocateSharedPages() or
> + VirtioMapAllBytesInSharedBuffer()
> @retval EFI_SUCCESS TX setup successful.
> */
>
> @@ -157,8 +161,11 @@ VirtioNetInitTx (
> IN OUT VNET_DEV *Dev
> )
> {
> - UINTN TxSharedReqSize;
> - UINTN PktIdx;
> + UINTN TxSharedReqSize;
> + UINTN PktIdx;
> + EFI_STATUS Status;
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
> + VOID *TxSharedReqBuffer;
>
> Dev->TxMaxPending = (UINT16) MIN (Dev->TxRing.QueueSize / 2,
> VNET_MAX_PENDING);
> @@ -170,12 +177,42 @@ VirtioNetInitTx (
> }
>
> //
> + // Allocate TxSharedReq header and map with BusMasterCommonBuffer so that it
> + // can be accessed equally by both processor and device.
> + //
> + Status = Dev->VirtIo->AllocateSharedPages (
> + Dev->VirtIo,
> + EFI_SIZE_TO_PAGES (sizeof *Dev->TxSharedReq),
> + &TxSharedReqBuffer
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeTxFreeStack;
> + }
> +
> + ZeroMem (TxSharedReqBuffer, sizeof *Dev->TxSharedReq);
> +
> + Status = VirtioMapAllBytesInSharedBuffer (
> + Dev->VirtIo,
> + VirtioOperationBusMasterCommonBuffer,
> + TxSharedReqBuffer,
> + sizeof *(Dev->TxSharedReq),
> + &DeviceAddress,
> + &Dev->TxSharedReqMap
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeTxSharedReqBuffer;
> + }
> +
> + Dev->TxSharedReq = TxSharedReqBuffer;
> +
> +
> + //
> // In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on
> // VIRTIO_NET_F_MRG_RXBUF, which we never negotiate.
> //
> TxSharedReqSize = (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) ?
> - sizeof Dev->TxSharedReq.V0_9_5 :
> - sizeof Dev->TxSharedReq;
> + sizeof (Dev->TxSharedReq->V0_9_5) :
> + sizeof *Dev->TxSharedReq;
>
> for (PktIdx = 0; PktIdx < Dev->TxMaxPending; ++PktIdx) {
> UINT16 DescIdx;
> @@ -187,7 +224,7 @@ VirtioNetInitTx (
> // For each possibly pending packet, lay out the descriptor for the common
> // (unmodified by the host) virtio-net request header.
> //
> - Dev->TxRing.Desc[DescIdx].Addr = (UINTN) &Dev->TxSharedReq;
> + Dev->TxRing.Desc[DescIdx].Addr = DeviceAddress;
> Dev->TxRing.Desc[DescIdx].Len = (UINT32) TxSharedReqSize;
> Dev->TxRing.Desc[DescIdx].Flags = VRING_DESC_F_NEXT;
> Dev->TxRing.Desc[DescIdx].Next = (UINT16) (DescIdx + 1);
> @@ -202,13 +239,13 @@ VirtioNetInitTx (
> //
> // virtio-0.9.5, Appendix C, Packet Transmission
> //
> - Dev->TxSharedReq.V0_9_5.Flags = 0;
> - Dev->TxSharedReq.V0_9_5.GsoType = VIRTIO_NET_HDR_GSO_NONE;
> + Dev->TxSharedReq->V0_9_5.Flags = 0;
> + Dev->TxSharedReq->V0_9_5.GsoType = VIRTIO_NET_HDR_GSO_NONE;
>
> //
> // For VirtIo 1.0 only -- the field exists, but it is unused
> //
> - Dev->TxSharedReq.NumBuffers = 0;
> + Dev->TxSharedReq->NumBuffers = 0;
>
> //
> // virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device
> @@ -223,6 +260,17 @@ VirtioNetInitTx (
> *Dev->TxRing.Avail.Flags = (UINT16) VRING_AVAIL_F_NO_INTERRUPT;
>
> return EFI_SUCCESS;
> +
> +FreeTxSharedReqBuffer:
> + Dev->VirtIo->FreeSharedPages (
> + Dev->VirtIo,
> + EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
> + TxSharedReqBuffer
> + );
> +FreeTxFreeStack:
> + FreePool (Dev->TxFreeStack);
> +
> + return Status;
> }
>
>
> diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> index ee4f9ed36ecd..2ec3dc385a9f 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> @@ -54,6 +54,13 @@ VirtioNetShutdownTx (
> IN OUT VNET_DEV *Dev
> )
> {
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap);
> + Dev->VirtIo->FreeSharedPages (
> + Dev->VirtIo,
> + EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
> + (VOID *) Dev->TxSharedReq
> + );
> +
> FreePool (Dev->TxFreeStack);
> }
>
>
(1) Regarding the last argument of FreeSharedPages(), from my previous
review at
http://mid.mail-archive.com/ea935749-2a4f-bedd-44ee-05b59524ea07@redhat.com
you missed:
On 09/06/17 11:11, Laszlo Ersek wrote:
> (18) The (VOID*) cast is unneeded.
The rest is good.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/8] OvmfPkg/VirtioNetDxe: update TechNotes
2017-09-11 12:16 ` [PATCH v2 5/8] OvmfPkg/VirtioNetDxe: update TechNotes Brijesh Singh
@ 2017-09-13 14:40 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-09-13 14:40 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 09/11/17 14:16, Brijesh Singh wrote:
> In next patches we will update Virtio transmit to use the device-mapped
> address of the caller-supplied packet. The patch documents the new model.
>
> 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/VirtioNetDxe/TechNotes.txt | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> index f39426fb13e4..c6ca341ead15 100644
> --- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
> +++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> @@ -312,10 +312,14 @@ in the following:
> that is shared by all of the head descriptors. This virtio-net request header
> is never modified by the host.
>
> -- Each tail descriptor is re-pointed to the caller-supplied packet buffer
> - whenever VirtioNetTransmit places the corresponding head descriptor on the
> - Available Ring. The caller is responsible to hang on to the unmodified buffer
> - until it is reported transmitted by VirtioNetGetStatus.
> +- Each tail descriptor is re-pointed to the device-mapped address of the
> + caller-supplied packet buffer whenever VirtioNetTransmit places the
> + corresponding head descriptor on the Available Ring. A reverse mapping, from
> + the device-mapped address to the caller-supplied packet address, is saved in
> + an associative data structure that belongs to the driver instance.
> +
> +- Per spec, the caller is responsible to hang on to the unmodified packet
> + buffer until it is reported transmitted by VirtioNetGetStatus.
>
> Steps of packet transmission:
>
> @@ -338,9 +342,11 @@ Steps of packet transmission:
> - Client code calls VirtioNetGetStatus. In case the Used Ring is empty, the
> function reports no Tx completion. Otherwise, a head descriptor's index is
> consumed from the Used Ring and recycled to the private stack. The client
> - code's original packet buffer address is fetched from the tail descriptor
> - (where it has been stored at VirtioNetTransmit time) and returned to the
> - caller.
> + code's original packet buffer address is calculated by fetching the
> + device-mapped address from the tail descriptor (where it has been stored at
> + VirtioNetTransmit time), and by looking up the device-mapped address in the
> + associative data structure. The reverse-mapped packet buffer address is
> + returned to the caller.
>
> - The Len field of the Used Ring Element is not checked. The host is assumed to
> have transmitted the entire packet -- VirtioNetTransmit had forced it below
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions
2017-09-11 12:16 ` [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions Brijesh Singh
@ 2017-09-13 18:07 ` Laszlo Ersek
2017-09-13 19:24 ` Laszlo Ersek
0 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-09-13 18:07 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 09/11/17 14:16, Brijesh Singh wrote:
> When device is behind IOMMU, driver is require to pass the device address
> of TxBuf in the Tx VRING. The patch adds helper functions and data
> structure to map and unmap the TxBuf system physical address to a device
> address.
>
> Since the TxBuf is returned back to caller from VirtioNetGetStatus() hence
> we use OrderedCollection interface to save the TxBuf system physical to
> device address mapping. After the TxBuf is succesfully transmitted
> VirtioNetUnmapTxBuf() does the reverse lookup in OrderedCollection data
> structure to get the system physical address of TxBuf for a given device
> address.
>
> 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/VirtioNetDxe/VirtioNet.inf | 1 +
> OvmfPkg/VirtioNetDxe/VirtioNet.h | 32 ++++
> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 15 +-
> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 188 ++++++++++++++++++++
> 4 files changed, 235 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.inf b/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> index a855ad4ac154..9ff6d87e6190 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> @@ -49,6 +49,7 @@
> DebugLib
> DevicePathLib
> MemoryAllocationLib
> + OrderedCollectionLib
> UefiBootServicesTableLib
> UefiDriverEntryPoint
> UefiLib
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 3f48bcc6b67c..906bec8e88f3 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -26,6 +26,7 @@
> #include <Protocol/DevicePath.h>
> #include <Protocol/DriverBinding.h>
> #include <Protocol/SimpleNetwork.h>
> +#include <Library/OrderedCollectionLib.h>
>
> #define VNET_SIG SIGNATURE_32 ('V', 'N', 'E', 'T')
>
> @@ -100,6 +101,7 @@ typedef struct {
> VOID *TxSharedReqMap; // VirtioNetInitTx
> UINT16 TxLastUsed; // VirtioNetInitTx
> EFI_PHYSICAL_ADDRESS RxBufDeviceBase; // VirtioNetInitRx
> + ORDERED_COLLECTION *TxBufMapInfoCollection; // VirtioNetInitTx
(1) Hmm, I like the name, but I don't like the unaligned comment.
I also don't like the idea of moving all the other comments.
Can you just call this field "TxBufCollection"? Then the comment can be
aligned.
> } VNET_DEV;
>
>
> @@ -281,6 +283,36 @@ VirtioNetUninitRing (
> );
>
> //
> +// utility functions to map caller-supplied Tx buffer system physical address
> +// to a device address and vice versa
> +//
I like this comment!
> +EFI_STATUS
> +EFIAPI
> +VirtioNetMapTxBuf (
> + IN VNET_DEV *Dev,
> + IN UINT16 DescIdx,
> + IN VOID *Buffer,
> + IN UINTN NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress
> + );
> +
> +INTN
> +EFIAPI
> +VirtioNetTxMapInfoCompare (
> + IN CONST VOID *UserStruct1,
> + IN CONST VOID *UserStruct2
> + );
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioNetUnmapTxBuf (
> + IN VNET_DEV *Dev,
> + IN UINT16 DescIdx,
> + OUT VOID **Buffer,
> + IN EFI_PHYSICAL_ADDRESS DeviceAddress
> + );
> +
I will comment on these functions in detail below, I just want to say:
- the location of the declarations in this header file is fine, but
(2) Please try to keep the same order between the new functions in
"VirtioNet.h", and in "SnpSharedHelpers.c".
> +//
> // event callbacks
> //
> VOID
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 6cedb406a172..a8ffb9a8a7b1 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -176,6 +176,15 @@ VirtioNetInitTx (
> return EFI_OUT_OF_RESOURCES;
> }
>
> + Dev->TxBufMapInfoCollection = OrderedCollectionInit (
> + VirtioNetTxMapInfoCompare,
> + VirtioNetTxMapInfoCompare
> + );
(3) VirtioNetTxMapInfoCompare is not right for both of the arguments,
but I'll come to that below.
> + if (Dev->TxBufMapInfoCollection == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto FreeTxFreeStack;
> + }
> +
(4) The code is good, but please update the documentation of "@retval
EFI_OUT_OF_RESOURCES".
> //
> // Allocate TxSharedReq header and map with BusMasterCommonBuffer so that it
> // can be accessed equally by both processor and device.
> @@ -186,7 +195,7 @@ VirtioNetInitTx (
> &TxSharedReqBuffer
> );
> if (EFI_ERROR (Status)) {
> - goto FreeTxFreeStack;
> + goto UninitMapInfoCollection;
> }
>
> ZeroMem (TxSharedReqBuffer, sizeof *Dev->TxSharedReq);
> @@ -267,6 +276,10 @@ FreeTxSharedReqBuffer:
> EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
> TxSharedReqBuffer
> );
> +
> +UninitMapInfoCollection:
> + OrderedCollectionUninit (Dev->TxBufMapInfoCollection);
> +
> FreeTxFreeStack:
> FreePool (Dev->TxFreeStack);
>
> diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> index 2ec3dc385a9f..dafb538b4b5a 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> @@ -18,6 +18,17 @@
>
> #include "VirtioNet.h"
>
> +//
> +// The user structure for the ordered collection that will track the mapping
> +// info of the packets queued in TxRing
> +//
> +typedef struct {
> + UINT16 DescIdx;
> + VOID *Buffer;
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
> + VOID *BufMap;
> +} TX_BUF_MAP_INFO;
> +
Great, I like the new name for the structure, and the leading comment
too. Let me comment on the fields:
(5) Please drop the DescIdx field. The reverse mapping should occur
strictly from DeviceAddress to Buffer, as we documented in the previous
patch.
DescIdx is indeed *how* we get at DeviceAddress in the next patch
(namely the hypervisor sends us the DescIdx for the head descriptor, we
calculate the DescIdx for the tail descriptor, and then grab
DeviceAddress from the tail descriptor.) However, for the reverse
mapping itself, DescIdx is irrelevant. The key field for the reverse
lookup is DeviceAddress.
(6) Please add a postfix style comment to the DeviceAddress field:
EFI_PHYSICAL_ADDRESS DeviceAddress; // lookup key for reverse mapping
> /**
> Release RX and TX resources on the boundary of the
> EfiSimpleNetworkInitialized state.
> @@ -54,6 +65,20 @@ VirtioNetShutdownTx (
> IN OUT VNET_DEV *Dev
> )
> {
> + ORDERED_COLLECTION_ENTRY *Entry, *Entry2;
> + TX_BUF_MAP_INFO *TxBufMapInfo;
> +
> + for (Entry = OrderedCollectionMin (Dev->TxBufMapInfoCollection);
> + Entry != NULL;
> + Entry = Entry2) {
> + Entry2 = OrderedCollectionNext (Entry);
> + TxBufMapInfo = (TX_BUF_MAP_INFO *)Entry2;
This assignment is wrong, for two reasons: first, you should be using
Entry, not Entry2, second, this is not how the OrderedCollectionLib API
works.
The OrderedCollectionLib class operates with four objects; from
primitive to abstract/complex:
- ordering key
- user structure (containing an ordering key)
- iterator (pointing to a collection node which in turn points to a user
structure)
- collection (consisting of all nodes, and also knowing the comparator
functions for ordering keys and user structures)
If you look over "MdePkg/Include/Library/OrderedCollectionLib.h", the
functions explain what they take and what they output.
Let me go over some of the functions quickly:
- comparator between two user structures: this is needed whenever you
insert (link) a brand new user structure into the collection. The
structure being inserted has to be compared against existing
structures, using the ordering key field in both.
- comparator between a bare ordering key and a full structure: this is
needed when searching for an existent structure in the collection. For
the lookup, you don't want to create a fake user structure, just
provide the bare key itself. So the comparator has to compare the bare
key against the complete user structures in the collection (using the
ordering keys embedded into the latter).
- OrderedCollectionUserStruct(): this is a conversion function from
"iterator" ("entry") to "user structure". Several functions output
iterators: (a) lookup, (b) minimum, (c) maximum, (d) next, (e) prev.
Furthermore, (f) "insert" outputs an iterator for the just inserted
structure, or to the conflicting structure, if there is one.
- find: takes a standalone key and looks up the user structure with that
key in the collection. Returns an iterator. If you care about the rest
of the fields in the found structure, you have to conver the iterator
to user structure, with OrderedCollectionUserStruct().
- min, max, next, prev: should be obvious if you look at the prototypes
- insert: takes a full user structure and links it into the collection,
if there is no conflict between keys. Optionally outputs an iterator
for the just-linked user structure (for example you can use that to
advance to the just-following element in the collection, for whatever
reason). If there is a conflict (user structure already present with
that key), the iterator for the conflicting user struct is output
(optionally). If you care about the rest of the fields in the
conflicting element, you have to convert the iterator with
OrderedCollectionUserStruct().
- delete: takes an iterator (which you may have generated in any way
described above), unlinks the corresponding user structure from the
tree, and (optionally) hands the user structure to the caller at once.
The last part is a convenience parameter, because frequently you have
to unlink a user structure (which invalidates its iterator) and then
destroy the user structure too. Normally you'd have to call
OrderedCollectionUserStruct() on the iterator *first* (to get the user
structure), and call delete *second*, on the same iterator. With the
convenience parameter, it's OK to call just delete.
(7) Therefore please simply drop this assignment, and below I'll show
how to lay out the loop body.
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, TxBufMapInfo->BufMap);
> + FreePool (TxBufMapInfo);
> + OrderedCollectionDelete (Dev->TxBufMapInfoCollection, Entry, NULL);
(8) OK, so here's how to do the loop body (after assigning "Entry2",
which is good):
VOID *UserStruct;
OrderedCollectionDelete (Dev->TxBufMapInfoCollection, Entry, &UserStruct);
TxBufMapInfo = UserStruct;
Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, TxBufMapInfo->BufMap);
FreePool (TxBufMapInfo);
> + }
> + OrderedCollectionUninit (Dev->TxBufMapInfoCollection);
> +
> Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap);
> Dev->VirtIo->FreeSharedPages (
> Dev->VirtIo,
(9) In this function (VirtioNetShutdownTx()), please keep the same order
of teardown as on the error path in VirtioNetInitTx().
Namely, the ordered collection should be emptied and uninited just
before freeing "Dev->TxFreeStack".
> @@ -83,3 +108,166 @@ VirtioNetUninitRing (
> Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RingMap);
> VirtioRingUninit (Dev->VirtIo, Ring);
> }
> +
> +
> +/**
> + Map Caller-supplied TxBuf buffer to the device-mapped address
> +
> + @param[in] Dev The VNET_DEV driver instance which wants to
> + map the Tx packet.
> + @param[in] DescIdx VRING descriptor index which will point to
> + the device address
(10) Please drop the "DescIdx" parameter.
> + @param[in] Buffer The system physical address of TxBuf
> + @param[in] NumberOfBytes Number of bytes to map
> + @param[out] DeviceAddress The resulting device address for the bus
> + master access.
> +
> + @retval EFI_OUT_OF_RESOURCES The request could not be completed due to
> + a lack of resources.
> + @retval EFI_INVALID_PARAMETER The VRING descriptor index is already mapped.
(11) Please drop the EFI_INVALID_PARAMETER retval (more on it below).
(12) Please add "@return Status codes from
VirtioMapAllBytesInSharedBuffer()".
(13) Please add "@retval EFI_SUCCESS ..." for completeness.
> +*/
> +EFI_STATUS
> +EFIAPI
> +VirtioNetMapTxBuf (
> + IN VNET_DEV *Dev,
> + IN UINT16 DescIdx,
> + IN VOID *Buffer,
> + IN UINTN NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress
> + )
> +{
> + EFI_STATUS Status;
> + TX_BUF_MAP_INFO *TxBufMapInfo;
> + EFI_PHYSICAL_ADDRESS Address;
> + VOID *Mapping;
> + ORDERED_COLLECTION_ENTRY *Entry;
> +
> + TxBufMapInfo = AllocatePool (sizeof (*TxBufMapInfo));
> + if (TxBufMapInfo == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + Status = VirtioMapAllBytesInSharedBuffer (
> + Dev->VirtIo,
> + VirtioOperationBusMasterRead,
> + Buffer,
> + NumberOfBytes,
> + &Address,
> + &Mapping
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeTxBufMapInfo;
> + }
> +
> + TxBufMapInfo->DescIdx = DescIdx;
> + TxBufMapInfo->Buffer = Buffer;
> + TxBufMapInfo->DeviceAddress = Address;
> + TxBufMapInfo->BufMap = Mapping;
> +
> + Status = OrderedCollectionInsert (
> + Dev->TxBufMapInfoCollection,
> + &Entry,
> + TxBufMapInfo
> + );
> + switch (Status) {
> + case RETURN_OUT_OF_RESOURCES:
> + Status = EFI_OUT_OF_RESOURCES;
> + goto UnmapTxBufBuffer;
> + case RETURN_ALREADY_STARTED:
> + Status = EFI_INVALID_PARAMETER;
> + goto UnmapTxBufBuffer;
> + default:
> + ASSERT (Status == RETURN_SUCCESS);
> + break;
> + }
(14) Given that, in v3, the ordering key will be
"TX_BUF_MAP_INFO.DeviceAddress", the Status check after
OrderedCollectionInsert() should work like this (i.e., replace the
"switch" with the following):
if (Status == EFI_OUT_OF_RESOURCES) {
goto UnmapTxBufBuffer;
}
ASSERT_EFI_ERROR (Status);
In other words, ALREADY_STARTED should *never* be returned, because the
key comes from VirtioMapAllBytesInSharedBuffer(), and should be unique.
If there is a conflict, then the breakage is so serious that we cannot
do anything about it.
> + ASSERT (OrderedCollectionUserStruct (Entry) == TxBufMapInfo);
(15) I suggest to drop the "Entry" local variable, together with the
ASSERT() that depends on it.
Also, pass NULL in &Entry's stead to OrderedCollectionInsert().
(... if you really would like to keep the ASSERT(), it's your call,
ultimately.)
> +
> + *DeviceAddress = Address;
> +
> + return EFI_SUCCESS;
> +
> +UnmapTxBufBuffer:
(16) I think this label would be nicer if we renamed it to "UnmapTxBuf".
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
> +
> +FreeTxBufMapInfo:
> + FreePool (TxBufMapInfo);
> + return Status;
> +}
> +
> +/**
> + Unmap (aka reverse mapping) device mapped TxBuf buffer to the system
> + physical address
> +
> + @param[in] Dev The VNET_DEV driver instance which wants to
> + map the Tx packet.
(17) s/map/reverse- and unmap/
> + @param[in] DescIdx VRING descriptor index which point to
> + the device address
(18) Please drop DescIdx
> + @param[out] Buffer The system physical address of TxBuf
> + @param[out] DeviceAddress The device address for the TxBuf
The [out] label on DeviceAddress is wrong, the variable is marked IN
below.
This error in the patch is not surprising, given that the function
doesn't actually use "DeviceAddress" for anything right now.
This will be fixed in v3, where DescIdx disappears from this
representation, and DeviceAddress will become the reverse lookup key.
(19) So please change [out] to [in] on DeviceAddress.
> +
> + @retval EFI_INVALID_PARAMETER The VRING descriptor index is not mapped
(20) Please update the comment: "DeviceAddress is not mapped"
(21) Please add
@retval EFI_SUCCESS The TxBuf at DeviceAddress has been unmapped, and Buffer
has been set to TxBuf's system physical address.
> +*/
> +EFI_STATUS
> +EFIAPI
> +VirtioNetUnmapTxBuf (
> + IN VNET_DEV *Dev,
> + IN UINT16 DescIdx,
> + OUT VOID **Buffer,
> + IN EFI_PHYSICAL_ADDRESS DeviceAddress
> + )
> +{
> + TX_BUF_MAP_INFO StandaloneKey;
> + ORDERED_COLLECTION_ENTRY *Entry;
> + TX_BUF_MAP_INFO *UserStruct;
> + VOID *Ptr;
> + EFI_STATUS Status;
> +
> + StandaloneKey.DescIdx = DescIdx;
> + Entry = OrderedCollectionFind (Dev->TxBufMapInfoCollection, &StandaloneKey);
Here you actually construct a full user structure, *not* a stand-alone
key.
The stand-alone key (in version 2) is simply the "DescIdx" input
parameter. It "stands alone" because it is not embedded in a user
structure (i.e., a TX_BUF_MAP_INFO object). Therefore, in version 3,
(22) please drop the "StandaloneKey" local variable, and call
OrderedCollectionFind() like this:
Entry = OrderedCollectionFind (Dev->TxBufMapInfoCollection, &DeviceAddress);
Namely, in version 3, DeviceAddress is the ordering key, and passing it
like above makes it "stand alone".
> + if (Entry == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + OrderedCollectionDelete (Dev->TxBufMapInfoCollection, Entry, &Ptr);
(23) This function call is correct, but the variable names can be
improved:
- please rename "UserStruct" to "TxBufMapInfo",
- please rename "Ptr" to "UserStruct".
> +
> + UserStruct = Ptr;
> + ASSERT (UserStruct->DescIdx == DescIdx);
(24) drop the ASSERT()
(... well, if you don't trust OrderedCollectionLib, you can keep the
ASSERT() -- same story as point (15), it's your call; but then update
the key field to DeviceAddress)
> +
> + *Buffer = UserStruct->Buffer;
(25) Too much whitespace.
> + Status = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, UserStruct->BufMap);
> + FreePool (UserStruct);
> +
> + return Status;
> +}
Here we unmap a bus master read operation. An unmap operation tears down
the association unconditionally, the only thing that can fail is
committing the device's data to system memory -- but that doesn't make
sense after a bus master read op. Even if we got an error status from
the unmap, the function still succeeds.
(26) Therefore please ignore the return value of UnmapSharedBuffer() --
do not assign it to Status --, and return the EFI_SUCCESS constant.
(27) In fact you can drop the "Status" variable.
> +
> +/**
> + Comparator function for two user structures.
> +
> + @param[in] UserStruct1 Pointer to the first user structure.
> +
> + @param[in] UserStruct2 Pointer to the second user structure.
> +
> + @retval <0 If UserStruct1 compares less than UserStruct2.
> +
> + @retval 0 If UserStruct1 compares equal to UserStruct2.
> +
> + @retval >0 If UserStruct1 compares greater than UserStruct2.
> +*/
(28) Please update the documentation to:
Comparator function for two TX_BUF_MAP_INFO objects.
...
... Pointer to the (first|second) TX_BUF_MAP_INFO object
The parameter names themselves (UserStruct1, UserStruct2) are fine.
> +INTN
> +EFIAPI
> +VirtioNetTxMapInfoCompare (
(29) The function name should be "VirtioNetTxBufMapInfoCompare".
> + IN CONST VOID *UserStruct1,
> + IN CONST VOID *UserStruct2
> + )
> +{
> + CONST TX_BUF_MAP_INFO *MapInfo1;
> + CONST TX_BUF_MAP_INFO *MapInfo2;
> +
> + MapInfo1 = UserStruct1;
> + MapInfo2 = UserStruct2;
> +
> + return MapInfo1->DescIdx < MapInfo2->DescIdx ? -1 :
> + MapInfo1->DescIdx > MapInfo2->DescIdx ? 1 :
> + 0;
> +}
>
(30) Please replace the DescIdx field references with "DeviceAddress"
field references.
(31) Please add the following function (to VirtioNet.h as well), and
pass it to OrderedCollectionInit() as second parameter:
---------------
/**
Compare a standalone DeviceAddress against a TX_BUF_MAP_INFO object
containing an embedded DeviceAddress.
@param[in] StandaloneKey Pointer to DeviceAddress, which has type
EFI_PHYSICAL_ADDRESS.
@param[in] UserStruct Pointer to the TX_BUF_MAP_INFO object with the
embedded DeviceAddress.
@retval <0 If StandaloneKey compares less than UserStruct's key.
@retval 0 If StandaloneKey compares equal to UserStruct's key.
@retval >0 If StandaloneKey compares greater than UserStruct's key.
**/
INTN
EFIAPI
VirtioNetTxBufDeviceAddressCompare (
IN CONST VOID *StandaloneKey,
IN CONST VOID *UserStruct
)
{
CONST EFI_PHYSICAL_ADDRESS *DeviceAddress;
CONST TX_BUF_MAP_INFO *MapInfo;
DeviceAddress = StandaloneKey;
MapInfo = UserStruct;
return *DeviceAddress < MapInfo->DeviceAddress ? -1 :
*DeviceAddress > MapInfo->DeviceAddress ? 1 :
0;
}
---------------
Thank you!
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions
2017-09-13 18:07 ` Laszlo Ersek
@ 2017-09-13 19:24 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-09-13 19:24 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Ard Biesheuvel, Tom Lendacky
On 09/13/17 20:07, Laszlo Ersek wrote:
> On 09/11/17 14:16, Brijesh Singh wrote:
>> +*/
>> +EFI_STATUS
>> +EFIAPI
>> +VirtioNetMapTxBuf (
>> + IN VNET_DEV *Dev,
>> + IN UINT16 DescIdx,
>> + IN VOID *Buffer,
>> + IN UINTN NumberOfBytes,
>> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress
>> + )
>> +{
>> + EFI_STATUS Status;
>> + TX_BUF_MAP_INFO *TxBufMapInfo;
>> + EFI_PHYSICAL_ADDRESS Address;
>> + VOID *Mapping;
>> + ORDERED_COLLECTION_ENTRY *Entry;
>> +
>> + TxBufMapInfo = AllocatePool (sizeof (*TxBufMapInfo));
>> + if (TxBufMapInfo == NULL) {
>> + return EFI_OUT_OF_RESOURCES;
>> + }
>> +
>> + Status = VirtioMapAllBytesInSharedBuffer (
>> + Dev->VirtIo,
>> + VirtioOperationBusMasterRead,
>> + Buffer,
>> + NumberOfBytes,
>> + &Address,
>> + &Mapping
>> + );
>> + if (EFI_ERROR (Status)) {
>> + goto FreeTxBufMapInfo;
>> + }
>> +
>> + TxBufMapInfo->DescIdx = DescIdx;
>> + TxBufMapInfo->Buffer = Buffer;
>> + TxBufMapInfo->DeviceAddress = Address;
>> + TxBufMapInfo->BufMap = Mapping;
>> +
>> + Status = OrderedCollectionInsert (
>> + Dev->TxBufMapInfoCollection,
>> + &Entry,
>> + TxBufMapInfo
>> + );
>> + switch (Status) {
>> + case RETURN_OUT_OF_RESOURCES:
>> + Status = EFI_OUT_OF_RESOURCES;
>> + goto UnmapTxBufBuffer;
>> + case RETURN_ALREADY_STARTED:
>> + Status = EFI_INVALID_PARAMETER;
>> + goto UnmapTxBufBuffer;
>> + default:
>> + ASSERT (Status == RETURN_SUCCESS);
>> + break;
>> + }
>
> (14) Given that, in v3, the ordering key will be
> "TX_BUF_MAP_INFO.DeviceAddress", the Status check after
> OrderedCollectionInsert() should work like this (i.e., replace the
> "switch" with the following):
>
> if (Status == EFI_OUT_OF_RESOURCES) {
> goto UnmapTxBufBuffer;
> }
> ASSERT_EFI_ERROR (Status);
>
> In other words, ALREADY_STARTED should *never* be returned, because
> the key comes from VirtioMapAllBytesInSharedBuffer(), and should be
> unique. If there is a conflict, then the breakage is so serious that
> we cannot do anything about it.
I'd like to elaborate on my above comment.
Let's consider what happens when client code calls SNP.Transmit()
several times, in quick succession, using the *exact same* Buffer
argument -- for queueing the same packet several times, for whatever
reason --, *AND* we are using a virtio protocol implementation that
identity-maps the packets.
That means ALREADY_STARTED *will* be returned, because DeviceAddress
will not be unique.
The question is: is this a valid use of SNP.Transmit(), so that we must
accommodate it?
In order to answer this, let's look at the SNP.GetStatus() interface.
SNP.GetStatus() reports transmit completion by returning the original
TxBuf address. From the UEFI-2.7 spec, "EFI_SIMPLE_NETWORK.GetStatus()":
If TxBuf is not NULL, a recycled transmit buffer address will be
retrieved. If a recycled transmit buffer address is returned in
TxBuf, then the buffer has been successfully transmitted, and the
status for that buffer is cleared.
It is clear that the transmit buffer address shall uniquely identify the
transmit buffer; and that given a transmit buffer address, there is
exactly *one status* for that transmit buffer / transmit buffer address.
Therefore the use pattern I described above is invalid.
However, to be on the safe side, even in RELEASE builds, I suggest that
we keep your original error handling code, with the following
modification (note that I'm replacing RETURN_ with EFI_, because we're
already investigating an EFI_STATUS variable):
--------
switch (Status) {
case EFI_OUT_OF_RESOURCES:
goto UnmapTxBufBuffer;
case EFI_ALREADY_STARTED:
//
// This should never happen: it implies
//
// - an identity-mapping VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer()
// implementation -- which is fine,
//
// - and an SNP client that queues multiple instances of the exact same
// buffer address with SNP.Transmit() -- which is undefined behavior,
// based on the TxBuf language in UEFI-2.7,
// EFI_SIMPLE_NETWORK.GetStatus().
//
ASSERT (FALSE);
Status = EFI_INVALID_PARAMETER;
goto UnmapTxBufBuffer;
default:
ASSERT_EFI_ERROR (Status);
break;
}
--------
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address
2017-09-11 12:16 ` [PATCH v2 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address Brijesh Singh
@ 2017-09-13 20:15 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-09-13 20:15 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 09/11/17 14:16, Brijesh Singh wrote:
> When device is behind the IOMMU, driver is require to pass the device
> address of caller-supplied transmit buffer for the bus master operations.
>
> The patch uses VirtioNetMapTxBuf() to map caller-supplied Tx packet to a
> device-address and enqueue the device address in VRING for transfer and
> perform the reverse mapping when transfer is completed so that we can
> return the caller-supplied buffer.
>
> 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/VirtioNetDxe/SnpGetStatus.c | 30 +++++++++++++----
> OvmfPkg/VirtioNetDxe/SnpTransmit.c | 34 ++++++++++++++++----
> 2 files changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> index 694940ea1d97..9bd62fbe8ec0 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> @@ -61,11 +61,12 @@ VirtioNetGetStatus (
> OUT VOID **TxBuf OPTIONAL
> )
> {
> - VNET_DEV *Dev;
> - EFI_TPL OldTpl;
> - EFI_STATUS Status;
> - UINT16 RxCurUsed;
> - UINT16 TxCurUsed;
> + VNET_DEV *Dev;
> + EFI_TPL OldTpl;
> + EFI_STATUS Status;
> + UINT16 RxCurUsed;
> + UINT16 TxCurUsed;
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
>
> if (This == NULL) {
> return EFI_INVALID_PARAMETER;
> @@ -141,9 +142,24 @@ VirtioNetGetStatus (
> ASSERT (DescIdx < (UINT32) (2 * Dev->TxMaxPending - 1));
>
> //
> - // report buffer address to caller that has been enqueued by caller
> + // get the device address to caller that has been enqueued by caller
(1) I suggest, "get the device address that has been enqueued for the
caller's transmit buffer".
> //
> - *TxBuf = (VOID *)(UINTN) Dev->TxRing.Desc[DescIdx + 1].Addr;
> + DeviceAddress = Dev->TxRing.Desc[DescIdx + 1].Addr;
> +
> + //
> + // Unmap the device address and perform the reverse mapping to find the
> + // caller buffer address.
> + //
> + Status = VirtioNetUnmapTxBuf (
> + Dev,
> + DescIdx + 1,
(2) As discussed, this argument should go away.
> + TxBuf,
> + DeviceAddress
> + );
> + if (EFI_ERROR (Status)) {
> + Status = EFI_DEVICE_ERROR;
> + goto Exit;
> + }
Now, normally I would request the following: "Because you are
introducing a new error path here, it is now possible that
*InterruptStatus is modified earlier, but we exit with and error. Please
introduce a local variable for InterruptStatus, and only assign
*InterruptStatus when everything is fine."
However, VirtioNetUnmapTxBuf() should never fail. It can only return an
error if "DeviceAddress is not mapped", and that means our internal
state has been corrupted somehow.
(3) Therefore, please add such a comment, plus an "ASSERT (FALSE)" right
above the "Status = EFI_DEVICE_ERROR" assignment.
>
> //
> // now this descriptor can be used again to enqueue a transmit buffer
(4) Please move the VirtioNetUnmapTxBuf() call *under* the TxFreeStack
manipulation. So that the order of operations is ultimately:
(4a) fetch DeviceAddress
(4b) put DescIdx back on TxFreeStack
(4c) call VirtioNetUnmapTxBuf() as final operation (followed by the
error check + ASSERT, as discussed above)
> diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> index 7ca40d5d0650..0be3243b4606 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> @@ -73,11 +73,12 @@ VirtioNetTransmit (
> IN UINT16 *Protocol OPTIONAL
> )
> {
> - VNET_DEV *Dev;
> - EFI_TPL OldTpl;
> - EFI_STATUS Status;
> - UINT16 DescIdx;
> - UINT16 AvailIdx;
> + VNET_DEV *Dev;
> + EFI_TPL OldTpl;
> + EFI_STATUS Status;
> + UINT16 DescIdx;
> + UINT16 AvailIdx;
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
>
> if (This == NULL || BufferSize == 0 || Buffer == NULL) {
> return EFI_INVALID_PARAMETER;
> @@ -144,10 +145,29 @@ VirtioNetTransmit (
> }
>
> //
> - // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
> + // Get the free VRING descriptor
> //
> DescIdx = Dev->TxFreeStack[Dev->TxCurPending++];
> - Dev->TxRing.Desc[DescIdx + 1].Addr = (UINTN) Buffer;
> +
> + //
> + // Map the transmit buffer system physical address to device address.
> + //
> + Status = VirtioNetMapTxBuf (
> + Dev,
> + DescIdx + 1,
(5) this argument is going away
> + Buffer,
> + BufferSize,
> + &DeviceAddress
> + );
> + if (EFI_ERROR (Status)) {
> + Status = EFI_DEVICE_ERROR;
> + goto Exit;
> + }
VirtioNetMapTxBuf() can genuinely fail for a number of reasons, and if
we exit here like this, we will have leaked a descriptor from "TxFreeStack".
(6) Therefore, please don't touch the comment
// virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
Instead, move the VirtioNetMapTxBuf() call right above that comment. The
error handling for VirtioNetMapTxBuf() becomes correct then.
Thanks!
Laszlo
> +
> + //
> + // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
> + //
> + Dev->TxRing.Desc[DescIdx + 1].Addr = DeviceAddress;
> Dev->TxRing.Desc[DescIdx + 1].Len = (UINT32) BufferSize;
>
> //
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-09-13 20:12 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-11 12:16 [PATCH v2 0/8] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
2017-09-11 12:16 ` [PATCH v2 1/8] OvmfPkg/VirtioNetDxe: add helper VirtioNetUninitRing() Brijesh Singh
2017-09-13 7:26 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 2/8] OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap() Brijesh Singh
2017-09-13 8:07 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 3/8] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages() Brijesh Singh
2017-09-13 9:14 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 4/8] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header Brijesh Singh
2017-09-13 14:31 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 5/8] OvmfPkg/VirtioNetDxe: update TechNotes Brijesh Singh
2017-09-13 14:40 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions Brijesh Singh
2017-09-13 18:07 ` Laszlo Ersek
2017-09-13 19:24 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address Brijesh Singh
2017-09-13 20:15 ` Laszlo Ersek
2017-09-11 12:16 ` [PATCH v2 8/8] OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox