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 <ler...@redhat.com> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Cc: Tom Lendacky <thomas.lenda...@amd.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh <brijesh.si...@amd.com> > --- > 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.<BR> > > 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.<BR> > Copyright (C) 2013, ARM Ltd. > + 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 > @@ -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.<BR> > Copyright (C) 2013, ARM Ltd. > + 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 > @@ -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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel