Re: [Qemu-devel] [PATCH 2/2 RFC] s390x/pci: rework pci infrastructure modeling

[Michael S. Tsirkin]

On Tue, Mar 10, 2015 at 02:03:34PM +0100, Frank Blaschka wrote:
> This patch changes the modeling of the s390 qemu pci infrastructure to
> better match the actual pci architecture defined by the real hardware.
> 
> A pci host bridge like device (s390-pcihost) models the abstract view
> of the bare pci function. It provides s390 specific configuration
> attributes (fid and uid) for the attached pci device. The host bridge
> restrict the pci bus to just hold one single pci device. Also we have
> to make the s390 pci host bridge hot plugable.

This requirement is really because of the 1 device per bus
limitation, isn't it?
If you supported many devices per bus, you could use
hotplug there and there won't be need to support hotplug
of the host bridge.

> This is done by
> implementing a s390 specific bus to attach this new host bridge like
> devices.
> 
> Sample qemu configuration:
> -device s390-pcihost,fid=16,uid=2216
> -device e1000,bus=pci.0
> -device s390-pcihost,fid=17,uid=2217,id=mydev
> -device ne2k_pci,bus=mydev.0,addr=0
> 
> A pci device references the corresponding host bridge via pci bus name
> (as usual). The pci device must be attached to slot 0 of the bus.
>
> The fid and uid must be unique for the qemu instance. The design
> allows to define (static and hotplug) multiple host bridges and support
> a large number of pci devices.

How about sticking a pci to pci bridge behind the host bridge?
You could also support hotplug using shpc, all
this without writing code.


