* Sasha Levin <levinsasha928@xxxxxxxxx> wrote: > Add support for multiple blk_devices by un-globalizing > the current blk_device and allow multiple blk_devices. Very nice! One nit: > + mpc_intsrc = last_addr; > + mpc_intsrc->type = MP_INTSRC; > + mpc_intsrc->irqtype = mp_INT; > + mpc_intsrc->irqflag = MP_IRQDIR_DEFAULT; > + mpc_intsrc->srcbus = pcibusid; > + mpc_intsrc->srcbusirq = 1; /* virtio block irq pin */ > + mpc_intsrc->dstapic = ioapicid; > + mpc_intsrc->dstirq = 10; /* VIRTIO_BLK_IRQ */ > + > + last_addr = (void *)&mpc_intsrc[1]; > + nentries++; > + > + mpc_intsrc = last_addr; > + mpc_intsrc->type = MP_INTSRC; > + mpc_intsrc->irqtype = mp_INT; > + mpc_intsrc->irqflag = MP_IRQDIR_DEFAULT; > + mpc_intsrc->srcbus = pcibusid; > + mpc_intsrc->srcbusirq = 1; /* virtio block irq pin */ > + mpc_intsrc->dstapic = ioapicid; > + mpc_intsrc->dstirq = 11; /* VIRTIO_BLK_IRQ */ > + > + last_addr = (void *)&mpc_intsrc[1]; > + nentries++; > + > + mpc_intsrc = last_addr; > + mpc_intsrc->type = MP_INTSRC; > + mpc_intsrc->irqtype = mp_INT; > + mpc_intsrc->irqflag = MP_IRQDIR_DEFAULT; > + mpc_intsrc->srcbus = pcibusid; > + mpc_intsrc->srcbusirq = 1; /* virtio block irq pin */ > + mpc_intsrc->dstapic = ioapicid; > + mpc_intsrc->dstirq = 12; /* VIRTIO_BLK_IRQ */ There should really be a helper function for these initializations - and a loop that creates VIRTIO_BLK_MAX_DEV of them, right? Also, the IRQs used by kvm should be enumerated in an include file in a single place, with ranges allocated for specific purposes, otherwise we'll quickly lose track of them. And a pet peeve of mine: > +struct blk_device_job { > + struct virt_queue *vq; > + struct blk_device *blk_device; > + void *job_id; > +}; > + > struct blk_device { > pthread_mutex_t mutex; > > struct virtio_blk_config blk_config; > - struct disk_image *disk; > + struct disk_image *disk; > uint32_t host_features; > uint32_t guest_features; > uint16_t config_vector; > uint8_t status; > + u8 idx; The vertical spacing of those two structures (blk_device_job and blk_device) is inconsistent. > static bool virtio_blk_pci_io_in(struct kvm *self, uint16_t port, void *data, int size, uint32_t count) > { > - unsigned long offset; > + u16 offset, dev_idx; > bool ret = true; > + struct blk_device *blk_device; Suggestion: please standardize on shorter but still obvious variable names. For blk_device a good one could be 'bdev'. Given how frequently it's used you might even abbreviate 'struct blk_device' to 'struct blk_dev' - even in that shortr form it's still very obvious what it means. The new, standardized and streamlined naming convention should be propagated to all 'struct blk_device' using functions. (in separate patches) Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html