From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D14CE21EA35D7 for ; Tue, 5 Sep 2017 04:45:06 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4623FC04D304; Tue, 5 Sep 2017 11:47:55 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4623FC04D304 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-185.rdu2.redhat.com [10.10.120.185]) by smtp.corp.redhat.com (Postfix) with ESMTP id B40831821D; Tue, 5 Sep 2017 11:47:53 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1504265045-19008-1-git-send-email-brijesh.singh@amd.com> <1504265045-19008-2-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <04fefb16-23d5-f6df-0657-9d4c5e96ac57@redhat.com> Date: Tue, 5 Sep 2017 13:47:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <1504265045-19008-2-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 05 Sep 2017 11:47:55 +0000 (UTC) Subject: Re: [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Sep 2017 11:45:07 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Jordan Justen > Cc: Tom Lendacky > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh > --- > 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