[PATCH 00/18] virtio: add version 1.0 support
Hi, This patch series adds virtio 1.0 support to the virtio blk and scsi drivers in seabios. With this series applied seabios happily boots in virtio 1.0 mode from both transitional and modern devices. Tested with Fedora 22 guest, booting from virtio-scsi cdrom (live iso), virtio-scsi disk and virtio-blk disk. The patches are also available in the git repository at: git://git.kraxel.org/seabios virtio please review, Gerd ---------------------------------------------------------------- Gerd Hoffmann (18): pci: allow to loop over capabilities virtio: run drivers in 32bit mode virtio: add struct vp_device virtio: pass struct pci_device to vp_init_simple virtio: add version 1.0 structs and #defines virtio: find version 1.0 virtio capabilities virtio: add version 1.0 read/write macros virtio: make features 64bit, support version 1.0 features virtio: add version 1.0 support to vp_{get,set}_status virtio: add version 1.0 support to vp_get_isr virtio: add version 1.0 support to vp_reset virtio: add version 1.0 support to vp_notify virtio: remove unused vp_del_vq virtio: add version 1.0 support to vp_find_vq virtio-scsi: fix initialization for version 1.0 virtio-blk: fix initialization for version 1.0 virtio: use version 1.0 if available (flip the big switch) virtio: also probe version 1.0 pci ids src/block.c | 8 +- src/fw/pciinit.c | 4 +- src/hw/pci.c | 11 ++- src/hw/pci.h | 2 +- src/hw/pci_ids.h | 8 +- src/hw/virtio-blk.c | 104 +++++++++++++++------ src/hw/virtio-pci.c | 250 ++++++++++++++++++++++++++++++++++++++++++++++----- src/hw/virtio-pci.h | 205 ++++++++++++++++++++++++++++++++---------- src/hw/virtio-ring.c | 4 +- src/hw/virtio-ring.h | 9 +- src/hw/virtio-scsi.c | 60 +++++++++---- 11 files changed, 537 insertions(+), 128 deletions(-)
Add a parameter to pci_find_capability, to specify the start point. This allows to find multiple capabilities of the same type, by calling pci_find_capability again with the offset of the last capability found. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/fw/pciinit.c | 4 ++-- src/hw/pci.c | 11 ++++++++--- src/hw/pci.h | 2 +- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index ac39d23..45870f2 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -642,7 +642,7 @@ pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev, static int pci_bus_hotplug_support(struct pci_bus *bus) { - u8 pcie_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_EXP); + u8 pcie_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_EXP, 0); u8 shpc_cap; if (pcie_cap) { @@ -666,7 +666,7 @@ static int pci_bus_hotplug_support(struct pci_bus *bus) return downstream_port && slot_implemented; } - shpc_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_SHPC); + shpc_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_SHPC, 0); return !!shpc_cap; } diff --git a/src/hw/pci.c b/src/hw/pci.c index 0379b55..a241d06 100644 --- a/src/hw/pci.c +++ b/src/hw/pci.c @@ -221,16 +221,21 @@ pci_find_init_device(const struct pci_device_id *ids, void *arg) return NULL; } -u8 pci_find_capability(struct pci_device *pci, u8 cap_id) +u8 pci_find_capability(struct pci_device *pci, u8 cap_id, u8 cap) { int i; - u8 cap; u16 status = pci_config_readw(pci->bdf, PCI_STATUS); if (!(status & PCI_STATUS_CAP_LIST)) return 0; - cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); + if (cap == 0) { + /* find first */ + cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); + } else { + /* find next */ + cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT); + } for (i = 0; cap && i <= 0xff; i++) { if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id) return cap; diff --git a/src/hw/pci.h b/src/hw/pci.h index 0aaa84c..fc5e7b9 100644 --- a/src/hw/pci.h +++ b/src/hw/pci.h @@ -123,7 +123,7 @@ int pci_init_device(const struct pci_device_id *ids , struct pci_device *pci, void *arg); struct pci_device *pci_find_init_device(const struct pci_device_id *ids , void *arg); -u8 pci_find_capability(struct pci_device *pci, u8 cap_id); +u8 pci_find_capability(struct pci_device *pci, u8 cap_id, u8 cap); int pci_bridge_has_region(struct pci_device *pci, enum pci_region_type region_type); void pci_reboot(void); -- 1.8.3.1
virtio version 1.0 registers can (and actually do in the qemu implementation) live in mmio space. So we must run the blk and scsi virtio drivers in 32bit mode, otherwise we can't access them. This also allows to drop a bunch of GET_LOWFLAT calls from the virtio code in the following patches. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/block.c | 8 +++++--- src/hw/virtio-blk.c | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/block.c b/src/block.c index 3f7ecb1..a9b9851 100644 --- a/src/block.c +++ b/src/block.c @@ -503,8 +503,10 @@ process_op(struct disk_op_s *op) case DTYPE_CDEMU: ret = process_cdemu_op(op); break; - case DTYPE_VIRTIO_BLK: - ret = process_virtio_blk_op(op); + case DTYPE_VIRTIO_BLK: ; + extern void _cfunc32flat_process_virtio_blk_op(void); + ret = call32(_cfunc32flat_process_virtio_blk_op + , (u32)MAKE_FLATPTR(GET_SEG(SS), op), DISK_RET_EPARAM); break; case DTYPE_AHCI: ; extern void _cfunc32flat_process_ahci_op(void); @@ -526,7 +528,6 @@ process_op(struct disk_op_s *op) break; case DTYPE_USB: case DTYPE_UAS: - case DTYPE_VIRTIO_SCSI: case DTYPE_LSI_SCSI: case DTYPE_ESP_SCSI: case DTYPE_MEGASAS: @@ -534,6 +535,7 @@ process_op(struct disk_op_s *op) break; case DTYPE_USB_32: case DTYPE_UAS_32: + case DTYPE_VIRTIO_SCSI: case DTYPE_PVSCSI: ; extern void _cfunc32flat_scsi_process_op(void); ret = call32(_cfunc32flat_scsi_process_op diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index e2dbd3c..15ac171 100644 --- a/src/hw/virtio-blk.c +++ b/src/hw/virtio-blk.c @@ -77,7 +77,7 @@ virtio_blk_op(struct disk_op_s *op, int write) return status == VIRTIO_BLK_S_OK ? DISK_RET_SUCCESS : DISK_RET_EBADTRACK; } -int +int VISIBLE32FLAT process_virtio_blk_op(struct disk_op_s *op) { if (! CONFIG_VIRTIO_BLK) -- 1.8.3.1
On 29/06/2015 10:53, Gerd Hoffmann wrote:
virtio version 1.0 registers can (and actually do in the qemu implementation) live in mmio space. So we must run the blk and scsi virtio drivers in 32bit mode, otherwise we can't access them.
This also allows to drop a bunch of GET_LOWFLAT calls from the virtio code in the following patches.
This isn't really necessary though. The config access capability exists for this reason. At least as a follow up it would be nice to add a switch. Paolo
On Fr, 2015-07-03 at 08:45 +0200, Paolo Bonzini wrote:
On 29/06/2015 10:53, Gerd Hoffmann wrote:
virtio version 1.0 registers can (and actually do in the qemu implementation) live in mmio space. So we must run the blk and scsi virtio drivers in 32bit mode, otherwise we can't access them.
This also allows to drop a bunch of GET_LOWFLAT calls from the virtio code in the following patches.
This isn't really necessary though. The config access capability exists for this reason. At least as a follow up it would be nice to add a switch.
Just saw mst's patch implementing it in qemu. I'll have a look. I think I want keep the virtio drivers as 32bit drivers. The modern virtio bar is a 64bit though, so accessing via config access capability might be needed even for the 32bit drivers in case it is mapped above 4G ... cheers, Gerd
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..a1e9939 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_low(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); } -- 1.8.3.1
On Mon, Jun 29, 2015 at 10:53:25AM +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; };
Is there a reason to make this a pointer as opposed to just placing the vp_device struct directly in struct virtiodrive_s?
-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_low(sizeof(*vp)); + + vp->ioaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0) & PCI_BASE_ADDRESS_IO_MASK;
If it has to be a pointer, then the result of malloc_low() has to be checked for NULL. Though, I don't think malloc_low() makes sense if it's a 32bit driver. -Kevin
On Mo, 2015-06-29 at 08:48 -0400, Kevin O'Connor wrote:
On Mon, Jun 29, 2015 at 10:53:25AM +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; };
Is there a reason to make this a pointer as opposed to just placing the vp_device struct directly in struct virtiodrive_s?
That'll work too, both blk and scsi have a struct where we can place vp_device inside. I'll change it for v2. cheers, Gerd
Hi,
Is there a reason to make this a pointer as opposed to just placing the vp_device struct directly in struct virtiodrive_s?
That'll work too, both blk and scsi have a struct where we can place vp_device inside. I'll change it for v2.
Oops, wrong. For virtio-blk it works, but for virtio-scsi all devices connected to the host adapter share one vp_device instance. And a per-hostadapter data structure doesn't exist because we don't need one. cheers, Gerd
... instead of the bdf only. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-blk.c | 2 +- src/hw/virtio-pci.c | 6 +++--- src/hw/virtio-pci.h | 3 ++- src/hw/virtio-scsi.c | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index 13cf09a..4f39aa2 100644 --- a/src/hw/virtio-blk.c +++ b/src/hw/virtio-blk.c @@ -113,7 +113,7 @@ init_virtio_blk(struct pci_device *pci) vdrive->drive.type = DTYPE_VIRTIO_BLK; vdrive->drive.cntl_id = bdf; - vdrive->vp = vp_init_simple(bdf); + vdrive->vp = vp_init_simple(pci); 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)); diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c index a1e9939..ed5bfcb 100644 --- a/src/hw/virtio-pci.c +++ b/src/hw/virtio-pci.c @@ -85,15 +85,15 @@ fail: return -1; } -struct vp_device *vp_init_simple(u16 bdf) +struct vp_device *vp_init_simple(struct pci_device *pci) { struct vp_device *vp = malloc_low(sizeof(*vp)); - vp->ioaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0) & + vp->ioaddr = pci_config_readl(pci->bdf, PCI_BASE_ADDRESS_0) & PCI_BASE_ADDRESS_IO_MASK; vp_reset(vp); - pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_MASTER); + pci_config_maskw(pci->bdf, PCI_COMMAND, 0, PCI_COMMAND_MASTER); vp_set_status(vp, VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER ); return vp; diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index 47bef3d..3cf0327 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -106,8 +106,9 @@ static inline void vp_del_vq(struct vp_device *vp, int queue_index) outl(0, ioaddr + VIRTIO_PCI_QUEUE_PFN); } +struct pci_device; struct vring_virtqueue; -struct vp_device *vp_init_simple(u16 bdf); +struct vp_device *vp_init_simple(struct pci_device *pci); 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-scsi.c b/src/hw/virtio-scsi.c index 25d2db7..b208d49 100644 --- a/src/hw/virtio-scsi.c +++ b/src/hw/virtio-scsi.c @@ -145,7 +145,7 @@ 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; - struct vp_device *vp = vp_init_simple(bdf); + struct vp_device *vp = vp_init_simple(pci); 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)); -- 1.8.3.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/hw/virtio-ring.h | 5 +++++ 2 files changed, 64 insertions(+) diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index 3cf0327..e6f9c0b 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -40,6 +40,65 @@ /* Virtio ABI version, this must match exactly */ #define VIRTIO_PCI_ABI_VERSION 0 + +/* Common configuration */ +#define VIRTIO_PCI_CAP_COMMON_CFG 1 +/* Notifications */ +#define VIRTIO_PCI_CAP_NOTIFY_CFG 2 +/* ISR access */ +#define VIRTIO_PCI_CAP_ISR_CFG 3 +/* Device specific configuration */ +#define VIRTIO_PCI_CAP_DEVICE_CFG 4 +/* PCI configuration access */ +#define VIRTIO_PCI_CAP_PCI_CFG 5 + +/* This is the PCI capability header: */ +struct virtio_pci_cap { + u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */ + u8 cap_next; /* Generic PCI field: next ptr. */ + u8 cap_len; /* Generic PCI field: capability length */ + u8 cfg_type; /* Identifies the structure. */ + u8 bar; /* Where to find it. */ + u8 padding[3]; /* Pad to full dword. */ + u32 offset; /* Offset within bar. */ + u32 length; /* Length of the structure, in bytes. */ +}; + +struct virtio_pci_notify_cap { + struct virtio_pci_cap cap; + u32 notify_off_multiplier; /* Multiplier for queue_notify_off. */ +}; + +typedef struct virtio_pci_common_cfg { + /* About the whole device. */ + u32 device_feature_select; /* read-write */ + u32 device_feature; /* read-only */ + u32 guest_feature_select; /* read-write */ + u32 guest_feature; /* read-write */ + u16 msix_config; /* read-write */ + u16 num_queues; /* read-only */ + u8 device_status; /* read-write */ + u8 config_generation; /* read-only */ + + /* About a specific virtqueue. */ + u16 queue_select; /* read-write */ + u16 queue_size; /* read-write, power of 2. */ + u16 queue_msix_vector; /* read-write */ + u16 queue_enable; /* read-write */ + u16 queue_notify_off; /* read-only */ + u32 queue_desc_lo; /* read-write */ + u32 queue_desc_hi; /* read-write */ + u32 queue_avail_lo; /* read-write */ + u32 queue_avail_hi; /* read-write */ + u32 queue_used_lo; /* read-write */ + u32 queue_used_hi; /* read-write */ +} virtio_pci_common_cfg; + +typedef struct virtio_pci_isr { + u8 isr; +} virtio_pci_isr; + + struct vp_device { unsigned int ioaddr; }; diff --git a/src/hw/virtio-ring.h b/src/hw/virtio-ring.h index fe5133b..553a508 100644 --- a/src/hw/virtio-ring.h +++ b/src/hw/virtio-ring.h @@ -20,9 +20,14 @@ #define VIRTIO_CONFIG_S_DRIVER 2 /* Driver has used its parts of the config, and is happy */ #define VIRTIO_CONFIG_S_DRIVER_OK 4 +/* Driver has finished configuring features */ +#define VIRTIO_CONFIG_S_FEATURES_OK 8 /* We've given up on this device. */ #define VIRTIO_CONFIG_S_FAILED 0x80 +/* v1.0 compliant. */ +#define VIRTIO_F_VERSION_1 32 + #define MAX_QUEUE_NUM (128) #define VRING_DESC_F_NEXT 1 -- 1.8.3.1
On Mon, Jun 29, 2015 at 10:53:27AM +0200, Gerd Hoffmann wrote:
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/hw/virtio-ring.h | 5 +++++ 2 files changed, 64 insertions(+)
diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index 3cf0327..e6f9c0b 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -40,6 +40,65 @@ /* Virtio ABI version, this must match exactly */ #define VIRTIO_PCI_ABI_VERSION 0
+ +/* Common configuration */ +#define VIRTIO_PCI_CAP_COMMON_CFG 1 +/* Notifications */ +#define VIRTIO_PCI_CAP_NOTIFY_CFG 2 +/* ISR access */ +#define VIRTIO_PCI_CAP_ISR_CFG 3 +/* Device specific configuration */ +#define VIRTIO_PCI_CAP_DEVICE_CFG 4 +/* PCI configuration access */ +#define VIRTIO_PCI_CAP_PCI_CFG 5 + +/* This is the PCI capability header: */ +struct virtio_pci_cap { + u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */ + u8 cap_next; /* Generic PCI field: next ptr. */ + u8 cap_len; /* Generic PCI field: capability length */ + u8 cfg_type; /* Identifies the structure. */ + u8 bar; /* Where to find it. */ + u8 padding[3]; /* Pad to full dword. */ + u32 offset; /* Offset within bar. */ + u32 length; /* Length of the structure, in bytes. */ +}; + +struct virtio_pci_notify_cap { + struct virtio_pci_cap cap; + u32 notify_off_multiplier; /* Multiplier for queue_notify_off. */ +}; + +typedef struct virtio_pci_common_cfg { + /* About the whole device. */ + u32 device_feature_select; /* read-write */
If I understand these structs correctly, they don't reside in memory, but instead represent an interface layout and are used with offsetof() and sizeof(). If so, I think they should be marked with PACKED. BTW, it would be nice (but certainly not required) to move these types of interface definitions to a new virtio header in the std/ directory. -Kevin
Hi,
+typedef struct virtio_pci_common_cfg { + /* About the whole device. */ + u32 device_feature_select; /* read-write */
If I understand these structs correctly, they don't reside in memory, but instead represent an interface layout and are used with offsetof() and sizeof().
Correct.
If so, I think they should be marked with PACKED.
They are carefully ordered and padded so that adding PACKED should not change anything. The linux kernel has them without PACKED too.
BTW, it would be nice (but certainly not required) to move these types of interface definitions to a new virtio header in the std/ directory.
Can do that. cheers, Gerd
On Mon, Jun 29, 2015 at 03:35:59PM +0200, Gerd Hoffmann wrote:
Hi,
+typedef struct virtio_pci_common_cfg { + /* About the whole device. */ + u32 device_feature_select; /* read-write */
If I understand these structs correctly, they don't reside in memory, but instead represent an interface layout and are used with offsetof() and sizeof().
Correct.
If so, I think they should be marked with PACKED.
They are carefully ordered and padded so that adding PACKED should not change anything. The linux kernel has them without PACKED too.
Okay - it's up to you. It helps a little with documentation - but moving to "std/" would acomplish the same thing. -Kevin
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 <kraxel@redhat.com> --- 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 ed5bfcb..2abe601 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_low(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; diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index e6f9c0b..893a7dd 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -99,8 +99,16 @@ typedef struct virtio_pci_isr { } virtio_pci_isr; +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
Add macros to read/write registers of virtio-1.0 regions. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.h | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index 893a7dd..e1d8b3e 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -111,6 +111,82 @@ struct vp_device { struct vp_cap common, notify, isr, device; }; +#define vp_modern_read(_cap, _struct, _field, _var) { \ + u32 addr = _cap.addr; \ + addr += offsetof(_struct, _field); \ + if (_cap.is_io) { \ + switch (sizeof(((_struct *)0)->_field)) { \ + case 8: \ + _var = inl(addr); \ + _var |= (u64)inl(addr+4) << 32; \ + break; \ + case 4: \ + _var = inl(addr); \ + break; \ + case 2: \ + _var = inw(addr); \ + break; \ + case 1: \ + _var = inb(addr); \ + break; \ + default: \ + _var = 0; \ + } \ + } else { \ + switch (sizeof(((_struct *)0)->_field)) { \ + case 8: \ + _var = readl((void*)addr); \ + _var |= (u64)readl((void*)(addr+4)) << 32; \ + break; \ + case 4: \ + _var = readl((void*)addr); \ + break; \ + case 2: \ + _var = readw((void*)addr); \ + break; \ + case 1: \ + _var = readb((void*)addr); \ + break; \ + default: \ + _var = 0; \ + } \ + } \ + dprintf(9, "vp read %x (%d) -> 0x%x\n", \ + addr, sizeof(((_struct *)0)->_field), (u32)_var); \ + } + +#define vp_modern_write(_cap, _struct, _field, _var) { \ + u32 addr = _cap.addr; \ + addr += offsetof(_struct, _field); \ + dprintf(9, "vp write %x (%d) <- 0x%x\n", \ + addr, sizeof(((_struct *)0)->_field), (u32)_var); \ + if (_cap.is_io) { \ + switch (sizeof(((_struct *)0)->_field)) { \ + case 4: \ + outl(_var, addr); \ + break; \ + case 2: \ + outw(_var, addr); \ + break; \ + case 1: \ + outb(_var, addr); \ + break; \ + } \ + } else { \ + switch (sizeof(((_struct *)0)->_field)) { \ + case 4: \ + writel((void*)addr, _var); \ + break; \ + case 2: \ + writew((void*)addr, _var); \ + break; \ + case 1: \ + writeb((void*)addr, _var); \ + break; \ + } \ + } \ + } + static inline u32 vp_get_features(struct vp_device *vp) { return inl(GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_HOST_FEATURES); -- 1.8.3.1
On Mon, Jun 29, 2015 at 10:53:29AM +0200, Gerd Hoffmann wrote:
Add macros to read/write registers of virtio-1.0 regions.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.h | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+)
diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index 893a7dd..e1d8b3e 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -111,6 +111,82 @@ struct vp_device { struct vp_cap common, notify, isr, device; };
+#define vp_modern_read(_cap, _struct, _field, _var) { \ + u32 addr = _cap.addr; \ + addr += offsetof(_struct, _field); \
Wouldn't this make more sense if the bulk of the code was in a function? That is, something like: extern u32 _vp_modern_read(struct vp_cap *cap, u32 offset, u8 size); #define vp_modern_read(_cap, _struct, _field, _var) { \ _var = _vp_modern_read(&_cap, offsetof(_struct, _field), sizeof((_struct *)0)->_field)
+ if (_cap.is_io) { \ + switch (sizeof(((_struct *)0)->_field)) { \ + case 8: \ + _var = inl(addr); \ + _var |= (u64)inl(addr+4) << 32; \ + break; \
It didn't look like there were any 64bit fields defined, but maybe I missed something. BTW, last I checked, gcc didn't do a good job with code generation when shifting 64bit values on a 32bit architecture. Using unions (eg, compiler.h:union u64_u32_u) seemed to produce better code. (Though, not worth bothering with if you want to use the same code in other projects.) -Kevin
On Mo, 2015-06-29 at 09:02 -0400, Kevin O'Connor wrote:
On Mon, Jun 29, 2015 at 10:53:29AM +0200, Gerd Hoffmann wrote:
Add macros to read/write registers of virtio-1.0 regions.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.h | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+)
diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index 893a7dd..e1d8b3e 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -111,6 +111,82 @@ struct vp_device { struct vp_cap common, notify, isr, device; };
+#define vp_modern_read(_cap, _struct, _field, _var) { \ + u32 addr = _cap.addr; \ + addr += offsetof(_struct, _field); \
Wouldn't this make more sense if the bulk of the code was in a function?
The idea is that 'sizeof((_struct *)0)->_field)' evaluates to a compile-time constant, so gcc can optimize away the bulk of the #define. Also we don't have to use u64 for _var then (except when it actually is a 64bit value). But having 'vp_modern_read(..., var)' in the code instead of 'var = vp_modern_read(...)' isn't that nice indeed.
+ if (_cap.is_io) { \ + switch (sizeof(((_struct *)0)->_field)) { \ + case 8: \ + _var = inl(addr); \ + _var |= (u64)inl(addr+4) << 32; \ + break; \
It didn't look like there were any 64bit fields defined, but maybe I missed something.
The is a single one (number of sectors for virtio-blk disks). cheers, Gerd
On Mon, Jun 29, 2015 at 03:46:54PM +0200, Gerd Hoffmann wrote:
On Mo, 2015-06-29 at 09:02 -0400, Kevin O'Connor wrote:
On Mon, Jun 29, 2015 at 10:53:29AM +0200, Gerd Hoffmann wrote:
Add macros to read/write registers of virtio-1.0 regions.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.h | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+)
diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index 893a7dd..e1d8b3e 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -111,6 +111,82 @@ struct vp_device { struct vp_cap common, notify, isr, device; };
+#define vp_modern_read(_cap, _struct, _field, _var) { \ + u32 addr = _cap.addr; \ + addr += offsetof(_struct, _field); \
Wouldn't this make more sense if the bulk of the code was in a function?
The idea is that 'sizeof((_struct *)0)->_field)' evaluates to a compile-time constant, so gcc can optimize away the bulk of the #define.
Would it matter though? Are any of these accesses performance sensitive? Also, if "_vp_modern_read()" was marked as inline wouldn't gcc ultimately produce the same thing?
Also we don't have to use u64 for _var then (except when it actually is a 64bit value).
But having 'vp_modern_read(..., var)' in the code instead of 'var = vp_modern_read(...)' isn't that nice indeed.
Hrmm. That's an interesting question - how well would gcc do if _vp_modern_read() was inline'd and returned a 64bit value that was then immediately truncated in the majority of cases. -Kevin
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-blk.c | 2 +- src/hw/virtio-pci.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/hw/virtio-pci.h | 12 +++--------- 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index 4f39aa2..9e56d42 100644 --- a/src/hw/virtio-blk.c +++ b/src/hw/virtio-blk.c @@ -123,7 +123,7 @@ init_virtio_blk(struct pci_device *pci) struct virtio_blk_config cfg; vp_get(vdrive->vp, 0, &cfg, sizeof(cfg)); - u32 f = vp_get_features(vdrive->vp); + u64 f = vp_get_features(vdrive->vp); vdrive->drive.blksize = (f & (1 << VIRTIO_BLK_F_BLK_SIZE)) ? cfg.blk_size : DISK_SECTOR_SIZE; diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c index 2abe601..68f1ded 100644 --- a/src/hw/virtio-pci.c +++ b/src/hw/virtio-pci.c @@ -24,6 +24,47 @@ #include "virtio-pci.h" #include "virtio-ring.h" +u64 vp_get_features(struct vp_device *vp) +{ + u32 f0, f1; + + if (vp->use_modern) { + vp_modern_write(vp->common, virtio_pci_common_cfg, + device_feature_select, 0); + vp_modern_read(vp->common, virtio_pci_common_cfg, + device_feature, f0); + vp_modern_write(vp->common, virtio_pci_common_cfg, + device_feature_select, 1); + vp_modern_read(vp->common, virtio_pci_common_cfg, + device_feature, f1); + } else { + f0 = inl(vp->ioaddr + VIRTIO_PCI_HOST_FEATURES); + f1 = 0; + } + return ((u64)f1 << 32) | f0; +} + +void vp_set_features(struct vp_device *vp, u64 features) +{ + u32 f0, f1; + + f0 = features; + f1 = features >> 32; + + if (vp->use_modern) { + vp_modern_write(vp->common, virtio_pci_common_cfg, + guest_feature_select, 0); + vp_modern_write(vp->common, virtio_pci_common_cfg, + guest_feature, f0); + vp_modern_write(vp->common, virtio_pci_common_cfg, + guest_feature_select, 1); + vp_modern_write(vp->common, virtio_pci_common_cfg, + guest_feature, f1); + } else { + outl(f0, vp->ioaddr + VIRTIO_PCI_GUEST_FEATURES); + } +} + int vp_find_vq(struct vp_device *vp, int queue_index, struct vring_virtqueue **p_vq) { diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index e1d8b3e..e275703 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -109,6 +109,7 @@ struct vp_cap { struct vp_device { unsigned int ioaddr; struct vp_cap common, notify, isr, device; + u8 use_modern; }; #define vp_modern_read(_cap, _struct, _field, _var) { \ @@ -187,15 +188,8 @@ struct vp_device { } \ } -static inline u32 vp_get_features(struct vp_device *vp) -{ - return inl(GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_HOST_FEATURES); -} - -static inline void vp_set_features(struct vp_device *vp, u32 features) -{ - outl(features, GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_GUEST_FEATURES); -} +u64 vp_get_features(struct vp_device *vp); +void vp_set_features(struct vp_device *vp, u64 features); static inline void vp_get(struct vp_device *vp, unsigned offset, void *buf, unsigned len) -- 1.8.3.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.c | 25 +++++++++++++++++++++++++ src/hw/virtio-pci.h | 13 ++----------- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c index 68f1ded..b414c71 100644 --- a/src/hw/virtio-pci.c +++ b/src/hw/virtio-pci.c @@ -65,6 +65,31 @@ void vp_set_features(struct vp_device *vp, u64 features) } } +u8 vp_get_status(struct vp_device *vp) +{ + u8 status; + + if (vp->use_modern) { + vp_modern_read(vp->common, virtio_pci_common_cfg, + device_status, status); + } else { + status = inb(vp->ioaddr + VIRTIO_PCI_STATUS); + } + return status; +} + +void vp_set_status(struct vp_device *vp, u8 status) +{ + if (status == 0) /* reset */ + return; + if (vp->use_modern) { + vp_modern_write(vp->common, virtio_pci_common_cfg, + device_status, status); + } else { + outb(status, vp->ioaddr + VIRTIO_PCI_STATUS); + } +} + int vp_find_vq(struct vp_device *vp, int queue_index, struct vring_virtqueue **p_vq) { diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index e275703..079aa53 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -202,17 +202,8 @@ static inline void vp_get(struct vp_device *vp, unsigned offset, ptr[i] = inb(ioaddr + VIRTIO_PCI_CONFIG + offset + i); } -static inline u8 vp_get_status(struct vp_device *vp) -{ - return inb(GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_STATUS); -} - -static inline void vp_set_status(struct vp_device *vp, u8 status) -{ - if (status == 0) /* reset */ - return; - outb(status, GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_STATUS); -} +u8 vp_get_status(struct vp_device *vp); +void vp_set_status(struct vp_device *vp, u8 status); static inline u8 vp_get_isr(struct vp_device *vp) { -- 1.8.3.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.c | 12 ++++++++++++ src/hw/virtio-pci.h | 6 +----- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c index b414c71..481b365 100644 --- a/src/hw/virtio-pci.c +++ b/src/hw/virtio-pci.c @@ -90,6 +90,18 @@ void vp_set_status(struct vp_device *vp, u8 status) } } +u8 vp_get_isr(struct vp_device *vp) +{ + u8 isr; + + if (vp->use_modern) { + vp_modern_read(vp->isr, virtio_pci_isr, isr, isr); + } else { + isr = inb(vp->ioaddr + VIRTIO_PCI_ISR); + } + return isr; +} + int vp_find_vq(struct vp_device *vp, int queue_index, struct vring_virtqueue **p_vq) { diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index 079aa53..c9e5a21 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -204,11 +204,7 @@ static inline void vp_get(struct vp_device *vp, unsigned offset, u8 vp_get_status(struct vp_device *vp); void vp_set_status(struct vp_device *vp, u8 status); - -static inline u8 vp_get_isr(struct vp_device *vp) -{ - return inb(GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_ISR); -} +u8 vp_get_isr(struct vp_device *vp); static inline void vp_reset(struct vp_device *vp) { -- 1.8.3.1
On Mon, Jun 29, 2015 at 10:53:32AM +0200, Gerd Hoffmann wrote:
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.c | 12 ++++++++++++ src/hw/virtio-pci.h | 6 +----- 2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c index b414c71..481b365 100644 --- a/src/hw/virtio-pci.c +++ b/src/hw/virtio-pci.c @@ -90,6 +90,18 @@ void vp_set_status(struct vp_device *vp, u8 status) } }
+u8 vp_get_isr(struct vp_device *vp) +{ + u8 isr; + + if (vp->use_modern) { + vp_modern_read(vp->isr, virtio_pci_isr, isr, isr); + } else { + isr = inb(vp->ioaddr + VIRTIO_PCI_ISR); + } + return isr; +}
How about renaming "use_modern" to something more descriptive - like "use_abi1"? Also, couldn't vp_modern_read just be renamed to vp_read. BTW, out of curiosity, did you consider retroactively making ABIv0 structs and using vp_read() for both the new and old cases? I'm not sure it would save any code, but mixing the struct/offset/sizeof method with the inb/define method seems a little awkward. -Kevin
Hi,
+u8 vp_get_isr(struct vp_device *vp) +{ + u8 isr; + + if (vp->use_modern) { + vp_modern_read(vp->isr, virtio_pci_isr, isr, isr); + } else { + isr = inb(vp->ioaddr + VIRTIO_PCI_ISR); + } + return isr; +}
How about renaming "use_modern" to something more descriptive - like "use_abi1"? Also, couldn't vp_modern_read just be renamed to vp_read.
In both qemu and linux kernel "legacy" and "modern" are used for the two interface revisions (0.9.5 and 1.0), and I'd prefer to stay consistent with that.
BTW, out of curiosity, did you consider retroactively making ABIv0 structs and using vp_read() for both the new and old cases? I'm not sure it would save any code, but mixing the struct/offset/sizeof method with the inb/define method seems a little awkward.
Makes sense to do that in indeed (and of course naming it vp_modern_read looks odd then ;) I'll look into that for v2. cheers, Gerd
On Mon, Jun 29, 2015 at 04:33:54PM +0200, Gerd Hoffmann wrote:
Hi,
+u8 vp_get_isr(struct vp_device *vp) +{ + u8 isr; + + if (vp->use_modern) { + vp_modern_read(vp->isr, virtio_pci_isr, isr, isr); + } else { + isr = inb(vp->ioaddr + VIRTIO_PCI_ISR); + } + return isr; +}
How about renaming "use_modern" to something more descriptive - like "use_abi1"? Also, couldn't vp_modern_read just be renamed to vp_read.
In both qemu and linux kernel "legacy" and "modern" are used for the two interface revisions (0.9.5 and 1.0), and I'd prefer to stay consistent with that.
Okay - that makes sense. Thanks, -Kevin
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.c | 16 ++++++++++++++++ src/hw/virtio-pci.h | 9 +-------- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c index 481b365..d2d06c5 100644 --- a/src/hw/virtio-pci.c +++ b/src/hw/virtio-pci.c @@ -102,6 +102,22 @@ u8 vp_get_isr(struct vp_device *vp) return isr; } +void vp_reset(struct vp_device *vp) +{ + u8 isr; + + if (vp->use_modern) { + vp_modern_write(vp->common, virtio_pci_common_cfg, + device_status, 0); + vp_modern_read(vp->isr, virtio_pci_isr, + isr, isr); + } else { + outb(0, vp->ioaddr + VIRTIO_PCI_STATUS); + isr = inb(vp->ioaddr + VIRTIO_PCI_ISR); + } + (void)isr; +} + int vp_find_vq(struct vp_device *vp, int queue_index, struct vring_virtqueue **p_vq) { diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index c9e5a21..608f492 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -205,14 +205,7 @@ static inline void vp_get(struct vp_device *vp, unsigned offset, u8 vp_get_status(struct vp_device *vp); void vp_set_status(struct vp_device *vp, u8 status); u8 vp_get_isr(struct vp_device *vp); - -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); -} +void vp_reset(struct vp_device *vp); static inline void vp_notify(struct vp_device *vp, int queue_index) { -- 1.8.3.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.c | 22 +++++++++++++++++++++- src/hw/virtio-pci.h | 7 ++----- src/hw/virtio-ring.c | 2 +- src/hw/virtio-ring.h | 1 + 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c index d2d06c5..8097901 100644 --- a/src/hw/virtio-pci.c +++ b/src/hw/virtio-pci.c @@ -118,6 +118,24 @@ void vp_reset(struct vp_device *vp) (void)isr; } +void vp_notify(struct vp_device *vp, struct vring_virtqueue *vq) +{ + if (vp->use_modern) { + u32 addr = vp->notify.addr + + vq->queue_notify_off * + vp->notify_off_multiplier; + if (vp->notify.is_io) { + outw(vq->queue_index, addr); + } else { + writew((void*)addr, vq->queue_index); + } + dprintf(9, "vp notify %x (%d) -- 0x%x\n", + addr, 2, vq->queue_index); + } else { + outw(vq->queue_index, vp->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY); + } +} + int vp_find_vq(struct vp_device *vp, int queue_index, struct vring_virtqueue **p_vq) { @@ -184,7 +202,7 @@ struct vp_device *vp_init_simple(struct pci_device *pci) struct vp_device *vp = malloc_low(sizeof(*vp)); u8 cap = pci_find_capability(pci, PCI_CAP_ID_VNDR, 0); struct vp_cap *vp_cap; - u32 addr, offset; + u32 addr, offset, mul; u8 type; memset(vp, 0, sizeof(*vp)); @@ -197,6 +215,8 @@ struct vp_device *vp_init_simple(struct pci_device *pci) break; case VIRTIO_PCI_CAP_NOTIFY_CFG: vp_cap = &vp->notify; + mul = offsetof(struct virtio_pci_notify_cap, notify_off_multiplier); + vp->notify_off_multiplier = pci_config_readl(pci->bdf, cap + mul); break; case VIRTIO_PCI_CAP_ISR_CFG: vp_cap = &vp->isr; diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index 608f492..495c0aa 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -109,6 +109,7 @@ struct vp_cap { struct vp_device { unsigned int ioaddr; struct vp_cap common, notify, isr, device; + u32 notify_off_multiplier; u8 use_modern; }; @@ -207,11 +208,6 @@ void vp_set_status(struct vp_device *vp, u8 status); u8 vp_get_isr(struct vp_device *vp); void vp_reset(struct vp_device *vp); -static inline void vp_notify(struct vp_device *vp, int queue_index) -{ - outw(queue_index, GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_QUEUE_NOTIFY); -} - static inline void vp_del_vq(struct vp_device *vp, int queue_index) { int ioaddr = GET_LOWFLAT(vp->ioaddr); @@ -226,6 +222,7 @@ static inline void vp_del_vq(struct vp_device *vp, int queue_index) struct pci_device; struct vring_virtqueue; struct vp_device *vp_init_simple(struct pci_device *pci); +void vp_notify(struct vp_device *vp, struct vring_virtqueue *vq); 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 5c6a32e..6c86c38 100644 --- a/src/hw/virtio-ring.c +++ b/src/hw/virtio-ring.c @@ -145,5 +145,5 @@ void vring_kick(struct vp_device *vp, struct vring_virtqueue *vq, int num_added) smp_wmb(); SET_LOWFLAT(avail->idx, GET_LOWFLAT(avail->idx) + num_added); - vp_notify(vp, GET_LOWFLAT(vq->queue_index)); + vp_notify(vp, vq); } diff --git a/src/hw/virtio-ring.h b/src/hw/virtio-ring.h index 553a508..7df9004 100644 --- a/src/hw/virtio-ring.h +++ b/src/hw/virtio-ring.h @@ -88,6 +88,7 @@ struct vring_virtqueue { u16 vdata[MAX_QUEUE_NUM]; /* PCI */ int queue_index; + int queue_notify_off; }; struct vring_list { -- 1.8.3.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.h | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index 495c0aa..468eb6f 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -208,17 +208,6 @@ void vp_set_status(struct vp_device *vp, u8 status); u8 vp_get_isr(struct vp_device *vp); void vp_reset(struct vp_device *vp); -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 pci_device; struct vring_virtqueue; struct vp_device *vp_init_simple(struct pci_device *pci); -- 1.8.3.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.c | 61 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 47 insertions(+), 14 deletions(-) diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c index 8097901..c80d38d 100644 --- a/src/hw/virtio-pci.c +++ b/src/hw/virtio-pci.c @@ -139,7 +139,6 @@ void vp_notify(struct vp_device *vp, struct vring_virtqueue *vq) 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(); @@ -150,34 +149,49 @@ int vp_find_vq(struct vp_device *vp, int queue_index, } memset(vq, 0, sizeof(*vq)); + /* select the queue */ - - outw(queue_index, ioaddr + VIRTIO_PCI_QUEUE_SEL); + if (vp->use_modern) { + vp_modern_write(vp->common, virtio_pci_common_cfg, + queue_select, queue_index); + } else { + outw(queue_index, vp->ioaddr + VIRTIO_PCI_QUEUE_SEL); + } /* check if the queue is available */ - - num = inw(ioaddr + VIRTIO_PCI_QUEUE_NUM); + if (vp->use_modern) { + vp_modern_read(vp->common, virtio_pci_common_cfg, + queue_size, num); + if (num > MAX_QUEUE_NUM) { + vp_modern_write(vp->common, virtio_pci_common_cfg, + queue_size, MAX_QUEUE_NUM); + vp_modern_read(vp->common, virtio_pci_common_cfg, + queue_size, num); + } + } else { + num = inw(vp->ioaddr + VIRTIO_PCI_QUEUE_NUM); + } if (!num) { dprintf(1, "ERROR: queue size is 0\n"); goto fail; } - if (num > MAX_QUEUE_NUM) { dprintf(1, "ERROR: queue size %d > %d\n", num, MAX_QUEUE_NUM); goto fail; } /* check if the queue is already active */ - - if (inl(ioaddr + VIRTIO_PCI_QUEUE_PFN)) { - dprintf(1, "ERROR: queue already active\n"); - goto fail; + if (vp->use_modern) { + /* WIP */ + } else { + if (inl(vp->ioaddr + VIRTIO_PCI_QUEUE_PFN)) { + dprintf(1, "ERROR: queue already active\n"); + goto fail; + } } - vq->queue_index = queue_index; /* initialize the queue */ - struct vring * vr = &vq->vring; vring_init(vr, num, (unsigned char*)&vq->queue); @@ -186,8 +200,27 @@ int vp_find_vq(struct vp_device *vp, int queue_index, * NOTE: vr->desc is initialized by vring_init() */ - outl((unsigned long)virt_to_phys(vr->desc) >> PAGE_SHIFT, - ioaddr + VIRTIO_PCI_QUEUE_PFN); + if (vp->use_modern) { + vp_modern_write(vp->common, virtio_pci_common_cfg, + queue_desc_lo, (unsigned long)virt_to_phys(vr->desc)); + vp_modern_write(vp->common, virtio_pci_common_cfg, + queue_desc_hi, 0); + vp_modern_write(vp->common, virtio_pci_common_cfg, + queue_avail_lo, (unsigned long)virt_to_phys(vr->avail)); + vp_modern_write(vp->common, virtio_pci_common_cfg, + queue_avail_hi, 0); + vp_modern_write(vp->common, virtio_pci_common_cfg, + queue_used_lo, (unsigned long)virt_to_phys(vr->used)); + vp_modern_write(vp->common, virtio_pci_common_cfg, + queue_used_hi, 0); + vp_modern_write(vp->common, virtio_pci_common_cfg, + queue_enable, 1); + vp_modern_read(vp->common, virtio_pci_common_cfg, + queue_notify_off, vq->queue_notify_off); + } else { + outl((unsigned long)virt_to_phys(vr->desc) >> PAGE_SHIFT, + vp->ioaddr + VIRTIO_PCI_QUEUE_PFN); + } return num; -- 1.8.3.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-scsi.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c index b208d49..3ee0f49 100644 --- a/src/hw/virtio-scsi.c +++ b/src/hw/virtio-scsi.c @@ -146,14 +146,35 @@ init_virtio_scsi(struct pci_device *pci) pci_bdf_to_dev(bdf)); struct vring_virtqueue *vq = NULL; struct vp_device *vp = vp_init_simple(pci); + u8 status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER; + + if (vp->use_modern) { + u64 features = vp_get_features(vp); + u64 version1 = 1ull << VIRTIO_F_VERSION_1; + if (!(features & version1)) { + dprintf(1, "modern device without virtio_1 feature bit: %x:%x\n", + pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); + goto fail; + } + + vp_set_features(vp, version1); + status |= VIRTIO_CONFIG_S_FEATURES_OK; + vp_set_status(vp, status); + if (!(vp_get_status(vp) & VIRTIO_CONFIG_S_FEATURES_OK)) { + dprintf(1, "device didn't accept features: %x:%x\n", + pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); + goto fail; + } + } + 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(vp, VIRTIO_CONFIG_S_ACKNOWLEDGE | - VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK); + status |= VIRTIO_CONFIG_S_DRIVER_OK; + vp_set_status(vp, status); int i, tot; for (tot = 0, i = 0; i < 256; i++) -- 1.8.3.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-blk.c | 85 +++++++++++++++++++++++++++++++++++++++++------------ src/hw/virtio-pci.h | 13 ++++---- 2 files changed, 72 insertions(+), 26 deletions(-) diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index 9e56d42..b7c24f8 100644 --- a/src/hw/virtio-blk.c +++ b/src/hw/virtio-blk.c @@ -102,6 +102,7 @@ static void init_virtio_blk(struct pci_device *pci) { u16 bdf = pci->bdf; + u8 status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER; dprintf(1, "found virtio-blk at %x:%x\n", pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); struct virtiodrive_s *vdrive = malloc_fseg(sizeof(*vdrive)); @@ -113,42 +114,88 @@ init_virtio_blk(struct pci_device *pci) vdrive->drive.type = DTYPE_VIRTIO_BLK; vdrive->drive.cntl_id = bdf; - vdrive->vp = vp_init_simple(pci); + struct vp_device *vp = vdrive->vp = vp_init_simple(pci); 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(vdrive->vp, 0, &cfg, sizeof(cfg)); + if (vp->use_modern) { + u64 features = vp_get_features(vp); + u64 version1 = 1ull << VIRTIO_F_VERSION_1; + u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE; + if (!(features & version1)) { + dprintf(1, "modern device without virtio_1 feature bit: %x:%x\n", + pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); + goto fail; + } - u64 f = vp_get_features(vdrive->vp); - vdrive->drive.blksize = (f & (1 << VIRTIO_BLK_F_BLK_SIZE)) ? - cfg.blk_size : DISK_SECTOR_SIZE; + features = features & (version1 | blk_size); + vp_set_features(vp, features); + status |= VIRTIO_CONFIG_S_FEATURES_OK; + vp_set_status(vp, status); + if (!(vp_get_status(vp) & VIRTIO_CONFIG_S_FEATURES_OK)) { + dprintf(1, "device didn't accept features: %x:%x\n", + pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); + goto fail; + } - vdrive->drive.sectors = cfg.capacity; - dprintf(3, "virtio-blk %x:%x blksize=%d sectors=%u\n", - pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), - vdrive->drive.blksize, (u32)vdrive->drive.sectors); + vp_modern_read(vp->device, struct virtio_blk_config, + capacity, vdrive->drive.sectors); + if (features & blk_size) { + vp_modern_read(vp->device, struct virtio_blk_config, + blk_size, vdrive->drive.blksize); + } else { + vdrive->drive.blksize = DISK_SECTOR_SIZE; + } + if (vdrive->drive.blksize != DISK_SECTOR_SIZE) { + dprintf(1, "virtio-blk %x:%x block size %d is unsupported\n", + pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), + vdrive->drive.blksize); + goto fail; + } + dprintf(3, "virtio-blk %x:%x blksize=%d sectors=%u\n", + pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), + vdrive->drive.blksize, (u32)vdrive->drive.sectors); + + vp_modern_read(vp->device, struct virtio_blk_config, + cylinders, vdrive->drive.pchs.cylinder); + vp_modern_read(vp->device, struct virtio_blk_config, + heads, vdrive->drive.pchs.head); + vp_modern_read(vp->device, struct virtio_blk_config, + sectors, vdrive->drive.pchs.sector); + } else { + struct virtio_blk_config cfg; + vp_get_legacy(vdrive->vp, 0, &cfg, sizeof(cfg)); - if (vdrive->drive.blksize != DISK_SECTOR_SIZE) { - dprintf(1, "virtio-blk %x:%x block size %d is unsupported\n", + u64 f = vp_get_features(vdrive->vp); + vdrive->drive.blksize = (f & (1 << VIRTIO_BLK_F_BLK_SIZE)) ? + cfg.blk_size : DISK_SECTOR_SIZE; + + vdrive->drive.sectors = cfg.capacity; + dprintf(3, "virtio-blk %x:%x blksize=%d sectors=%u\n", pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), - vdrive->drive.blksize); - goto fail; + vdrive->drive.blksize, (u32)vdrive->drive.sectors); + + if (vdrive->drive.blksize != DISK_SECTOR_SIZE) { + dprintf(1, "virtio-blk %x:%x block size %d is unsupported\n", + pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), + vdrive->drive.blksize); + goto fail; + } + vdrive->drive.pchs.cylinder = cfg.cylinders; + vdrive->drive.pchs.head = cfg.heads; + vdrive->drive.pchs.sector = cfg.sectors; } - vdrive->drive.pchs.cylinder = cfg.cylinders; - vdrive->drive.pchs.head = cfg.heads; - vdrive->drive.pchs.sector = cfg.sectors; char *desc = znprintf(MAXDESCSIZE, "Virtio disk PCI:%x:%x", pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); boot_add_hd(&vdrive->drive, desc, bootprio_find_pci_device(pci)); - vp_set_status(vdrive->vp, VIRTIO_CONFIG_S_ACKNOWLEDGE | - VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK); + status |= VIRTIO_CONFIG_S_DRIVER_OK; + vp_set_status(vdrive->vp, status); return; fail: diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h index 468eb6f..29ab775 100644 --- a/src/hw/virtio-pci.h +++ b/src/hw/virtio-pci.h @@ -192,15 +192,14 @@ struct vp_device { u64 vp_get_features(struct vp_device *vp); void vp_set_features(struct vp_device *vp, u64 features); -static inline void vp_get(struct vp_device *vp, unsigned offset, - void *buf, unsigned len) +static inline void vp_get_legacy(struct vp_device *vp, unsigned offset, + void *buf, unsigned len) { - int ioaddr = GET_LOWFLAT(vp->ioaddr); - u8 *ptr = buf; - unsigned i; + u8 *ptr = buf; + unsigned i; - for (i = 0; i < len; i++) - ptr[i] = inb(ioaddr + VIRTIO_PCI_CONFIG + offset + i); + for (i = 0; i < len; i++) + ptr[i] = inb(vp->ioaddr + VIRTIO_PCI_CONFIG + offset + i); } u8 vp_get_status(struct vp_device *vp); -- 1.8.3.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c index c80d38d..808e102 100644 --- a/src/hw/virtio-pci.c +++ b/src/hw/virtio-pci.c @@ -287,13 +287,16 @@ struct vp_device *vp_init_simple(struct pci_device *pci) } if (vp->common.cap && vp->notify.cap && vp->isr.cap && vp->device.cap) { - dprintf(1, "pci dev %x:%x supports virtio 1.0\n", + dprintf(1, "pci dev %x:%x using virtio 1.0 mode\n", pci_bdf_to_bus(pci->bdf), pci_bdf_to_dev(pci->bdf)); + vp->use_modern = 1; + } else { + dprintf(1, "pci dev %x:%x using legacy virtio mode\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; } - vp->ioaddr = pci_config_readl(pci->bdf, PCI_BASE_ADDRESS_0) & - PCI_BASE_ADDRESS_IO_MASK; - vp_reset(vp); pci_config_maskw(pci->bdf, PCI_COMMAND, 0, PCI_COMMAND_MASTER); vp_set_status(vp, VIRTIO_CONFIG_S_ACKNOWLEDGE | -- 1.8.3.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/pci_ids.h | 8 ++++++-- src/hw/virtio-blk.c | 5 +++-- src/hw/virtio-scsi.c | 5 +++-- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/hw/pci_ids.h b/src/hw/pci_ids.h index 1cd4f72..cdf9b3c 100644 --- a/src/hw/pci_ids.h +++ b/src/hw/pci_ids.h @@ -2616,8 +2616,12 @@ #define PCI_DEVICE_ID_RME_DIGI32_8 0x9898 #define PCI_VENDOR_ID_REDHAT_QUMRANET 0x1af4 -#define PCI_DEVICE_ID_VIRTIO_BLK 0x1001 -#define PCI_DEVICE_ID_VIRTIO_SCSI 0x1004 +/* virtio 0.9.5 ids (legacy/transitional devices) */ +#define PCI_DEVICE_ID_VIRTIO_BLK_09 0x1001 +#define PCI_DEVICE_ID_VIRTIO_SCSI_09 0x1004 +/* virtio 1.0 ids (modern devices) */ +#define PCI_DEVICE_ID_VIRTIO_BLK_10 0x1042 +#define PCI_DEVICE_ID_VIRTIO_SCSI_10 0x1048 #define PCI_VENDOR_ID_VMWARE 0x15ad #define PCI_DEVICE_ID_VMWARE_PVSCSI 0x07C0 diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index b7c24f8..24eba2a 100644 --- a/src/hw/virtio-blk.c +++ b/src/hw/virtio-blk.c @@ -216,8 +216,9 @@ virtio_blk_setup(void) struct pci_device *pci; foreachpci(pci) { - if (pci->vendor != PCI_VENDOR_ID_REDHAT_QUMRANET - || pci->device != PCI_DEVICE_ID_VIRTIO_BLK) + if (pci->vendor != PCI_VENDOR_ID_REDHAT_QUMRANET || + (pci->device != PCI_DEVICE_ID_VIRTIO_BLK_09 && + pci->device != PCI_DEVICE_ID_VIRTIO_BLK_10)) continue; init_virtio_blk(pci); } diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c index 3ee0f49..946fe5f 100644 --- a/src/hw/virtio-scsi.c +++ b/src/hw/virtio-scsi.c @@ -202,8 +202,9 @@ virtio_scsi_setup(void) struct pci_device *pci; foreachpci(pci) { - if (pci->vendor != PCI_VENDOR_ID_REDHAT_QUMRANET - || pci->device != PCI_DEVICE_ID_VIRTIO_SCSI) + if (pci->vendor != PCI_VENDOR_ID_REDHAT_QUMRANET || + (pci->device != PCI_DEVICE_ID_VIRTIO_SCSI_09 && + pci->device != PCI_DEVICE_ID_VIRTIO_SCSI_10)) continue; init_virtio_scsi(pci); } -- 1.8.3.1
On Mon, Jun 29, 2015 at 10:53:22AM +0200, Gerd Hoffmann wrote:
Hi,
This patch series adds virtio 1.0 support to the virtio blk and scsi drivers in seabios. With this series applied seabios happily boots in virtio 1.0 mode from both transitional and modern devices.
Thanks. The series looks good to me. (I sent a couple of comments separately, but they are all minor.) -Kevin
participants (3)
-
Gerd Hoffmann -
Kevin O'Connor -
Paolo Bonzini