[RFC,1/4] pci: add I/O registration functions

Convert also APB to use the registration so that
we can remove mem_base.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 hw/apb_pci.c |   23 ++++++++++++++++++++-
 hw/pci.c     |   64 ++++++++++++++++++++++++++++++++++-----------------------
 hw/pci.h     |    9 +++++++-
 3 files changed, 68 insertions(+), 28 deletions(-)

                         const char *default_devaddr);

On Sun, May 23, 2010 at 08:34:30PM +0000, Blue Swirl wrote:
> Convert also APB to use the registration so that
> we can remove mem_base.
> 
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  hw/apb_pci.c |   23 ++++++++++++++++++++-
>  hw/pci.c     |   64 ++++++++++++++++++++++++++++++++++-----------------------
>  hw/pci.h     |    9 +++++++-
>  3 files changed, 68 insertions(+), 28 deletions(-)

Probably should mention pci.c changes in the changelog.

> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 65d8ba6..fb23397 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -74,6 +74,7 @@ typedef struct APBState {
>      qemu_irq pci_irqs[32];
>      uint32_t reset_control;
>      unsigned int nr_resets;
> +    target_phys_addr_t mem_base;
>  } APBState;
> 
>  static void apb_config_writel (void *opaque, target_phys_addr_t addr,
> @@ -316,6 +317,24 @@ static void apb_pci_bridge_init(PCIBus *b)
>                   PCI_HEADER_TYPE_MULTI_FUNCTION);
>  }
> 
> +static void apb_register_mem(void *opaque, pcibus_t addr, pcibus_t
> size, int mm)
> +{
> +    APBState *d = opaque;
> +
> +    APB_DPRINTF("%s: addr %" FMT_PCIBUS " size %" FMT_PCIBUS "mm %x\n",
> +                __func__, addr, size, mm);
> +    cpu_register_physical_memory(addr + d->mem_base, size, mm);
> +}
> +
> +static void apb_unregister_mem(void *opaque, pcibus_t addr, pcibus_t size)
> +{
> +    APBState *d = opaque;
> +
> +    APB_DPRINTF("%s: addr %" FMT_PCIBUS " size %" FMT_PCIBUS "\n",
> +                __func__, addr, size);
> +    cpu_register_physical_memory(addr + d->mem_base, size, IO_MEM_UNASSIGNED);
> +}
> +
>  PCIBus *pci_apb_init(target_phys_addr_t special_base,
>                       target_phys_addr_t mem_base,
>                       qemu_irq *pic, PCIBus **bus2, PCIBus **bus3)
> @@ -338,10 +357,12 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
>      /* mem_data */
>      sysbus_mmio_map(s, 3, mem_base);
>      d = FROM_SYSBUS(APBState, s);
> +    d->mem_base = mem_base;
>      d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
>                                           pci_apb_set_irq, pci_pbm_map_irq, d,
>                                           0, 32);
> -    pci_bus_set_mem_base(d->host_state.bus, mem_base);
> +    pci_bus_set_register_mem_fn(d->host_state.bus, apb_register_mem,
> +                                apb_unregister_mem, d);
> 
>      for (i = 0; i < 32; i++) {
>          sysbus_connect_irq(s, i, pic[i]);
> diff --git a/hw/pci.c b/hw/pci.c
> index 8d84651..ffd6dc3 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -46,7 +46,9 @@ struct PCIBus {
>      void *irq_opaque;
>      PCIDevice *devices[256];
>      PCIDevice *parent_dev;
> -    target_phys_addr_t mem_base;
> +    pci_register_mem_fn register_mem;
> +    pci_unregister_mem_fn unregister_mem;
> +    void *register_fn_opaque;
> 
>      QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */
>      QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */
> @@ -163,6 +165,18 @@ static void pci_device_reset(PCIDevice *dev)
>      pci_update_mappings(dev);
>  }
> 
> +static void pci_bus_default_register_mem(void *opaque, pcibus_t addr,
> +                                         pcibus_t size, int mm)
> +{
> +    cpu_register_physical_memory(addr, size, mm);
> +}
> +
> +static void pci_bus_default_unregister_mem(void *opaque, pcibus_t addr,
> +                                           pcibus_t size)
> +{
> +    cpu_register_physical_memory(addr, size, IO_MEM_UNASSIGNED);
> +}
> +
>  static void pci_bus_reset(void *opaque)
>  {
>      PCIBus *bus = opaque;
> @@ -205,6 +219,8 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
>  {
>      qbus_create_inplace(&bus->qbus, &pci_bus_info, parent, name);
>      bus->devfn_min = devfn_min;
> +    bus->register_mem = pci_bus_default_register_mem;
> +    bus->unregister_mem = pci_bus_default_unregister_mem;
> 
>      /* host bridge */
>      QLIST_INIT(&bus->child);
> @@ -241,11 +257,6 @@ void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn
> hotplug, DeviceState *qdev)
>      bus->hotplug_qdev = qdev;
>  }
> 
> -void pci_bus_set_mem_base(PCIBus *bus, target_phys_addr_t base)
> -{
> -    bus->mem_base = base;
> -}
> -
>  PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>                           pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
>                           void *irq_opaque, int devfn_min, int nirq)
> @@ -651,12 +662,6 @@ PCIDevice *pci_register_device(PCIBus *bus, const
> char *name,
>      return pci_dev;
>  }
> 
> -static target_phys_addr_t pci_to_cpu_addr(PCIBus *bus,
> -                                          target_phys_addr_t addr)
> -{
> -    return addr + bus->mem_base;
> -}
> -
>  static void pci_unregister_io_regions(PCIDevice *pci_dev)
>  {
>      PCIIORegion *r;
> @@ -669,10 +674,9 @@ static void pci_unregister_io_regions(PCIDevice *pci_dev)
>          if (r->type == PCI_BASE_ADDRESS_SPACE_IO) {
>              isa_unassign_ioport(r->addr, r->filtered_size);
>          } else {
> -            cpu_register_physical_memory(pci_to_cpu_addr(pci_dev->bus,
> -                                                         r->addr),
> -                                         r->filtered_size,
> -                                         IO_MEM_UNASSIGNED);
> +            pci_dev->bus->unregister_mem(pci_dev->bus->register_fn_opaque,
> +                                        r->addr,
> +                                        r->filtered_size);
>          }
>      }
>  }
> @@ -903,6 +907,19 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>      return new_addr;
>  }
> 
> +void pci_bus_set_register_mem_fn(PCIBus *bus, pci_register_mem_fn regfn,
> +                                 pci_unregister_mem_fn unregfn, void *opaque)
> +{
> +    bus->register_mem = regfn;
> +    bus->unregister_mem = unregfn;
> +    bus->register_fn_opaque = opaque;
> +}
> +
> +void pci_register_memory(PCIBus *bus, pcibus_t addr, pcibus_t size, int mm)
> +{
> +    bus->register_mem(bus->register_fn_opaque, addr, size, mm);
> +}
> +

