On Wed, 2011-05-04 at 15:56 +0200, Ingo Molnar wrote:
> * 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.
>
The plan is that mptable.c is going to change either way, Cyrill noted
we need a way to manage IRQs earlier in that file. That's also why I
tried not changing it too much.
> 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.
>
Will fix.
> > 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)
Will fix.
--
Sasha.
--
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
