Re: [SeaBIOS] [PATCH v2 03/22] virtio: add struct vp_device

[Kevin O'Connor]

On Tue, Jun 30, 2015 at 10:38:54AM +0200, Gerd Hoffmann wrote:
> For virtio 1.0 support we will need more state than just the (legacy
> mode) ioaddr for each virtio-pci device.  Prepare for that by adding
> a new struct for it.  For now it carries the ioaddr only.
> Signed-off-by: Gerd Hoffmann
>
> src/hw/virtio-blk.c  | 20 ++++++++++----------
>  src/hw/virtio-pci.c  | 15 +++++++++------
>  src/hw/virtio-pci.h  | 46 +++++++++++++++++++++++++++-------------------
>  src/hw/virtio-ring.c |  4 ++--
>  src/hw/virtio-ring.h |  3 ++-
>  src/hw/virtio-scsi.c | 32 +++++++++++++++++---------------
>  6 files changed, 67 insertions(+), 53 deletions(-)
>
> diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c
> index 15ac171..13cf09a 100644
> --- a/src/hw/virtio-blk.c
> +++ b/src/hw/virtio-blk.c
> @@ -25,7 +25,7 @@
>  struct virtiodrive_s {
>      struct drive_s drive;
>      struct vring_virtqueue *vq;
> u16 ioaddr;
> struct vp_device *vp;
>
> };
>
> static int
> @@ -60,7 +60,7 @@ virtio_blk_op(struct disk_op_s *op, int write)
>          vring_add_buf(vq, sg, 2, 1, 0, 0);
>      else
>          vring_add_buf(vq, sg, 1, 2, 0, 0);
> vring_kick(GET_GLOBALFLAT(vdrive_gf->ioaddr), vq, 1);
>
> vring_kick(GET_GLOBALFLAT(vdrive_gf->vp), vq, 1);
>
> /* Wait for reply */
> while (!vring_more_used(vq))
>
> @@ -72,7 +72,7 @@ virtio_blk_op(struct disk_op_s *op, int write)
>      /* Clear interrupt status register.  Avoid leaving interrupts stuck if
>       * VRING_AVAIL_F_NO_INTERRUPT was ignored and interrupts were raised.
>       */
> vp_get_isr(GET_GLOBALFLAT(vdrive_gf->ioaddr));
>
> vp_get_isr(GET_GLOBALFLAT(vdrive_gf->vp));
>
> return status == VIRTIO_BLK_S_OK ? DISK_RET_SUCCESS : DISK_RET_EBADTRACK;
>
> }
> @@ -113,18 +113,17 @@ init_virtio_blk(struct pci_device *pci)
>      vdrive->drive.type = DTYPE_VIRTIO_BLK;
>      vdrive->drive.cntl_id = bdf;
> u16 ioaddr = vp_init_simple(bdf);
> vdrive->ioaddr = ioaddr;
> if (vp_find_vq(ioaddr, 0, &vdrive->vq) < 0 ) {
>
> vdrive->vp = vp_init_simple(bdf);
>
> if (vp_find_vq(vdrive->vp, 0, &vdrive->vq) < 0 ) {
>     dprintf(1, "fail to find vq for virtio-blk %x:%x\n",
>             pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf));
>     goto fail;
> }
>
> struct virtio_blk_config cfg;
> vp_get(ioaddr, 0, &cfg, sizeof(cfg));
> vp_get(vdrive->vp, 0, &cfg, sizeof(cfg));
> u32 f = vp_get_features(ioaddr);
> u32 f = vp_get_features(vdrive->vp);
> vdrive->drive.blksize = (f & (1 << VIRTIO_BLK_F_BLK_SIZE)) ?
>     cfg.blk_size : DISK_SECTOR_SIZE;
>
> @@ -148,12 +147,13 @@ init_virtio_blk(struct pci_device *pci)
>  boot_add_hd(&vdrive->drive, desc, bootprio_find_pci_device(pci));
>
> vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> vp_set_status(vdrive->vp, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>               VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
> return;
>
> fail:
> vp_reset(ioaddr);
> vp_reset(vdrive->vp);
> free(vdrive->vp);
> free(vdrive->vq);
> free(vdrive);
>
> }
> diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c
> index b9b3ab1..3cd478d 100644
> --- a/src/hw/virtio-pci.c
> +++ b/src/hw/virtio-pci.c
> @@ -24,9 +24,10 @@
>  #include "virtio-pci.h"
>  #include "virtio-ring.h"
>
> -int vp_find_vq(unsigned int ioaddr, int queue_index,
> +int vp_find_vq(struct vp_device *vp, int queue_index,
>                 struct vring_virtqueue **p_vq)
>  {
>
> int ioaddr = GET_LOWFLAT(vp->ioaddr);
> u16 num;
>
> ASSERT32FLAT();
>
> @@ -84,14 +85,16 @@ fail:
>     return -1;
>  }
>
> -u16 vp_init_simple(u16 bdf)
> +struct vp_device *vp_init_simple(u16 bdf)
>  {
> u16 ioaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0) &
> struct vp_device *vp = malloc_high(sizeof(*vp));
> vp->ioaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0) &
>     PCI_BASE_ADDRESS_IO_MASK;

The malloc_high() call can fail - so the code needs to check if it
returns NULL.  (It really can fail in practice and if code writes to
NULL it will corrupt the 16bit isr table, which is painful to debug.)

Why not pass in a vp_device* to vp_init_simple() and have
vp_init_simple() fill it.  Then init_virtio_scsi() can stack allocate
a temporary vp_device and virtio_scsi_add_lun() can memcpy it to
virtio_lun_s.
> vp_reset(ioaddr);
> vp_reset(vp);
> pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_MASTER);
> vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> vp_set_status(vp, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>               VIRTIO_CONFIG_S_DRIVER );
> return ioaddr;
> return vp;
>
> }
> diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h
> index bc04b03..47bef3d 100644
> --- a/src/hw/virtio-pci.h
> +++ b/src/hw/virtio-pci.h
> @@ -2,6 +2,7 @@
>  #define _VIRTIO_PCI_H
>
> #include "x86.h" // inl
> +#include "biosvar.h" // GET_LOWFLAT
>
> /* A 32-bit r/o bitmask of the features supported by the host */
>  #define VIRTIO_PCI_HOST_FEATURES        0
> @@ -39,19 +40,24 @@
>  /* Virtio ABI version, this must match exactly */
>  #define VIRTIO_PCI_ABI_VERSION          0
>
> -static inline u32 vp_get_features(unsigned int ioaddr)
> +struct vp_device {
> unsigned int ioaddr;
>
> +};
>
> +static inline u32 vp_get_features(struct vp_device *vp)
>  {
> return inl(ioaddr + VIRTIO_PCI_HOST_FEATURES);
> return inl(GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_HOST_FEATURES);
>
> }

These GET_LOWFLAT() calls are confusing (as they are only valid for
memory allocated with malloc_low() ).  Granted, they are no-ops in
32bit mode, but they are still confusing.

The rest of the series looks good to me.  (Two other minor points
would be that patch 9 should be squashed into patch 6, and I have some
comments on patch 2.)

Thanks.
-Kevin