diff mbox

[v2,03/22] virtio: add struct vp_device

Message ID 1435653553-7728-4-git-send-email-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann June 30, 2015, 8:38 a.m. UTC 
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 <kraxel@redhat.com>
---
 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(-)

Comments

Kevin O'Connor June 30, 2015, 2:33 p.m. UTC | #1 
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 <kraxel@redhat.com>
> ---
>  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
Gerd Hoffmann July 1, 2015, 7:34 a.m. UTC | #2 
> > -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.

Moved allocation to callers now.  virtio-blk embeds the struct, so there
is no extra malloc needed.  virtio-scsi continues to allocate it
separately and stick a pointer to each device, which makes sense IMHO
considering that the struct will grow with the following patches.

> > +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.

Leftover oddity from the initial revision of the patch, which was
ordered before the 32bit switchover patch.  The following patches which
switch over all the vp_*() functions to use vp_{read,write) clean that
up.

cheers,
  Gerd
diff mbox

Patch

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;
 
-    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);
 }
 
-static inline void vp_set_features(unsigned int ioaddr, u32 features)
+static inline void vp_set_features(struct vp_device *vp, u32 features)
 {
-        outl(features, ioaddr + VIRTIO_PCI_GUEST_FEATURES);
+    outl(features, GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_GUEST_FEATURES);
 }
 
