[Qemu-devel] Re: [PATCH, RFC 1/4] pci: add I/O registration functions

[Blue Swirl]

On Thu, May 27, 2010 at 2:39 PM, Michael S. Tsirkin <address@hidden> 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 <address@hidden>
>> ---
>>  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
>