On 10/16/13 19:29, Olivier Martin wrote:
> This change implements the VIRTIO_DEVICE_PROTOCOL for the PCI transport
> layer.
> The VirtIo device drivers will interact with the PCI-based VirtIo devices
> through this protocol implementation.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Olivier Martin <olivier.mar...@arm.com>
> ---
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c | 667
> +++++++++++++++++++++
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h | 166 +++++
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf | 43 ++
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c | 285 +++++++++
> 4 files changed, 1161 insertions(+), 0 deletions(-)
> create mode 100644 OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
> create mode 100644 OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
> create mode 100644 OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
> create mode 100644 OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
Again, I compared this patch against the corresponding v3 one.
> +/**
> +
> + Read a word from Region 0 of the device specified by PciIo.
> +
> + Region 0 must be an iomem region. This is an internal function for the PCI
> + implementation of the protocol.
> +
> + @param[in] Dev Virtio PCI device.
> +
> + @param[in] FieldOffset Source offset.
> +
> + @param[in] FieldSize Source field size, must be in { 1, 2, 4, 8 }.
> +
> + @param[in] BufferSize Number of bytes available in the target buffer.
> Must
> + equal FieldSize.
> +
> + @param[out] Buffer Target buffer.
> +
> +
> + @return Status code returned by PciIo->Io.Read().
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioPciIoRead (
> + IN VIRTIO_PCI_DEVICE *Dev,
> + IN UINTN FieldOffset,
> + IN UINTN FieldSize,
> + IN UINTN BufferSize,
> + OUT VOID *Buffer
> + )
> +{
> + UINTN Count;
> + EFI_PCI_IO_PROTOCOL_WIDTH Width;
> + EFI_PCI_IO_PROTOCOL *PciIo;
> +
> + // The BufferSize must be a multiple of FieldSize
> + ASSERT ((BufferSize % FieldSize) == 0);
Not sure what motivated this change (and the corresponding Count
doubling below). It doesn't match the comment on BufferSize.
But, I don't mind.
> +
> + PciIo = Dev->PciIo;
> + Count = BufferSize / FieldSize;
> +
> + switch (FieldSize) {
> + case 1:
> + Width = EfiPciIoWidthUint8;
> + break;
> +
> + case 2:
> + Width = EfiPciIoWidthUint16;
> + break;
> +
> + case 8:
> + // The 64bit PCI I/O is broken down into two 32bit reads to prevent
> + // any alignment or width issues.
> + // The UEFI spec says under EFI_PCI_IO_PROTOCOL.Io.Write():
> + //
> + // The I/O operations are carried out exactly as requested. The caller
> + // is responsible for any alignment and I/O width issues which the
> + // bus, device, platform, or type of I/O might require. For example on
> + // some platforms, width requests of EfiPciIoWidthUint64 do not work
> + Count = Count * 2;
> + // fall through
> +
> + case 4:
> + Width = EfiPciIoWidthUint32;
> + break;
> +
> + default:
> + ASSERT (FALSE);
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + return PciIo->Io.Read (
> + PciIo,
> + Width,
> + PCI_BAR_IDX0,
> + FieldOffset,
> + Count,
> + Buffer
> + );
> +}
Then:
> diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
> b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
> new file mode 100644
> index 0000000..4b5d1a4
> --- /dev/null
> +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
> @@ -0,0 +1,43 @@
> +## @file
> +# This driver produces the VirtIo Device Protocol instances for VirtIo PCI
> +# Device
> +#
> +# Copyright (C) 2013, ARM Ltd
> +#
> +# This program and the accompanying materials are licensed and made available
> +# under the terms and conditions of the BSD License which accompanies this
> +# distribution. The full text of the license may be found at
> +# http://opensource.org/licenses/bsd-license.php
> +#
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> WITHOUT
> +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010006
What was the reason for incrementing this?
I looked up INF_VERSION in the INF spec now -- it says "Tools use this
value to handle parsing of previous releases of the specification if
there are incompatible changes."
The INF spec release that I have locally uses 0x00010016. So the above
0x00010006 value is unclear to me, but it probably shouldn't hurt. I can
also see that it's a valid INF_VERSION value, a bunch of other INF files
use it in the tree. OK.
> +/**
> +
> + Read a word from Region 0 of the device specified by PciIo.
> +
> + The function implements the ReadDevice protocol member of
> + VIRTIO_DEVICE_PROTOCOL.
> +
> + @param[in] PciIo Source PCI device.
I missed this stale comment block the last time around, and caught only
the write counterpart below it... No problem.
> +
> + @param[in] FieldOffset Source offset.
> +
> + @param[in] FieldSize Source field size, must be in { 1, 2, 4, 8 }.
> +
> + @param[in] BufferSize Number of bytes available in the target buffer.
> Must
> + equal FieldSize.
> +
> + @param[out] Buffer Target buffer.
> +
> +
> + @return Status code returned by PciIo->Io.Read().
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioPciDeviceRead (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN FieldOffset,
> + IN UINTN FieldSize,
> + IN UINTN BufferSize,
> + OUT VOID *Buffer
> + )
> +{
[snip]
> +EFI_STATUS
> +EFIAPI
> +VirtioPciSetPageSize (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINT32 PageSize
> + )
> +{
> + ASSERT (PageSize == EFI_PAGE_SIZE);
> +
> + return EFI_SUCCESS;
> +}
In
<http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4373/focus=4409> I
suggested
backend take page size return EFI_UNSUPPORTED configure
parameter if the value is not host side
EFI_PAGE_SIZE
----------- -------------- ---------------------- ---------
virtio-pci yes yes no
virtio-mmio yes yes yes
but I don't mind the ASSERT() in place of EFI_UNSUPPORTED.
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks,
Laszlo
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
