From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MsxPk-0006jn-2T for qemu-devel@nongnu.org; Wed, 30 Sep 2009 07:32:08 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MsxPe-0006eT-RL for qemu-devel@nongnu.org; Wed, 30 Sep 2009 07:32:06 -0400 Received: from [199.232.76.173] (port=60693 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MsxPe-0006eA-DD for qemu-devel@nongnu.org; Wed, 30 Sep 2009 07:32:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35142) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MsxPd-0007L5-Qj for qemu-devel@nongnu.org; Wed, 30 Sep 2009 07:32:02 -0400 Date: Wed, 30 Sep 2009 13:30:01 +0200 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] [PATCH 7/9] pci: factor out the logic to get pci device from address. Message-ID: <20090930113001.GC18802@redhat.com> References: <1247656509-32227-1-git-send-email-yamahata@valinux.co.jp> <1247656509-32227-8-git-send-email-yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1247656509-32227-8-git-send-email-yamahata@valinux.co.jp> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: qemu-devel@nongnu.org On Wed, Jul 15, 2009 at 08:15:07PM +0900, Isaku Yamahata wrote: > factor out conversion logic from io port address into bus+dev+func > with bit shift operation and conversion bus+dev+func into pci device. > They will be used later. > This patch also eliminates the logic duplication. > > Signed-off-by: Isaku Yamahata > --- > hw/pci.c | 105 ++++++++++++++++++++++++++++++++++--------------------------- > 1 files changed, 58 insertions(+), 47 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index f06e1da..3b19c3d 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -584,71 +584,82 @@ static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num) > return s; > } > > -void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len) > +static PCIDevice *pci_bdf_to_dev(PCIBus *s, int bus_num, unsigned int devfn) This name is pretty bad. > { > - PCIBus *s = opaque; > - PCIDevice *pci_dev; > - int config_addr, bus_num; > - > -#if 0 > - PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n", > - addr, val, len); > -#endif > - bus_num = (addr >> 16) & 0xff; > s = pci_find_bus_from(s, bus_num); > if (!s) > - return; > - pci_dev = s->devices[(addr >> 8) & 0xff]; > + return NULL; > + > + return s->devices[devfn]; > +} > +static void pci_dev_data_write(PCIDevice *pci_dev, > + uint32_t config_addr, uint32_t val, int len) There seems to be 1 caller, and it's confusing to have 2 functions with basically the same nhame. Please open-code it. > +{ > + assert(len == 1 || len == 2 || len == 4); > if (!pci_dev) > return; > - config_addr = addr & 0xff; > - PCI_DPRINTF("pci_config_write: %s: " > - "addr=%02"PRIx32" val=%08"PRI32x" len=%d\n", > - pci_dev->name, config_addr, val, len); > + > + PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n", > + __func__, pci_dev->name, config_addr, val, len); > pci_dev->config_write(pci_dev, config_addr, val, len); > } > > -uint32_t pci_data_read(void *opaque, uint32_t addr, int len) > +static uint32_t pci_dev_data_read(PCIDevice *pci_dev, > + uint32_t config_addr, int len) > { > - PCIBus *s = opaque; > - PCIDevice *pci_dev; > - int config_addr, bus_num; > uint32_t val; > > - bus_num = (addr >> 16) & 0xff; > - s = pci_find_bus_from(s, bus_num); > - if (!s) > - goto fail; > - pci_dev = s->devices[(addr >> 8) & 0xff]; > + assert(len == 1 || len == 2 || len == 4); > if (!pci_dev) { > - fail: > - switch(len) { > - case 1: > - val = 0xff; > - break; > - case 2: > - val = 0xffff; > - break; > - default: > - case 4: > - val = 0xffffffff; > - break; > - } > - goto the_end; > + val = (1 << (len * 8)) - 1; You can't do this: len * 8 will be 32 if len is 4, and shift by 32 is undefined in C. > + } else { > + val = pci_dev->config_read(pci_dev, config_addr, len); > + PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n", > + __func__, pci_dev->name, config_addr, val, len); > } > - config_addr = addr & 0xff; > - val = pci_dev->config_read(pci_dev, config_addr, len); > - PCI_DPRINTF("pci_config_read: %s: " > - "addr=%02"PRIx32" val=%08"PRIx32" len=%d\n", > - pci_dev->name, config_addr, val, len); > - the_end: > + > #if 0 > - PCI_DPRINTF("pci_data_read: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n", > - addr, val, len); > + PCI_DPRINTF("%s: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n", > + __func__, addr, val, len); > #endif > return val; > } > > +static void pci_addr_to_dev(PCIBus *s, uint32_t addr, > + PCIDevice **pci_dev, uint32_t *config_addr) > +{ > + int bus_num = (addr >> 16) & 0xff; > + unsigned int devfn = (addr >> 8) & 0xff; > + > + *pci_dev = pci_bdf_to_dev(s, bus_num, devfn); > + *config_addr = addr & 0xff; > +} Just open-code this function. If you really must, add ADDR_TO_DEVFN and ADDR_TO_BUS macros. > +void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len) > +{ > + PCIBus *s = opaque; > + PCIDevice *pci_dev; > + uint32_t config_addr; > + > +#if 0 > + PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n", > + addr, val, len); > +#endif > + > + pci_addr_to_dev(s, addr, &pci_dev, &config_addr); It's better to have 2 incline functions that return a single value each here. > + pci_dev_data_write(pci_dev, config_addr, val, len); > +} > + > +uint32_t pci_data_read(void *opaque, uint32_t addr, int len) > +{ > + PCIBus *s = opaque; > + PCIDevice *pci_dev; > + uint32_t config_addr; > + > + pci_addr_to_dev(s, addr, &pci_dev, &config_addr); > + return pci_dev_data_read(pci_dev, config_addr, len); > +} > /***********************************************************/ > /* generic PCI irq support */ > > -- > 1.6.0.2 > >