Re: [PATCH 0/4] Virtio non-discoverable devices

[Laszlo Ersek <lersek@redhat.com>]

Hi Daniil,

On 03/07/18 02:36, Daniil Egranov wrote:
> This is an attempt to add MMIO Virtio devices into the
> non-discoverable device registration procedure and allow
> Virtio PCI drivers to recognize and program such devices 
> correctly.
> The main issue is that the set of MMIO registers is different
> from PCI, plus the width of similar registers are not 
> always the same. The code implements the translation of 
> the PCI IO registers to MMIO registers. 
> Another difference between PCI and MMIO Virtio devices found 
> during the testing is that MMIO devices may require more 
> registers to be programmed compared to PCI. The VirtioPciDeviceDxe
> was patched to detect non-discoverable MMIO devices and allow
> calling a PCI MemIo protocol function.
> 
> This set of patches was tested with MMIO Virtio Block and
> Virtio Net devices.

I'm commenting on this series because of patch 4/4, which is for OvmfPkg.

OvmfPkg supports the following virtio transports:

- virtio-pci, spec version 0.9.5 ("legacy"):

  OvmfPkg/VirtioPciDeviceDxe consumes EFI_PCI_IO_PROTOCOL,
  and produces VIRTIO_DEVICE_PROTOCOL;

- virtio-pci, spec version 1.0 ("modern"):

  OvmfPkg/Virtio10Dxe consumes EFI_PCI_IO_PROTOCOL and produces
  VIRTIO_DEVICE_PROTOCOL;

- virtio-mmio, spec version 0.9.5 ("legacy"):

  OvmfPkg/Library/VirtioMmioDeviceLib takes (a) an EFI_HANDLE that
  carries a vendor-specific device path protocol instance, (b) the base
  address of the virtio-mmio transport (register block), and produces
  VIRTIO_DEVICE_PROTOCOL.

The individual virtio device drivers under OvmfPkg -- such as
VirtioBlkDxe, VirtioGpuDxe, VirtioNetDxe, VirtioRngDxe and VirtioScsiDxe
-- consume VIRTIO_DEVICE_PROTOCOL, and produce the corresponding (device
type specific) UEFI protocols on top. They perform a *union* of the
steps that are required for generic device configuration over either
virtio transport (going through VIRTIO_DEVICE_PROTOCOL), and the
transport drivers take care of mapping the actions to the actual
transports. In some cases, this means simply ignoring an action (because
it has no mapping defined for the given transport type).

Now, this patch set:

(1) extends NonDiscoverableDeviceRegistrationLib, so that a platform
DXE_DRIVER can register a new kind of non-discoverable device,

(2) extends the NonDiscoverablePciDeviceDxe PCI emulation driver in
MdeModulePkg to recognize the new kind of device.

However: the PciIo protocol instances that are consequently produced
represent virtio devices that do *not* conform to the PCI binding of
either virtio specification (0.9.5 or 1.0). Namely,

- The QueueAlign register (at offset 0x3C in the MMIO register block)
  and the GuestPageSize register (at offset 0x28) are only defined for
  the virtio-mmio binding, spec version 0.9.5 ("legacy").

- The QueueNum register (at offset 0x38) is:

  - defined in the virtio-mmio binding, spec version 0.9.5 ("legacy"),

  - defined in the virtio-mmio binding, spec version 1.0 ("modern"),

  - "sort of defined" in the virtio-pci binding, spec version 1.0
    only ("modern" only). (By "sort of defined", I mean the fact that
    the "queue_size" register of the PCI binding is read-write in spec
    version 1.0.)

Therefore adding any actual handling for these registers to
VirtioPciDeviceDxe, which implements the 0.9.5 PCI binding, is wrong.

If your platform provides a virtio device over an MMIO transport, then
the right way to expose the device to the firmware is *not* to

(a) simulate a PciIo interface that does not conform to the PCI binding
    of the virtio-0.9.5 spec,

(b) then patch that shortcoming up in VirtioPciDeviceDxe, which already
    conforms to the PCI binding of the virtio-0.9.5 spec.

Instead, the right way is to use "OvmfPkg/Library/VirtioMmioDeviceLib"
in a platform driver, for producing VIRTIO_DEVICE_PROTOCOL instances on
top of the virtio-mmio transports (MMIO register blocks) directly.

You must already have a platform DXE_DRIVER that produces the
non-discoverable device protocol instances described in (1). I suggest
that you modify that driver to use VirtioMmioDeviceLib instead:
enumerate the same set of virtio-mmio transports that you must already
be doing, and call VirtioMmioInstallDevice() on each. (You can see an
example in "ArmVirtPkg/VirtioFdtDxe".)

This will give you VIRTIO_DEVICE_PROTOCOL instances right off the bat
that can be driven by VirtioBlkDxe, VirtioGpuDxe, etc.

Thanks,
Laszlo