> Signed-off-by: Frank Blaschka <address@hidden>
> ---
>  hw/s390x/s390-pci-bus.c    | 174 
> ++++++++++++++++++++++++++-------------------
>  hw/s390x/s390-pci-bus.h    |  24 ++++++-
>  hw/s390x/s390-pci-inst.c   |   2 +-
>  hw/s390x/s390-virtio-ccw.c |   4 +-
>  4 files changed, 128 insertions(+), 76 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index dc455a2..6ad80d9 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -32,12 +32,8 @@ int chsc_sei_nt2_get_event(void *res)
>      PciCcdfErr *eccdf;
>      int rc = 1;
>      SeiContainer *sei_cont;
> -    S390pciState *s = S390_PCI_HOST_BRIDGE(
> -        object_resolve_path(TYPE_S390_PCI_HOST_BRIDGE, NULL));
> -
> -    if (!s) {
> -        return rc;
> -    }
> +    PCIFacility *s = PCI_FACILITY(
> +        object_resolve_path(TYPE_PCI_FACILITY, NULL));
>  
>      sei_cont = QTAILQ_FIRST(&s->pending_sei);
>      if (sei_cont) {
> @@ -71,31 +67,23 @@ int chsc_sei_nt2_get_event(void *res)
>  
>  int chsc_sei_nt2_have_event(void)
>  {
> -    S390pciState *s = S390_PCI_HOST_BRIDGE(
> -        object_resolve_path(TYPE_S390_PCI_HOST_BRIDGE, NULL));
> -
> -    if (!s) {
> -        return 0;
> -    }
> +    PCIFacility *s = PCI_FACILITY(
> +        object_resolve_path(TYPE_PCI_FACILITY, NULL));
>  
>      return !QTAILQ_EMPTY(&s->pending_sei);
>  }
>  
>  S390PCIBusDevice *s390_pci_find_dev_by_fid(uint32_t fid)
>  {
> -    S390PCIBusDevice *pbdev;
> -    int i;
> -    S390pciState *s = S390_PCI_HOST_BRIDGE(
> -        object_resolve_path(TYPE_S390_PCI_HOST_BRIDGE, NULL));
> +    BusChild *kid;
> +    S390pciState *state;
> +    PCIFacility *s = PCI_FACILITY(
> +        object_resolve_path(TYPE_PCI_FACILITY, NULL));
>  
> -    if (!s) {
> -        return NULL;
> -    }
> -
> -    for (i = 0; i < PCI_SLOT_MAX; i++) {
> -        pbdev = &s->pbdev[i];
> -        if ((pbdev->fh != 0) && (pbdev->fid == fid)) {
> -            return pbdev;
> +    QTAILQ_FOREACH(kid, &s->sbus.qbus.children, sibling) {
> +        state = (S390pciState *)kid->child;
> +        if ((state->pbdev[0].fh != 0) && (state->pbdev[0].fid == fid)) {
> +            return &state->pbdev[0];
>          }
>      }
>  
> @@ -125,37 +113,23 @@ void s390_pci_sclp_configure(int configure, SCCB *sccb)
>      return;
>  }
>  
> -static uint32_t s390_pci_get_pfid(PCIDevice *pdev)
> -{
> -    return PCI_SLOT(pdev->devfn);
> -}
> -
> -static uint32_t s390_pci_get_pfh(PCIDevice *pdev)
> -{
> -    return PCI_SLOT(pdev->devfn) | FH_VIRT;
> -}
> -
>  S390PCIBusDevice *s390_pci_find_dev_by_idx(uint32_t idx)
>  {
> -    S390PCIBusDevice *pbdev;
> -    int i;
> +    BusChild *kid;
>      int j = 0;
> -    S390pciState *s = S390_PCI_HOST_BRIDGE(
> -        object_resolve_path(TYPE_S390_PCI_HOST_BRIDGE, NULL));
> -
> -    if (!s) {
> -        return NULL;
> -    }
> +    S390pciState *state;
> +    PCIFacility *s = PCI_FACILITY(
> +        object_resolve_path(TYPE_PCI_FACILITY, NULL));
>  
> -    for (i = 0; i < PCI_SLOT_MAX; i++) {
> -        pbdev = &s->pbdev[i];
> +    QTAILQ_FOREACH(kid, &s->sbus.qbus.children, sibling) {
> +        state = (S390pciState *)kid->child;
>  
> -        if (pbdev->fh == 0) {
> +        if (state->pbdev[0].fh == 0) {
>              continue;
>          }
>  
>          if (j == idx) {
> -            return pbdev;
> +            return &state->pbdev[0];
>          }
>          j++;
>      }
> @@ -165,19 +139,19 @@ S390PCIBusDevice *s390_pci_find_dev_by_idx(uint32_t idx)
>  
>  S390PCIBusDevice *s390_pci_find_dev_by_fh(uint32_t fh)
>  {
> -    S390PCIBusDevice *pbdev;
> -    int i;
> -    S390pciState *s = S390_PCI_HOST_BRIDGE(
> -        object_resolve_path(TYPE_S390_PCI_HOST_BRIDGE, NULL));
> +    BusChild *kid;
> +    S390pciState *state;
> +    PCIFacility *s = PCI_FACILITY(
> +        object_resolve_path(TYPE_PCI_FACILITY, NULL));
>  
> -    if (!s || !fh) {
> +    if (!fh) {
>          return NULL;
>      }
>  
> -    for (i = 0; i < PCI_SLOT_MAX; i++) {
> -        pbdev = &s->pbdev[i];
> -        if (pbdev->fh == fh) {
> -            return pbdev;
> +    QTAILQ_FOREACH(kid, &s->sbus.qbus.children, sibling) {
> +        state = (S390pciState *)kid->child;
> +        if (state->pbdev[0].fh == fh) {
> +            return &state->pbdev[0];
>          }
>      }
>  
> @@ -188,12 +162,8 @@ static void s390_pci_generate_event(uint8_t cc, uint16_t 
> pec, uint32_t fh,
>                                      uint32_t fid, uint64_t faddr, uint32_t e)
>  {
>      SeiContainer *sei_cont;
> -    S390pciState *s = S390_PCI_HOST_BRIDGE(
> -        object_resolve_path(TYPE_S390_PCI_HOST_BRIDGE, NULL));
> -
> -    if (!s) {
> -        return;
> -    }
> +    PCIFacility *s = PCI_FACILITY(
> +        object_resolve_path(TYPE_PCI_FACILITY, NULL));
>  
>      sei_cont = g_malloc0(sizeof(SeiContainer));
>      sei_cont->fh = fh;
> @@ -461,15 +431,29 @@ static void s390_pcihost_init_as(S390pciState *s)
>      address_space_init(&s->msix_notify_as, &s->msix_notify_mr, "msix-pci");
>  }
>  
> -static int s390_pcihost_init(SysBusDevice *dev)
> +static void s390_pcihost_realize(DeviceState *dev, Error **errp)
>  {
>      PCIBus *b;
>      BusState *bus;
> -    PCIHostState *phb = PCI_HOST_BRIDGE(dev);
>      S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
> +    S390pciState *state;
> +    BusChild *kid;
> +    PCIFacility *fac = PCI_FACILITY(
> +        object_resolve_path(TYPE_PCI_FACILITY, NULL));
>  
>      DPRINTF("host_init\n");
>  
> +    QTAILQ_FOREACH(kid, &fac->sbus.qbus.children, sibling) {
> +        state = (S390pciState *)kid->child;
> +        if (state == s) {
> +            continue;
> +        }
> +        if (state->fid == s->fid || state->uid == s->uid) {
> +            error_setg(errp, "s390-pcihost needs unique fid and uid");
> +            return;
> +        }
> +    }
> +
>      b = pci_register_bus(DEVICE(dev), NULL,
>                           s390_pci_set_irq, s390_pci_map_irq, NULL,
>                           get_system_memory(), get_system_io(), 0, 64,
> @@ -479,9 +463,6 @@ static int s390_pcihost_init(SysBusDevice *dev)
>  
>      bus = BUS(b);
>      qbus_set_hotplug_handler(bus, DEVICE(dev), NULL);
> -    phb->bus = b;
> -    QTAILQ_INIT(&s->pending_sei);
> -    return 0;
>  }
>  
>  static int s390_pcihost_setup_msix(S390PCIBusDevice *pbdev)
> @@ -520,12 +501,18 @@ static void s390_pcihost_hot_plug(HotplugHandler 
> *hotplug_dev,
>      S390pciState *s = S390_PCI_HOST_BRIDGE(pci_device_root_bus(pci_dev)
>                                             ->qbus.parent);
>  
> +    if (PCI_SLOT(pci_dev->devfn) != 0) {
> +        error_setg(errp, "s390-pcihost only slot 0 allowed.");
> +        return;
> +    }
> +
>      pbdev = &s->pbdev[PCI_SLOT(pci_dev->devfn)];
>  
> -    pbdev->fid = s390_pci_get_pfid(pci_dev);
> +    pbdev->fid = s->fid;
> +    pbdev->uid = s->uid;
>      pbdev->pdev = pci_dev;
>      pbdev->configured = true;
> -    pbdev->fh = s390_pci_get_pfh(pci_dev);
> +    pbdev->fh = s->fid | FH_VIRT;
>  
>      s390_pcihost_setup_msix(pbdev);
>  
> @@ -560,14 +547,20 @@ static void s390_pcihost_hot_unplug(HotplugHandler 
> *hotplug_dev,
>      object_unparent(OBJECT(pci_dev));
>  }
>  
> +static Property s390_pcihost_properties[] = {
> +    DEFINE_PROP_UINT32("fid", S390pciState, fid, 0),
> +    DEFINE_PROP_UINT32("uid", S390pciState, uid, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void s390_pcihost_class_init(ObjectClass *klass, void *data)
>  {
> -    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>  
> -    dc->cannot_instantiate_with_device_add_yet = true;
> -    k->init = s390_pcihost_init;
> +    dc->props = s390_pcihost_properties;
> +    dc->bus_type = TYPE_PCI_FACILITY_BUS;
> +    dc->realize = s390_pcihost_realize;
>      hc->plug = s390_pcihost_hot_plug;
>      hc->unplug = s390_pcihost_hot_unplug;
>      msi_supported = true;
> @@ -575,7 +568,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, 
> void *data)
>  
>  static const TypeInfo s390_pcihost_info = {
>      .name          = TYPE_S390_PCI_HOST_BRIDGE,
> -    .parent        = TYPE_PCI_HOST_BRIDGE,
> +    .parent        = TYPE_DEVICE,
>      .instance_size = sizeof(S390pciState),
>      .class_init    = s390_pcihost_class_init,
>      .interfaces = (InterfaceInfo[]) {
> @@ -584,9 +577,46 @@ static const TypeInfo s390_pcihost_info = {
>      }
>  };
>  
> +static const TypeInfo pci_facility_bus_info = {
> +    .name = TYPE_PCI_FACILITY_BUS,
> +    .parent = TYPE_BUS,
> +};
> +
> +static void pci_facility_realize(DeviceState *qdev, Error **errp)
> +{
> +    PCIFacility *facility = PCI_FACILITY(qdev);
> +
> +    qbus_create_inplace(&facility->sbus, sizeof(facility->sbus),
> +                        TYPE_PCI_FACILITY_BUS, qdev, NULL);
> +
> +    qbus_set_hotplug_handler(&facility->sbus.qbus, qdev, NULL);
> +    QTAILQ_INIT(&facility->pending_sei);
> +}
> +
> +static void pci_facility_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = pci_facility_realize;
> +}
> +
> +static const TypeInfo pci_facility_info = {
> +    .name          = TYPE_PCI_FACILITY,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(PCIFacility),
> +    .class_init    = pci_facility_class_init,
> +    .class_size    = sizeof(PCIFacilityClass),
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
> +};
> +
>  static void s390_pci_register_types(void)
>  {
>      type_register_static(&s390_pcihost_info);
> +    type_register_static(&pci_facility_info);
> +    type_register_static(&pci_facility_bus_info);
>  }
>  
>  type_init(s390_pci_register_types)
> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> index 464a92e..78a5c83 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -220,6 +220,7 @@ typedef struct S390PCIBusDevice {
>      bool lgstg_blocked;
>      uint32_t fh;
>      uint32_t fid;
> +    uint32_t uid;
>      uint64_t g_iota;
>      uint64_t pba;
>      uint64_t pal;
> @@ -238,7 +239,8 @@ typedef struct S390pciState {
>      S390PCIBusDevice pbdev[PCI_SLOT_MAX];
>      AddressSpace msix_notify_as;
>      MemoryRegion msix_notify_mr;
> -    QTAILQ_HEAD(, SeiContainer) pending_sei;
> +    uint32_t fid;
> +    uint32_t uid;
>  } S390pciState;
>  
>  int chsc_sei_nt2_get_event(void *res);
> @@ -248,4 +250,24 @@ S390PCIBusDevice *s390_pci_find_dev_by_idx(uint32_t idx);
>  S390PCIBusDevice *s390_pci_find_dev_by_fh(uint32_t fh);
>  S390PCIBusDevice *s390_pci_find_dev_by_fid(uint32_t fid);
>  
> +#define TYPE_PCI_FACILITY "pci-facility"
> +#define TYPE_PCI_FACILITY_BUS "pci-facility-bus"
> +
> +#define PCI_FACILITY(obj) \
> +    OBJECT_CHECK(PCIFacility, (obj), TYPE_PCI_FACILITY)
> +
> +typedef struct PCIFacilityBus {
> +    BusState qbus;
> +} PCIFacilityBus;
> +
> +typedef struct PCIFacility {
> +    SysBusDevice parent_obj;
> +    PCIFacilityBus sbus;
> +    QTAILQ_HEAD(, SeiContainer) pending_sei;
> +} PCIFacility;
> +
> +typedef struct PCIFacilityClass {
> +    DeviceClass parent_class;
> +    int (*init)(PCIFacility *pf);
> +} PCIFacilityClass;
>  #endif
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 08d8aa6..d138b28 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -254,7 +254,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2)
>          stq_p(&resquery->edma, ZPCI_EDMA_ADDR);
>          stw_p(&resquery->pchid, 0);
>          stw_p(&resquery->ug, 1);
> -        stl_p(&resquery->uid, pbdev->fid);
> +        stl_p(&resquery->uid, pbdev->uid);
>          stw_p(&resquery->hdr.rsp, CLP_RC_OK);
>          break;
>      }
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 8f0ae59..358b192 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -129,8 +129,8 @@ static void ccw_init(MachineState *machine)
>                        machine->initrd_filename, "s390-ccw.img", true);
>      s390_flic_init();
>  
> -    dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
> -    object_property_add_child(qdev_get_machine(), TYPE_S390_PCI_HOST_BRIDGE,
> +    dev = qdev_create(NULL, TYPE_PCI_FACILITY);
> +    object_property_add_child(qdev_get_machine(), TYPE_PCI_FACILITY,
>                                OBJECT(dev), NULL);
>      qdev_init_nofail(dev);
>  
> -- 
> 2.1.4