If we are moving all devices from cpu_map, I think we should map mm to
an offset/size range within bar, and have pci.c register/unregister
with bus/cpu.


>  static void pci_update_mappings(PCIDevice *d)
>  {
>      PCIIORegion *r;
> @@ -941,9 +958,8 @@ static void pci_update_mappings(PCIDevice *d)
>                      isa_unassign_ioport(r->addr, r->filtered_size);
>                  }
>              } else {
> -                cpu_register_physical_memory(pci_to_cpu_addr(d->bus, r->addr),
> -                                             r->filtered_size,
> -                                             IO_MEM_UNASSIGNED);
> +                d->bus->unregister_mem(d->bus->register_fn_opaque, r->addr,
> +                                             r->filtered_size);
>                  qemu_unregister_coalesced_mmio(r->addr, r->filtered_size);
>              }
>          }
> @@ -957,12 +973,7 @@ static void pci_update_mappings(PCIDevice *d)
>               * Teach them such cases, such that filtered_size < size and
>               * addr & (size - 1) != 0.
>               */
> -            if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> -                r->map_func(d, i, r->addr, r->filtered_size, r->type);
> -            } else {
> -                r->map_func(d, i, pci_to_cpu_addr(d->bus, r->addr),
> -                            r->filtered_size, r->type);
> -            }
> +            r->map_func(d, i, r->addr, r->filtered_size, r->type);
>          }
>      }
>  }
> @@ -1747,7 +1758,8 @@ static uint8_t
> pci_find_capability_list(PCIDevice *pdev, uint8_t cap_id,
> 
>  static void pci_map_option_rom(PCIDevice *pdev, int region_num,
> pcibus_t addr, pcibus_t size, int type)
>  {
> -    cpu_register_physical_memory(addr, size, pdev->rom_offset);
> +    pdev->bus->register_mem(pdev->bus->register_fn_opaque, addr, size,
> +                            pdev->rom_offset);
>  }
> 
>  /* Add an option rom for the device */
> diff --git a/hw/pci.h b/hw/pci.h
> index f6e6c5f..a04e4b9 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -220,7 +220,14 @@ PCIBus *pci_register_bus(DeviceState *parent,
> const char *name,
>                           pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
>                           void *irq_opaque, int devfn_min, int nirq);
> 
> -void pci_bus_set_mem_base(PCIBus *bus, target_phys_addr_t base);
> +typedef void (*pci_register_mem_fn)(void *opaque, pcibus_t addr, pcibus_t size,
> +                                    int mm);
> +typedef void (*pci_unregister_mem_fn)(void *opaque, pcibus_t addr,
> +                                      pcibus_t size);
> +void pci_bus_set_register_mem_fn(PCIBus *bus, pci_register_mem_fn regfn,
> +                                 pci_unregister_mem_fn unregfn, void *opaque);
> +
> +void pci_register_memory(PCIBus *bus, pcibus_t addr, pcibus_t size, int mm);
> 
>  PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
>                          const char *default_devaddr);
> -- 
> 1.6.2.4

On Thu, May 27, 2010 at 2:39 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, May 23, 2010 at 08:34:30PM +0000, Blue Swirl wrote:
>> Convert also APB to use the registration so that
>> we can remove mem_base.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  hw/apb_pci.c |   23 ++++++++++++++++++++-
>>  hw/pci.c     |   64 ++++++++++++++++++++++++++++++++++-----------------------
>>  hw/pci.h     |    9 +++++++-
>>  3 files changed, 68 insertions(+), 28 deletions(-)
>
> Probably should mention pci.c changes in the changelog.

It's the subject.

>
>> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
>> index 65d8ba6..fb23397 100644
>> --- a/hw/apb_pci.c
>> +++ b/hw/apb_pci.c
>> @@ -74,6 +74,7 @@ typedef struct APBState {
>>      qemu_irq pci_irqs[32];
>>      uint32_t reset_control;
>>      unsigned int nr_resets;
>> +    target_phys_addr_t mem_base;
>>  } APBState;
>>
>>  static void apb_config_writel (void *opaque, target_phys_addr_t addr,
>> @@ -316,6 +317,24 @@ static void apb_pci_bridge_init(PCIBus *b)
>>                   PCI_HEADER_TYPE_MULTI_FUNCTION);
>>  }
>>
>> +static void apb_register_mem(void *opaque, pcibus_t addr, pcibus_t
>> size, int mm)
>> +{
>> +    APBState *d = opaque;
>> +
>> +    APB_DPRINTF("%s: addr %" FMT_PCIBUS " size %" FMT_PCIBUS "mm %x\n",
>> +                __func__, addr, size, mm);
>> +    cpu_register_physical_memory(addr + d->mem_base, size, mm);
>> +}
>> +
>> +static void apb_unregister_mem(void *opaque, pcibus_t addr, pcibus_t size)
>> +{
>> +    APBState *d = opaque;
>> +
>> +    APB_DPRINTF("%s: addr %" FMT_PCIBUS " size %" FMT_PCIBUS "\n",
>> +                __func__, addr, size);
>> +    cpu_register_physical_memory(addr + d->mem_base, size, IO_MEM_UNASSIGNED);
>> +}
>> +
>>  PCIBus *pci_apb_init(target_phys_addr_t special_base,
>>                       target_phys_addr_t mem_base,
>>                       qemu_irq *pic, PCIBus **bus2, PCIBus **bus3)
>> @@ -338,10 +357,12 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
>>      /* mem_data */
>>      sysbus_mmio_map(s, 3, mem_base);
>>      d = FROM_SYSBUS(APBState, s);
>> +    d->mem_base = mem_base;
>>      d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
>>                                           pci_apb_set_irq, pci_pbm_map_irq, d,
>>                                           0, 32);
>> -    pci_bus_set_mem_base(d->host_state.bus, mem_base);
>> +    pci_bus_set_register_mem_fn(d->host_state.bus, apb_register_mem,
>> +                                apb_unregister_mem, d);
>>
>>      for (i = 0; i < 32; i++) {
>>          sysbus_connect_irq(s, i, pic[i]);
>> diff --git a/hw/pci.c b/hw/pci.c
>> index 8d84651..ffd6dc3 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -46,7 +46,9 @@ struct PCIBus {
>>      void *irq_opaque;
>>      PCIDevice *devices[256];
>>      PCIDevice *parent_dev;
>> -    target_phys_addr_t mem_base;
>> +    pci_register_mem_fn register_mem;
>> +    pci_unregister_mem_fn unregister_mem;
>> +    void *register_fn_opaque;
>>
>>      QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */
>>      QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */
>> @@ -163,6 +165,18 @@ static void pci_device_reset(PCIDevice *dev)
>>      pci_update_mappings(dev);
>>  }
>>
>> +static void pci_bus_default_register_mem(void *opaque, pcibus_t addr,
>> +                                         pcibus_t size, int mm)
>> +{
>> +    cpu_register_physical_memory(addr, size, mm);
>> +}
>> +
>> +static void pci_bus_default_unregister_mem(void *opaque, pcibus_t addr,
>> +                                           pcibus_t size)
>> +{
>> +    cpu_register_physical_memory(addr, size, IO_MEM_UNASSIGNED);
>> +}
>> +
>>  static void pci_bus_reset(void *opaque)
>>  {
>>      PCIBus *bus = opaque;
>> @@ -205,6 +219,8 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
>>  {
>>      qbus_create_inplace(&bus->qbus, &pci_bus_info, parent, name);
>>      bus->devfn_min = devfn_min;
>> +    bus->register_mem = pci_bus_default_register_mem;
>> +    bus->unregister_mem = pci_bus_default_unregister_mem;
>>
>>      /* host bridge */
>>      QLIST_INIT(&bus->child);
>> @@ -241,11 +257,6 @@ void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn
>> hotplug, DeviceState *qdev)
>>      bus->hotplug_qdev = qdev;
>>  }
>>
>> -void pci_bus_set_mem_base(PCIBus *bus, target_phys_addr_t base)
>> -{
>> -    bus->mem_base = base;
>> -}
>> -
>>  PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>>                           pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
>>                           void *irq_opaque, int devfn_min, int nirq)
>> @@ -651,12 +662,6 @@ PCIDevice *pci_register_device(PCIBus *bus, const
>> char *name,
>>      return pci_dev;
>>  }
>>
>> -static target_phys_addr_t pci_to_cpu_addr(PCIBus *bus,
>> -                                          target_phys_addr_t addr)
>> -{
>> -    return addr + bus->mem_base;
>> -}
>> -
>>  static void pci_unregister_io_regions(PCIDevice *pci_dev)
>>  {
>>      PCIIORegion *r;
>> @@ -669,10 +674,9 @@ static void pci_unregister_io_regions(PCIDevice *pci_dev)
>>          if (r->type == PCI_BASE_ADDRESS_SPACE_IO) {
>>              isa_unassign_ioport(r->addr, r->filtered_size);
>>          } else {
>> -            cpu_register_physical_memory(pci_to_cpu_addr(pci_dev->bus,
>> -                                                         r->addr),
>> -                                         r->filtered_size,
>> -                                         IO_MEM_UNASSIGNED);
>> +            pci_dev->bus->unregister_mem(pci_dev->bus->register_fn_opaque,
>> +                                        r->addr,
>> +                                        r->filtered_size);
>>          }
>>      }
>>  }
>> @@ -903,6 +907,19 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>>      return new_addr;
>>  }
>>
>> +void pci_bus_set_register_mem_fn(PCIBus *bus, pci_register_mem_fn regfn,
>> +                                 pci_unregister_mem_fn unregfn, void *opaque)
>> +{
>> +    bus->register_mem = regfn;
>> +    bus->unregister_mem = unregfn;
>> +    bus->register_fn_opaque = opaque;
>> +}
>> +
>> +void pci_register_memory(PCIBus *bus, pcibus_t addr, pcibus_t size, int mm)
>> +{
>> +    bus->register_mem(bus->register_fn_opaque, addr, size, mm);
>> +}
>> +
>
> If we are moving all devices from cpu_map, I think we should map mm to
> an offset/size range within bar, and have pci.c register/unregister
> with bus/cpu.

Good idea. It may make the change much bigger though.

>
>
>>  static void pci_update_mappings(PCIDevice *d)
>>  {
>>      PCIIORegion *r;
>> @@ -941,9 +958,8 @@ static void pci_update_mappings(PCIDevice *d)
>>                      isa_unassign_ioport(r->addr, r->filtered_size);
>>                  }
>>              } else {
>> -                cpu_register_physical_memory(pci_to_cpu_addr(d->bus, r->addr),
>> -                                             r->filtered_size,
>> -                                             IO_MEM_UNASSIGNED);
>> +                d->bus->unregister_mem(d->bus->register_fn_opaque, r->addr,
>> +                                             r->filtered_size);
>>                  qemu_unregister_coalesced_mmio(r->addr, r->filtered_size);
>>              }
>>          }
>> @@ -957,12 +973,7 @@ static void pci_update_mappings(PCIDevice *d)
>>               * Teach them such cases, such that filtered_size < size and
>>               * addr & (size - 1) != 0.
>>               */
>> -            if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
>> -                r->map_func(d, i, r->addr, r->filtered_size, r->type);
>> -            } else {
>> -                r->map_func(d, i, pci_to_cpu_addr(d->bus, r->addr),
>> -                            r->filtered_size, r->type);
>> -            }
>> +            r->map_func(d, i, r->addr, r->filtered_size, r->type);
>>          }
>>      }
>>  }
>> @@ -1747,7 +1758,8 @@ static uint8_t
>> pci_find_capability_list(PCIDevice *pdev, uint8_t cap_id,
>>
>>  static void pci_map_option_rom(PCIDevice *pdev, int region_num,
>> pcibus_t addr, pcibus_t size, int type)
>>  {
>> -    cpu_register_physical_memory(addr, size, pdev->rom_offset);
>> +    pdev->bus->register_mem(pdev->bus->register_fn_opaque, addr, size,
>> +                            pdev->rom_offset);
>>  }
>>
>>  /* Add an option rom for the device */
>> diff --git a/hw/pci.h b/hw/pci.h
>> index f6e6c5f..a04e4b9 100644
>> --- a/hw/pci.h
>> +++ b/hw/pci.h
>> @@ -220,7 +220,14 @@ PCIBus *pci_register_bus(DeviceState *parent,
>> const char *name,
>>                           pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
>>                           void *irq_opaque, int devfn_min, int nirq);
>>
>> -void pci_bus_set_mem_base(PCIBus *bus, target_phys_addr_t base);
>> +typedef void (*pci_register_mem_fn)(void *opaque, pcibus_t addr, pcibus_t size,
>> +                                    int mm);
>> +typedef void (*pci_unregister_mem_fn)(void *opaque, pcibus_t addr,
>> +                                      pcibus_t size);
>> +void pci_bus_set_register_mem_fn(PCIBus *bus, pci_register_mem_fn regfn,
>> +                                 pci_unregister_mem_fn unregfn, void *opaque);
>> +
>> +void pci_register_memory(PCIBus *bus, pcibus_t addr, pcibus_t size, int mm);
>>
>>  PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
>>                          const char *default_devaddr);
>> --
>> 1.6.2.4
>

> On Thu, May 27, 2010 at 2:39 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, May 23, 2010 at 08:34:30PM +0000, Blue Swirl wrote:
> >> Convert also APB to use the registration so that
> >> we can remove mem_base.
> >> 
> >> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> >> ---
> >>  hw/apb_pci.c |   23 ++++++++++++++++++++-
> >>  hw/pci.c     |   64
> >> ++++++++++++++++++++++++++++++++++----------------------- hw/pci.h    
> >> |    9 +++++++-
> >>  3 files changed, 68 insertions(+), 28 deletions(-)
> > 
> > Probably should mention pci.c changes in the changelog.
> 
> It's the subject.

IMO the body of the commit message should be self-contained.  Many mail 
clients display the body text separately from the subject - the subject tends 
to be grouped with other metadata like to/from addresses.
This makes reading both as a whole confusing and unintuitive.

You can argue that this is a bug in git (and/or many mail clients). However I 
don't see that changing any time soon, so we should adapt our work process 
appropriately.

Some other version control systems (e.g. CVS and SVN) don't have commit 
summary, so this can seem a strange concept when migrating from those systems.

Paul