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 8E2C821D0A27A for ; Wed, 16 Aug 2017 13:54:15 -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 5E347C04D313; Wed, 16 Aug 2017 20:56:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5E347C04D313 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-116-59.phx2.redhat.com [10.3.116.59]) by smtp.corp.redhat.com (Postfix) with ESMTP id B4C895D973; Wed, 16 Aug 2017 20:56:39 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1502710605-8058-1-git-send-email-brijesh.singh@amd.com> <1502710605-8058-13-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <72c6cdca-8364-bb2d-227d-30278d9d086a@redhat.com> Date: Wed, 16 Aug 2017 22:56:38 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1502710605-8058-13-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]); Wed, 16 Aug 2017 20:56:41 +0000 (UTC) Subject: Re: [PATCH v2 12/23] OvmfPkg/VirtioLib: add functions to map/unmap VRING 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: Wed, 16 Aug 2017 20:54:15 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/14/17 13:36, Brijesh Singh wrote: > Add functions to map and unmap the ring buffer with BusMasterCommonBuffer > so that ring can be accessed by both guest and hypervisor. > > 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/Include/Library/VirtioLib.h | 25 +++++++++++ > OvmfPkg/Library/VirtioLib/VirtioLib.c | 44 ++++++++++++++++++++ > 2 files changed, 69 insertions(+) (1) The subject and the commit message both should say "a function" (singular) now -- we've only kept VirtioRingMap(). Unmap shoud not be referenced in either the subject or the commit message. > > diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h > index ca0b217a04eb..1efb02a68cf1 100644 > --- a/OvmfPkg/Include/Library/VirtioLib.h > +++ b/OvmfPkg/Include/Library/VirtioLib.h > @@ -62,6 +62,31 @@ VirtioRingInit ( > > /** > > + Map the ring buffer so that it can be accessed equally by both guest > + and hypervisor. > + > + @param[in] VirtIo The virtio device instance. > + > + @param[in] Ring The virtio ring to map. > + > + @param[out] RingBaseShift The fact that you either forgot, or had difficulties with, the explanation of the RingBaseShift output parameter, is not random. It has a deeper cause. The cause is that placing this patch at this exact position in the series constitutes a layering violation. In my previous review , point (10), I tried to clarify this ("*Then* please include the present patch", emphasis added now), but I must have been unclear. So, the idea is that - the VirtioLib helper functions (code, comments, and commit messages) are allowed to reference VIRTIO_DEVICE_PROTOCOL specifics, - but references in the opposite direction should be avoided if possible. You are introducing this patch at position 12 in the series, and in the next patch, for the virtio protocol, you refer to VirtioRingMap() in the commit message. This should not be done -- the opposite dependency should be constructed. (2) So please, move this patch two positions toward the tail of the series: OvmfPkg/VirtioLib: add functions to map/unmap VRING -----------+ OvmfPkg/Virtio: take RingBaseShift in VirtioSetQueueAddress() | OvmfPkg/Virtio10Dxe: add the RingBaseShift offset | <-------------------------------------------------------+ OvmfPkg/VirtioLib: alloc vring buffer with AllocateSharedPages() (3) Then you can document the RingBaseShift parameter simply like this, in this patch: @param[out] RingBaseShift A resulting translation offset, to be passed to VirtIo->SetQueueAddress(). > + > + @param[out] Mapping A resulting token to pass to > + VirtIo->UnmapSharedBuffer(). > + > + @return Status from VirtIo->MapSharedBuffer() (4) This is an improvement on the previous version of the patch, but the .c and .h files differ ("Status code" vs "Status"). So please sync the full comment blocks (after addressing (3) as well). > +**/ > +EFI_STATUS > +EFIAPI > +VirtioRingMap ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VRING *Ring, > + OUT UINT64 *RingBaseShift, > + OUT VOID **Mapping > + ); > + > +/** > + > Tear down the internal resources of a configured virtio ring. > > The caller is responsible to stop the host from using this ring before > diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c > index 5b64d18a8d6f..7d07dcc09d3d 100644 > --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c > +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c > @@ -498,3 +498,47 @@ Failed: > *Mapping = NULL; > return EFI_OUT_OF_RESOURCES; > } > + > +/** > + > + Map the ring buffer so that it can be accessed equally by both guest > + and hypervisor. > + > + @param[in] VirtIo The virtio device instance. > + > + @param[in] Ring The virtio ring to map. > + > + @param[out] RingBaseShift RingBaseShift > + > + @param[out] Mapping A resulting token to pass to > + VirtIo->UnmapSharedBuffer(). > + > + @return Status code from VirtIo->MapSharedBuffer() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioRingMap ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VRING *Ring, > + OUT UINT64 *RingBaseShift, > + OUT VOID **Mapping > + ) > +{ > + EFI_STATUS Status; > + PHYSICAL_ADDRESS DeviceAddress; (5) Technically this is correct (both EFI_PHYSICAL_ADDRESS and PHYSICAL_ADDRESS are typedefs for UINT64). For consistency with the prototype of VirtioMapAllBytesInSharedBuffer(), EFI_PHYSICAL_ADDRESS would be slightly better in my opinion. > + > + Status = VirtioMapAllBytesInSharedBuffer ( > + VirtIo, > + VirtioOperationBusMasterCommonBuffer, > + Ring->Base, > + EFI_PAGES_TO_SIZE (Ring->NumPages), > + &DeviceAddress, > + Mapping This is fine -- we're passing "Mapping" directly to VirtioMapAllBytesInSharedBuffer(), but we know that said function won't modify Mapping if it fails. (Just please address point (8) in my "PATCH v2 10/23" feedback -- i.e., VirtioMapAllBytesInSharedBuffer() should not null "Mapping" on error.) > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + *RingBaseShift = DeviceAddress - (UINT64)(UINTN)Ring->Base; > + return EFI_SUCCESS; > +} > Looks good! Thank you! Laszlo