-static inline void vp_get(unsigned int ioaddr, unsigned offset,
+static inline void vp_get(struct vp_device *vp, unsigned offset,
                      void *buf, unsigned len)
 {
+   int ioaddr = GET_LOWFLAT(vp->ioaddr);
    u8 *ptr = buf;
    unsigned i;
 
@@ -59,47 +65,49 @@  static inline void vp_get(unsigned int ioaddr, unsigned offset,
            ptr[i] = inb(ioaddr + VIRTIO_PCI_CONFIG + offset + i);
 }
 
-static inline u8 vp_get_status(unsigned int ioaddr)
+static inline u8 vp_get_status(struct vp_device *vp)
 {
-   return inb(ioaddr + VIRTIO_PCI_STATUS);
+    return inb(GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_STATUS);
 }
 
-static inline void vp_set_status(unsigned int ioaddr, u8 status)
+static inline void vp_set_status(struct vp_device *vp, u8 status)
 {
    if (status == 0)        /* reset */
            return;
-   outb(status, ioaddr + VIRTIO_PCI_STATUS);
+   outb(status, GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_STATUS);
 }
 
-static inline u8 vp_get_isr(unsigned int ioaddr)
+static inline u8 vp_get_isr(struct vp_device *vp)
 {
-   return inb(ioaddr + VIRTIO_PCI_ISR);
+    return inb(GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_ISR);
 }
 
-static inline void vp_reset(unsigned int ioaddr)
+static inline void vp_reset(struct vp_device *vp)
 {
+   int ioaddr = GET_LOWFLAT(vp->ioaddr);
+
    outb(0, ioaddr + VIRTIO_PCI_STATUS);
    (void)inb(ioaddr + VIRTIO_PCI_ISR);
 }
 
-static inline void vp_notify(unsigned int ioaddr, int queue_index)
+static inline void vp_notify(struct vp_device *vp, int queue_index)
 {
-   outw(queue_index, ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
+    outw(queue_index, GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_QUEUE_NOTIFY);
 }
 
-static inline void vp_del_vq(unsigned int ioaddr, int queue_index)
+static inline void vp_del_vq(struct vp_device *vp, int queue_index)
 {
+   int ioaddr = GET_LOWFLAT(vp->ioaddr);
+
    /* select the queue */
-
    outw(queue_index, ioaddr + VIRTIO_PCI_QUEUE_SEL);
 
    /* deactivate the queue */
-
    outl(0, ioaddr + VIRTIO_PCI_QUEUE_PFN);
 }
 
 struct vring_virtqueue;
-u16 vp_init_simple(u16 bdf);
-int vp_find_vq(unsigned int ioaddr, int queue_index,
+struct vp_device *vp_init_simple(u16 bdf);
+int vp_find_vq(struct vp_device *vp, int queue_index,
                struct vring_virtqueue **p_vq);
 #endif /* _VIRTIO_PCI_H_ */
diff --git a/src/hw/virtio-ring.c b/src/hw/virtio-ring.c
index 97e0b34..5c6a32e 100644
--- a/src/hw/virtio-ring.c
+++ b/src/hw/virtio-ring.c
@@ -136,7 +136,7 @@  void vring_add_buf(struct vring_virtqueue *vq,
     SET_LOWFLAT(avail->ring[av], head);
 }
 
-void vring_kick(unsigned int ioaddr, struct vring_virtqueue *vq, int num_added)
+void vring_kick(struct vp_device *vp, struct vring_virtqueue *vq, int num_added)
 {
     struct vring *vr = &vq->vring;
     struct vring_avail *avail = GET_LOWFLAT(vr->avail);
@@ -145,5 +145,5 @@  void vring_kick(unsigned int ioaddr, struct vring_virtqueue *vq, int num_added)
     smp_wmb();
     SET_LOWFLAT(avail->idx, GET_LOWFLAT(avail->idx) + num_added);
 
-    vp_notify(ioaddr, GET_LOWFLAT(vq->queue_index));
+    vp_notify(vp, GET_LOWFLAT(vq->queue_index));
 }
diff --git a/src/hw/virtio-ring.h b/src/hw/virtio-ring.h
index b7a7aaf..fe5133b 100644
--- a/src/hw/virtio-ring.h
+++ b/src/hw/virtio-ring.h
@@ -120,12 +120,13 @@  static inline void vring_init(struct vring *vr,
    vr->desc[i].next = 0;
 }
 
+struct vp_device;
 int vring_more_used(struct vring_virtqueue *vq);
 void vring_detach(struct vring_virtqueue *vq, unsigned int head);
 int vring_get_buf(struct vring_virtqueue *vq, unsigned int *len);
 void vring_add_buf(struct vring_virtqueue *vq, struct vring_list list[],
                    unsigned int out, unsigned int in,
                    int index, int num_added);
-void vring_kick(unsigned int ioaddr, struct vring_virtqueue *vq, int num_added);
+void vring_kick(struct vp_device *vp, struct vring_virtqueue *vq, int num_added);
 
 #endif /* _VIRTIO_RING_H_ */
diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c
index 8f96687..25d2db7 100644
--- a/src/hw/virtio-scsi.c
+++ b/src/hw/virtio-scsi.c
@@ -27,14 +27,15 @@  struct virtio_lun_s {
     struct drive_s drive;
     struct pci_device *pci;
     struct vring_virtqueue *vq;
-    u16 ioaddr;
+    struct vp_device *vp;
     u16 target;
     u16 lun;
 };
 
 static int
-virtio_scsi_cmd(u16 ioaddr, struct vring_virtqueue *vq, struct disk_op_s *op,
-                void *cdbcmd, u16 target, u16 lun, u16 blocksize)
+virtio_scsi_cmd(struct vp_device *vp, struct vring_virtqueue *vq,
+                struct disk_op_s *op, void *cdbcmd, u16 target, u16 lun,
+                u16 blocksize)
 {
     struct virtio_scsi_req_cmd req;
     struct virtio_scsi_resp_cmd resp;
@@ -66,7 +67,7 @@  virtio_scsi_cmd(u16 ioaddr, struct vring_virtqueue *vq, struct disk_op_s *op,
 
     /* Add to virtqueue and kick host */
     vring_add_buf(vq, sg, out_num, in_num, 0, 0);
-    vring_kick(ioaddr, vq, 1);
+    vring_kick(vp, vq, 1);
 
     /* Wait for reply */
     while (!vring_more_used(vq))
@@ -78,7 +79,7 @@  virtio_scsi_cmd(u16 ioaddr, struct vring_virtqueue *vq, struct disk_op_s *op,
     /* Clear interrupt status register.  Avoid leaving interrupts stuck if
      * VRING_AVAIL_F_NO_INTERRUPT was ignored and interrupts were raised.
      */
-    vp_get_isr(ioaddr);
+    vp_get_isr(vp);
 
     if (resp.response == VIRTIO_SCSI_S_OK && resp.status == 0) {
         return DISK_RET_SUCCESS;
@@ -92,7 +93,7 @@  virtio_scsi_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize)
     struct virtio_lun_s *vlun_gf =
         container_of(op->drive_gf, struct virtio_lun_s, drive);
 
-    return virtio_scsi_cmd(GET_GLOBALFLAT(vlun_gf->ioaddr),
+    return virtio_scsi_cmd(GET_GLOBALFLAT(vlun_gf->vp),
                            GET_GLOBALFLAT(vlun_gf->vq), op, cdbcmd,
                            GET_GLOBALFLAT(vlun_gf->target),
                            GET_GLOBALFLAT(vlun_gf->lun),
@@ -100,7 +101,7 @@  virtio_scsi_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize)
 }
 
 static int
-virtio_scsi_add_lun(struct pci_device *pci, u16 ioaddr,
+virtio_scsi_add_lun(struct pci_device *pci, struct vp_device *vp,
                     struct vring_virtqueue *vq, u16 target, u16 lun)
 {
     struct virtio_lun_s *vlun = malloc_fseg(sizeof(*vlun));
@@ -112,7 +113,7 @@  virtio_scsi_add_lun(struct pci_device *pci, u16 ioaddr,
     vlun->drive.type = DTYPE_VIRTIO_SCSI;
     vlun->drive.cntl_id = pci->bdf;
     vlun->pci = pci;
-    vlun->ioaddr = ioaddr;
+    vlun->vp = vp;
     vlun->vq = vq;
     vlun->target = target;
     vlun->lun = lun;
@@ -129,11 +130,11 @@  fail:
 }
 
 static int
-virtio_scsi_scan_target(struct pci_device *pci, u16 ioaddr,
+virtio_scsi_scan_target(struct pci_device *pci, struct vp_device *vp,
                         struct vring_virtqueue *vq, u16 target)
 {
     /* TODO: send REPORT LUNS.  For now, only LUN 0 is recognized.  */
-    int ret = virtio_scsi_add_lun(pci, ioaddr, vq, target, 0);
+    int ret = virtio_scsi_add_lun(pci, vp, vq, target, 0);
     return ret < 0 ? 0 : 1;
 }
 
@@ -144,19 +145,19 @@  init_virtio_scsi(struct pci_device *pci)
     dprintf(1, "found virtio-scsi at %x:%x\n", pci_bdf_to_bus(bdf),
             pci_bdf_to_dev(bdf));
     struct vring_virtqueue *vq = NULL;
-    u16 ioaddr = vp_init_simple(bdf);
-    if (vp_find_vq(ioaddr, 2, &vq) < 0 ) {
+    struct vp_device *vp = vp_init_simple(bdf);
+    if (vp_find_vq(vp, 2, &vq) < 0 ) {
         dprintf(1, "fail to find vq for virtio-scsi %x:%x\n",
                 pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf));
         goto fail;
     }
 
-    vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
+    vp_set_status(vp, VIRTIO_CONFIG_S_ACKNOWLEDGE |
                   VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
 
     int i, tot;
     for (tot = 0, i = 0; i < 256; i++)
-        tot += virtio_scsi_scan_target(pci, ioaddr, vq, i);
+        tot += virtio_scsi_scan_target(pci, vp, vq, i);
 
     if (!tot)
         goto fail;
@@ -164,7 +165,8 @@  init_virtio_scsi(struct pci_device *pci)
     return;
 
 fail:
-    vp_reset(ioaddr);
+    vp_reset(vp);
+    free(vp);
     free(vq);
 }