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 496F921AEB0BE for ; Wed, 9 Aug 2017 10:06:52 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 414492A9CD7; Wed, 9 Aug 2017 17:09:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 414492A9CD7 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-102.phx2.redhat.com [10.3.116.102]) by smtp.corp.redhat.com (Postfix) with ESMTP id D1B535C468; Wed, 9 Aug 2017 17:09:08 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1502107139-412-1-git-send-email-brijesh.singh@amd.com> <1502107139-412-4-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <3a8ede72-03f0-5627-f680-72f41da588e9@redhat.com> Date: Wed, 9 Aug 2017 19:09:07 +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: <1502107139-412-4-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 09 Aug 2017 17:09:10 +0000 (UTC) Subject: Re: [PATCH v1 03/14] OvmfPkg/VirtioPciDeviceDxe: Implement new member functions 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, 09 Aug 2017 17:06:52 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/07/17 13:58, Brijesh Singh wrote: > The patch implements the newly added member functions by respectively > delegating the job to: > > - MemoryApplicationLib.AllocatePages () -- with BootServicesData > - MemoryApplicationLib.FreePages () > - no-op (host address is same as device DMA address) > - no-op (1) (2) Please see points (1) and (2) in my "PATCH v1 02/14" feedback. (3) s/Application/Allocation/ (4) the "with BootServicesData" remark is not needed after the MemoryAllocationLib.AllocatePages() reference. MemoryAllocationLib encodes the memory type in the API names. So you have AllocatePool / AllocatePages, AllocateReservedPool / AllocateReservedPages, AllocateRuntimePool / AllocateRuntimePages, etc. Please see the lib class header MdePkg/Include/Library/MemoryAllocationLib.h > > Suggested-by: Laszlo Ersek > 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/VirtioPciDeviceDxe/VirtioPciDevice.h | 34 ++++++++++ > OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c | 7 ++- > OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c | 66 ++++++++++++++++++++ > 3 files changed, 106 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h > index 8f17a16c88f5..da98de123000 100644 > --- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h > +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h > @@ -3,6 +3,7 @@ > Internal definitions for the VirtIo PCI Device driver > > Copyright (C) 2013, ARM Ltd > + Copyright (c) 2017, AMD Inc, All rights reserved.
> > This program and the accompanying materials are licensed and made available > under the terms and conditions of the BSD License which accompanies this > @@ -156,4 +157,37 @@ VirtioPciSetDeviceStatus ( > UINT8 DeviceStatus > ); > > +EFI_STATUS > +EFIAPI > +VirtioPciAllocateSharedPages ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINTN NumPages, > + VOID **HostAddress > + ); > + > +VOID > +EFIAPI > +VirtioPciFreeSharedPages ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINTN NumPages, > + VOID *HostAddress > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioPciMapSharedBuffer ( > + VIRTIO_DEVICE_PROTOCOL *This, > + VIRTIO_MAP_OPERATION Operation, > + VOID *HostAddress, > + UINTN *NumberOfBytes, > + EFI_PHYSICAL_ADDRESS *DeviceAddress, > + VOID **Mapping > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioPciUnmapSharedBuffer ( > + VIRTIO_DEVICE_PROTOCOL *This, > + VOID *Mapping > + ); > #endif // _VIRTIO_PCI_DEVICE_DXE_H_ > diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c > index bc4f6fe8bfa3..4e4e21d9a477 100644 > --- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c > +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c > @@ -5,6 +5,7 @@ > Copyright (C) 2012, Red Hat, Inc. > Copyright (c) 2012 - 2016, Intel Corporation. All rights reserved.
> Copyright (C) 2013, ARM Ltd. > + Copyright (C) 2017, AMD Inc, All rights reserved.
> > This program and the accompanying materials are licensed and made available > under the terms and conditions of the BSD License which accompanies this > @@ -40,7 +41,11 @@ STATIC VIRTIO_DEVICE_PROTOCOL mDeviceProtocolTemplate = { > VirtioPciGetDeviceStatus, // GetDeviceStatus > VirtioPciSetDeviceStatus, // SetDeviceStatus > VirtioPciDeviceWrite, // WriteDevice > - VirtioPciDeviceRead // ReadDevice > + VirtioPciDeviceRead, // ReadDevice > + VirtioPciAllocateSharedPages, // AllocateSharedPages > + VirtioPciFreeSharedPages, // FreeSharedPages > + VirtioPciMapSharedBuffer, // MapSharedBuffer > + VirtioPciUnmapSharedBuffer, // UnmapSharedBuffer > }; > > /** > diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c > index 243aa14c2421..1c587e184311 100644 > --- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c > +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c > @@ -5,6 +5,7 @@ > Copyright (C) 2012, Red Hat, Inc. > Copyright (c) 2012, Intel Corporation. All rights reserved.
> Copyright (C) 2013, ARM Ltd. > + Copyright (C) 2017, AMD Inc, All rights reserved.
> > This program and the accompanying materials are licensed and made available > under the terms and conditions of the BSD License which accompanies this > @@ -271,3 +272,68 @@ VirtioPciSetDeviceStatus ( > return VirtioPciIoWrite (Dev, VIRTIO_PCI_OFFSET_QUEUE_DEVICE_STATUS, > sizeof (UINT8), DeviceStatus); > } > + > +EFI_STATUS > +EFIAPI > +VirtioPciAllocateSharedPages ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINTN NumPages, > + VOID **HostAddress > + ) > +{ > + EFI_STATUS Status; > + EFI_PHYSICAL_ADDRESS PhysicalAddress; > + > + Status = gBS->AllocatePages ( > + AllocateAnyPages, > + EfiBootServicesData, > + NumPages, > + &PhysicalAddress > + ); > + if (!EFI_ERROR (Status)) { > + *HostAddress = (VOID *) (UINTN) PhysicalAddress; > + } > + > + return Status; > +} (5) We might want do what we promise in the commit message :) Namely, use MemoryAllocationLib APIs (for simplicity), rather than boot services. Such as: VOID *Buffer; Buffer = AllocatePages (NumPages); if (Buffer == NULL) { return EFI_OUT_OF_RESOURCES; } *HostAddress = Buffer; return EFI_SUCCESS; > + > +VOID > +EFIAPI > +VirtioPciFreeSharedPages ( > + VIRTIO_DEVICE_PROTOCOL *This, > + UINTN NumPages, > + VOID *HostAddress > + ) > +{ > + gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN)HostAddress, NumPages); > +} (6) Similarly: FreePages (HostAddress, NumPages); > + > +EFI_STATUS > +EFIAPI > +VirtioPciMapSharedBuffer ( > + VIRTIO_DEVICE_PROTOCOL *This, > + VIRTIO_MAP_OPERATION Operation, > + VOID *HostAddress, > + UINTN *NumberOfBytes, > + EFI_PHYSICAL_ADDRESS *DeviceAddress, > + VOID **Mapping > + ) > +{ > + EFI_PHYSICAL_ADDRESS PhysicalAddress; > + > + PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress; > + > + *DeviceAddress = PhysicalAddress; > + > + return EFI_SUCCESS; > +} (7) We could condense it to a single assignment to *DeviceAddress. Up to you. (8) Please set *Mapping to NULL -- let's not cause the future caller of Unmap() to evaluate (for argument passing) a pointer with garbage contents at that point; that's undefined behavior in itself, at least in theory. > + > +EFI_STATUS > +EFIAPI > +VirtioPciUnmapSharedBuffer ( > + VIRTIO_DEVICE_PROTOCOL *This, > + VOID *Mapping > + ) > +{ > + return EFI_SUCCESS; > +} > (9) Please refresh the function signatures in both "VirtioPciDevice.h" and "VirtioPciFunctions.c", from the protocol definition in "OvmfPkg/Include/Protocol/VirtioDevice.h". In particular, all the IN and OUT decoration is missing here. To be continued. Thanks, Laszlo