On 09/23/13 15:37, Olivier Martin wrote:
> +/**
> +
> + Write a word to the device-specific I/O region of the Virtio Header.
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> + @param[in] FieldOffset Destination offset.
> +
> + @param[in] FieldSize Destination field size in bytes,
> + must be in {1, 2, 4, 8}.
> +
> + @param[out] Value Value to write.
> +
> + @retval EFI_SUCCESS The data was written successfully.
> + @retval EFI_UNSUPPORTED The underlying IO device doesn't support the
> + provided address offset and read size.
The explanation for EFI_UNSUPPORTED should say "write size", but it's
not a show-stopper of course -- fix it only in case a v4 is needed.
> + @retval EFI_OUT_OF_RESOURCES The request could not be completed due to a
> + lack of resources.
> + @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *VIRTIO_DEVICE_WRITE) (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN FieldOffset,
> + IN UINTN FieldSize,
> + IN UINT64 Value
> + );
I wonder about the byte order in Value...
The virtio spec says that device-specific fields take values in
guest-native endianness. So the Value parameter is transparent here. Can
edk2 be built for big-endian ARM? I'll have to remember to check that in
the implementations of this interface. (For big endian we'll have to
copy out stuff from the far end of Value.)
> +
> +/**
> + Read the device features field from the Virtio Header.
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> + @return The 32-bit device features field.
> +
> +**/
> +typedef
> +UINT32
> +(EFIAPI *VIRTIO_GET_DEVICE_FEATURES) (
> + IN VIRTIO_DEVICE_PROTOCOL *This
> + );
The underlying field is always little endian, so we might have to
byte-swap in the implementations. (VirtioLib used to have it easy in
this regard, because it targeted x64 / ia32 only; everything was little
endian.)
(1) Also, regarding the function prototype. For virtio-mmio I guess this
is a non-issue, but (in theory at least) the PciIo.Io.Read() function
can return many kinds of errors, and this interface doesn't seem to
propagate that.
This holds for all of the getters below.
I'll have to see the PCI implementation.
... Hm, yes. For example, VirtioPciGetDeviceStatus() in patch #4 ignores
the return value of VIRTIO_CFG_READ() (which wraps VirtioPciIoRead(),
which in turn wraps PciIo->Io.Read()).
As a consequence, patch #6 has hunks like:
> @@ -646,11 +620,9 @@ VirtioBlkInit (
> //
> // step 4a -- retrieve and validate features
> //
> - Status = VIRTIO_CFG_READ (Dev, Generic.VhdrDeviceFeatureBits, &Features);
> - if (EFI_ERROR (Status)) {
> - goto Failed;
> - }
> - Status = VIRTIO_CFG_READ (Dev, VhdrCapacity, &NumSectors);
> + Features = Dev->VirtIo->GetDeviceFeatures (Dev->VirtIo);
which actively throws away the preexistent error check.
I think this is not a good thing. The retval checks should remain in
place, and the virtio-mmio implementation of the protocol should always
return EFI_SUCCESS for reads.
... Unless, of course, you can convince me that such PciIo->Io.Read()
calls will *always* succeed on a properly configured PciIo
device/protocol. (That may be the case, I'm no PCI expert by any means,
I just wanted to cover all possibilities listed in the UEFI spec.)
In this case though, please add the appropriate ASSERT_EFI_ERROR() call
right after PciIo->Io.Read(), in VirtioPciIoRead().
> +
> +/**
> + Read the guest features field from the Virtio Header.
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> + @return The 32-bit guest features field.
> +
> +**/
> +typedef
> +UINT32
> +(EFIAPI *VIRTIO_GET_GUEST_FEATURES) (
> + IN VIRTIO_DEVICE_PROTOCOL *This
> + );
(same)
(2) Plus: I think this field is write-only for virtio-mmio. Can we drop
this from the protocol? I doubt PCI would use it for anything.
> +
> +/**
> + Write the guest features field in the Virtio Header.
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> + @param[in] Features The 32-bit guest guest features field
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *VIRTIO_SET_GUEST_FEATURES) (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINT32 Features
> + );
A note about endianness would be nice (maybe a leading comment in the
file regarding any internal conversion?)
> +
> +/**
> + Read the queue address field from the Virtio Header.
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> + @return The 32-bit queue address field.
> +
> +**/
> +typedef
> +UINT32
> +(EFIAPI *VIRTIO_GET_QUEUE_ADDRESS) (
> + IN VIRTIO_DEVICE_PROTOCOL *This
> + );
> +
> +/**
> + Write the queue address field in the Virtio Header.
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> + @param[in] Address The 32-bit Queue Address field
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *VIRTIO_SET_QUEUE_ADDRESS) (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINT32 Address
> + );
Perhaps a note about the divisor to be used here (= page size of the
guest)?
> +
> +/**
> +
> + Write the queue select field in the Virtio Header.
> +
> + Writing to the queue select field sets the index of the queue to which
> + operations such as SetQueueAlign and GetQueueNumMax apply.
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> + @param[in] Index The index of the queue to select
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *VIRTIO_SET_QUEUE_SEL) (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINT16 Index
> + );
A VIRTIO_GET_QUEUE_SEL member / typedef could have been appropriate for
PCI, but I agree that virtio-mmio allows write-only here.
> +
> +/**
> +
> + Write the queue notify field in the Virtio Header.
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> + @param[in] Address The 32-bit Queue Notify field
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *VIRTIO_SET_QUEUE_NOTIFY) (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINT16 Index
> + );
(same)
> +
> +/**
> + Write the queue alignment field in the Virtio Header.
> +
> + The queue to which the alignment applies is selected by the Queue Select
> + field.
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> + @param[in] Alignment The alignment boundary of the Used Ring in bytes.
> + Must be a power of 2.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *VIRTIO_SET_QUEUE_ALIGN) (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINT32 Alignment
> + );
Seems to be virtio-mmio only. Probably a NOP for PCI. (Maybe add a
comment about it?)
Plus, indeed write-only. OK.
> +
> +/**
> +
> + Get the size of the virtqueue selected by the queue select field.
> +
> + See Virtio spec Section 2.3
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> + @return The size of the virtqueue in bytes.
> + Always a power of 2.
> +
> +**/
> +typedef
> +UINT16
> +(EFIAPI *VIRTIO_GET_QUEUE_NUM_MAX) (
> + IN VIRTIO_DEVICE_PROTOCOL *This
> + );
> +
> +/**
> +
> + Write to the QueueNum field in the Virtio Header.
> +
> + This function only applies to Virtio-MMIO and may be a stub for other
> + implementations. See Virtio Spec appendix X.
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> + @param[in] QueueSize The number of elements in the queue.
> +
> +**/
> +typedef
> +VOID
> +(EFIAPI *VIRTIO_SET_QUEUE_NUM) (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINT16 QueueSize
> + );
> +
> +/**
> +
> + Write the QueueNum field in the Virtio Header.
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> +**/
> +typedef
> +UINT8
> +(EFIAPI *VIRTIO_GET_DEVICE_STATUS) (
> + IN VIRTIO_DEVICE_PROTOCOL *This
> + );
The leading comment of this member is stale (copy/paste error from
QueueNum above).
> +
> +/**
> +
> + Write the DeviceStatus field in the Virtio Header.
> +
> + @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL
> +
> + @param[in] DeviceStatus The 8-bit value for the Device status field
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *VIRTIO_SET_DEVICE_STATUS) (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINT8 DeviceStatus
> + );
> +
> +
> +///
> +/// This protocol provides an abstraction over the VirtIo transport layer
> +///
> +struct _VIRTIO_DEVICE_PROTOCOL {
> + /// From the Virtio Spec
> + INT32 SubSystemDeviceId;
> +
> + VIRTIO_GET_DEVICE_FEATURES GetDeviceFeatures;
> + VIRTIO_GET_GUEST_FEATURES GetGuestFeatures;
> + VIRTIO_SET_GUEST_FEATURES SetGuestFeatures;
> +
> + VIRTIO_GET_QUEUE_ADDRESS GetQueueAddress;
> + VIRTIO_SET_QUEUE_ADDRESS SetQueueAddress;
> +
> + VIRTIO_SET_QUEUE_SEL SetQueueSel;
> +
> + VIRTIO_SET_QUEUE_NOTIFY SetQueueNotify;
> +
> + VIRTIO_SET_QUEUE_ALIGN SetQueueAlign;
> +
> + VIRTIO_GET_QUEUE_NUM_MAX GetQueueNumMax;
> + VIRTIO_SET_QUEUE_NUM SetQueueNum;
> +
> + VIRTIO_GET_DEVICE_STATUS GetDeviceStatus;
> + VIRTIO_SET_DEVICE_STATUS SetDeviceStatus;
> +
> + // Functions to read/write Device Specific headers
> + VIRTIO_DEVICE_WRITE WriteDevice;
> + VIRTIO_DEVICE_READ ReadDevice;
> +};
(3) I wonder if the structure should start with a revision field. Using
a revision field, we can later extend the protocol in a backward
compatible manner, and keep the same GUID. Even if we don't have any
ideas for extensions now, I think we should keep that door open. (If we
add no revision field now, then any later extensions, even if backward
compatible, will require a new GUID -- there won't be any way for a
client to figure out if the protocol is basic or extended.)
> +
> +extern EFI_GUID gVirtioDeviceProtocolGuid;
> +
> +#endif
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index d874f0c..7ccc025 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -44,6 +44,7 @@
> gEfiXenInfoGuid = {0xd3b46f3b, 0xd441, 0x1244, {0x9a,
> 0x12, 0x0, 0x12, 0x27, 0x3f, 0xc1, 0x4d}}
>
> [Protocols]
> + gVirtioDeviceProtocolGuid = {0xfa920010, 0x6785, 0x4941, {0xb6,
> 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}
> gBlockMmioProtocolGuid = {0x6b558ce3, 0x69e5, 0x4c67, {0xa6,
> 0x34, 0xf7, 0xfe, 0x72, 0xad, 0xbe, 0x84}}
>
> [PcdsFixedAtBuild]
>
I think points (1) to (3) should be addressed:
(1) error propagation and explicit error handling for all the getters,
*or* a deep ASSERT_EFI_ERROR() for PciIo.Io.Read(),
(2) please investigate and drop VIRTIO_GET_GUEST_FEATURES (write-only
for virtio-mmio, and r/w but likely useless/unused on virtio-pci &
current drivers),
(3) Please add a UINT64 revision field to the beginning of
_VIRTIO_DEVICE_PROTOCOL. (At this point I don't think we need to define
any macros or values for it, just set it to zero.)
_EFI_BLOCK_IO_PROTOCOL is a good example I think.
The rest is just "nice to have, maybe".
I apologize for the number of turns I'm taking to find stuff that I
could have found in v1 as well. My time is very fragmented,
unfortunately. (Just like everyone else's, I guess...)
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
