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 5C1A021CFA5F7 for ; Wed, 16 Aug 2017 08:56:25 -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 13B04C05679B; Wed, 16 Aug 2017 15:58:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 13B04C05679B Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.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 A8C005D96E; Wed, 16 Aug 2017 15:58:49 +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-10-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Wed, 16 Aug 2017 17:58:48 +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-10-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.32]); Wed, 16 Aug 2017 15:58:51 +0000 (UTC) Subject: Re: [PATCH v2 09/23] OvmfPkg/VirtioMmioDeviceLib: implement IOMMU-like 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, 16 Aug 2017 15:56:25 -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: > The patch implements the newly added IOMMU-like member functions by > respectively delegating the job to: > > - VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages () -> > MemoryAllocationLib.AllocatePages() > > - VIRTIO_DEVICE_PROTOCOL.FreeSharedPages () -> > MemoryAllocationLib.FreePages () > > - VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer () -> no-op > > - VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer () -> no-op > > 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/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h | 34 +++++++++++ > OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c | 59 ++++++++++++++++++++ > 2 files changed, 93 insertions(+) > > diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h > index bedd635e1a86..b69f6d7b7a85 100644 > --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h > +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h > @@ -137,4 +137,38 @@ VirtioMmioSetGuestFeatures ( > IN UINT64 Features > ); > > +EFI_STATUS > +EFIAPI > +VirtioMmioAllocateSharedPages ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN NumPages, > + OUT VOID **HostAddress > + ); > + > +VOID > +EFIAPI > +VirtioMmioFreeSharedPages ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN NumPages, > + IN VOID *HostAddress > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioMmioMapSharedBuffer ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN VIRTIO_MAP_OPERATION Operation, > + IN VOID *HostAddress, > + IN OUT UINTN *NumberOfBytes, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > + OUT VOID **Mapping > + ); > + > +EFI_STATUS > +EFIAPI > +VirtioMmioUnmapSharedBuffer ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN VOID *Mapping > + ); > + > #endif // _VIRTIO_MMIO_DEVICE_INTERNAL_H_ > diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c > index 9142d4a162c0..f3f69f324c6c 100644 > --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c > +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c > @@ -16,6 +16,8 @@ > > **/ > > +#include > + > #include "VirtioMmioDevice.h" > (1) The "VirtioMmioDevice.h" header includes the header files that are used by both "VirtioMmioDevice.c" and "VirtioMmioDeviceFunctions.c". Before your patch, "MemoryAllocationLib.h" was needed only by the former C file. After your patch, "MemoryAllocationLib.h" is needed by both C files. Therefore, rather than adding this #include directive here, please move the same #include directive from "VirtioMmioDevice.c" to "VirtioMmioDevice.h". > EFI_STATUS > @@ -293,3 +295,60 @@ VirtioMmioDeviceRead ( > > return EFI_SUCCESS; > } > + > +EFI_STATUS > +EFIAPI > +VirtioMmioAllocateSharedPages ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN NumPages, > + OUT VOID **HostAddress > + ) > +{ > + VOID *Buffer; > + > + Buffer = AllocatePages (NumPages); > + if (Buffer == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + *HostAddress = Buffer; > + return EFI_SUCCESS; > +} > + > +VOID > +EFIAPI > +VirtioMmioFreeSharedPages ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN NumPages, > + IN VOID *HostAddress > + ) > +{ > + FreePages (HostAddress, NumPages); > +} > + > +EFI_STATUS > +EFIAPI > +VirtioMmioMapSharedBuffer ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN VIRTIO_MAP_OPERATION Operation, > + IN VOID *HostAddress, > + IN OUT UINTN *NumberOfBytes, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > + OUT VOID **Mapping > + ) > +{ > + *DeviceAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress; > + *Mapping = NULL; > + > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +EFIAPI > +VirtioMmioUnmapSharedBuffer ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + OUT VOID *Mapping (2) "Mapping" should be "IN". (The function declaration is correct, only the definition has this wart.) > + ) > +{ > + return EFI_SUCCESS; > +} > (3) You forgot to populate the IOMMU-like member functions in the "mMmioDeviceProtocolTemplate" structure, in "OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c". (I realize this wasn't caught on your side due to difficulties with aarch64 testing.) (4) You might want to add AMD copyright notices to these files as well. (Not sure what your company policy requires -- nowadays the git history should be totally enough for preexistent files, but some companies require their associates to add explicit (C) notices to every file they touch.) Thanks! Laszlo