On 09/26/13 20:49, Laszlo Ersek wrote:
> On 09/23/13 15:37, Olivier Martin wrote:
>
> I will continue the review of this patch from here.
>
> [...]
>
>> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
>> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
>> new file mode 100644
>> index 0000000..284c25a
>> --- /dev/null
>> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
[snip]
>> +EFI_STATUS
>> +EFIAPI
>> +VirtioMmioDeviceWrite (
>> + IN VIRTIO_DEVICE_PROTOCOL *This,
>> + IN UINTN FieldOffset,
>> + IN UINTN FieldSize,
>> + IN UINT64 Value
>> + )
>> +{
>> + UINTN DstBaseAddress;
>> + UINT8 *DstByteAddress;
>> + UINT8 SrcByte;
>> + UINTN Index;
>> + VIRTIO_MMIO_DEVICE *Device;
>> +
>> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
>> +
>> + // Double-check fieldsize
>> + if (FieldSize > 8) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> + if ((FieldSize % 2) && FieldSize != 1) {
>> + return EFI_INVALID_PARAMETER;
>> + }
This will accept FieldSize==6 as well; is it the intent? If not, then I
think it would be simplest to explicitly list accepted sizes (only four
cases), like
if (FieldSize != 1 && FieldSize != 2 &&
FieldSize != 4 && FieldSize != 8) {
return EFI_INVALID_PARAMETER;
}
>> +
>> + // Compute base address
>> + DstBaseAddress = Device->BaseAddress +
>> + VIRTIO_MMIO_OFFSET_DEVICE_SPECIFIC_CONFIG + FieldOffset;
>> +
>> + //
>> + // The device-specific memory area of Virtio-MMIO can only be written in
>> + // byte accesses. This is not currently in the Virtio spec.
>> + //
>> + for (Index = 0; Index < FieldSize; Index++) {
>> + DstByteAddress = ((UINT8 *)DstBaseAddress) + Index;
>> + SrcByte = *(((UINT8 *)&Value) + Index);
>> + MmioWrite8 ((UINTN) DstByteAddress, SrcByte);
>> + }
I think you could simplify this with MmioWriteBuffer8(); it's provided
by the same IoLib (see "MdePkg/Include/Library/IoLib.h").
"Copy data from system memory to MMIO region by using 8-bit access."
>> +
>> + return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +VirtioMmioDeviceRead (
>> + IN VIRTIO_DEVICE_PROTOCOL *This,
>> + IN UINTN FieldOffset,
>> + IN UINTN FieldSize,
>> + IN UINTN BufferSize,
>> + OUT VOID *Buffer
>> + )
>> +{
>> + UINTN SrcBaseAddress;
>> + UINT8 *SrcByteAddress;
>> + UINTN Index;
>> + VIRTIO_MMIO_DEVICE *Device;
>> +
>> + Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
>> +
>> + // Parameter validation
>> + if (FieldSize != BufferSize) {
>> + return EFI_INVALID_PARAMETER;
>> + }
I think we should rather ASSERT() this, to be in synch with the current
in-tree code, and with virtio-pci.
>> +
>> + // Double-check fieldsize
>> + if (FieldSize > 8) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> + if ((FieldSize % 2) && FieldSize != 1) {
>> + return EFI_INVALID_PARAMETER;
>> + }
Same comment as for VirtioMmioDeviceWrite().
>> +
>> + // Compute base address
>> + SrcBaseAddress = Device->BaseAddress +
>> + VIRTIO_MMIO_OFFSET_DEVICE_SPECIFIC_CONFIG + FieldOffset;
>> +
>> + //
>> + // The device-specific memory area of Virtio-MMIO can only be read in
>> + // byte reads. This is not currently in the Virtio spec.
>> + //
>> + for (Index = 0; Index < FieldSize; Index++) {
>> + SrcByteAddress = ((UINT8 *)SrcBaseAddress) + Index;
>> + *(UINT8 *)(Buffer + Index) = MmioRead8 ((UINTN) SrcByteAddress);
>> + }
Similarly, I'm proposing MmioReadBuffer8().
>> +
>> + return EFI_SUCCESS;
>> +}
>> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
>> b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
>> new file mode 100644
>> index 0000000..e6deccb
>> --- /dev/null
>> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
>> @@ -0,0 +1,43 @@
>> +## @file
>> +# This driver produces the VirtIo Device Protocol instances for VirtIo Mmio
>> +# 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 = 0x00010005
>> + BASE_NAME = VirtioMmioDeviceLib
>> + FILE_GUID = 3b6ed966-b5d1-46a8-965b-867ff22d9c89
>> + MODULE_TYPE = UEFI_DRIVER
As I wrote earlier, I don't know enough about module types to actually
propose here something, but I wonder if this should state DXE_DRIVER
instead, since the client code in patches #9/#10 is a DXE_DRIVER.
>> + VERSION_STRING = 1.0
>> + LIBRARY_CLASS = VirtioMmioDeviceLib
>> +
>> +[Sources]
>> + VirtioMmioDevice.c
>> + VirtioMmioDeviceFunctions.c
>> +
>> +[Packages]
>> + MdePkg/MdePkg.dec
>> + OvmfPkg/OvmfPkg.dec
>> +
>> +[LibraryClasses]
>> + BaseMemoryLib
>> + DebugLib
>> + IoLib
>> + MemoryAllocationLib
>> + UefiBootServicesTableLib
>> + UefiDriverEntryPoint
>> + UefiLib
It's probably useless for me to obsess about this :), but I think at
least UefiDriverEntryPoint is superfluous here.
>> +
>> +[Protocols]
>> + gVirtioDeviceProtocolGuid
A <UsageField> of
## BY_START
is only allowed if "The protocol is produced by a Driver Binding
protocol Start function", which is not the case, so I'm not recommending
## BY_START.
But
## PRODUCES
is available, and I think it would be nice to use it. (Example:
"MdePkg/Library/UefiLib/UefiLib.inf".)
No more comments for this patch.
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=60133471&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
