[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 7/9] pci: factor out the logic to get pci device
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 7/9] pci: factor out the logic to get pci device from address. |
Date: |
Wed, 30 Sep 2009 13:30:01 +0200 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
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 <address@hidden>
> ---
> 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
>
>
- Re: [Qemu-devel] [PATCH 7/9] pci: factor out the logic to get pci device from address.,
Michael S. Tsirkin <=