1. Check if blk_size is valid in virtio_blk config. 2. Disable interrupt otherwise interrupt may stuck with some guests.
Signed-off-by: Gleb Natapov gleb@redhat.com
ChangeLog: v1->v2: - Treat sector size not equal to 512 bytes as error.
diff --git a/src/virtio-blk.c b/src/virtio-blk.c index 7cc2edb..e6167e9 100644 --- a/src/virtio-blk.c +++ b/src/virtio-blk.c @@ -127,23 +127,34 @@ virtio_blk_setup(void) VIRTIO_CONFIG_S_DRIVER );
if (vp_find_vq(ioaddr, 0, vdrive_g->vq) < 0 ) { + dprintf(1, "fail to find vq for virtio-blk %x:%x\n", + pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); + next: free(vdrive_g); free(desc); free(vq); - dprintf(1, "fail to find vq for virtio-blk %x:%x\n", - pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); continue; }
struct virtio_blk_config cfg; vp_get(ioaddr, 0, &cfg, sizeof(cfg));
- vdrive_g->drive.blksize = cfg.blk_size; + u32 f = vp_get_features(ioaddr); + vdrive_g->drive.blksize = (f & (1 << VIRTIO_BLK_F_BLK_SIZE)) ? + cfg.blk_size : DISK_SECTOR_SIZE; + vdrive_g->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_g->drive.blksize, (u32)vdrive_g->drive.sectors);
+ if (vdrive_g->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_g->drive.blksize); + goto next; + } + vdrive_g->drive.pchs.cylinders = cfg.cylinders; vdrive_g->drive.pchs.heads = cfg.heads; vdrive_g->drive.pchs.spt = cfg.sectors; diff --git a/src/virtio-blk.h b/src/virtio-blk.h index 8095d5b..7243704 100644 --- a/src/virtio-blk.h +++ b/src/virtio-blk.h @@ -16,6 +16,8 @@ struct virtio_blk_config u32 opt_io_size; } __attribute__((packed));
+#define VIRTIO_BLK_F_BLK_SIZE 6 + /* These two define direction. */ #define VIRTIO_BLK_T_IN 0 #define VIRTIO_BLK_T_OUT 1 diff --git a/src/virtio-ring.h b/src/virtio-ring.h index 3fb86fe..014defc 100644 --- a/src/virtio-ring.h +++ b/src/virtio-ring.h @@ -105,6 +105,8 @@ static inline void vring_init(struct vring *vr, vr->desc = phys_to_virt(pa);
vr->avail = (struct vring_avail *)&vr->desc[num]; + /* disable interrupts */ + vr->avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
/* physical address of used must be page aligned */
-- Gleb.
On Mon, May 17, 2010 at 04:27:27PM +0300, Gleb Natapov wrote:
- Check if blk_size is valid in virtio_blk config.
- Disable interrupt otherwise interrupt may stuck with some guests.
Signed-off-by: Gleb Natapov gleb@redhat.com
Thanks. I committed your change to my local repo. I don't much like that goto though - is it okay if we break up virtio_blk_setup with the patch below (on top of your patch)?
-Kevin
diff --git a/src/virtio-blk.c b/src/virtio-blk.c index e6167e9..7f9b3d2 100644 --- a/src/virtio-blk.c +++ b/src/virtio-blk.c @@ -85,6 +85,79 @@ process_virtio_op(struct disk_op_s *op) } }
+static void +init_virtio_blk(u16 bdf) +{ + dprintf(1, "found virtio-blk at %x:%x\n", pci_bdf_to_bus(bdf), + pci_bdf_to_dev(bdf)); + char *desc = malloc_tmphigh(MAXDESCSIZE); + struct virtiodrive_s *vdrive_g = malloc_fseg(sizeof(*vdrive_g)); + struct vring_virtqueue *vq = memalign_low(PAGE_SIZE, sizeof(*vq)); + if (!vdrive_g || !desc || !vq) { + warn_noalloc(); + goto fail; + } + memset(vdrive_g, 0, sizeof(*vdrive_g)); + vdrive_g->drive.type = DTYPE_VIRTIO; + vdrive_g->drive.cntl_id = bdf; + vdrive_g->vq = vq; + + u16 ioaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0) & + PCI_BASE_ADDRESS_IO_MASK; + + vdrive_g->ioaddr = ioaddr; + + vp_reset(ioaddr); + vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE | + VIRTIO_CONFIG_S_DRIVER ); + + if (vp_find_vq(ioaddr, 0, vdrive_g->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)); + + u32 f = vp_get_features(ioaddr); + vdrive_g->drive.blksize = (f & (1 << VIRTIO_BLK_F_BLK_SIZE)) ? + cfg.blk_size : DISK_SECTOR_SIZE; + + vdrive_g->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_g->drive.blksize, (u32)vdrive_g->drive.sectors); + + if (vdrive_g->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_g->drive.blksize); + goto fail; + } + + vdrive_g->drive.pchs.cylinders = cfg.cylinders; + vdrive_g->drive.pchs.heads = cfg.heads; + vdrive_g->drive.pchs.spt = cfg.sectors; + + setup_translation(&vdrive_g->drive); + add_bcv_internal(&vdrive_g->drive); + + snprintf(desc, MAXDESCSIZE, "Virtio disk PCI:%x:%x", + pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); + + vdrive_g->drive.desc = desc; + + vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE | + VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK); + return; + +fail: + free(vdrive_g); + free(desc); + free(vq); +} + void virtio_blk_setup(void) { @@ -100,74 +173,6 @@ virtio_blk_setup(void) u32 v = pci_config_readl(bdf, PCI_VENDOR_ID); if (v != id) continue; - dprintf(3, "found virtio-blk at %x:%x\n", pci_bdf_to_bus(bdf), - pci_bdf_to_dev(bdf)); - char *desc = malloc_tmphigh(MAXDESCSIZE); - struct virtiodrive_s *vdrive_g = malloc_fseg(sizeof(*vdrive_g)); - struct vring_virtqueue *vq = memalign_low(PAGE_SIZE, sizeof(*vq)); - if (!vdrive_g || !desc || !vq) { - free(vdrive_g); - free(desc); - free(vq); - warn_noalloc(); - return; - } - memset(vdrive_g, 0, sizeof(*vdrive_g)); - vdrive_g->drive.type = DTYPE_VIRTIO; - vdrive_g->drive.cntl_id = bdf; - vdrive_g->vq = vq; - - u16 ioaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0) & - PCI_BASE_ADDRESS_IO_MASK; - - vdrive_g->ioaddr = ioaddr; - - vp_reset(ioaddr); - vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE | - VIRTIO_CONFIG_S_DRIVER ); - - if (vp_find_vq(ioaddr, 0, vdrive_g->vq) < 0 ) { - dprintf(1, "fail to find vq for virtio-blk %x:%x\n", - pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); - next: - free(vdrive_g); - free(desc); - free(vq); - continue; - } - - struct virtio_blk_config cfg; - vp_get(ioaddr, 0, &cfg, sizeof(cfg)); - - u32 f = vp_get_features(ioaddr); - vdrive_g->drive.blksize = (f & (1 << VIRTIO_BLK_F_BLK_SIZE)) ? - cfg.blk_size : DISK_SECTOR_SIZE; - - vdrive_g->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_g->drive.blksize, (u32)vdrive_g->drive.sectors); - - if (vdrive_g->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_g->drive.blksize); - goto next; - } - - vdrive_g->drive.pchs.cylinders = cfg.cylinders; - vdrive_g->drive.pchs.heads = cfg.heads; - vdrive_g->drive.pchs.spt = cfg.sectors; - - setup_translation(&vdrive_g->drive); - add_bcv_internal(&vdrive_g->drive); - - snprintf(desc, MAXDESCSIZE, "Virtio disk PCI:%x:%x", - pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); - - vdrive_g->drive.desc = desc; - - vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE | - VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK); + init_virtio_blk(bdf); } }
On Mon, May 17, 2010 at 07:27:30PM -0400, Kevin O'Connor wrote:
On Mon, May 17, 2010 at 04:27:27PM +0300, Gleb Natapov wrote:
- Check if blk_size is valid in virtio_blk config.
- Disable interrupt otherwise interrupt may stuck with some guests.
Signed-off-by: Gleb Natapov gleb@redhat.com
Thanks. I committed your change to my local repo. I don't much like that goto though - is it okay if we break up virtio_blk_setup with the patch below (on top of your patch)?
Yes, but the previous code bailed out from virtio_blk_setup() completely if allocation failed. What is the point to continue trying to allocate same amount of memory again and again. You patch changes this. What about making init_virtio_blk() return -1 on failure and check return value in virtio_blk_setup()?
-- Gleb.
On Tue, May 18, 2010 at 08:19:03AM +0300, Gleb Natapov wrote:
On Mon, May 17, 2010 at 07:27:30PM -0400, Kevin O'Connor wrote:
On Mon, May 17, 2010 at 04:27:27PM +0300, Gleb Natapov wrote:
- Check if blk_size is valid in virtio_blk config.
- Disable interrupt otherwise interrupt may stuck with some guests.
Signed-off-by: Gleb Natapov gleb@redhat.com
Thanks. I committed your change to my local repo. I don't much like that goto though - is it okay if we break up virtio_blk_setup with the patch below (on top of your patch)?
Yes, but the previous code bailed out from virtio_blk_setup() completely if allocation failed. What is the point to continue trying to allocate same amount of memory again and again. You patch changes this.
I think I'd prefer to have the debug report for every discovered device. (So that I knew all the devices my memory problem was preventing me from using.)
What about making init_virtio_blk() return -1 on failure and check return value in virtio_blk_setup()?
I don't think a sane setup would lead to a malloc call failing, but if you would like to see this another way, that's fine - just send a patch.
-Kevin
On Tue, May 18, 2010 at 07:30:00PM -0400, Kevin O'Connor wrote:
On Tue, May 18, 2010 at 08:19:03AM +0300, Gleb Natapov wrote:
On Mon, May 17, 2010 at 07:27:30PM -0400, Kevin O'Connor wrote:
On Mon, May 17, 2010 at 04:27:27PM +0300, Gleb Natapov wrote:
- Check if blk_size is valid in virtio_blk config.
- Disable interrupt otherwise interrupt may stuck with some guests.
Signed-off-by: Gleb Natapov gleb@redhat.com
Thanks. I committed your change to my local repo. I don't much like that goto though - is it okay if we break up virtio_blk_setup with the patch below (on top of your patch)?
Yes, but the previous code bailed out from virtio_blk_setup() completely if allocation failed. What is the point to continue trying to allocate same amount of memory again and again. You patch changes this.
I think I'd prefer to have the debug report for every discovered device. (So that I knew all the devices my memory problem was preventing me from using.)
What about making init_virtio_blk() return -1 on failure and check return value in virtio_blk_setup()?
I don't think a sane setup would lead to a malloc call failing, but if you would like to see this another way, that's fine - just send a patch.
Nah, if you like it that way, let it be.
-- Gleb.
-
Gleb Natapov -
Kevin O'Connor