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 <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> 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 <Library/MemoryAllocationLib.h>
> +
> #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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel