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

[Isaku Yamahata]

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>
> ---
>  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);
> +}
> +

Isn't something like cpu_to_pci_addr() needed in theory?


> +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);
> +}

They pass addr, not offset from base address.
pci_io_region_ioport_ {read, write}[bwl]() pass offset.
It's inconsistent. offset should be passed.

Isn't the inverse of pci_to_cpu_addr() necessary in theory?



> +
> +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);
> +}
> +

addr - (r->addr & ~(r->size - 1))
r->addr isn't always base address because of bridge filtering.


>  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
> 
> 
> 

-- 
yamahata