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() ->
> EFI_PCI_IO_PROTOCOL.AllocateBuffer()
>
> - VIRTIO_DEVICE_PROTOCOL.FreeSharedPages() ->
> EFI_PCI_IO_PROTOCOL.FreeBuffer()
>
> - VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() ->
> EFI_PCI_IO_PROTOCOL.Map()
>
> - VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer() ->
> EFI_PCI_IO_PROTOCOL.Unmap()
>
> 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/Virtio10Dxe/Virtio10.c | 121 +++++++++++++++++++-
> 1 file changed, 119 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.c b/OvmfPkg/Virtio10Dxe/Virtio10.c
> index 43dcfd7cb2a4..413ffa06cf35 100644
> --- a/OvmfPkg/Virtio10Dxe/Virtio10.c
> +++ b/OvmfPkg/Virtio10Dxe/Virtio10.c
> @@ -2,6 +2,7 @@
> A non-transitional driver for VirtIo 1.0 PCI devices.
>
> Copyright (C) 2016, Red Hat, Inc.
> + 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
> @@ -15,6 +16,7 @@
> #include <IndustryStandard/Pci.h>
> #include <IndustryStandard/Virtio.h>
> #include <Protocol/PciIo.h>
> +#include <Protocol/PciRootBridgeIo.h>
> #include <Protocol/VirtioDevice.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> @@ -772,6 +774,117 @@ Virtio10ReadDevice (
> return Status;
> }
>
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Virtio10AllocateSharedPages (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN Pages,
> + IN OUT VOID **HostAddress
> + )
> +{
> + VIRTIO_1_0_DEV *Dev;
> + EFI_STATUS Status;
> +
> + Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
> +
> + Status = Dev->PciIo->AllocateBuffer (
> + Dev->PciIo,
> + AllocateAnyPages,
> + EfiBootServicesData,
> + Pages,
> + HostAddress,
> + EFI_PCI_ATTRIBUTE_MEMORY_CACHED
> + );
> + return Status;
> +}
> +
> +STATIC
> +VOID
> +EFIAPI
> +Virtio10FreeSharedPages (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN Pages,
> + IN VOID *HostAddress
> + )
> +{
> + VIRTIO_1_0_DEV *Dev;
> +
> + Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
> +
> + Dev->PciIo->FreeBuffer (
> + Dev->PciIo,
> + Pages,
> + HostAddress
> + );
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Virtio10MapSharedBuffer (
> + 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 Status;
> + VIRTIO_1_0_DEV *Dev;
> + EFI_PCI_IO_PROTOCOL_OPERATION PciIoOperation;
> +
> + Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
> +
> + //
> + // Map VIRTIO_MAP_OPERATION to EFI_PCI_IO_PROTOCOL_OPERATION
> + //
> + switch (Operation) {
> + case VirtioOperationBusMasterRead:
> + PciIoOperation = EfiPciIoOperationBusMasterRead;
> + break;
> + case VirtioOperationBusMasterWrite:
> + PciIoOperation = EfiPciIoOperationBusMasterWrite;
> + break;
> + case VirtioOperationBusMasterCommonBuffer:
> + PciIoOperation = EfiPciIoOperationBusMasterCommonBuffer;
> + break;
> + default:
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + Status = Dev->PciIo->Map (
> + Dev->PciIo,
> + PciIoOperation,
> + HostAddress,
> + NumberOfBytes,
> + DeviceAddress,
> + Mapping
> + );
> + return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Virtio10UnmapSharedBuffer (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN VOID *Mapping
> + )
> +{
> + EFI_STATUS Status;
> + VIRTIO_1_0_DEV *Dev;
> +
> + Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
> +
> + Status = Dev->PciIo->Unmap (
> + Dev->PciIo,
> + Mapping
> + );
> +
> + return Status;
> +}
>
> STATIC CONST VIRTIO_DEVICE_PROTOCOL mVirtIoTemplate = {
> VIRTIO_SPEC_REVISION (1, 0, 0),
> @@ -788,7 +901,11 @@ STATIC CONST VIRTIO_DEVICE_PROTOCOL mVirtIoTemplate = {
> Virtio10GetDeviceStatus,
> Virtio10SetDeviceStatus,
> Virtio10WriteDevice,
> - Virtio10ReadDevice
> + Virtio10ReadDevice,
> + Virtio10AllocateSharedPages,
> + Virtio10FreeSharedPages,
> + Virtio10MapSharedBuffer,
> + Virtio10UnmapSharedBuffer
> };
>
>
> @@ -906,7 +1023,7 @@ Virtio10BindingStart (
> goto ClosePciIo;
> }
>
> - SetAttributes = EFI_PCI_IO_ATTRIBUTE_BUS_MASTER;
> + SetAttributes = EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE;
> UpdateAttributes (&Device->CommonConfig, &SetAttributes);
> UpdateAttributes (&Device->NotifyConfig, &SetAttributes);
> UpdateAttributes (&Device->SpecificConfig, &SetAttributes);
>
(1) This patch is almost complete in my opinion, except the last hunk is
incorrect -- I think it may be due to an incorrect conflict resolution
during rebase.
Namely, we shouldn't assign "SetAttributes" the value
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE
but the value
(EFI_PCI_IO_ATTRIBUTE_BUS_MASTER |
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE)
IOW, adding EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE is good, but
EFI_PCI_IO_ATTRIBUTE_BUS_MASTER should be preserved too.
Please refer to the following sentence in my v1 review
<http://mid.mail-archive.com/659fe2f7-0d17-cde7-c3d3-36e541ff7805@redhat.com>,
point (5):
"(And then do point (4) on top of that, separately.)"
Thank you!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel