[Qemu-devel] Re: [PATCH 01/15] pci: add new bus functions

[Michael S. Tsirkin]

On Tue, Feb 09, 2010 at 04:01:25PM -0600, Anthony Liguori wrote:
>  - mapping and managing io regions
>  - reading and writing physical memory
> 
> Signed-off-by: Anthony Liguori <address@hidden>

One thing I'm concerned with here is that this adds
another level of indirection on datapath operations.
Note that rwhandler is only used for config cycles,
which aren't normally datapath.

Can the patch be split to separate mapping and read/write parts?
I think mapping is the harder part, and also where we have actual
emulation bugs now. It would thus probably be uncontroversial.

Also, maybe it's time to bite the bullet and change memory/io callbacks
in exec.c (that rwhandler currently layers above): pass in length
and use some generic type like addr_t for address?
This would let us get rid of both rwhandler and most of this patch.

> ---
>  hw/pci.c |  172 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  hw/pci.h |   18 ++++++-
>  2 files changed, 181 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 9ad63dd..5460f27 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -664,6 +664,9 @@ static void pci_unregister_io_regions(PCIDevice *pci_dev)
>                                                           r->addr),
>                                           r->filtered_size,
>                                           IO_MEM_UNASSIGNED);
> +            if (!r->map_func && r->read && r->write) {
> +                cpu_unregister_io_memory(r->io_memory_addr);
> +            }
>          }
>      }
>  }
> @@ -687,16 +690,15 @@ static int pci_unregister_device(DeviceState *dev)
>      return 0;
>  }
>  
> -void pci_register_bar(PCIDevice *pci_dev, int region_num,
> -                            pcibus_t size, int type,
> -                            PCIMapIORegionFunc *map_func)
> +static PCIIORegion *do_pci_register_bar(PCIDevice *pci_dev, int region_num,
> +                                        pcibus_t size, int type)
>  {
>      PCIIORegion *r;
>      uint32_t addr;
>      pcibus_t wmask;
>  
>      if ((unsigned int)region_num >= PCI_NUM_REGIONS)
> -        return;
> +        return NULL;
>  
>      if (size & (size-1)) {
>          fprintf(stderr, "ERROR: PCI region size must be pow2 "
> @@ -709,7 +711,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>      r->size = size;
>      r->filtered_size = size;
>      r->type = type;
> -    r->map_func = map_func;
>  
>      wmask = ~(size - 1);
>      addr = pci_bar(pci_dev, region_num);
> @@ -726,6 +727,100 @@ void pci_register_bar(PCIDevice *pci_dev, int 
> region_num,
>          pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff);
>          pci_set_long(pci_dev->cmask + addr, 0xffffffff);
>      }
> +
> +    return r;
> +}
> +
> +void pci_register_bar(PCIDevice *pci_dev, int region_num,
> +                      pcibus_t size, int type,
> +                      PCIMapIORegionFunc *map_func)
> +{
> +    PCIIORegion *r;
> +    r = do_pci_register_bar(pci_dev, region_num, size, type);
> +    if (r) {
> +        r->map_func = map_func;
> +    }
> +}
> +
> +void pci_memory_read(PCIDevice *pci_dev, pcibus_t addr, void *buf, int len)
> +{
> +    cpu_physical_memory_read(addr, buf, len);
> +}
> +
> +void pci_memory_write(PCIDevice *pci_dev, pcibus_t addr,
> +                      const void *buf, int len)
> +{
> +    cpu_physical_memory_write(addr, buf, len);
> +}
> +
> +static void pci_io_region_writeb(void *opaque, target_phys_addr_t addr,
> +                                 uint32_t value)
> +{
> +    PCIIORegion *r = opaque;
> +    r->write(r->dev, addr, 1, value);
> +}
> +
> +static void pci_io_region_writew(void *opaque, target_phys_addr_t addr,
> +                                 uint32_t value)
> +{
> +    PCIIORegion *r = opaque;
> +    r->write(r->dev, addr, 2, value);
> +}
> +
> +static void pci_io_region_writel(void *opaque, target_phys_addr_t addr,
> +                                 uint32_t value)
> +{
> +    PCIIORegion *r = opaque;
> +    r->write(r->dev, addr, 4, value);
> +}
> +
> +static uint32_t pci_io_region_readb(void *opaque, target_phys_addr_t addr)
> +{
> +    PCIIORegion *r = opaque;
> +    return r->read(r->dev, addr, 1);
> +}
> +
> +static uint32_t pci_io_region_readw(void *opaque, target_phys_addr_t addr)
> +{
> +    PCIIORegion *r = opaque;
> +    return r->read(r->dev, addr, 2);
> +}
> +
> +static uint32_t pci_io_region_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    PCIIORegion *r = opaque;
> +    return r->read(r->dev, addr, 4);
> +}
> +
> +static CPUReadMemoryFunc * const pci_io_region_readfn[3] = {
> +    pci_io_region_readb,
> +    pci_io_region_readw,
> +    pci_io_region_readl,
> +};
> +    
> +static CPUWriteMemoryFunc * const pci_io_region_writefn[3] = {
> +    pci_io_region_writeb,
> +    pci_io_region_writew,
> +    pci_io_region_writel,
> +};
> +
> +void pci_register_io_region(PCIDevice *d, int region_num,
> +                            pcibus_t size, int type,
> +                            PCIIOReadFunc *readcb, PCIIOWriteFunc *writecb)
> +{
> +    PCIIORegion *r;
> +    r = do_pci_register_bar(d, region_num, size, type);
> +    if (r) {
> +        r->map_func = NULL;
> +        r->dev = d;
> +        r->read = readcb;
> +        r->write = writecb;
> +        if (r->type != PCI_BASE_ADDRESS_SPACE_IO && r->read && r->write) {
> +            r->io_memory_addr = cpu_register_io_memory(pci_io_region_readfn,
> +                                                       pci_io_region_writefn,
> +                                                       r);
> +        }
> +    }
>  }
>  
>  static uint32_t pci_config_get_io_base(PCIDevice *d,
> @@ -897,6 +992,45 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>      return new_addr;
>  }
>  
> +static void pci_io_region_ioport_writeb(void *opaque, uint32_t addr,
> +                                        uint32_t value)
> +{
> +    PCIIORegion *r = opaque;
> +    r->write(r->dev, (addr - r->addr), 1, value);
> +}
> +
> +static void pci_io_region_ioport_writew(void *opaque, uint32_t addr,
> +                                        uint32_t value)
> +{
> +    PCIIORegion *r = opaque;
> +    r->write(r->dev, (addr - r->addr), 2, value);
> +}
> +
> +static void pci_io_region_ioport_writel(void *opaque, uint32_t addr,
> +                                        uint32_t value)
> +{
> +    PCIIORegion *r = opaque;
> +    r->write(r->dev, (addr - r->addr), 4, value);
> +}
> +
> +static uint32_t pci_io_region_ioport_readb(void *opaque, uint32_t addr)
> +{
> +    PCIIORegion *r = opaque;
> +    return r->read(r->dev, (addr - r->addr), 1);
> +}
> +
> +static uint32_t pci_io_region_ioport_readw(void *opaque, uint32_t addr)
> +{
> +    PCIIORegion *r = opaque;
> +    return r->read(r->dev, (addr - r->addr), 2);
> +}
> +
> +static uint32_t pci_io_region_ioport_readl(void *opaque, uint32_t addr)
> +{
> +    PCIIORegion *r = opaque;
> +    return r->read(r->dev, (addr - r->addr), 4);
> +}
> +
>  static void pci_update_mappings(PCIDevice *d)
>  {
>      PCIIORegion *r;
> @@ -952,10 +1086,32 @@ static void pci_update_mappings(PCIDevice *d)
>               * addr & (size - 1) != 0.
>               */
>              if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> -                r->map_func(d, i, r->addr, r->filtered_size, r->type);
> +                if (r->map_func) {
> +                    r->map_func(d, i, r->addr, r->filtered_size, r->type);
> +                } else {
> +                    register_ioport_write(r->addr, r->filtered_size, 1,
> +                                          pci_io_region_ioport_writeb, r);
> +                    register_ioport_write(r->addr, r->filtered_size, 2,
> +                                          pci_io_region_ioport_writew, r);
> +                    register_ioport_write(r->addr, r->filtered_size, 4,
> +                                          pci_io_region_ioport_writel, r);
> +                    register_ioport_read(r->addr, r->filtered_size, 1,
> +                                         pci_io_region_ioport_readb, r);
> +                    register_ioport_read(r->addr, r->filtered_size, 2,
> +                                         pci_io_region_ioport_readw, r);
> +                    register_ioport_read(r->addr, r->filtered_size, 4,
> +                                         pci_io_region_ioport_readl, r);
> +                }
>              } else {
> -                r->map_func(d, i, pci_to_cpu_addr(d->bus, r->addr),
> -                            r->filtered_size, r->type);
> +                if (r->map_func) {
> +                    r->map_func(d, i, pci_to_cpu_addr(d->bus, r->addr),
> +                                r->filtered_size, r->type);
> +                } else if (r->read && r->write) {
> +                    cpu_register_physical_memory(pci_to_cpu_addr(d->bus,
> +                                                                 r->addr),
> +                                                 r->filtered_size,
> +                                                 r->io_memory_addr);
> +                }
>              }
>          }
>      }
> diff --git a/hw/pci.h b/hw/pci.h
> index 8b511d2..3edf28f 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -81,13 +81,21 @@ typedef void PCIMapIORegionFunc(PCIDevice *pci_dev, int 
> region_num,
>                                  pcibus_t addr, pcibus_t size, int type);
>  typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
>  
> +typedef uint32_t (PCIIOReadFunc)(PCIDevice *pci_dev, pcibus_t addr, int 
> size);
> +typedef void (PCIIOWriteFunc)(PCIDevice *pci_dev, pcibus_t addr, int size,
> +                              uint32_t value);
> +
>  typedef struct PCIIORegion {
> +    PCIDevice *dev;
>      pcibus_t addr; /* current PCI mapping address. -1 means not mapped */
>  #define PCI_BAR_UNMAPPED (~(pcibus_t)0)
>      pcibus_t size;
>      pcibus_t filtered_size;
>      uint8_t type;
> -    PCIMapIORegionFunc *map_func;
> +    PCIMapIORegionFunc *map_func; /* legacy mapping function */
> +    PCIIOReadFunc *read;
> +    PCIIOWriteFunc *write;
> +    int io_memory_addr;
>  } PCIIORegion;
>  
>  #define PCI_ROM_SLOT 6
> @@ -190,6 +198,14 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>                              pcibus_t size, int type,
>                              PCIMapIORegionFunc *map_func);
>  
> +void pci_register_io_region(PCIDevice *d, int region_num,
> +                            pcibus_t size, int type,
> +                            PCIIOReadFunc *readcb, PCIIOWriteFunc *writecb);
> +
> +void pci_memory_read(PCIDevice *pci_dev, pcibus_t addr, void *buf, int len);
> +void pci_memory_write(PCIDevice *pci_dev, pcibus_t addr,
> +                      const void *buf, int len);
> +
>  int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
>  
>  void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t 
> cap_size);
> -- 
> 1.6.5.2
> 
>