Re: [Qemu-devel] [PATCH V6 for-2.3 14/26] hw/pci-host/piix: implement TYPE_PCI_HOST_BRIDGE_SNOOPED interface

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Mar 19, 2015 at 08:52:49PM +0200, Marcel Apfelbaum wrote:
> The PIIX holds a list of snooping host bridges that is populated
> by a registration function that shall be used by any host bridge
> that needs to use PIIX configuration space to complete their
> configuration cycles.
> 
> PIIX monitors acceses to configuration registers and passes them
> to the corresponding host bridge if the input bus number is
> in its bus range.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  hw/pci-host/piix.c        | 118 +++++++++++++++++++++++++++++++++++++++++++++-
>  hw/pci/pci_host.c         |  28 +++++++++++
>  include/hw/i386/pc.h      |   6 +++
>  include/hw/pci/pci_host.h |   3 ++
>  4 files changed, 154 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 0033ab4..f512765 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -50,6 +50,12 @@ typedef struct I440FXState {
>      PcPciInfo pci_info;
>      uint64_t pci_hole64_size;
>      uint32_t short_root_bus;
> +
> +    /*
> +     * List of PCI host bridges listening to
> +     * the same configuration registers.
> +     */
> +    GPtrArray *snooping_hosts;
>  } I440FXState;
>  
>  #define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
> @@ -255,14 +261,80 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
>      visit_type_uint64(v, &w64.end, name, errp);
>  }
>  
> +static PCIBus *i440fx_find_target_bus(I440FXState *i440fx, uint8_t bus_num)
> +{
> +    PCIHostState *host = PCI_HOST_BRIDGE(i440fx);
> +    PCIBus *target_bus = NULL;
> +
> +    if (bus_num == pci_bus_num(host->bus)) {
> +        target_bus = host->bus;
> +    } else {
> +        int i;
> +
> +        for (i = 0; i < i440fx->snooping_hosts->len; i++) {
> +            PCIHostState *s_host = g_ptr_array_index(i440fx->snooping_hosts, i);
> +            Range r = pci_host_get_buses_range(s_host);
> +
> +            if (r.begin <= bus_num && r.end >= bus_num) {
> +                target_bus = s_host->bus;
> +            }
> +


extra empty line

> +        }
> +    }
> +
> +    return target_bus;
> +}
> +
> +static void i440fx_pcihost_data_write(void *opaque, hwaddr addr,
> +                                      uint64_t val, unsigned len)
> +{
> +    uint32_t config_reg = PCI_HOST_BRIDGE(opaque)->config_reg;
> +
> +    if (config_reg & (1u << 31)) {
> +        uint8_t bus_num = (config_reg >> 16) & 0xFF;
> +        I440FXState *i440fx = I440FX_PCI_HOST_BRIDGE(opaque);
> +        PCIBus *target_bus = i440fx_find_target_bus(i440fx, bus_num);
> +
> +        if (target_bus) {
> +            pci_data_write(target_bus, config_reg | (addr & 3), val, len);
> +        }
> +    }
> +}
> +
> +static uint64_t i440fx_pcihost_data_read(void *opaque,
> +                                         hwaddr addr, unsigned len)
> +{
> +    uint32_t config_reg = PCI_HOST_BRIDGE(opaque)->config_reg;
> +
> +    if (config_reg & (1U << 31)) {
> +        int bus_num = (config_reg >> 16) & 0xFF;
> +        I440FXState *i440fx = I440FX_PCI_HOST_BRIDGE(opaque);
> +        PCIBus *target_bus = i440fx_find_target_bus(i440fx, bus_num);
> +
> +        if (target_bus) {
> +            return pci_data_read(target_bus, config_reg | (addr & 3), len);
> +        }
> +    }
> +
> +    return 0xffffffff;
> +}
> +
> +const MemoryRegionOps i440fx_pcihost_data_le_ops = {
> +    .read = i440fx_pcihost_data_read,
> +    .write = i440fx_pcihost_data_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +

Can't you just add pxb as a child bus?
If you do, pci_find_bus_nr will do the right thing
more or less automatically, no?


>  static void i440fx_pcihost_initfn(Object *obj)
>  {
>      PCIHostState *s = PCI_HOST_BRIDGE(obj);
>      I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
>  
> +    d->snooping_hosts = g_ptr_array_new();
> +
>      memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
>                            "pci-conf-idx", 4);
> -    memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
> +    memory_region_init_io(&s->data_mem, obj, &i440fx_pcihost_data_le_ops, s,
>                            "pci-conf-data", 4);
>  
>      object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
> @@ -400,6 +472,40 @@ PCIBus *find_i440fx(void)
>      return s ? s->bus : NULL;
>  }
>  
> +/*
> + * Returns 0 on successs, -1 if i440fx host was not
> + * found or the bus number is already in use.
> + */
> +int i440fx_register_snooping_host(PCIHostState *host)
> +{
> +    I440FXState *s = OBJECT_CHECK(I440FXState,
> +                                  object_resolve_path("/machine/i440fx", NULL),
> +                                  TYPE_I440FX_PCI_HOST_BRIDGE);
> +    uint8_t bus_num = pci_bus_num(host->bus);
> +    int i;
> +
> +    /* i440fx host not found */
> +    if (!s) {
> +        return -1;
> +    }
> +
> +    /* 0 is reserved for i440fx host bridge */
> +    if (bus_num == 0) {
> +        return -1;
> +    }
> +
> +    /* check if the bus number is unique */
> +    for (i = 0; i < s->snooping_hosts->len; i++) {
> +        PCIHostState *s_host = g_ptr_array_index(s->snooping_hosts, i);
> +        if (bus_num == pci_bus_num(s_host->bus)) {
> +            return -1;
> +        }
> +    }
> +
> +    g_ptr_array_add(s->snooping_hosts, host);
> +    return 0;
> +}
> +
>  /* PIIX3 PCI to ISA bridge */
>  static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
>  {
> @@ -742,6 +848,13 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
>      return "0000:00";
>  }
>  
> +static GPtrArray *i440fx_snooping_hosts(PCIHostState *host_bridge)
> +{
> +    I440FXState *s = I440FX_PCI_HOST_BRIDGE(host_bridge);
> +
> +    return s->snooping_hosts;
> +}
> +
>  static Property i440fx_props[] = {
>      DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, I440FXState,
>                       pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
> @@ -753,11 +866,13 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> +    PCIHostBridgeSnoopedClass *sc = TYPE_PCI_HOST_BRIDGE_SNOOPED_CLASS(klass);
>  
>      hc->root_bus_path = i440fx_pcihost_root_bus_path;
>      dc->realize = i440fx_pcihost_realize;
>      dc->fw_name = "pci";
>      dc->props = i440fx_props;
> +    sc->snooping_hosts = i440fx_snooping_hosts;
>  }
>  
>  static const TypeInfo i440fx_pcihost_info = {
> @@ -768,6 +883,7 @@ static const TypeInfo i440fx_pcihost_info = {
>      .class_init    = i440fx_pcihost_class_init,
>      .interfaces    = (InterfaceInfo[]) {
>          { TYPE_PCI_MAIN_HOST_BRIDGE },
> +        { TYPE_PCI_HOST_BRIDGE_SNOOPED },
>          { }
>      }
>  
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 288e74c..35e462b 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -20,6 +20,8 @@
>  
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_host.h"
> +#include "hw/pci/pci_bus.h"
> +#include "qemu/range.h"
>  #include "trace.h"
>  
>  /* debug PCI */
> @@ -32,6 +34,32 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
>  #define PCI_DPRINTF(fmt, ...)
>  #endif
>  
> +Range pci_host_get_buses_range(PCIHostState *host)

new api, needs documentation.


> +{
> +    uint8_t root_bus_num = pci_bus_num(host->bus);
> +    Range r = {root_bus_num, root_bus_num};
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(host->bus->devices); ++i) {
> +        PCIDevice *dev = host->bus->devices[i];
> +        PCIDeviceClass *pc;
> +
> +        if (!dev) {
> +            continue;
> +        }
> +
> +        pc = PCI_DEVICE_GET_CLASS(dev);
> +
> +        if (pc->is_bridge) {
> +            uint8_t subordinate = dev->config[PCI_SUBORDINATE_BUS];
> +            if (subordinate > r.end) {
> +                r.end = subordinate;
> +            }
> +        }
> +    }
> +
> +    return r;
> +}
>  /*
>   * PCI address
>   * bit 16 - 24: bus number
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 1b35168..a918a61 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -241,6 +241,12 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
>                      MemoryRegion *ram_memory);
>  
>  PCIBus *find_i440fx(void);
> +/*
> + * Register hosts without their own configuration registers
> + * to i440fx main PCI host bridge
> + */
> +int i440fx_register_snooping_host(PCIHostState *host);
> +
>  /* piix4.c */
>  extern PCIDevice *piix4_dev;
>  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index a041919..dc7ccb2 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -87,6 +87,9 @@ typedef struct PCIHostBridgeSnoopedClass {
>      GPtrArray *(*snooping_hosts)(PCIHostState *);
>  } PCIHostBridgeSnoopedClass;
>  
> +/* Returns the buses interval owned by the input host */
> +Range pci_host_get_buses_range(PCIHostState *host);
> +
>  /* common internal helpers for PCI/PCIe hosts, cut off overflows */
>  void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
>                                    uint32_t limit, uint32_t val, uint32_t len);
> -- 
> 2.1.0