Re: [SeaBIOS] [PATCH v2 07/22] virtio: find version 1.0 virtio capabilities

[Michael S. Tsirkin]

On Tue, Jun 30, 2015 at 10:38:58AM +0200, Gerd Hoffmann wrote:
> virtio 1.0 specifies the location of the various virtio regions
> using pci capabilities.  Look them up and store the results.
> Signed-off-by: Gerd Hoffmann
>
> src/hw/virtio-pci.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/hw/virtio-pci.h |  8 ++++++++
>  2 files changed, 63 insertions(+)
>
> diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c
> index 4971021..0acf65f 100644
> --- a/src/hw/virtio-pci.c
> +++ b/src/hw/virtio-pci.c
> @@ -88,6 +88,61 @@ fail:
>  struct vp_device *vp_init_simple(struct pci_device *pci)
>  {
>      struct vp_device *vp = malloc_high(sizeof(*vp));
>
> u8 cap = pci_find_capability(pci, PCI_CAP_ID_VNDR, 0);
>
> struct vp_cap *vp_cap;
>
> u32 addr, offset;
>
> u8 type;
>
> memset(vp, 0, sizeof(*vp));
>
> while (cap != 0) {
>    type = pci_config_readb(pci->bdf, cap +
>                            offsetof(struct virtio_pci_cap, cfg_type));
>    switch (type) {
>    case VIRTIO_PCI_CAP_COMMON_CFG:
>        vp_cap = &vp->common;
>        break;
>    case VIRTIO_PCI_CAP_NOTIFY_CFG:
>        vp_cap = &vp->notify;
>        break;
>    case VIRTIO_PCI_CAP_ISR_CFG:
>        vp_cap = &vp->isr;
>        break;
>    case VIRTIO_PCI_CAP_DEVICE_CFG:
>        vp_cap = &vp->device;
>        break;
>    default:
>        vp_cap = NULL;
>        break;
>    }
>    if (vp_cap) {
>        vp_cap->cap = cap;
>        vp_cap->bar = pci_config_readb(pci->bdf, cap +
>                                       offsetof(struct virtio_pci_cap, bar));
>        offset = pci_config_readl(pci->bdf, cap +
>                                  offsetof(struct virtio_pci_cap, offset));
>        addr = pci_config_readl(pci->bdf, PCI_BASE_ADDRESS_0 + 4 * vp_cap->bar);
>        if (addr & PCI_BASE_ADDRESS_SPACE_IO) {
>            vp_cap->is_io = 1;
>            addr &= PCI_BASE_ADDRESS_IO_MASK;
>        } else {
>            vp_cap->is_io = 0;
>            addr &= PCI_BASE_ADDRESS_MEM_MASK;
>        }
>        vp_cap->addr = addr + offset;
>        dprintf(3, "pci dev %x:%x virtio cap at 0x%x type %d "
>                "bar %d at 0x%08x off +0x%04x [%s]\n",
>                pci_bdf_to_bus(pci->bdf), pci_bdf_to_dev(pci->bdf),
>                vp_cap->cap, type, vp_cap->bar, addr, offset,
>                vp_cap->is_io ? "io" : "mmio");
>    }
>    cap = pci_find_capability(pci, PCI_CAP_ID_VNDR, cap);
>
> }
>
> if (vp->common.cap && vp->notify.cap && vp->isr.cap && vp->device.cap) {
>    dprintf(1, "pci dev %x:%x supports virtio 1.0\n",
>            pci_bdf_to_bus(pci->bdf), pci_bdf_to_dev(pci->bdf));
>
> }
>
> vp->ioaddr = pci_config_readl(pci->bdf, PCI_BASE_ADDRESS_0) &
>     PCI_BASE_ADDRESS_IO_MASK;

Hmm this seems to violate this rule in the spec:

The driver SHOULD use the first instance of each virtio structure type
    they can support.

"can support" here means that bios was able to allocate
it during enumeration.

For example there could be both IO and memory, in this order
you need to check that IO/memory got enabled (in theory,
also that they are within parent bridge's windows - used
by some guests, but
seabios doesn't disable memmory/io in this strange way).
> diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h
> index bc2eb05..0f57ca8 100644
> --- a/src/hw/virtio-pci.h
> +++ b/src/hw/virtio-pci.h
> @@ -115,8 +115,16 @@ typedef struct virtio_pci_isr {
>
> /* --- driver structs ----------------------------------------------- */
>
> +struct vp_cap {
> u32 addr;
> u8 cap;
> u8 bar;
> u8 is_io;
>
> +};
>
> struct vp_device {
>      unsigned int ioaddr;
> struct vp_cap common, notify, isr, device;
>
> };
> static inline u32 vp_get_features(struct vp_device *vp)
>
> 1.8.3.1