* [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap()
2017-09-01 11:24 [PATCH 0/5] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
@ 2017-09-01 11:24 ` Brijesh Singh
2017-09-05 11:47 ` Laszlo Ersek
2017-09-01 11:24 ` [PATCH 2/5] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages() Brijesh Singh
` (4 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Brijesh Singh @ 2017-09-01 11:24 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 to 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.h | 2 +
OvmfPkg/VirtioNetDxe/Events.c | 7 ++++
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 40 ++++++++++++++++----
OvmfPkg/VirtioNetDxe/SnpShutdown.c | 2 +
4 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 710859bc6115..d80d441b50a4 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -82,10 +82,12 @@ typedef struct {
EFI_HANDLE MacHandle; // VirtioNetDriverBindingStart
VRING RxRing; // VirtioNetInitRing
+ VOID *RxRingMap; // VirtioRingMap
UINT8 *RxBuf; // VirtioNetInitRx
UINT16 RxLastUsed; // VirtioNetInitRx
VRING TxRing; // VirtioNetInitRing
+ VOID *TxRingMap; // VirtioRingMap
UINT16 TxMaxPending; // VirtioNetInitTx
UINT16 TxCurPending; // VirtioNetInitTx
UINT16 *TxFreeStack; // VirtioNetInitTx
diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
index 5be1af6ffbee..6950c4d56df1 100644
--- a/OvmfPkg/VirtioNetDxe/Events.c
+++ b/OvmfPkg/VirtioNetDxe/Events.c
@@ -88,4 +88,11 @@ VirtioNetExitBoot (
if (Dev->Snm.State == EfiSimpleNetworkInitialized) {
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
}
+
+ //
+ // Unmap Tx and Rx rings so that hypervisor will not be able get readable data
+ // after device is reset.
+ //
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxRingMap);
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxRingMap);
}
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 0ecfe044a977..803a38bd4239 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 token return from the VirtioRingMap().
@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,13 @@ 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;
//
// step 4b -- allocate selected queue
@@ -79,30 +83,38 @@ VirtioNetInitRing (
return Status;
}
+ Status = VirtioRingMap (Dev->VirtIo, Ring, &RingBaseShift, Mapping);
+ 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.
//
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;
}
return EFI_SUCCESS;
+UnmapQueue:
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
+
ReleaseQueue:
VirtioRingUninit (Dev->VirtIo, Ring);
@@ -456,12 +468,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,9 +532,11 @@ AbortDevice:
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
ReleaseTxRing:
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxRingMap);
VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
ReleaseRxRing:
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxRingMap);
VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
DeviceFailed:
diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
index 5e84191fbbdd..36f3253e77ad 100644
--- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
+++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
@@ -67,7 +67,9 @@ VirtioNetShutdown (
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
VirtioNetShutdownRx (Dev);
VirtioNetShutdownTx (Dev);
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxRingMap);
VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxRingMap);
VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
Dev->Snm.State = EfiSimpleNetworkStarted;
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap()
2017-09-01 11:24 ` [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap() Brijesh Singh
@ 2017-09-05 11:47 ` Laszlo Ersek
2017-09-05 18:57 ` Brijesh Singh
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2017-09-05 11:47 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 09/01/17 13:24, 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 to 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.h | 2 +
> OvmfPkg/VirtioNetDxe/Events.c | 7 ++++
> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 40 ++++++++++++++++----
> OvmfPkg/VirtioNetDxe/SnpShutdown.c | 2 +
> 4 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 710859bc6115..d80d441b50a4 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -82,10 +82,12 @@ typedef struct {
> EFI_HANDLE MacHandle; // VirtioNetDriverBindingStart
>
> VRING RxRing; // VirtioNetInitRing
> + VOID *RxRingMap; // VirtioRingMap
> UINT8 *RxBuf; // VirtioNetInitRx
> UINT16 RxLastUsed; // VirtioNetInitRx
>
> VRING TxRing; // VirtioNetInitRing
> + VOID *TxRingMap; // VirtioRingMap
> UINT16 TxMaxPending; // VirtioNetInitTx
> UINT16 TxCurPending; // VirtioNetInitTx
> UINT16 *TxFreeStack; // VirtioNetInitTx
(1) Hmmm, here I could be inconsistent with my earlier requests for the
other drivers, but, in order to remain consistent with the comments
here, I think the new comments should say "VirtioNetInitRing" too.
Namely, the existent RxRing and TxRing fields are set up on the
following call path:
VirtioNetInitialize() -- this is the exported Initialize() protocol
member
VirtioNetInitRing()
VirtioRingInit() -- set up here
VirtioRingMap() -- this is added now
So, because we name VirtioNetInitRing() -- and not VirtioRingInit() --
for "RxRing" and "TxRing", the comments for "RxRingMap" and "TxRingMap"
should also say VirtioNetInitRing.
> diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
> index 5be1af6ffbee..6950c4d56df1 100644
> --- a/OvmfPkg/VirtioNetDxe/Events.c
> +++ b/OvmfPkg/VirtioNetDxe/Events.c
> @@ -88,4 +88,11 @@ VirtioNetExitBoot (
> if (Dev->Snm.State == EfiSimpleNetworkInitialized) {
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> }
> +
> + //
> + // Unmap Tx and Rx rings so that hypervisor will not be able get readable data
> + // after device is reset.
> + //
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxRingMap);
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxRingMap);
> }
(2) This is not correct; the mappings will exist if, and only if,
VirtioNetInitialize() completed successfully.
That is equivalent to (Dev->Snm.State == EfiSimpleNetworkInitialized).
Therefore please move both of these calls into the bracket just above,
visible in the context, after the virtio reset.
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 0ecfe044a977..803a38bd4239 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 token return from the VirtioRingMap().
>
> @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,13 @@ 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;
>
> //
> // step 4b -- allocate selected queue
> @@ -79,30 +83,38 @@ VirtioNetInitRing (
> return Status;
> }
>
(3) Please add a comment here:
//
// If anything fails from here on, we must release the ring resources.
//
> + Status = VirtioRingMap (Dev->VirtIo, Ring, &RingBaseShift, Mapping);
(4) Please use a local variable here; we shouldn't modify *Mapping until
the function succeeds fully.
> + 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.
> //
(5) In the above comment, please replace "release" with "unmap".
> 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;
> }
>
(6) So we should set *Mapping from the local variable here.
> return EFI_SUCCESS;
>
> +UnmapQueue:
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
> +
(7) The last argument should be (*Mapping), not (Mapping).
But, given my points (4) and (6), the local variable should be used
here.
> ReleaseQueue:
> VirtioRingUninit (Dev->VirtIo, Ring);
>
> @@ -456,12 +468,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,9 +532,11 @@ AbortDevice:
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>
> ReleaseTxRing:
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxRingMap);
> VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
>
> ReleaseRxRing:
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxRingMap);
> VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
>
> DeviceFailed:
> diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> index 5e84191fbbdd..36f3253e77ad 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> @@ -67,7 +67,9 @@ VirtioNetShutdown (
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> VirtioNetShutdownRx (Dev);
> VirtioNetShutdownTx (Dev);
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxRingMap);
> VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxRingMap);
> VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
>
> Dev->Snm.State = EfiSimpleNetworkStarted;
>
(8) The logic seems good (all locations are covered correctly), but I
feel we're adding too many standalone UnmapSharedBuffer() calls.
Therefore, please prepend a patch to the series that does the following:
(8a) affected files: "VirtioNet.h", "SnpSharedHelpers.c"
(8b) leading comments for the new function are not needed in *either*
"VirtioNet.h" *or* "SnpSharedHelpers.c". The global comments currently
seen in "SnpSharedHelpers.c" suffice.
(8c) the new function should be added right under VirtioNetShutdownTx().
(8d) This is the new function:
VOID
EFIAPI
VirtioNetUninitRing (
IN OUT VNET_DEV *Dev,
IN OUT VRING *Ring
)
{
VirtioRingUninit (Dev->VirtIo, Ring);
}
(EFIAPI wouldn't be strictly necessary, but I'd like to remain
consistent with the rest of the code.)
(8e) The VirtioRingUninit() calls in VirtioNetInitialize() and in
VirtioNetShutdown() -- four (4) in total -- should be replaced with
calls to this new function.
(8f) In this extra patch, please modify the file
"OvmfPkg/VirtioNetDxe/TechNotes.txt" to the following state:
> +-------------------------+
> | EfiSimpleNetworkStarted |
> +-------------------------+
> | ^
> [SnpInitialize.c] | | [SnpShutdown.c]
> VirtioNetInitialize | | VirtioNetShutdown
> VirtioNetInitRing {Rx, Tx} | | VirtioNetShutdownRx [SnpSharedHelpers.c]
> VirtioRingInit | | VirtioNetShutdownTx [SnpSharedHelpers.c]
> VirtioNetInitTx | | VirtioNetUninitRing [SnpSharedHelpers.c]
> VirtioNetInitRx | | {Tx, Rx}
> | | VirtioRingUninit
> v |
> +-----------------------------+
> | EfiSimpleNetworkInitialized |
> +-----------------------------+
(9) Then, in v2 of this patch,
(9a) modify the VirtioNetUninitRing() function like this:
VOID
EFIAPI
VirtioNetUninitRing (
IN OUT VNET_DEV *Dev,
IN OUT VRING *Ring,
IN VOID *RingMap
)
{
Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RingMap);
VirtioRingUninit (Dev->VirtIo, Ring);
}
(9b) The documentation of the "Mapping" parameter, for the
VirtioNetInitialize() function, should say, "a resulting token to pass
to VirtioNetUninitRing()".
(9c) modify the call sites to pass in Dev->TxRingMap / Dev->RxRingMap as
appropriate.
(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 |
> +-----------------------------+
(10) Please modify the subject to say "VRINGs" (plural).
Please also modify the commit message similarly: "map the VRING system
physical address[es] to device address[es]".
Would it be OK with you to submit only these two patches next?
I wouldn't like to look at the rest of the patches just now. I expect
quite a few tweaks -- especially because I would *really* like to keep
"TechNotes.txt" up to date! --, and I'd like to keep my focus, and
advance in small steps. I must re-read TechNotes.txt myself, in parallel
to the progress that we make with this series.
Once we consider these patches complete, we can test them and commit
them in isolation. Further versions of the series won't have to repeat
these patches.
I'll strive to be very responsive, so that you don't have to wait long
after every small step.
Thank you,
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap()
2017-09-05 11:47 ` Laszlo Ersek
@ 2017-09-05 18:57 ` Brijesh Singh
2017-09-05 20:17 ` Laszlo Ersek
0 siblings, 1 reply; 21+ messages in thread
From: Brijesh Singh @ 2017-09-05 18:57 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel
Cc: brijesh.singh, Jordan Justen, Tom Lendacky, Ard Biesheuvel
Hi Laszlo,
Thanks for quick reviews. I will go through each feedback address them in v2.
On 09/05/2017 06:47 AM, Laszlo Ersek wrote:
[...]
>
> Please also modify the commit message similarly: "map the VRING system
> physical address[es] to device address[es]".
>
>
> Would it be OK with you to submit only these two patches next?
I saw that you looked at other patches, just wondering, do you still want me
to submit two patches and advance in small steps ?
>
> I wouldn't like to look at the rest of the patches just now. I expect
> quite a few tweaks -- especially because I would *really* like to keep
> "TechNotes.txt" up to date! --, and I'd like to keep my focus, and
> advance in small steps. I must re-read TechNotes.txt myself, in parallel
> to the progress that we make with this series.
>
> Once we consider these patches complete, we can test them and commit
> them in isolation. Further versions of the series won't have to repeat
> these patches.
>
> I'll strive to be very responsive, so that you don't have to wait long
> after every small step.
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap()
2017-09-05 18:57 ` Brijesh Singh
@ 2017-09-05 20:17 ` Laszlo Ersek
2017-09-05 21:11 ` Ard Biesheuvel
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2017-09-05 20:17 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel
Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel, Jiewen Yao,
Star Zeng
On 09/05/17 20:57, Brijesh Singh wrote:
> Hi Laszlo,
>
> Thanks for quick reviews. I will go through each feedback address them
> in v2.
>
>
>
> On 09/05/2017 06:47 AM, Laszlo Ersek wrote:
>
> [...]
>
>>
>> Please also modify the commit message similarly: "map the VRING system
>> physical address[es] to device address[es]".
>>
>>
>> Would it be OK with you to submit only these two patches next?
>
>
> I saw that you looked at other patches, just wondering, do you still
> want me
> to submit two patches and advance in small steps ?
I'm not so sure anymore...
I haven't looked at patch #3 yet. I guess I could review it tomorrow.
... Based on the discussion in the other thread ("[edk2] [PATCH 0/4]
MdeModulePkg: some PCI HC drivers: unmap common buffers at
ExitBootServices()"), I'm worried that we might need another
restructuring for the IOMMU driver, and for the ExitBootServices()
handlers for the virtio drivers (removing the Unmap() calls).
If that's the case, then it wouldn't be good if you wasted work on
VirtioNetExitBoot() in v2 of this series.
Also under patch v1 #4, I requested that you not use
VirtioOperationBusMasterRead for DMA that remains pending after the
protocol member function returns, because we cannot Unmap()
BusMasterRead in VirtioNetExitBoot() without freeing memory. Dependent
of the outcome of said other thread, this might not be good advice after
all.
I'd be pretty disappointed if we had to go back to the drawing board at
this stage. In my opinion, the UEFI-2.7 spec doesn't offer any
facilities to us that would let us reliably and safely Unmap() bus
master operations (= re-encrypt memory) at ExitBootServices(), for such
PCI device drivers that we cannot modify. Whatever we do at this point
looks like a hack:
* Option #1: modify Virtio and other PCI drivers to use only
CommonBuffer operations for asynch DMA, and manually Unmap() those
operations in the ExitBootServices() handlers of the drivers. In
addition, guarantee that Unmap() for CommonBuffer will not release
memory.
This is the approach I've been supporting. We could implement it for
OVMF, because the community controls most of the platform (QEMU,
KVM, OVMF), OVMF is 100% open source, and we can propose patches for
other (e.g. MdeModulePkg) PCI drivers in edk2, if necessary.
Problem: PCI drivers are not required by the spec, or the Driver
Writer's Guide, to Unmap() on ExitBootServices() (and then to Unmap()
only CommonBuffer). Also, PciIo implementations are not required by
the spec to behave like this (= not free memory when Unmap()ing
CommonBuffer). We can satisfy those assumptions in OvmfPkg, but
perhaps not in MdeModulePkg drivers.
* Option #2: add an ExitBootServices() handler to the IOMMU driver, and
clean up mappings (= re-encrypt memory) after the PCI / Virtio drivers
are finalized in their ExitBootServices() handlers. Ignore mappings in
the drivers' ExitBootServices() handlers.
Problem: the keyword is "after". Under this approach, we *must* clean
up the mappings in the IOMMU driver, but we *must not* do that before
the device drivers are finished. And the UEFI spec does not allow us
to express a dependency order between ExitBootServices() handlers.
We could hack that around by deferring the actual cleanup to *another*
event handler, signaled from the IOMMU's "normal" ExitBootServices()
handler.
Problem: the UEFI spec does not promise that signaling events from
ExitBootServices() handlers is safe.
Problem: if some PCI driver does the same trick for whatever reason in
its ExitBootServices() handler, then we're back to square one.
* Option #3: ignore pending mappings (= decrypted memory areas) at
ExitBootServices(), leave them all to the OS.
Problem: how will the OS find them? Should we introduce another UEFI
configuration table? Config tables are generally allocated in
BootServicesData type memory, so the boot loader has to save them. Who
will write the grub2 code? Who will write the Linux kernel code to
parse the table, and to re-encrypt the memory (inherited from the
firmware) in-place?
* Option #4: ignore pending mappings (= decrypted memory areas) at
ExitBootServices(). Ignore them in the OS too. Let's hope that "it's
just a few pages of stale data, it won't compromise guest
confidentiality when the hypervisor is breached". Doesn't sound like a
good sales pitch for SEV.
Our approach thus far has been option #1 for the OvmfPkg VirtIo drivers,
and option #4 for all PCI drivers outside of OvmfPkg. If you are fine
with this, I am as well; we can continue this approach. (We should be
conscious of it though.)
My other series (see above) was inteded to extend option #1 to some
MdeModulePkg drivers (the main ones that we pull into OVMF, USB 1-2-3
and AHCI), and to start a discussion. If you believe that extending
option #1 to MdeModulePkg would be better for SEV, we should await the
outcome of that discussion. (Please do participate.)
Thanks,
Laszlo
>> I wouldn't like to look at the rest of the patches just now. I expect
>> quite a few tweaks -- especially because I would *really* like to keep
>> "TechNotes.txt" up to date! --, and I'd like to keep my focus, and
>> advance in small steps. I must re-read TechNotes.txt myself, in parallel
>> to the progress that we make with this series.
>>
>> Once we consider these patches complete, we can test them and commit
>> them in isolation. Further versions of the series won't have to repeat
>> these patches.
>>
>> I'll strive to be very responsive, so that you don't have to wait long
>> after every small step.
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap()
2017-09-05 20:17 ` Laszlo Ersek
@ 2017-09-05 21:11 ` Ard Biesheuvel
2017-09-05 21:59 ` Laszlo Ersek
0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2017-09-05 21:11 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Brijesh Singh, edk2-devel@lists.01.org, Jordan Justen,
Tom Lendacky, Jiewen Yao, Star Zeng
On 5 September 2017 at 21:17, Laszlo Ersek <lersek@redhat.com> wrote:
[...]
> ... Based on the discussion in the other thread ("[edk2] [PATCH 0/4]
> MdeModulePkg: some PCI HC drivers: unmap common buffers at
> ExitBootServices()"), I'm worried that we might need another
> restructuring for the IOMMU driver, and for the ExitBootServices()
> handlers for the virtio drivers (removing the Unmap() calls).
>
> If that's the case, then it wouldn't be good if you wasted work on
> VirtioNetExitBoot() in v2 of this series.
>
> Also under patch v1 #4, I requested that you not use
> VirtioOperationBusMasterRead for DMA that remains pending after the
> protocol member function returns, because we cannot Unmap()
> BusMasterRead in VirtioNetExitBoot() without freeing memory. Dependent
> of the outcome of said other thread, this might not be good advice after
> all.
>
> I'd be pretty disappointed if we had to go back to the drawing board at
> this stage. In my opinion, the UEFI-2.7 spec doesn't offer any
> facilities to us that would let us reliably and safely Unmap() bus
> master operations (= re-encrypt memory) at ExitBootServices(), for such
> PCI device drivers that we cannot modify. Whatever we do at this point
> looks like a hack:
>
>
> * Option #1: modify Virtio and other PCI drivers to use only
> CommonBuffer operations for asynch DMA, and manually Unmap() those
> operations in the ExitBootServices() handlers of the drivers. In
> addition, guarantee that Unmap() for CommonBuffer will not release
> memory.
>
> This is the approach I've been supporting. We could implement it for
> OVMF, because the community controls most of the platform (QEMU,
> KVM, OVMF), OVMF is 100% open source, and we can propose patches for
> other (e.g. MdeModulePkg) PCI drivers in edk2, if necessary.
>
> Problem: PCI drivers are not required by the spec, or the Driver
> Writer's Guide, to Unmap() on ExitBootServices() (and then to Unmap()
> only CommonBuffer). Also, PciIo implementations are not required by
> the spec to behave like this (= not free memory when Unmap()ing
> CommonBuffer). We can satisfy those assumptions in OvmfPkg, but
> perhaps not in MdeModulePkg drivers.
>
I think you will have much bigger problems with out of tree PCI
drivers that never use Map/Unmap in the first place, because stuff
just works on PCs if you omit them.
This is actually the reason I am quite pleased with this SEV support:
it means x86 drivers will start breaking in the same way as they would
have on ARM systems with non-coherent DMA or non-1:1 mapped PCI, and
vendors will have to clean up their code anyway, not just for ARM
compatibility.
The only requirement imposed on DMA capable devices is that they cease
to perform DMA after ExitBootServices. Anything beyond that should not
be the responsibility of the device driver, simply because that
requirement did not exist before, and so we cannot go back in time and
add it.
> * Option #2: add an ExitBootServices() handler to the IOMMU driver, and
> clean up mappings (= re-encrypt memory) after the PCI / Virtio drivers
> are finalized in their ExitBootServices() handlers. Ignore mappings in
> the drivers' ExitBootServices() handlers.
>
> Problem: the keyword is "after". Under this approach, we *must* clean
> up the mappings in the IOMMU driver, but we *must not* do that before
> the device drivers are finished. And the UEFI spec does not allow us
> to express a dependency order between ExitBootServices() handlers.
>
> We could hack that around by deferring the actual cleanup to *another*
> event handler, signaled from the IOMMU's "normal" ExitBootServices()
> handler.
>
> Problem: the UEFI spec does not promise that signaling events from
> ExitBootServices() handlers is safe.
>
> Problem: if some PCI driver does the same trick for whatever reason in
> its ExitBootServices() handler, then we're back to square one.
>
>
I think this is the only feasible option. Fortunately, we don't have
to solve this at the spec level, but only have to deal with the
implementation.
What we need is a method in the IOMMU protocol that is invoked when
the ExitBootServices implementation is about to return EFI_SUCCESS
(which means it could be the second time it was called). This severely
limits what the method is actually capable of doing, and I think it
should not be allowed to call any boot or DXE services at all, but it
should still be sufficient for some linked list traversals and
CopyMem() operations, and whatever else is needed to re-encrypt the
memory.
And actually, this is not only useful for SEV, other IOMMU drivers
will have to deal with the same issue: in most cases, you'll want to
disable it before handing over to the OS, but this can never be done
safely in a EBS event handler for precisely the reasons you pointed
out. Most PCI devices probably deal with this gracefully, but tearing
down mappings should simply be avoided if a device could still be
accessing it.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap()
2017-09-05 21:11 ` Ard Biesheuvel
@ 2017-09-05 21:59 ` Laszlo Ersek
2017-09-05 22:18 ` Ard Biesheuvel
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2017-09-05 21:59 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Brijesh Singh, edk2-devel@lists.01.org, Jordan Justen,
Tom Lendacky, Jiewen Yao, Star Zeng
On 09/05/17 23:11, Ard Biesheuvel wrote:
> On 5 September 2017 at 21:17, Laszlo Ersek <lersek@redhat.com> wrote:
> [...]
>> ... Based on the discussion in the other thread ("[edk2] [PATCH 0/4]
>> MdeModulePkg: some PCI HC drivers: unmap common buffers at
>> ExitBootServices()"), I'm worried that we might need another
>> restructuring for the IOMMU driver, and for the ExitBootServices()
>> handlers for the virtio drivers (removing the Unmap() calls).
>>
>> If that's the case, then it wouldn't be good if you wasted work on
>> VirtioNetExitBoot() in v2 of this series.
>>
>> Also under patch v1 #4, I requested that you not use
>> VirtioOperationBusMasterRead for DMA that remains pending after the
>> protocol member function returns, because we cannot Unmap()
>> BusMasterRead in VirtioNetExitBoot() without freeing memory. Dependent
>> of the outcome of said other thread, this might not be good advice after
>> all.
>>
>> I'd be pretty disappointed if we had to go back to the drawing board at
>> this stage. In my opinion, the UEFI-2.7 spec doesn't offer any
>> facilities to us that would let us reliably and safely Unmap() bus
>> master operations (= re-encrypt memory) at ExitBootServices(), for such
>> PCI device drivers that we cannot modify. Whatever we do at this point
>> looks like a hack:
>>
>>
>> * Option #1: modify Virtio and other PCI drivers to use only
>> CommonBuffer operations for asynch DMA, and manually Unmap() those
>> operations in the ExitBootServices() handlers of the drivers. In
>> addition, guarantee that Unmap() for CommonBuffer will not release
>> memory.
>>
>> This is the approach I've been supporting. We could implement it for
>> OVMF, because the community controls most of the platform (QEMU,
>> KVM, OVMF), OVMF is 100% open source, and we can propose patches for
>> other (e.g. MdeModulePkg) PCI drivers in edk2, if necessary.
>>
>> Problem: PCI drivers are not required by the spec, or the Driver
>> Writer's Guide, to Unmap() on ExitBootServices() (and then to Unmap()
>> only CommonBuffer). Also, PciIo implementations are not required by
>> the spec to behave like this (= not free memory when Unmap()ing
>> CommonBuffer). We can satisfy those assumptions in OvmfPkg, but
>> perhaps not in MdeModulePkg drivers.
>>
>
> I think you will have much bigger problems with out of tree PCI
> drivers that never use Map/Unmap in the first place, because stuff
> just works on PCs if you omit them.
I'm currently not worried about out-of-tree PCI drivers in the least :)
I'm worried about finding common grounds with MdeModulePkg drivers,
because they do use Map/Unmap, they seek to conform to the UEFI spec
(mostly), and they are an example / reference implementation for other
dirvers / driver writers.
If out-of-tree drivers break, let them break (as first step); exactly
for the reason you state.
> This is actually the reason I am quite pleased with this SEV support:
> it means x86 drivers will start breaking in the same way as they would
> have on ARM systems with non-coherent DMA or non-1:1 mapped PCI, and
> vendors will have to clean up their code anyway, not just for ARM
> compatibility.
I certainly agree this is a benefit!
>From the virtio conversion thus far (even from the mostly-reviewer
side), I can say it is a lot of work.
> The only requirement imposed on DMA capable devices is that they cease
> to perform DMA after ExitBootServices. Anything beyond that should not
> be the responsibility of the device driver, simply because that
> requirement did not exist before, and so we cannot go back in time and
> add it.
Fine by me (as a general guideline, not necessarily as a practical one);
what can we use instead, for closing down the IOMMU holes late enough,
originally opened by the PciIo.Map() calls?
>
>> * Option #2: add an ExitBootServices() handler to the IOMMU driver, and
>> clean up mappings (= re-encrypt memory) after the PCI / Virtio drivers
>> are finalized in their ExitBootServices() handlers. Ignore mappings in
>> the drivers' ExitBootServices() handlers.
>>
>> Problem: the keyword is "after". Under this approach, we *must* clean
>> up the mappings in the IOMMU driver, but we *must not* do that before
>> the device drivers are finished. And the UEFI spec does not allow us
>> to express a dependency order between ExitBootServices() handlers.
>>
>> We could hack that around by deferring the actual cleanup to *another*
>> event handler, signaled from the IOMMU's "normal" ExitBootServices()
>> handler.
>>
>> Problem: the UEFI spec does not promise that signaling events from
>> ExitBootServices() handlers is safe.
>>
>> Problem: if some PCI driver does the same trick for whatever reason in
>> its ExitBootServices() handler, then we're back to square one.
>>
>>
>
> I think this is the only feasible option. Fortunately, we don't have
> to solve this at the spec level, but only have to deal with the
> implementation.
>
> What we need is a method in the IOMMU protocol that is invoked when
> the ExitBootServices implementation is about to return EFI_SUCCESS
Yes, after "everything else" is done.
(Of course, this is the age-old problem with UEFI, when components that
were originally meant to be independent now try to order themselves in
some fashion. For example, "let me just install, or patch, this ACPI
table or configuration table quickly at ReadyToBoot, *after* everyone
else is done, and I get a good look at system state". Now combine two
such DXE drivers into a firmware, and hilarity ensues as they fight for
the last word.)
No facility exists to my knowledge (on the spec level) that would enable
such fine delaying or dependency ordering between EBS handlers. The only
ordering is between notification functions enqueued at TPL_NOTIFY vs.
TPL_CALLBACK, and there is a guarantee (I think?) that if you get
signalled demonstrably later (i.e., not via an event group), then you
get queued for later, within the same TPL.
The problem (for this discussion) is that any random PCI driver can
register its EBS event notification function at either TPL_NOTIFY or at
TPL_CALLBACK, plus that all EBS events are signaled together as an event
group. Consequently, if the IOMMU driver registers its EBS notifier at
TPL_NOTIFY, it is guaranteed to run earlier than all notifiers with
TPL_CALLBACK (which is exactly what we *don't* want). If the IOMMU
registers its EBS handler at TPL_CALLBACK, then (due to being signaled
through a large event group) the notifier will still be invoked
somewhere in the middle of a bunch of other TPL_CALLBACK-level notifiers
-- that is, not necessarily in the last position.
Hence my floating of a hack to re-queue the notification (to another
TPL_CALLBACK handler), so that we get to the "end of the low-prio
queue". But calling SignalEvent() from an EBS handler is not explicitly
permitted in the spec. (Below you even suggest that an EBS handler
should not call any boot service.)
Of course, if CoreExitBootServices() can be updated specifically for
this use case, I won't protest.
>
> (which means it could be the second time it was called).
Side remark: the CoreExitBootServices() implementation does not notice
memory map changes incurred by the ExitBootServices() handlers, in my
interpretation.
CoreExitBootServices() has a MapKey (= "memmap generation") check early
on, in CoreTerminateMemoryMap(). This check catches memmap changes
between the last GetMemoryMap(), and the call to ExitBootServices().
After this check succeeds, the notify functions are kicked, and on this
path, CoreExitBootServices() simply cannot return any other value than
EFI_SUCCESS. So whatever mess the individual callbacks make, it doesn't
notice or report.
Perhaps this is better for your argument, actually. I'm not fully sure.
> This severely
> limits what the method is actually capable of doing, and I think it
> should not be allowed to call any boot or DXE services at all, but it
> should still be sufficient for some linked list traversals and
> CopyMem() operations, and whatever else is needed to re-encrypt the
> memory.
Yes, the necessary actions are sufficiently low-level for this. The
problem is the ordering.
>
> And actually, this is not only useful for SEV, other IOMMU drivers
> will have to deal with the same issue: in most cases, you'll want to
> disable it before handing over to the OS, but this can never be done
> safely in a EBS event handler for precisely the reasons you pointed
> out. Most PCI devices probably deal with this gracefully, but tearing
> down mappings should simply be avoided if a device could still be
> accessing it.
>
Fully agreed. Once Jiewen adds the policy option to lock down VT-d at
EBS (I hope I understood that plan right), dependent on OS support for
the IOMMU, the IOMMU faults can show up just the same at EBS, until the
rest of the PCI driver EBS handlers finish up.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap()
2017-09-05 21:59 ` Laszlo Ersek
@ 2017-09-05 22:18 ` Ard Biesheuvel
2017-09-05 22:37 ` Laszlo Ersek
0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2017-09-05 22:18 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Tom Lendacky, Jordan Justen, edk2-devel@lists.01.org, Jiewen Yao,
Star Zeng
On 5 September 2017 at 22:59, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/05/17 23:11, Ard Biesheuvel wrote:
>> On 5 September 2017 at 21:17, Laszlo Ersek <lersek@redhat.com> wrote:
[...]
>>> * Option #2: add an ExitBootServices() handler to the IOMMU driver, and
>>> clean up mappings (= re-encrypt memory) after the PCI / Virtio drivers
>>> are finalized in their ExitBootServices() handlers. Ignore mappings in
>>> the drivers' ExitBootServices() handlers.
>>>
>>> Problem: the keyword is "after". Under this approach, we *must* clean
>>> up the mappings in the IOMMU driver, but we *must not* do that before
>>> the device drivers are finished. And the UEFI spec does not allow us
>>> to express a dependency order between ExitBootServices() handlers.
>>>
>>> We could hack that around by deferring the actual cleanup to *another*
>>> event handler, signaled from the IOMMU's "normal" ExitBootServices()
>>> handler.
>>>
>>> Problem: the UEFI spec does not promise that signaling events from
>>> ExitBootServices() handlers is safe.
>>>
>>> Problem: if some PCI driver does the same trick for whatever reason in
>>> its ExitBootServices() handler, then we're back to square one.
>>>
>>>
>>
>> I think this is the only feasible option. Fortunately, we don't have
>> to solve this at the spec level, but only have to deal with the
>> implementation.
>>
>> What we need is a method in the IOMMU protocol that is invoked when
>> the ExitBootServices implementation is about to return EFI_SUCCESS
>
> Yes, after "everything else" is done.
>
> (Of course, this is the age-old problem with UEFI, when components that
> were originally meant to be independent now try to order themselves in
> some fashion. For example, "let me just install, or patch, this ACPI
> table or configuration table quickly at ReadyToBoot, *after* everyone
> else is done, and I get a good look at system state". Now combine two
> such DXE drivers into a firmware, and hilarity ensues as they fight for
> the last word.)
>
> No facility exists to my knowledge (on the spec level) that would enable
> such fine delaying or dependency ordering between EBS handlers. The only
> ordering is between notification functions enqueued at TPL_NOTIFY vs.
> TPL_CALLBACK, and there is a guarantee (I think?) that if you get
> signalled demonstrably later (i.e., not via an event group), then you
> get queued for later, within the same TPL.
>
> The problem (for this discussion) is that any random PCI driver can
> register its EBS event notification function at either TPL_NOTIFY or at
> TPL_CALLBACK, plus that all EBS events are signaled together as an event
> group. Consequently, if the IOMMU driver registers its EBS notifier at
> TPL_NOTIFY, it is guaranteed to run earlier than all notifiers with
> TPL_CALLBACK (which is exactly what we *don't* want). If the IOMMU
> registers its EBS handler at TPL_CALLBACK, then (due to being signaled
> through a large event group) the notifier will still be invoked
> somewhere in the middle of a bunch of other TPL_CALLBACK-level notifiers
> -- that is, not necessarily in the last position.
>
> Hence my floating of a hack to re-queue the notification (to another
> TPL_CALLBACK handler), so that we get to the "end of the low-prio
> queue". But calling SignalEvent() from an EBS handler is not explicitly
> permitted in the spec. (Below you even suggest that an EBS handler
> should not call any boot service.)
>
>
> Of course, if CoreExitBootServices() can be updated specifically for
> this use case, I won't protest.
>
I think this is the only solution, and not unreasonable given that the
IOMMU protocol is private to EDK2.
>>
>> (which means it could be the second time it was called).
>
> Side remark: the CoreExitBootServices() implementation does not notice
> memory map changes incurred by the ExitBootServices() handlers, in my
> interpretation.
>
> CoreExitBootServices() has a MapKey (= "memmap generation") check early
> on, in CoreTerminateMemoryMap(). This check catches memmap changes
> between the last GetMemoryMap(), and the call to ExitBootServices().
>
> After this check succeeds, the notify functions are kicked, and on this
> path, CoreExitBootServices() simply cannot return any other value than
> EFI_SUCCESS. So whatever mess the individual callbacks make, it doesn't
> notice or report.
>
CoreExitBootServices() disables the timer first, and so it will return
with event dispatch disabled if it fails, ensuring that it is no
longer possible for an event handler to be invoked between
GetMemoryMap and ExitBootServices.
> Perhaps this is better for your argument, actually. I'm not fully sure.
>
Not really :-)
>> This severely
>> limits what the method is actually capable of doing, and I think it
>> should not be allowed to call any boot or DXE services at all, but it
>> should still be sufficient for some linked list traversals and
>> CopyMem() operations, and whatever else is needed to re-encrypt the
>> memory.
>
> Yes, the necessary actions are sufficiently low-level for this. The
> problem is the ordering.
>
>>
>> And actually, this is not only useful for SEV, other IOMMU drivers
>> will have to deal with the same issue: in most cases, you'll want to
>> disable it before handing over to the OS, but this can never be done
>> safely in a EBS event handler for precisely the reasons you pointed
>> out. Most PCI devices probably deal with this gracefully, but tearing
>> down mappings should simply be avoided if a device could still be
>> accessing it.
>>
>
> Fully agreed. Once Jiewen adds the policy option to lock down VT-d at
> EBS (I hope I understood that plan right), dependent on OS support for
> the IOMMU, the IOMMU faults can show up just the same at EBS, until the
> rest of the PCI driver EBS handlers finish up.
>
Indeed. I think it is justified to treat the IOMMU protocol specially.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap()
2017-09-05 22:18 ` Ard Biesheuvel
@ 2017-09-05 22:37 ` Laszlo Ersek
2017-09-05 23:03 ` Ard Biesheuvel
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2017-09-05 22:37 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Tom Lendacky, Jordan Justen, edk2-devel@lists.01.org, Jiewen Yao,
Star Zeng
On 09/06/17 00:18, Ard Biesheuvel wrote:
> On 5 September 2017 at 22:59, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/05/17 23:11, Ard Biesheuvel wrote:
>>> (which means it could be the second time it was called).
>>
>> Side remark: the CoreExitBootServices() implementation does not notice
>> memory map changes incurred by the ExitBootServices() handlers, in my
>> interpretation.
>>
>> CoreExitBootServices() has a MapKey (= "memmap generation") check early
>> on, in CoreTerminateMemoryMap(). This check catches memmap changes
>> between the last GetMemoryMap(), and the call to ExitBootServices().
>>
>> After this check succeeds, the notify functions are kicked, and on this
>> path, CoreExitBootServices() simply cannot return any other value than
>> EFI_SUCCESS. So whatever mess the individual callbacks make, it doesn't
>> notice or report.
>>
>
> CoreExitBootServices() disables the timer first, and so it will return
> with event dispatch disabled if it fails, ensuring that it is no
> longer possible for an event handler to be invoked between
> GetMemoryMap and ExitBootServices.
True, but that's not what I meant -- I meant that, if an EBS handler
violates its contract and changes the memory map (= "it makes mess"), on
the call stack of
CoreNotifySignalList (&gEfiEventExitBootServicesGuid);
then CoreExitBootServices() won't notice it, it won't return an error,
and the caller of EBS() won't go back to calling GetMemoryMap() again.
> Indeed. I think it is justified to treat the IOMMU protocol specially.
The MemoryProtectionExitBootServicesCallback() function call looks like
prior art for such customizations.
This would mean more delay for SEV, but it would be a very desirable
cleanup, for the long term!
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap()
2017-09-05 22:37 ` Laszlo Ersek
@ 2017-09-05 23:03 ` Ard Biesheuvel
0 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-09-05 23:03 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Tom Lendacky, Jordan Justen, edk2-devel@lists.01.org, Jiewen Yao,
Star Zeng
On 5 September 2017 at 23:37, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/06/17 00:18, Ard Biesheuvel wrote:
>> On 5 September 2017 at 22:59, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 09/05/17 23:11, Ard Biesheuvel wrote:
>
>>>> (which means it could be the second time it was called).
>>>
>>> Side remark: the CoreExitBootServices() implementation does not notice
>>> memory map changes incurred by the ExitBootServices() handlers, in my
>>> interpretation.
>>>
>>> CoreExitBootServices() has a MapKey (= "memmap generation") check early
>>> on, in CoreTerminateMemoryMap(). This check catches memmap changes
>>> between the last GetMemoryMap(), and the call to ExitBootServices().
>>>
>>> After this check succeeds, the notify functions are kicked, and on this
>>> path, CoreExitBootServices() simply cannot return any other value than
>>> EFI_SUCCESS. So whatever mess the individual callbacks make, it doesn't
>>> notice or report.
>>>
>>
>> CoreExitBootServices() disables the timer first, and so it will return
>> with event dispatch disabled if it fails, ensuring that it is no
>> longer possible for an event handler to be invoked between
>> GetMemoryMap and ExitBootServices.
>
> True, but that's not what I meant -- I meant that, if an EBS handler
> violates its contract and changes the memory map (= "it makes mess"), on
> the call stack of
>
> CoreNotifySignalList (&gEfiEventExitBootServicesGuid);
>
> then CoreExitBootServices() won't notice it, it won't return an error,
> and the caller of EBS() won't go back to calling GetMemoryMap() again.
>
Ah right. So the only thing the memory map key protects against is
inadvertent interruptions by event handlers that modify the memory map
after GetMemoryMap(). It does not protect against programming errors.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/5] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages()
2017-09-01 11:24 [PATCH 0/5] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
2017-09-01 11:24 ` [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap() Brijesh Singh
@ 2017-09-01 11:24 ` Brijesh Singh
2017-09-05 15:06 ` Laszlo Ersek
2017-09-01 11:24 ` [PATCH 3/5] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header Brijesh Singh
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Brijesh Singh @ 2017-09-01 11:24 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.AllocateSharedPage() to allocate
the RxBuf and map with BusMasterCommonBuffer 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 | 3 +
OvmfPkg/VirtioNetDxe/Events.c | 6 ++
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 75 ++++++++++++++++----
OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 7 +-
4 files changed, 77 insertions(+), 14 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index d80d441b50a4..7df51bd044f5 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
@@ -85,6 +86,8 @@ typedef struct {
VOID *RxRingMap; // VirtioRingMap
UINT8 *RxBuf; // VirtioNetInitRx
UINT16 RxLastUsed; // VirtioNetInitRx
+ UINTN RxBufNoPages; // VirtioNetInitRx
+ VOID *RxBufMap; // VirtioMapSharedBuffer
VRING TxRing; // VirtioNetInitRing
VOID *TxRingMap; // VirtioRingMap
diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
index 6950c4d56df1..0586a82cdf09 100644
--- a/OvmfPkg/VirtioNetDxe/Events.c
+++ b/OvmfPkg/VirtioNetDxe/Events.c
@@ -95,4 +95,10 @@ VirtioNetExitBoot (
//
Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxRingMap);
Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxRingMap);
+
+ //
+ // Unmap Rx buffer so that hypervisor will not be able get readable data after
+ // device is reset.
+ //
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
}
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 803a38bd4239..6052d40fc6cb 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -236,7 +236,7 @@ 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.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate or map RX destination area.
@return Status codes from VIRTIO_CFG_WRITE().
@retval EFI_SUCCESS RX setup successful. The device is live and may
already be writing to the receive area.
@@ -249,13 +249,17 @@ 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;
+ UINT8 *RxPtr;
+ UINTN NumBytes;
+ EFI_PHYSICAL_ADDRESS RxBufDeviceAddress;
+ UINT64 BufBaseShift;
+ VOID *RxBuffer;
//
// In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on
@@ -280,11 +284,40 @@ VirtioNetInitRx (
//
RxAlwaysPending = (UINT16) MIN (Dev->RxRing.QueueSize / 2, VNET_MAX_PENDING);
- Dev->RxBuf = AllocatePool (RxAlwaysPending * RxBufSize);
- if (Dev->RxBuf == NULL) {
+ //
+ // 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->RxBufNoPages = EFI_SIZE_TO_PAGES (NumBytes);
+ Status = Dev->VirtIo->AllocateSharedPages (
+ Dev->VirtIo,
+ Dev->RxBufNoPages,
+ &RxBuffer
+ );
+ if (EFI_ERROR (Status)) {
return EFI_OUT_OF_RESOURCES;
}
+ Status = VirtioMapAllBytesInSharedBuffer (
+ Dev->VirtIo,
+ VirtioOperationBusMasterCommonBuffer,
+ RxBuffer,
+ NumBytes,
+ &RxBufDeviceAddress,
+ &Dev->RxBufMap
+ );
+ if (EFI_ERROR (Status)) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto FreeSharedBuffer;
+ }
+
+ Dev->RxBuf = RxBuffer;
+
+ BufBaseShift = (UINT64) (UINTN)Dev->RxBuf - (UINT64) RxBufDeviceAddress;
+
//
// virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device
//
@@ -306,6 +339,11 @@ VirtioNetInitRx (
DescIdx = 0;
RxPtr = Dev->RxBuf;
for (PktIdx = 0; PktIdx < RxAlwaysPending; ++PktIdx) {
+ UINT64 Address;
+
+ Address = (UINTN) RxPtr;
+ Address += BufBaseShift;
+
//
// virtio-0.9.5, 2.4.1.2 Updating the Available Ring
// invisible to the host until we update the Index Field
@@ -315,13 +353,13 @@ 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 = Address;
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;
- Dev->RxRing.Desc[DescIdx].Addr = (UINTN) RxPtr;
+ Dev->RxRing.Desc[DescIdx].Addr = Address;
Dev->RxRing.Desc[DescIdx].Len = (UINT32) (RxBufSize - VirtioNetReqSize);
Dev->RxRing.Desc[DescIdx].Flags = VRING_DESC_F_WRITE;
RxPtr += Dev->RxRing.Desc[DescIdx++].Len;
@@ -345,10 +383,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->RxBufNoPages,
+ (VOID *) Dev->RxBuf
+ );
+ return Status;
}
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 9fedb72fdbd4..4c9d9ece0790 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->RxBufNoPages,
+ (VOID *) Dev->RxBuf
+ );
}
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/5] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages()
2017-09-01 11:24 ` [PATCH 2/5] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages() Brijesh Singh
@ 2017-09-05 15:06 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2017-09-05 15:06 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
Looks like I'm slowly getting to the 2nd patch after all:
On 09/01/17 13:24, 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.AllocateSharedPage() to allocate
> the RxBuf and map with BusMasterCommonBuffer so that we can obtain the
> device address for RxBuf.
(1) the "AllocateSharedPage" member function is called
"AllocateSharedPages" (plural)
(2) since we mention mapping, it's probably best to mention the helper
function that we use -- VirtioMapAllBytesInSharedBuffer()
>
> 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/Events.c | 6 ++
> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 75 ++++++++++++++++----
> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 7 +-
> 4 files changed, 77 insertions(+), 14 deletions(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index d80d441b50a4..7df51bd044f5 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
> @@ -85,6 +86,8 @@ typedef struct {
> VOID *RxRingMap; // VirtioRingMap
> UINT8 *RxBuf; // VirtioNetInitRx
> UINT16 RxLastUsed; // VirtioNetInitRx
> + UINTN RxBufNoPages; // VirtioNetInitRx
> + VOID *RxBufMap; // VirtioMapSharedBuffer
(3) The comment on both new fields should be "VirtioNetInitRx".
(4) Please rename "RxBufNoPages" to "RxBufNrPages".
("RxBufNumberOfPages" would be too long for this table, and its "mixed
verbosity" wouldn't be consistent with the rest of the fields.)
>
> VRING TxRing; // VirtioNetInitRing
> VOID *TxRingMap; // VirtioRingMap
(5) I guess I can add this remark here, about the documentation:
I've now re-read "TechNotes.txt", until the end of the
Virtio internals -- Rx
section (i.e. I stopped just before "Virtio internals -- Tx"). I think
that this section needs to be updated in one spot only; namely the
expression
(guest-physical) addresses of these sub-slices
should be replaced with
(device-mapped) addresses of these sub-slices
Please include such an update in this patch.
> diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
> index 6950c4d56df1..0586a82cdf09 100644
> --- a/OvmfPkg/VirtioNetDxe/Events.c
> +++ b/OvmfPkg/VirtioNetDxe/Events.c
> @@ -95,4 +95,10 @@ VirtioNetExitBoot (
> //
> Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxRingMap);
> Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxRingMap);
> +
> + //
> + // Unmap Rx buffer so that hypervisor will not be able get readable data after
> + // device is reset.
> + //
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
> }
(6) My earlier comment applies; the RX structures are erected in
VirtioNetInitialize() --> VirtioNetInitRx(), so they are only available
in EfiSimpleNetworkInitialized state.
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 803a38bd4239..6052d40fc6cb 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -236,7 +236,7 @@ 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.
> + @retval EFI_OUT_OF_RESOURCES Failed to allocate or map RX destination area.
> @return Status codes from VIRTIO_CFG_WRITE().
> @retval EFI_SUCCESS RX setup successful. The device is live and may
> already be writing to the receive area.
> @@ -249,13 +249,17 @@ 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;
> + UINT8 *RxPtr;
> + UINTN NumBytes;
> + EFI_PHYSICAL_ADDRESS RxBufDeviceAddress;
> + UINT64 BufBaseShift;
> + VOID *RxBuffer;
>
> //
> // In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on
> @@ -280,11 +284,40 @@ VirtioNetInitRx (
> //
> RxAlwaysPending = (UINT16) MIN (Dev->RxRing.QueueSize / 2, VNET_MAX_PENDING);
>
> - Dev->RxBuf = AllocatePool (RxAlwaysPending * RxBufSize);
> - if (Dev->RxBuf == NULL) {
> + //
> + // 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->RxBufNoPages = EFI_SIZE_TO_PAGES (NumBytes);
> + Status = Dev->VirtIo->AllocateSharedPages (
> + Dev->VirtIo,
> + Dev->RxBufNoPages,
> + &RxBuffer
> + );
> + if (EFI_ERROR (Status)) {
> return EFI_OUT_OF_RESOURCES;
> }
(7) I suggest propagating Status here, and updating the comment block
above accordingly.
>
> + Status = VirtioMapAllBytesInSharedBuffer (
> + Dev->VirtIo,
> + VirtioOperationBusMasterCommonBuffer,
> + RxBuffer,
> + NumBytes,
> + &RxBufDeviceAddress,
> + &Dev->RxBufMap
> + );
> + if (EFI_ERROR (Status)) {
> + Status = EFI_OUT_OF_RESOURCES;
(8) Same here, I suggest propagating the status code, and updating the
leading comment block.
> + goto FreeSharedBuffer;
> + }
> +
> + Dev->RxBuf = RxBuffer;
> +
> + BufBaseShift = (UINT64) (UINTN)Dev->RxBuf - (UINT64) RxBufDeviceAddress;
> +
> //
> // virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device
> //
> @@ -306,6 +339,11 @@ VirtioNetInitRx (
> DescIdx = 0;
> RxPtr = Dev->RxBuf;
> for (PktIdx = 0; PktIdx < RxAlwaysPending; ++PktIdx) {
> + UINT64 Address;
> +
> + Address = (UINTN) RxPtr;
> + Address += BufBaseShift;
> +
(9) Please remove the following local variables: "BufBaseShift" (new),
"Address" (new), "RxPtr" (old).
(10) In "VirtioNet.h", please introduce a new field as follows:
EFI_PHYSICAL_ADDRESS RxBufDeviceBase; // VirtioNetInitRx
(11) Rather than the "RxBufDeviceAddress" local variable, the
"Dev->RxBufDeviceBase" field should receive the mapped address in this
function.
(12) The "RxBufDeviceAddress" (new) local variable should be preserved,
and it should take the role of the current "RxPtr" variable. Namely, it
should be
- set to "Dev->RxBufDeviceBase" before the loop,
- incremented in the loop,
- and assigned to the "Addr" field.
(13) And now, the explanation for all of the above: in this patch you
missed the VirtioNetReceive() function, in "SnpReceive.c". In that
function, we currently have
RxPtr = (UINT8 *)(UINTN) Dev->RxRing.Desc[DescIdx + 1].Addr;
In this patch, the "Addr" field has to be reverse-mapped, like this:
UINTN RxBufOffset;
RxBufOffset = (UINTN)(Dev->RxRing.Desc[DescIdx + 1].Addr -
Dev->RxBufDeviceBase);
RxPtr = Dev->RxBuf + RxBufOffset;
(No change to the rest of the code in VirtioNetReceive().)
> //
> // virtio-0.9.5, 2.4.1.2 Updating the Available Ring
> // invisible to the host until we update the Index Field
> @@ -315,13 +353,13 @@ 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 = Address;
> 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;
>
> - Dev->RxRing.Desc[DescIdx].Addr = (UINTN) RxPtr;
> + Dev->RxRing.Desc[DescIdx].Addr = Address;
> Dev->RxRing.Desc[DescIdx].Len = (UINT32) (RxBufSize - VirtioNetReqSize);
> Dev->RxRing.Desc[DescIdx].Flags = VRING_DESC_F_WRITE;
> RxPtr += Dev->RxRing.Desc[DescIdx++].Len;
> @@ -345,10 +383,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->RxBufNoPages,
> + (VOID *) Dev->RxBuf
> + );
(14) The last argument is incorrect. If you jump here because
VirtioMapAllBytesInSharedBuffer() fails, then Dev->RxBuf is not set yet,
only RxBuffer. Please say RxBuffer.
> + return Status;
> }
>
>
> diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> index 9fedb72fdbd4..4c9d9ece0790 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->RxBufNoPages,
> + (VOID *) Dev->RxBuf
> + );
> }
(15) There's no need for the explicit (VOID*) cast.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/5] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header
2017-09-01 11:24 [PATCH 0/5] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
2017-09-01 11:24 ` [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap() Brijesh Singh
2017-09-01 11:24 ` [PATCH 2/5] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages() Brijesh Singh
@ 2017-09-01 11:24 ` Brijesh Singh
2017-09-06 9:11 ` Laszlo Ersek
2017-09-01 11:24 ` [PATCH 4/5] OvmfPkg/VirtioNetDxe: map virtio-net transmit request buffer Brijesh Singh
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Brijesh Singh @ 2017-09-01 11:24 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
A network packets are transmitted by placing them into a transmit queue,
each packet is preceded by a VIRTIO_1_0_NET_REQ header. VirtioNetInitTx(),
builds the header once and fills the vring descriptors with the system
physical address of the VIRTIO_1_0_NET_REQ header.
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 could map it with BusMasterRead but since the header pointer is
used after it was added into vring hence I choose to dynamically allocate
and map it with BusMasterCommonBuffer to avoid the code complexity.
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/Events.c | 6 ++
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 65 +++++++++++++++++---
OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 7 +++
4 files changed, 72 insertions(+), 9 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 7df51bd044f5..3efe95a735d8 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -94,7 +94,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; // VirtioMapSharedBuffer
UINT16 TxLastUsed; // VirtioNetInitTx
} VNET_DEV;
diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
index 0586a82cdf09..9366aa00a1b3 100644
--- a/OvmfPkg/VirtioNetDxe/Events.c
+++ b/OvmfPkg/VirtioNetDxe/Events.c
@@ -101,4 +101,10 @@ VirtioNetExitBoot (
// device is reset.
//
Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
+
+ //
+ // Unmap shared Tx request mapping so that hypervisor will not be able to get
+ // readable data after device is reset.
+ //
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap);
}
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 6052d40fc6cb..551820e30f36 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>
@@ -151,8 +152,12 @@ VirtioNetInitTx (
IN OUT VNET_DEV *Dev
)
{
- UINTN TxSharedReqSize;
- UINTN PktIdx;
+ UINTN TxSharedReqSize;
+ UINTN PktIdx;
+ EFI_STATUS Status;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+ UINT64 BufBaseShift;
+ VOID *TxSharedReqBuffer;
Dev->TxMaxPending = (UINT16) MIN (Dev->TxRing.QueueSize / 2,
VNET_MAX_PENDING);
@@ -164,24 +169,60 @@ 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)) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ Status = VirtioMapAllBytesInSharedBuffer (
+ Dev->VirtIo,
+ VirtioOperationBusMasterCommonBuffer,
+ TxSharedReqBuffer,
+ sizeof *(Dev->TxSharedReq),
+ &DeviceAddress,
+ &Dev->TxSharedReqMap
+ );
+ if (EFI_ERROR (Status)) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto FreeBuffer;
+ }
+
+ Dev->TxSharedReq = TxSharedReqBuffer;
+
+ BufBaseShift = (UINT64) (UINTN)Dev->TxSharedReq - (UINT64) DeviceAddress;
+
+ ZeroMem ((VOID *) Dev->TxSharedReq, sizeof *(Dev->TxSharedReq));
+
+ //
// 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;
+ UINT64 Address;
DescIdx = (UINT16) (2 * PktIdx);
Dev->TxFreeStack[PktIdx] = DescIdx;
+ Address = (UINTN) Dev->TxSharedReq;
+ Address += BufBaseShift;
+
//
// 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 = Address;
Dev->TxRing.Desc[DescIdx].Len = (UINT32) TxSharedReqSize;
Dev->TxRing.Desc[DescIdx].Flags = VRING_DESC_F_NEXT;
Dev->TxRing.Desc[DescIdx].Next = (UINT16) (DescIdx + 1);
@@ -196,13 +237,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
@@ -217,6 +258,14 @@ VirtioNetInitTx (
*Dev->TxRing.Avail.Flags = (UINT16) VRING_AVAIL_F_NO_INTERRUPT;
return EFI_SUCCESS;
+
+FreeBuffer:
+ Dev->VirtIo->FreeSharedPages (
+ Dev->VirtIo,
+ EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
+ (VOID *) Dev->TxSharedReq
+ );
+ return Status;
}
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 4c9d9ece0790..aeb9e6605f0d 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -54,5 +54,12 @@ 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.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 3/5] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header
2017-09-01 11:24 ` [PATCH 3/5] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header Brijesh Singh
@ 2017-09-06 9:11 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2017-09-06 9:11 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 09/01/17 13:24, Brijesh Singh wrote:
> A network packets are transmitted by placing them into a transmit queue,
> each packet is preceded by a VIRTIO_1_0_NET_REQ header. VirtioNetInitTx(),
> builds the header once and fills the vring descriptors with the system
> physical address of the VIRTIO_1_0_NET_REQ header.
(1) Please *replace* this paragraph (emphasis added to avoid
misunderstandings) with the following:
----
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 could map it with BusMasterRead but since the header pointer is
> used after it was added into vring hence I choose to dynamically allocate
> and map it with BusMasterCommonBuffer to avoid the code complexity.
(2) This is a very good choice!, but I'd like the commit message to be
clearer. How about this, as a replacement for the last paragraph:
-------
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
second.
-------
(And then, we're not even mentioning that the header has to be
unmappable at ExitBootServices() without freeing memory. But see the
parallel discussion about that anyway...)
>
> 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/Events.c | 6 ++
> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 65 +++++++++++++++++---
> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 7 +++
> 4 files changed, 72 insertions(+), 9 deletions(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 7df51bd044f5..3efe95a735d8 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -94,7 +94,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; // VirtioMapSharedBuffer
(3) Consistently with this code, and with my other comments for this
patch-set (and inconsistently with the other virtio drivers, sorry about
that), please make the comment say "VirtioNetInitTx" for
"TxSharedReqMap" as well.
> UINT16 TxLastUsed; // VirtioNetInitTx
> } VNET_DEV;
>
> diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
> index 0586a82cdf09..9366aa00a1b3 100644
> --- a/OvmfPkg/VirtioNetDxe/Events.c
> +++ b/OvmfPkg/VirtioNetDxe/Events.c
> @@ -101,4 +101,10 @@ VirtioNetExitBoot (
> // device is reset.
> //
> Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
> +
> + //
> + // Unmap shared Tx request mapping so that hypervisor will not be able to get
> + // readable data after device is reset.
> + //
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap);
> }
(4) This too belongs in the "EfiSimpleNetworkInitialized" scope.
(Assuming we're going to stick with the unmap-at-ExitBootServices idea
at all... See the other discussion. For now, I recommend going ahead
with this approach.)
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 6052d40fc6cb..551820e30f36 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>
>
> @@ -151,8 +152,12 @@ VirtioNetInitTx (
> IN OUT VNET_DEV *Dev
> )
> {
> - UINTN TxSharedReqSize;
> - UINTN PktIdx;
> + UINTN TxSharedReqSize;
> + UINTN PktIdx;
> + EFI_STATUS Status;
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
> + UINT64 BufBaseShift;
(5) Please drop "BufBaseShift".
> + VOID *TxSharedReqBuffer;
>
> Dev->TxMaxPending = (UINT16) MIN (Dev->TxRing.QueueSize / 2,
> VNET_MAX_PENDING);
> @@ -164,24 +169,60 @@ 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)) {
> + return EFI_OUT_OF_RESOURCES;
(6) This is wrong, we would leak "Dev->TxFreeStack" by returning here.
Please jump to an error handling label like "FreeTxFreeStack".
(7) The Status value should be propagated here, not masked as
EFI_OUT_OF_RESOURCES.
The error handling label will handle this anyway, but I'm spelling it
out so that you please update the leading comment block on the function
(= error conditions).
> + }
> +
> + Status = VirtioMapAllBytesInSharedBuffer (
> + Dev->VirtIo,
> + VirtioOperationBusMasterCommonBuffer,
> + TxSharedReqBuffer,
> + sizeof *(Dev->TxSharedReq),
> + &DeviceAddress,
> + &Dev->TxSharedReqMap
> + );
> + if (EFI_ERROR (Status)) {
> + Status = EFI_OUT_OF_RESOURCES;
(8) Please don't mask the returned Status.
> + goto FreeBuffer;
(9) For clarity, I suggest "FreeTxSharedReqBuffer" for the label.
> + }
> +
> + Dev->TxSharedReq = TxSharedReqBuffer;
> +
> + BufBaseShift = (UINT64) (UINTN)Dev->TxSharedReq - (UINT64) DeviceAddress;
> +
> + ZeroMem ((VOID *) Dev->TxSharedReq, sizeof *(Dev->TxSharedReq));
(10) This ZeroMem() is in the wrong spot; it should be done *between*
AllocateSharedPages() and VirtioMapAllBytesInSharedBuffer().
(11) No need for the explicit (VOID*) cast.
(12) I have a question about
*(Dev->TxSharedReq)
Did you write the expression like this because you genuinely find the
parens easier to read, or because you did a search-and-replace, and
wanted to ensure that the operator bindings stay fine?
If you genuinely like it better this way, I can accept that -- I dislike
it :) --, but if you just did a search-and-replace, then please replace
all occurrences with
*Dev->TxSharedReq
(this affects other files modified in the patch as well).
> +
> + //
> // 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);
(13) More of the above, but in this case, I don't feel so lenient; the
following expression:
(Dev->TxSharedReq)->V0_9_5
simply stumps me :) Please replace it with:
Dev->TxSharedReq->V0_9_5
>
> for (PktIdx = 0; PktIdx < Dev->TxMaxPending; ++PktIdx) {
> UINT16 DescIdx;
> + UINT64 Address;
(14) Please drop "Address".
>
> DescIdx = (UINT16) (2 * PktIdx);
> Dev->TxFreeStack[PktIdx] = DescIdx;
>
> + Address = (UINTN) Dev->TxSharedReq;
> + Address += BufBaseShift;
> +
> //
> // 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 = Address;
(15) Simply assign "DeviceAddress".
(BTW, your BufBaseShift calculation should be negated anyway, because
you want to go from host address to device address. You start with the
host address, so your addend, i.e. BusBaseShift, has to *subtract* the
host address, and *add* the device address.
But it's moot anyway, just assign DeviceAddress to Addr.)
> Dev->TxRing.Desc[DescIdx].Len = (UINT32) TxSharedReqSize;
> Dev->TxRing.Desc[DescIdx].Flags = VRING_DESC_F_NEXT;
> Dev->TxRing.Desc[DescIdx].Next = (UINT16) (DescIdx + 1);
> @@ -196,13 +237,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
> @@ -217,6 +258,14 @@ VirtioNetInitTx (
> *Dev->TxRing.Avail.Flags = (UINT16) VRING_AVAIL_F_NO_INTERRUPT;
>
> return EFI_SUCCESS;
> +
> +FreeBuffer:
> + Dev->VirtIo->FreeSharedPages (
> + Dev->VirtIo,
> + EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
> + (VOID *) Dev->TxSharedReq
> + );
(16) The last argument is wrong. If VirtioMapAllBytesInSharedBuffer()
fails and you jump here, you won't have assigned "Dev->TxSharedReq" the
value of "TxSharedReqBuffer" just yet.
So please replace the last argument with "TxSharedReqBuffer".
(17) The explicit (VOID*) cast is unnecessary.
> + return Status;
> }
>
>
> diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> index 4c9d9ece0790..aeb9e6605f0d 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> @@ -54,5 +54,12 @@ 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
> + );
> +
(18) The (VOID*) cast is unneeded.
> FreePool (Dev->TxFreeStack);
> }
>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/5] OvmfPkg/VirtioNetDxe: map virtio-net transmit request buffer
2017-09-01 11:24 [PATCH 0/5] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
` (2 preceding siblings ...)
2017-09-01 11:24 ` [PATCH 3/5] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header Brijesh Singh
@ 2017-09-01 11:24 ` Brijesh Singh
2017-09-05 12:41 ` Laszlo Ersek
2017-09-01 11:24 ` [PATCH 5/5] OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh
2017-09-07 22:55 ` [PATCH 0/5] OvmfPkg/VirtioNetDxe: map host address to device address Laszlo Ersek
5 siblings, 1 reply; 21+ messages in thread
From: Brijesh Singh @ 2017-09-01 11:24 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 transmit buffer for the bus master operations.
The patch uses VirtioMapAllBytesInSharedBuffer() to map transmit buffer
system physical address to the device address.
Since the transmit buffers are returned back to caller in
VirtioNetGetStatus() hence we use OrderCollection library interface to
save the host to device address mapping. After the buffer is succesfully
transmited we do reverse lookup in OrderCollection data structure to get
the host address for the transmitted 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 | 19 +++
OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 30 +++-
OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 157 ++++++++++++++++++++
OvmfPkg/VirtioNetDxe/SnpTransmit.c | 37 ++++-
5 files changed, 232 insertions(+), 12 deletions(-)
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 @@ [LibraryClasses]
DebugLib
DevicePathLib
MemoryAllocationLib
+ OrderedCollectionLib
UefiBootServicesTableLib
UefiDriverEntryPoint
UefiLib
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 3efe95a735d8..d931969af795 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -269,6 +269,25 @@ VirtioNetShutdownTx (
IN OUT VNET_DEV *Dev
);
+EFI_STATUS
+EFIAPI
+VirtioMapTxBuf (
+ IN VNET_DEV *Dev,
+ IN UINT16 Index,
+ IN EFI_PHYSICAL_ADDRESS HostAddress,
+ IN UINTN NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress
+ );
+
+EFI_STATUS
+EFIAPI
+VirtioUnmapTxBuf (
+ IN VNET_DEV *Dev,
+ IN UINT16 Index,
+ OUT EFI_PHYSICAL_ADDRESS *HostAddress,
+ IN EFI_PHYSICAL_ADDRESS DeviceAddress
+ );
+
//
// event callbacks
//
diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
index 694940ea1d97..10ca26de6d7a 100644
--- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
+++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
@@ -5,6 +5,7 @@
Copyright (C) 2013, Red Hat, Inc.
Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
+ 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
@@ -47,7 +48,8 @@
@retval EFI_INVALID_PARAMETER One or more of the parameters has an
unsupported value.
@retval EFI_DEVICE_ERROR The command could not be sent to the network
- interface.
+ interface or failed to map TxBuf to bus master
+ address.
@retval EFI_UNSUPPORTED This function is not supported by the network
interface.
@@ -126,8 +128,10 @@ VirtioNetGetStatus (
*TxBuf = NULL;
}
else {
- UINT16 UsedElemIdx;
- UINT32 DescIdx;
+ UINT16 UsedElemIdx;
+ UINT32 DescIdx;
+ EFI_PHYSICAL_ADDRESS BufferAddress;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
//
// fetch the first descriptor among those that the hypervisor reports
@@ -141,9 +145,27 @@ VirtioNetGetStatus (
ASSERT (DescIdx < (UINT32) (2 * Dev->TxMaxPending - 1));
//
+ // Ring descriptor contains the device address for the caller buffer.
+ // Lets unmap the device address and find its corresponding system
+ // physical address.
+ //
+ DeviceAddress = Dev->TxRing.Desc[DescIdx + 1].Addr;
+ Status = VirtioUnmapTxBuf (
+ Dev,
+ DescIdx + 1,
+ &BufferAddress,
+ DeviceAddress
+ );
+ if (EFI_ERROR (Status)) {
+ Status = EFI_DEVICE_ERROR;
+ goto Exit;
+ }
+
+ //
+ //
// report buffer address to caller that has been enqueued by caller
//
- *TxBuf = (VOID *)(UINTN) Dev->TxRing.Desc[DescIdx + 1].Addr;
+ *TxBuf = (VOID *)(UINTN) BufferAddress;
//
// now this descriptor can be used again to enqueue a transmit buffer
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index aeb9e6605f0d..b91ddd3e4ede 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -14,10 +14,25 @@
**/
+#include <Library/BaseLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/OrderedCollectionLib.h>
#include "VirtioNet.h"
+typedef struct {
+ UINT16 Value;
+} USER_KEY;
+
+typedef struct {
+ USER_KEY Key;
+ EFI_PHYSICAL_ADDRESS HostAddress;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+ VOID *BufMap;
+} USER_STRUCT;
+
+STATIC ORDERED_COLLECTION *Collection;
+
/**
Release RX and TX resources on the boundary of the
EfiSimpleNetworkInitialized state.
@@ -63,3 +78,145 @@ VirtioNetShutdownTx (
FreePool (Dev->TxFreeStack);
}
+
+STATIC
+INTN
+EFIAPI
+KeyCompare (
+ IN CONST VOID *StandaloneKey,
+ IN CONST VOID *UserStruct
+ )
+{
+ CONST USER_KEY *CmpKey;
+ CONST USER_STRUCT *CmpStruct;
+
+ CmpKey = StandaloneKey;
+ CmpStruct = UserStruct;
+
+ return CmpKey->Value < CmpStruct->Key.Value ? -1 :
+ CmpKey->Value > CmpStruct->Key.Value ? 1 :
+ 0;
+}
+
+STATIC
+INTN
+EFIAPI
+UserStructCompare (
+ IN CONST VOID *UserStruct1,
+ IN CONST VOID *UserStruct2
+ )
+{
+ CONST USER_STRUCT *CmpStruct1;
+
+ CmpStruct1 = UserStruct1;
+
+ return KeyCompare (&CmpStruct1->Key, UserStruct2);
+}
+
+EFI_STATUS
+EFIAPI
+VirtioMapTxBuf (
+ IN VNET_DEV *Dev,
+ IN UINT16 Index,
+ IN EFI_PHYSICAL_ADDRESS HostAddress,
+ IN UINTN NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress
+ )
+{
+ EFI_STATUS Status;
+ ORDERED_COLLECTION_ENTRY *Entry;
+ USER_STRUCT *UserStruct;
+ EFI_PHYSICAL_ADDRESS Address;
+ VOID *Mapping;
+
+ //
+ // If ordered collection is not initialized then initialize it.
+ //
+ if (Collection == NULL) {
+ Collection = OrderedCollectionInit (UserStructCompare, KeyCompare);
+ if (Collection == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+ }
+
+ UserStruct = AllocatePool (sizeof (*UserStruct));
+ if (UserStruct == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ Status = VirtioMapAllBytesInSharedBuffer (
+ Dev->VirtIo,
+ VirtioOperationBusMasterRead,
+ (VOID *) (UINTN) HostAddress,
+ NumberOfBytes,
+ &Address,
+ &Mapping
+ );
+ if (EFI_ERROR (Status)) {
+ goto ReleaseUserStruct;
+ }
+
+ UserStruct->Key.Value = Index;
+ UserStruct->HostAddress = HostAddress;
+ UserStruct->DeviceAddress = Address;
+ UserStruct->BufMap = Mapping;
+
+ Status = OrderedCollectionInsert (Collection, &Entry, UserStruct);
+ switch (Status) {
+ case RETURN_OUT_OF_RESOURCES:
+ Status = EFI_OUT_OF_RESOURCES;
+ goto UnmapBuffer;
+ case RETURN_ALREADY_STARTED:
+ Status = EFI_INVALID_PARAMETER;
+ goto UnmapBuffer;
+ default:
+ ASSERT (Status == RETURN_SUCCESS);
+ break;
+ }
+
+ ASSERT (OrderedCollectionUserStruct (Entry) == UserStruct);
+
+ *DeviceAddress = Address;
+ return EFI_SUCCESS;
+
+UnmapBuffer:
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
+
+ReleaseUserStruct:
+ FreePool (UserStruct);
+ return Status;
+}
+
+EFI_STATUS
+EFIAPI
+VirtioUnmapTxBuf (
+ IN VNET_DEV *Dev,
+ IN UINT16 Index,
+ OUT EFI_PHYSICAL_ADDRESS *HostAddress,
+ IN EFI_PHYSICAL_ADDRESS DeviceAddress
+ )
+{
+ USER_KEY StandaloneKey;
+ ORDERED_COLLECTION_ENTRY *Entry;
+ USER_STRUCT *UserStruct;
+ VOID *Ptr;
+ EFI_STATUS Status;
+
+ StandaloneKey.Value = Index;
+ Entry = OrderedCollectionFind (Collection, &StandaloneKey);
+ if (Entry == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ OrderedCollectionDelete (Collection, Entry, &Ptr);
+
+ UserStruct = Ptr;
+ ASSERT (UserStruct->DeviceAddress == DeviceAddress);
+ ASSERT (UserStruct->Key.Value == Index);
+
+ *HostAddress = UserStruct->HostAddress;
+ Status = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, UserStruct->BufMap);
+ FreePool (UserStruct);
+
+ return Status;
+}
diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
index 7ca40d5d0650..1bd6e0d70d7c 100644
--- a/OvmfPkg/VirtioNetDxe/SnpTransmit.c
+++ b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
@@ -55,7 +55,8 @@
@retval EFI_INVALID_PARAMETER One or more of the parameters has an
unsupported value.
@retval EFI_DEVICE_ERROR The command could not be sent to the network
- interface.
+ interface or failed to map the Buffer to
+ bus master address.
@retval EFI_UNSUPPORTED This function is not supported by the network
interface.
@@ -73,11 +74,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 +146,29 @@ VirtioNetTransmit (
}
//
- // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
+ // Get the available 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 = VirtioMapTxBuf (
+ Dev,
+ DescIdx + 1,
+ (EFI_PHYSICAL_ADDRESS) (UINTN) 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.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 4/5] OvmfPkg/VirtioNetDxe: map virtio-net transmit request buffer
2017-09-01 11:24 ` [PATCH 4/5] OvmfPkg/VirtioNetDxe: map virtio-net transmit request buffer Brijesh Singh
@ 2017-09-05 12:41 ` Laszlo Ersek
2017-09-05 12:44 ` Laszlo Ersek
2017-09-06 8:11 ` Laszlo Ersek
0 siblings, 2 replies; 21+ messages in thread
From: Laszlo Ersek @ 2017-09-05 12:41 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 09/01/17 13:24, Brijesh Singh wrote:
> When device is behind the IOMMU, driver is require to pass the device
> address of transmit buffer for the bus master operations.
>
> The patch uses VirtioMapAllBytesInSharedBuffer() to map transmit buffer
> system physical address to the device address.
>
> Since the transmit buffers are returned back to caller in
> VirtioNetGetStatus() hence we use OrderCollection library interface to
> save the host to device address mapping. After the buffer is succesfully
> transmited we do reverse lookup in OrderCollection data structure to get
> the host address for the transmitted 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 | 19 +++
> OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 30 +++-
> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 157 ++++++++++++++++++++
> OvmfPkg/VirtioNetDxe/SnpTransmit.c | 37 ++++-
> 5 files changed, 232 insertions(+), 12 deletions(-)
I have some preliminary comments for this patch. This is by no means a
full review.
(1) Using "SnpSharedHelpers.c" for this kind of functionality is
appropriate, but then please add a comment block between
- the current group of functions (VirtioNetShutdownRx,
VirtioNetShutdownTx, and the upcoming VirtioNetUninitRing),
- and between these other data types and functions.
The current comment block only covers the first group of functions.
(2) Please use more sensible names for the data structures and their
fields. I'm not sure just yet if I'm going to agree with the current
usage, but USER_STRUCT and "Value" are not good names. Please add
documentation to the fields.
(3) The "HostAddress" field and parameter can be / should be (VOID*).
(4) You don't need to define a structure for USER_KEY if it has only one
field.
(5) The ordered collection should be initialized separately, in
VirtioNetInitTx().
(6) Currently OrderedCollectionUninit() is not called anywhere. The
collection should be torn down in VirtioNetShutdownTx().
Beware, tearing down the transmit mappings takes more work than just
calling OrderedCollectionUninit(). You'll have to iterate over the
structure, remove each node, destroy (unmap + free) each shared buffer,
free each user structure, and finally call OrderedCollectionUninit().
Generally it looks like this,
for (Entry = OrderedCollectionMin (Collection); Entry != NULL;
Entry = Entry2) {
Entry2 = OrderedCollectionNext (Entry);
OrderedCollectionDelete (Collection, Entry, &UserStruct);
//
// now unmap and free the shared buffer identified by UserStruct
//
// finally, free UserStruct itself
//
}
OrderedCollectionUninit (Collection);
(7) We'll have to document the new model in "TechNotes.txt".
(8) VirtioOperationBusMasterRead is not appropriate for mappings that
remain in existence after we return from an async protocol member
function, such as SNP.Transmit(). Namely, if ExitBootServices() is
called next, we have to tear down all the mappings (outgoing buffers)
*without* releasing memory, and BusMasterRead is not OK for that. Only
CommonBuffer is.
(9) Which makes me realize that VirtioNetExitBoot() is not updated in
this patch at all. It should basically do the same as (5), except nodes
from the collection should not be deleted, shared buffers should only be
unmapped, not freed, and user structures should not be deleted.
Something like this:
for (Entry = OrderedCollectionMin (Collection);
Entry != NULL;
Entry = OrderedCollectionNext (Entry)) {
UserStruct = OrderedCollectionUserStruct (Entry);
//
// now unmap the shared buffer identified by UserStruct
//
}
(10) This patch is going to be large (even larger than now); can we
split it up? First, update the documentation. Second, just introduce the
data structure and the helper functions. Third, update the current code
to set up the data structure, call the helpers whenever appropriate, and
tear down the data structure.
(11) Regarding points (8) and (9), i.e., unmapping at
ExitBootServices(), have you been following the related discussion in:
[edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common
buffers at ExitBootServices()
?
Let's return to this patch when we reach it with the gradual advancement
that I'm requesting.
Thank you,
Laszlo
> 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 @@ [LibraryClasses]
> DebugLib
> DevicePathLib
> MemoryAllocationLib
> + OrderedCollectionLib
> UefiBootServicesTableLib
> UefiDriverEntryPoint
> UefiLib
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 3efe95a735d8..d931969af795 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -269,6 +269,25 @@ VirtioNetShutdownTx (
> IN OUT VNET_DEV *Dev
> );
>
> +EFI_STATUS
> +EFIAPI
> +VirtioMapTxBuf (
> + IN VNET_DEV *Dev,
> + IN UINT16 Index,
> + IN EFI_PHYSICAL_ADDRESS HostAddress,
> + IN UINTN NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress
> + );
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioUnmapTxBuf (
> + IN VNET_DEV *Dev,
> + IN UINT16 Index,
> + OUT EFI_PHYSICAL_ADDRESS *HostAddress,
> + IN EFI_PHYSICAL_ADDRESS DeviceAddress
> + );
> +
> //
> // event callbacks
> //
> diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> index 694940ea1d97..10ca26de6d7a 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> @@ -5,6 +5,7 @@
>
> Copyright (C) 2013, Red Hat, Inc.
> Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
> + 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
> @@ -47,7 +48,8 @@
> @retval EFI_INVALID_PARAMETER One or more of the parameters has an
> unsupported value.
> @retval EFI_DEVICE_ERROR The command could not be sent to the network
> - interface.
> + interface or failed to map TxBuf to bus master
> + address.
> @retval EFI_UNSUPPORTED This function is not supported by the network
> interface.
>
> @@ -126,8 +128,10 @@ VirtioNetGetStatus (
> *TxBuf = NULL;
> }
> else {
> - UINT16 UsedElemIdx;
> - UINT32 DescIdx;
> + UINT16 UsedElemIdx;
> + UINT32 DescIdx;
> + EFI_PHYSICAL_ADDRESS BufferAddress;
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
>
> //
> // fetch the first descriptor among those that the hypervisor reports
> @@ -141,9 +145,27 @@ VirtioNetGetStatus (
> ASSERT (DescIdx < (UINT32) (2 * Dev->TxMaxPending - 1));
>
> //
> + // Ring descriptor contains the device address for the caller buffer.
> + // Lets unmap the device address and find its corresponding system
> + // physical address.
> + //
> + DeviceAddress = Dev->TxRing.Desc[DescIdx + 1].Addr;
> + Status = VirtioUnmapTxBuf (
> + Dev,
> + DescIdx + 1,
> + &BufferAddress,
> + DeviceAddress
> + );
> + if (EFI_ERROR (Status)) {
> + Status = EFI_DEVICE_ERROR;
> + goto Exit;
> + }
> +
> + //
> + //
> // report buffer address to caller that has been enqueued by caller
> //
> - *TxBuf = (VOID *)(UINTN) Dev->TxRing.Desc[DescIdx + 1].Addr;
> + *TxBuf = (VOID *)(UINTN) BufferAddress;
>
> //
> // now this descriptor can be used again to enqueue a transmit buffer
> diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> index aeb9e6605f0d..b91ddd3e4ede 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> @@ -14,10 +14,25 @@
>
> **/
>
> +#include <Library/BaseLib.h>
> #include <Library/MemoryAllocationLib.h>
> +#include <Library/OrderedCollectionLib.h>
>
> #include "VirtioNet.h"
>
> +typedef struct {
> + UINT16 Value;
> +} USER_KEY;
> +
> +typedef struct {
> + USER_KEY Key;
> + EFI_PHYSICAL_ADDRESS HostAddress;
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
> + VOID *BufMap;
> +} USER_STRUCT;
> +
> +STATIC ORDERED_COLLECTION *Collection;
> +
> /**
> Release RX and TX resources on the boundary of the
> EfiSimpleNetworkInitialized state.
> @@ -63,3 +78,145 @@ VirtioNetShutdownTx (
>
> FreePool (Dev->TxFreeStack);
> }
> +
> +STATIC
> +INTN
> +EFIAPI
> +KeyCompare (
> + IN CONST VOID *StandaloneKey,
> + IN CONST VOID *UserStruct
> + )
> +{
> + CONST USER_KEY *CmpKey;
> + CONST USER_STRUCT *CmpStruct;
> +
> + CmpKey = StandaloneKey;
> + CmpStruct = UserStruct;
> +
> + return CmpKey->Value < CmpStruct->Key.Value ? -1 :
> + CmpKey->Value > CmpStruct->Key.Value ? 1 :
> + 0;
> +}
> +
> +STATIC
> +INTN
> +EFIAPI
> +UserStructCompare (
> + IN CONST VOID *UserStruct1,
> + IN CONST VOID *UserStruct2
> + )
> +{
> + CONST USER_STRUCT *CmpStruct1;
> +
> + CmpStruct1 = UserStruct1;
> +
> + return KeyCompare (&CmpStruct1->Key, UserStruct2);
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioMapTxBuf (
> + IN VNET_DEV *Dev,
> + IN UINT16 Index,
> + IN EFI_PHYSICAL_ADDRESS HostAddress,
> + IN UINTN NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress
> + )
> +{
> + EFI_STATUS Status;
> + ORDERED_COLLECTION_ENTRY *Entry;
> + USER_STRUCT *UserStruct;
> + EFI_PHYSICAL_ADDRESS Address;
> + VOID *Mapping;
> +
> + //
> + // If ordered collection is not initialized then initialize it.
> + //
> + if (Collection == NULL) {
> + Collection = OrderedCollectionInit (UserStructCompare, KeyCompare);
> + if (Collection == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> + }
> +
> + UserStruct = AllocatePool (sizeof (*UserStruct));
> + if (UserStruct == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + Status = VirtioMapAllBytesInSharedBuffer (
> + Dev->VirtIo,
> + VirtioOperationBusMasterRead,
> + (VOID *) (UINTN) HostAddress,
> + NumberOfBytes,
> + &Address,
> + &Mapping
> + );
> + if (EFI_ERROR (Status)) {
> + goto ReleaseUserStruct;
> + }
> +
> + UserStruct->Key.Value = Index;
> + UserStruct->HostAddress = HostAddress;
> + UserStruct->DeviceAddress = Address;
> + UserStruct->BufMap = Mapping;
> +
> + Status = OrderedCollectionInsert (Collection, &Entry, UserStruct);
> + switch (Status) {
> + case RETURN_OUT_OF_RESOURCES:
> + Status = EFI_OUT_OF_RESOURCES;
> + goto UnmapBuffer;
> + case RETURN_ALREADY_STARTED:
> + Status = EFI_INVALID_PARAMETER;
> + goto UnmapBuffer;
> + default:
> + ASSERT (Status == RETURN_SUCCESS);
> + break;
> + }
> +
> + ASSERT (OrderedCollectionUserStruct (Entry) == UserStruct);
> +
> + *DeviceAddress = Address;
> + return EFI_SUCCESS;
> +
> +UnmapBuffer:
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
> +
> +ReleaseUserStruct:
> + FreePool (UserStruct);
> + return Status;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioUnmapTxBuf (
> + IN VNET_DEV *Dev,
> + IN UINT16 Index,
> + OUT EFI_PHYSICAL_ADDRESS *HostAddress,
> + IN EFI_PHYSICAL_ADDRESS DeviceAddress
> + )
> +{
> + USER_KEY StandaloneKey;
> + ORDERED_COLLECTION_ENTRY *Entry;
> + USER_STRUCT *UserStruct;
> + VOID *Ptr;
> + EFI_STATUS Status;
> +
> + StandaloneKey.Value = Index;
> + Entry = OrderedCollectionFind (Collection, &StandaloneKey);
> + if (Entry == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + OrderedCollectionDelete (Collection, Entry, &Ptr);
> +
> + UserStruct = Ptr;
> + ASSERT (UserStruct->DeviceAddress == DeviceAddress);
> + ASSERT (UserStruct->Key.Value == Index);
> +
> + *HostAddress = UserStruct->HostAddress;
> + Status = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, UserStruct->BufMap);
> + FreePool (UserStruct);
> +
> + return Status;
> +}
> diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> index 7ca40d5d0650..1bd6e0d70d7c 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> @@ -55,7 +55,8 @@
> @retval EFI_INVALID_PARAMETER One or more of the parameters has an
> unsupported value.
> @retval EFI_DEVICE_ERROR The command could not be sent to the network
> - interface.
> + interface or failed to map the Buffer to
> + bus master address.
> @retval EFI_UNSUPPORTED This function is not supported by the network
> interface.
>
> @@ -73,11 +74,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 +146,29 @@ VirtioNetTransmit (
> }
>
> //
> - // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
> + // Get the available 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 = VirtioMapTxBuf (
> + Dev,
> + DescIdx + 1,
> + (EFI_PHYSICAL_ADDRESS) (UINTN) 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;
>
> //
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/5] OvmfPkg/VirtioNetDxe: map virtio-net transmit request buffer
2017-09-05 12:41 ` Laszlo Ersek
@ 2017-09-05 12:44 ` Laszlo Ersek
2017-09-06 8:11 ` Laszlo Ersek
1 sibling, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2017-09-05 12:44 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 09/05/17 14:41, Laszlo Ersek wrote:
> On 09/01/17 13:24, Brijesh Singh wrote:
>> When device is behind the IOMMU, driver is require to pass the device
>> address of transmit buffer for the bus master operations.
>>
>> The patch uses VirtioMapAllBytesInSharedBuffer() to map transmit buffer
>> system physical address to the device address.
>>
>> Since the transmit buffers are returned back to caller in
>> VirtioNetGetStatus() hence we use OrderCollection library interface to
>> save the host to device address mapping. After the buffer is succesfully
>> transmited we do reverse lookup in OrderCollection data structure to get
>> the host address for the transmitted 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 | 19 +++
>> OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 30 +++-
>> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 157 ++++++++++++++++++++
>> OvmfPkg/VirtioNetDxe/SnpTransmit.c | 37 ++++-
>> 5 files changed, 232 insertions(+), 12 deletions(-)
>
> I have some preliminary comments for this patch. This is by no means a
> full review.
(12) The helper functions should be called VirtioNet*, not Virtio*.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] OvmfPkg/VirtioNetDxe: map virtio-net transmit request buffer
2017-09-05 12:41 ` Laszlo Ersek
2017-09-05 12:44 ` Laszlo Ersek
@ 2017-09-06 8:11 ` Laszlo Ersek
1 sibling, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2017-09-06 8:11 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Ard Biesheuvel, Tom Lendacky
On 09/05/17 14:41, Laszlo Ersek wrote:
> On 09/01/17 13:24, Brijesh Singh wrote:
>> When device is behind the IOMMU, driver is require to pass the device
>> address of transmit buffer for the bus master operations.
>>
>> The patch uses VirtioMapAllBytesInSharedBuffer() to map transmit buffer
>> system physical address to the device address.
>>
>> Since the transmit buffers are returned back to caller in
>> VirtioNetGetStatus() hence we use OrderCollection library interface to
>> save the host to device address mapping. After the buffer is succesfully
>> transmited we do reverse lookup in OrderCollection data structure to get
>> the host address for the transmitted 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 | 19 +++
>> OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 30 +++-
>> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 157 ++++++++++++++++++++
>> OvmfPkg/VirtioNetDxe/SnpTransmit.c | 37 ++++-
>> 5 files changed, 232 insertions(+), 12 deletions(-)
> (7) We'll have to document the new model in "TechNotes.txt".
I've now re-read the final section of the text file, section
Virtio internals -- Tx
I propose the following updates.
(Here I'm providing a diff, not a desired "end status", like I did for
the diagram earlier. A diff is harder to interpret for a diagram, but
easy for plain text.)
Please verify that the proposed documentation updates match the logic
that you've implemented:
> diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> index 9c1dfe6a773e..7a7b3071abba 100644
> --- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
> +++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> @@ -310,10 +310,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:
>
> @@ -336,9 +340,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
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/5] OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM
2017-09-01 11:24 [PATCH 0/5] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
` (3 preceding siblings ...)
2017-09-01 11:24 ` [PATCH 4/5] OvmfPkg/VirtioNetDxe: map virtio-net transmit request buffer Brijesh Singh
@ 2017-09-01 11:24 ` Brijesh Singh
2017-09-06 7:33 ` Laszlo Ersek
2017-09-07 22:55 ` [PATCH 0/5] OvmfPkg/VirtioNetDxe: map host address to device address Laszlo Ersek
5 siblings, 1 reply; 21+ messages in thread
From: Brijesh Singh @ 2017-09-01 11:24 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>
---
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 551820e30f36..dded19b25dd2 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -550,7 +550,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
@@ -590,7 +591,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.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 5/5] OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM
2017-09-01 11:24 ` [PATCH 5/5] OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh
@ 2017-09-06 7:33 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2017-09-06 7:33 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 09/01/17 13:24, Brijesh Singh wrote:
> 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>
> ---
> 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 551820e30f36..dded19b25dd2 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -550,7 +550,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
> @@ -590,7 +591,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;
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/5] OvmfPkg/VirtioNetDxe: map host address to device address
2017-09-01 11:24 [PATCH 0/5] OvmfPkg/VirtioNetDxe: map host address to device address Brijesh Singh
` (4 preceding siblings ...)
2017-09-01 11:24 ` [PATCH 5/5] OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh
@ 2017-09-07 22:55 ` Laszlo Ersek
5 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2017-09-07 22:55 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
Hi Brijesh,
On 09/01/17 13:24, Brijesh Singh wrote:
> 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).
>
> 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>
>
> Repo: https://github.com/codomania/edk2
> Branch: virtio-net-1
>
> Brijesh Singh (5):
> OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap()
> OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages()
> OvmfPkg/VirtioNetDxe: dynamically alloc transmit header
> OvmfPkg/VirtioNetDxe: map virtio-net transmit request buffer
> OvmfPkg/VirtioNetDxe: negotiate VIRTIO_F_IOMMU_PLATFORM
>
> OvmfPkg/VirtioNetDxe/VirtioNet.inf | 1 +
> OvmfPkg/VirtioNetDxe/VirtioNet.h | 27 ++-
> OvmfPkg/VirtioNetDxe/Events.c | 19 ++
> OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 30 +++-
> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 185 ++++++++++++++++----
> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 171 +++++++++++++++++-
> OvmfPkg/VirtioNetDxe/SnpShutdown.c | 2 +
> OvmfPkg/VirtioNetDxe/SnpTransmit.c | 37 +++-
> 8 files changed, 427 insertions(+), 45 deletions(-)
>
just adding a pointer so that my comment is linked under this thread as well:
[edk2] [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at
ExitBootServices
https://lists.01.org/pipermail/edk2-devel/2017-September/014304.html
On 09/08/17 00:41, Laszlo Ersek wrote:
> (The conversion of VirtioNetDxe to device addresses is still in
> progress -- Brijesh, when you submit v2 of that, under this approach,
> there is no need to change VirtioNetExitBoot() relative to current
> upstream, and you can use VirtioOperationBusMasterRead to map outgoing
> packets.)
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread