Re: [PATCH 2/3 V2] kvm tools: Add support for multiple virtio-blk

[Sasha Levin <levinsasha928@gmail.com>]

On Wed, 2011-05-04 at 15:56 +0200, Ingo Molnar wrote:
> * Sasha Levin <levinsasha928@gmail.com> 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.