From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sasha Levin Subject: Re: [PATCH 2/3 V2] kvm tools: Add support for multiple virtio-blk Date: Wed, 04 May 2011 17:44:28 +0300 Message-ID: <1304520268.22299.6.camel@lappy> References: <1304516717-24512-1-git-send-email-levinsasha928@gmail.com> <1304516717-24512-3-git-send-email-levinsasha928@gmail.com> <20110504135616.GA11941@elte.hu> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: penberg@kernel.org, kvm@vger.kernel.org, asias.hejun@gmail.com, gorcunov@gmail.com, prasadjoshi124@gmail.com To: Ingo Molnar Return-path: Received: from mail-ww0-f44.google.com ([74.125.82.44]:58748 "EHLO mail-ww0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754224Ab1EDOom (ORCPT ); Wed, 4 May 2011 10:44:42 -0400 Received: by wwa36 with SMTP id 36so1305919wwa.1 for ; Wed, 04 May 2011 07:44:41 -0700 (PDT) In-Reply-To: <20110504135616.GA11941@elte.hu> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, 2011-05-04 at 15:56 +0200, Ingo Molnar wrote: > * Sasha Levin 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.