[2/2,RFC] s390x/pci: rework pci infrastructure modeling

Message ID	1425992614-8938-3-git-send-email-blaschka@linux.vnet.ibm.com
State	New
Headers	show Return-Path: <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org> Gateway: Authorized Use Only! Violators will be prosecuted for <qemu-devel@nongnu.org> from <blaschka@linux.vnet.ibm.com>; Tue, 10 Mar 2015 13:03:45 -0000 Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 10 Mar 2015 13:03:42 -0000 From: Frank Blaschka <blaschka@linux.vnet.ibm.com> To: qemu-devel@nongnu.org, cornelia.huck@de.ibm.com, borntraeger@de.ibm.com, agraf@suse.de, mst@redhat.com Date: Tue, 10 Mar 2015 14:03:34 +0100 Message-Id: <1425992614-8938-3-git-send-email-blaschka@linux.vnet.ibm.com> In-Reply-To: <1425992614-8938-1-git-send-email-blaschka@linux.vnet.ibm.com> References: <1425992614-8938-1-git-send-email-blaschka@linux.vnet.ibm.com> Cc: MIHAJLOV@de.ibm.com, Frank Blaschka <blaschka@linux.vnet.ibm.com>, fiuczy@linux.vnet.ibm.com Subject: [Qemu-devel] [PATCH 2/2 RFC] s390x/pci: rework pci infrastructure modeling Precedence: list Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org

Frank Blaschka March 10, 2015, 1:03 p.m. UTC

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

Signed-off-by: Frank Blaschka <blaschka@linux.vnet.ibm.com>
---
 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(-)

Michael S. Tsirkin March 10, 2015, 2:26 p.m. UTC | #1

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 <blaschka@linux.vnet.ibm.com>
> ---
>  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

Frank Blaschka March 11, 2015, 2:38 p.m. UTC | #2

On Tue, Mar 10, 2015 at 03:26:23PM +0100, Michael S. Tsirkin wrote:
> 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.
>
Absolutely yes. Have you seen my first proposal?
It basically exploits the normal pci bridge/bus/slot mechanism but need
a place to store s390 specific configuration attributes.
The idea of a host bridge having this attributes and limit the bus
to one slot was an alternate design approach suggested by Alex.

I like Alex's idea because:
1) It reflects pretty well the actual nature of the pci system in real s390 hw
2) It does not create an somehow "artifical" pci topology

 
> > 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.
> 
Hm, I don't understand this in detail, can you elaborate a little bit more
on this?

For me it looks like this has the same issues like my first proposal. We
build a complete artifical pci topology in qemu, which has nothing to do with
the real hw. If we include pci 2 pci bridges this makes configuration even
more a nightmare for users.

Do you think detangle pci bus from bridge breaks some fundamental design?
If so, I would rather go with my first proposal than adding even more
complexity to implementation and configuration.

Thx, Frank
> 
> > Signed-off-by: Frank Blaschka <blaschka@linux.vnet.ibm.com>
> > ---
> >  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
>

Michael S. Tsirkin March 11, 2015, 2:57 p.m. UTC | #3

On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
> I like Alex's idea because:
> 1) It reflects pretty well the actual nature of the pci system in real s390 hw

why do you say this? does real hw has this
one device per bridge limit?

Michael S. Tsirkin March 11, 2015, 5:42 p.m. UTC | #4

On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
> On Tue, Mar 10, 2015 at 03:26:23PM +0100, Michael S. Tsirkin wrote:
> > 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.
> >
> Absolutely yes. Have you seen my first proposal?
> It basically exploits the normal pci bridge/bus/slot mechanism but need
> a place to store s390 specific configuration attributes.
>
> The idea of a host bridge having this attributes and limit the bus
> to one slot was an alternate design approach suggested by Alex.
> 
> I like Alex's idea because:
> 1) It reflects pretty well the actual nature of the pci system in real s390 hw
> 2) It does not create an somehow "artifical" pci topology
> 

I'll have to re-read but here's a thought: use your patch but
remove host bridge hotplug support code.
Stick a standard bridge with shpc support in the single slot
behind your host bridge (existing pci-bridge-dev should do the trick,
though not many people use it, so you might
run into bugs, but fixing them is a good idea anyway).
You can instanciate it automatically like Marcel's patches do
for PXB.

You get hotplug + multiple devices for free.

And the resulting topology is clearly something that *can* exist
on bare-metal, since no one can prevent users sticking
a pci bridge device in a pci slot.

Why do I prefer pci hotplug to s390 hotplug?
s390 hotplug is really surprise removal. There's no way
to get ack from guest. Linux isn't very well equipped to
handle that, we might be able to fix it with time but current
virtio drivers aren't very happy.

>  
> > > 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.
> > 
> Hm, I don't understand this in detail, can you elaborate a little bit more
> on this?
> 
> For me it looks like this has the same issues like my first proposal. We
> build a complete artifical pci topology in qemu, which has nothing to do with
> the real hw. If we include pci 2 pci bridges this makes configuration even
> more a nightmare for users.
> 
> Do you think detangle pci bus from bridge breaks some fundamental design?
> If so, I would rather go with my first proposal than adding even more
> complexity to implementation and configuration.
> 
> Thx, Frank
> > 
> > > Signed-off-by: Frank Blaschka <blaschka@linux.vnet.ibm.com>
> > > ---
> > >  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
> >

Frank Blaschka March 12, 2015, 8:46 a.m. UTC | #5

On Wed, Mar 11, 2015 at 03:57:05PM +0100, Michael S. Tsirkin wrote:
> On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
> > I like Alex's idea because:
> > 1) It reflects pretty well the actual nature of the pci system in real s390 hw
> 
> why do you say this? does real hw has this
> one device per bridge limit?
> 
Actually we don't know. HW does not expose this information. All we know is each
pci function is completely separated. So it is a good assumption to have a separate
bridge/bus for each pci function. By the way the Linux kernel for s390 makes the
same assumption by creating a new pci domain for each function. You may want
to read the cover letter again to get more technical details on this.

Michael S. Tsirkin March 12, 2015, 9:26 a.m. UTC | #6

On Thu, Mar 12, 2015 at 09:46:44AM +0100, Frank Blaschka wrote:
> On Wed, Mar 11, 2015 at 03:57:05PM +0100, Michael S. Tsirkin wrote:
> > On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
> > > I like Alex's idea because:
> > > 1) It reflects pretty well the actual nature of the pci system in real s390 hw
> > 
> > why do you say this? does real hw has this
> > one device per bridge limit?
> > 
> Actually we don't know. HW does not expose this information. All we know is each
> pci function is completely separated. So it is a good assumption to have a separate
> bridge/bus for each pci function. By the way the Linux kernel for s390 makes the
> same assumption by creating a new pci domain for each function. You may want
> to read the cover letter again to get more technical details on this.

I'm sorry, my question wasn't clear.
I'll try to rephrase.
Imagine that I'm using s390, and I add a pci to pci bridge:

-device s390-pcihost,fid=17,uid=2217,id=mydev
-device ne2k_pci,bus=mydev.0,addr=0
-device pci-bridge,chassis_nr=2,bus=bus=mydev.0,addr=0,id=newdev
-device ne2k_pci,bus=newdev,addr=8

What happens with your scheme then?

It seems reasonable to assume that this is a legal configuration on real
hardware.

Frank Blaschka March 12, 2015, 9:54 a.m. UTC | #7

On Wed, Mar 11, 2015 at 06:42:34PM +0100, Michael S. Tsirkin wrote:
> On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
> > On Tue, Mar 10, 2015 at 03:26:23PM +0100, Michael S. Tsirkin wrote:
> > > 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.
> > >
> > Absolutely yes. Have you seen my first proposal?
> > It basically exploits the normal pci bridge/bus/slot mechanism but need
> > a place to store s390 specific configuration attributes.
> >
> > The idea of a host bridge having this attributes and limit the bus
> > to one slot was an alternate design approach suggested by Alex.
> > 
> > I like Alex's idea because:
> > 1) It reflects pretty well the actual nature of the pci system in real s390 hw
> > 2) It does not create an somehow "artifical" pci topology
> > 
> 
> I'll have to re-read but here's a thought: use your patch but
> remove host bridge hotplug support code.
> Stick a standard bridge with shpc support in the single slot
> behind your host bridge (existing pci-bridge-dev should do the trick,
> though not many people use it, so you might
> run into bugs, but fixing them is a good idea anyway).
> You can instanciate it automatically like Marcel's patches do
> for PXB.
Still don't undertsand so I try to summarize in my words please corrent me
if I got something wrong

- create a standard host bridge
- change the s390-pcihost to be a pci 2 pci bridge
- now we can hotplug the s390-pcihost + hotplug a pci device to this
  s390-pcihost using standard pci hotplug mechanism
- we keep the 1 slot limit on the s390-pcihost. We need a place to
  store fid and uid information (see mail thread from my 1 proposal)
- If we need more than 32 pci functions we have to extend the primary pci bus
  via standart pci 2 pci bridges or add another standart host bridge

Is this your suggestion?

> 
> You get hotplug + multiple devices for free.
> 
> And the resulting topology is clearly something that *can* exist
> on bare-metal, since no one can prevent users sticking
> a pci bridge device in a pci slot.

Again on bar s390 metal we do not have bridge bus slot at all.
OS does not have any knowledge about it. So anything we try to model
is somehow artificial.

> 
> Why do I prefer pci hotplug to s390 hotplug?
> s390 hotplug is really surprise removal. There's no way

Don't understand this neither. s390 linux kernel is well prepared
to receive s390 specific events to signal a pci function is
going away. Anyhow we end up I can generate this events

> to get ack from guest. Linux isn't very well equipped to
> handle that, we might be able to fix it with time but current
> virtio drivers aren't very happy.
> 
> >  
> > > > 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.
> > > 
> > Hm, I don't understand this in detail, can you elaborate a little bit more
> > on this?
> > 
> > For me it looks like this has the same issues like my first proposal. We
> > build a complete artifical pci topology in qemu, which has nothing to do with
> > the real hw. If we include pci 2 pci bridges this makes configuration even
> > more a nightmare for users.
> > 
> > Do you think detangle pci bus from bridge breaks some fundamental design?
> > If so, I would rather go with my first proposal than adding even more
> > complexity to implementation and configuration.
> > 
> > Thx, Frank
> > > 
> > > > Signed-off-by: Frank Blaschka <blaschka@linux.vnet.ibm.com>
> > > > ---
> > > >  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
> > > 
>

Michael S. Tsirkin March 12, 2015, 10:03 a.m. UTC | #8

On Thu, Mar 12, 2015 at 10:54:24AM +0100, Frank Blaschka wrote:
> On Wed, Mar 11, 2015 at 06:42:34PM +0100, Michael S. Tsirkin wrote:
> > On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
> > > On Tue, Mar 10, 2015 at 03:26:23PM +0100, Michael S. Tsirkin wrote:
> > > > 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.
> > > >
> > > Absolutely yes. Have you seen my first proposal?
> > > It basically exploits the normal pci bridge/bus/slot mechanism but need
> > > a place to store s390 specific configuration attributes.
> > >
> > > The idea of a host bridge having this attributes and limit the bus
> > > to one slot was an alternate design approach suggested by Alex.
> > > 
> > > I like Alex's idea because:
> > > 1) It reflects pretty well the actual nature of the pci system in real s390 hw
> > > 2) It does not create an somehow "artifical" pci topology
> > > 
> > 
> > I'll have to re-read but here's a thought: use your patch but
> > remove host bridge hotplug support code.
> > Stick a standard bridge with shpc support in the single slot
> > behind your host bridge (existing pci-bridge-dev should do the trick,
> > though not many people use it, so you might
> > run into bugs, but fixing them is a good idea anyway).
> > You can instanciate it automatically like Marcel's patches do
> > for PXB.
> Still don't undertsand so I try to summarize in my words please corrent me
> if I got something wrong
> 
> - create a standard host bridge
> - change the s390-pcihost to be a pci 2 pci bridge

Actually I suggested simply adding a pci 2 pci bridge behind
s390-pcihost.

> - now we can hotplug the s390-pcihost + hotplug a pci device to this
>   s390-pcihost using standard pci hotplug mechanism

My idea was to just hotplug a pci device behind the standard pci 2 pci
bridge. don't support hotplugging bridge itself or s390-pcihost itself.

> - we keep the 1 slot limit on the s390-pcihost. We need a place to
>   store fid and uid information (see mail thread from my 1 proposal)

Yes.

> - If we need more than 32 pci functions we have to extend the primary pci bus
>   via standart pci 2 pci bridges or add another standart host bridge
> 
> Is this your suggestion?

Almost, clarifications above.

> > 
> > You get hotplug + multiple devices for free.
> > 
> > And the resulting topology is clearly something that *can* exist
> > on bare-metal, since no one can prevent users sticking
> > a pci bridge device in a pci slot.
> 
> Again on bar s390 metal we do not have bridge bus slot at all.
> OS does not have any knowledge about it. So anything we try to model
> is somehow artificial.

So I wonder: what does guest see if you do it like the above in QEMU?
Do s390 guests even have ability to enumerate pci to pci bridges?

> > 
> > Why do I prefer pci hotplug to s390 hotplug?
> > s390 hotplug is really surprise removal. There's no way
> 
> Don't understand this neither. s390 linux kernel is well prepared
> to receive s390 specific events to signal a pci function is
> going away. Anyhow we end up I can generate this events
> 
> > to get ack from guest. Linux isn't very well equipped to
> > handle that, we might be able to fix it with time but current
> > virtio drivers aren't very happy.
> > 
> > >  
> > > > > 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.
> > > > 
> > > Hm, I don't understand this in detail, can you elaborate a little bit more
> > > on this?
> > > 
> > > For me it looks like this has the same issues like my first proposal. We
> > > build a complete artifical pci topology in qemu, which has nothing to do with
> > > the real hw. If we include pci 2 pci bridges this makes configuration even
> > > more a nightmare for users.
> > > 
> > > Do you think detangle pci bus from bridge breaks some fundamental design?
> > > If so, I would rather go with my first proposal than adding even more
> > > complexity to implementation and configuration.
> > > 
> > > Thx, Frank
> > > > 
> > > > > Signed-off-by: Frank Blaschka <blaschka@linux.vnet.ibm.com>
> > > > > ---
> > > > >  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
> > > > 
> >

Frank Blaschka March 12, 2015, 10:50 a.m. UTC | #9

On Thu, Mar 12, 2015 at 11:03:50AM +0100, Michael S. Tsirkin wrote:
> On Thu, Mar 12, 2015 at 10:54:24AM +0100, Frank Blaschka wrote:
> > On Wed, Mar 11, 2015 at 06:42:34PM +0100, Michael S. Tsirkin wrote:
> > > On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
> > > > On Tue, Mar 10, 2015 at 03:26:23PM +0100, Michael S. Tsirkin wrote:
> > > > > 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.
> > > > >
> > > > Absolutely yes. Have you seen my first proposal?
> > > > It basically exploits the normal pci bridge/bus/slot mechanism but need
> > > > a place to store s390 specific configuration attributes.
> > > >
> > > > The idea of a host bridge having this attributes and limit the bus
> > > > to one slot was an alternate design approach suggested by Alex.
> > > > 
> > > > I like Alex's idea because:
> > > > 1) It reflects pretty well the actual nature of the pci system in real s390 hw
> > > > 2) It does not create an somehow "artifical" pci topology
> > > > 
> > > 
> > > I'll have to re-read but here's a thought: use your patch but
> > > remove host bridge hotplug support code.
> > > Stick a standard bridge with shpc support in the single slot
> > > behind your host bridge (existing pci-bridge-dev should do the trick,
> > > though not many people use it, so you might
> > > run into bugs, but fixing them is a good idea anyway).
> > > You can instanciate it automatically like Marcel's patches do
> > > for PXB.
> > Still don't undertsand so I try to summarize in my words please corrent me
> > if I got something wrong
> > 
> > - create a standard host bridge
> > - change the s390-pcihost to be a pci 2 pci bridge
> 
> Actually I suggested simply adding a pci 2 pci bridge behind
> s390-pcihost.
> 
> > - now we can hotplug the s390-pcihost + hotplug a pci device to this
> >   s390-pcihost using standard pci hotplug mechanism
> 
> My idea was to just hotplug a pci device behind the standard pci 2 pci
> bridge. don't support hotplugging bridge itself or s390-pcihost itself.
> 
> > - we keep the 1 slot limit on the s390-pcihost. We need a place to
> >   store fid and uid information (see mail thread from my 1 proposal)
> 
> Yes.
> 
> > - If we need more than 32 pci functions we have to extend the primary pci bus
> >   via standart pci 2 pci bridges or add another standart host bridge
> > 
> > Is this your suggestion?
> 
> Almost, clarifications above.
>
OK, got your idea. Have to think about it and may do some prototyping. THX!
 
> > > 
> > > You get hotplug + multiple devices for free.
> > > 
> > > And the resulting topology is clearly something that *can* exist
> > > on bare-metal, since no one can prevent users sticking
> > > a pci bridge device in a pci slot.
> > 
> > Again on bar s390 metal we do not have bridge bus slot at all.
> > OS does not have any knowledge about it. So anything we try to model
> > is somehow artificial.
> 
> So I wonder: what does guest see if you do it like the above in QEMU?
> Do s390 guests even have ability to enumerate pci to pci bridges?
I think the answer is no. All a s390 guest can do is call a s390 
instruction to get a list of pci functions. There is no further
information about the function. The fact there might be a bridge/bus/slot
is completely abstracted in hw and not exposed to the OS.

I think the fact, the hw guaranties a pci function is completetly
independent, made the kernel folks to decide to enumarate the domain.
Looking at a pci address on the guest (or in general on s390 kernel)
you will never see a bus or slot != 0 (xxxx:00:00.0)

Anyhow what we are doing do model pci in qemu, the guest will only see
a list of pci functions and enumerate the domain to create a pci
address and that's it 

> 
> > > 
> > > Why do I prefer pci hotplug to s390 hotplug?
> > > s390 hotplug is really surprise removal. There's no way
> > 
> > Don't understand this neither. s390 linux kernel is well prepared
> > to receive s390 specific events to signal a pci function is
> > going away. Anyhow we end up I can generate this events
> > 
> > > to get ack from guest. Linux isn't very well equipped to
> > > handle that, we might be able to fix it with time but current
> > > virtio drivers aren't very happy.
> > > 
> > > >  
> > > > > > 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.
> > > > > 
> > > > Hm, I don't understand this in detail, can you elaborate a little bit more
> > > > on this?
> > > > 
> > > > For me it looks like this has the same issues like my first proposal. We
> > > > build a complete artifical pci topology in qemu, which has nothing to do with
> > > > the real hw. If we include pci 2 pci bridges this makes configuration even
> > > > more a nightmare for users.
> > > > 
> > > > Do you think detangle pci bus from bridge breaks some fundamental design?
> > > > If so, I would rather go with my first proposal than adding even more
> > > > complexity to implementation and configuration.
> > > > 
> > > > Thx, Frank
> > > > > 
> > > > > > Signed-off-by: Frank Blaschka <blaschka@linux.vnet.ibm.com>
> > > > > > ---
> > > > > >  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
> > > > > 
> > > 
>

Frank Blaschka March 12, 2015, 1:16 p.m. UTC | #10

On Thu, Mar 12, 2015 at 11:50:02AM +0100, Frank Blaschka wrote:
> On Thu, Mar 12, 2015 at 11:03:50AM +0100, Michael S. Tsirkin wrote:
> > On Thu, Mar 12, 2015 at 10:54:24AM +0100, Frank Blaschka wrote:
> > > On Wed, Mar 11, 2015 at 06:42:34PM +0100, Michael S. Tsirkin wrote:
> > > > On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
> > > > > On Tue, Mar 10, 2015 at 03:26:23PM +0100, Michael S. Tsirkin wrote:
> > > > > > 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.
> > > > > >
> > > > > Absolutely yes. Have you seen my first proposal?
> > > > > It basically exploits the normal pci bridge/bus/slot mechanism but need
> > > > > a place to store s390 specific configuration attributes.
> > > > >
> > > > > The idea of a host bridge having this attributes and limit the bus
> > > > > to one slot was an alternate design approach suggested by Alex.
> > > > > 
> > > > > I like Alex's idea because:
> > > > > 1) It reflects pretty well the actual nature of the pci system in real s390 hw
> > > > > 2) It does not create an somehow "artifical" pci topology
> > > > > 
> > > > 
> > > > I'll have to re-read but here's a thought: use your patch but
> > > > remove host bridge hotplug support code.
> > > > Stick a standard bridge with shpc support in the single slot
> > > > behind your host bridge (existing pci-bridge-dev should do the trick,
> > > > though not many people use it, so you might
> > > > run into bugs, but fixing them is a good idea anyway).
> > > > You can instanciate it automatically like Marcel's patches do
> > > > for PXB.
> > > Still don't undertsand so I try to summarize in my words please corrent me
> > > if I got something wrong
> > > 
> > > - create a standard host bridge
> > > - change the s390-pcihost to be a pci 2 pci bridge
> > 
> > Actually I suggested simply adding a pci 2 pci bridge behind
> > s390-pcihost.
> > 
> > > - now we can hotplug the s390-pcihost + hotplug a pci device to this
> > >   s390-pcihost using standard pci hotplug mechanism
> > 
> > My idea was to just hotplug a pci device behind the standard pci 2 pci
> > bridge. don't support hotplugging bridge itself or s390-pcihost itself.
> > 
> > > - we keep the 1 slot limit on the s390-pcihost. We need a place to
> > >   store fid and uid information (see mail thread from my 1 proposal)
> > 
> > Yes.
> > 
> > > - If we need more than 32 pci functions we have to extend the primary pci bus
> > >   via standart pci 2 pci bridges or add another standart host bridge
> > > 
> > > Is this your suggestion?
> > 
> > Almost, clarifications above.
> >
> OK, got your idea. Have to think about it and may do some prototyping. THX!
>

hm, after thinking more about this I realized this is not working for us.
Remember we need a place to store the fid and uid attributes. This place
must be:
1) uid/fid per pci device
2) uid/fid in a hotplugable device

I have the feeling we are at the beginning again. Although I liked Alex's
idea (host bridge containing uid/fid and having only 1 slot on the bus), it
looks like we end up at my first proposal. This does not require any
modification in base pci/bus code.

Thx to all of you for the discussion and suggestions.

> > > > 
> > > > You get hotplug + multiple devices for free.
> > > > 
> > > > And the resulting topology is clearly something that *can* exist
> > > > on bare-metal, since no one can prevent users sticking
> > > > a pci bridge device in a pci slot.
> > > 
> > > Again on bar s390 metal we do not have bridge bus slot at all.
> > > OS does not have any knowledge about it. So anything we try to model
> > > is somehow artificial.
> > 
> > So I wonder: what does guest see if you do it like the above in QEMU?
> > Do s390 guests even have ability to enumerate pci to pci bridges?
> I think the answer is no. All a s390 guest can do is call a s390 
> instruction to get a list of pci functions. There is no further
> information about the function. The fact there might be a bridge/bus/slot
> is completely abstracted in hw and not exposed to the OS.
> 
> I think the fact, the hw guaranties a pci function is completetly
> independent, made the kernel folks to decide to enumarate the domain.
> Looking at a pci address on the guest (or in general on s390 kernel)
> you will never see a bus or slot != 0 (xxxx:00:00.0)
> 
> Anyhow what we are doing do model pci in qemu, the guest will only see
> a list of pci functions and enumerate the domain to create a pci
> address and that's it 
> 
> > 
> > > > 
> > > > Why do I prefer pci hotplug to s390 hotplug?
> > > > s390 hotplug is really surprise removal. There's no way
> > > 
> > > Don't understand this neither. s390 linux kernel is well prepared
> > > to receive s390 specific events to signal a pci function is
> > > going away. Anyhow we end up I can generate this events
> > > 
> > > > to get ack from guest. Linux isn't very well equipped to
> > > > handle that, we might be able to fix it with time but current
> > > > virtio drivers aren't very happy.
> > > > 
> > > > >  
> > > > > > > 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.
> > > > > > 
> > > > > Hm, I don't understand this in detail, can you elaborate a little bit more
> > > > > on this?
> > > > > 
> > > > > For me it looks like this has the same issues like my first proposal. We
> > > > > build a complete artifical pci topology in qemu, which has nothing to do with
> > > > > the real hw. If we include pci 2 pci bridges this makes configuration even
> > > > > more a nightmare for users.
> > > > > 
> > > > > Do you think detangle pci bus from bridge breaks some fundamental design?
> > > > > If so, I would rather go with my first proposal than adding even more
> > > > > complexity to implementation and configuration.
> > > > > 
> > > > > Thx, Frank
> > > > > > 
> > > > > > > Signed-off-by: Frank Blaschka <blaschka@linux.vnet.ibm.com>
> > > > > > > ---
> > > > > > >  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
> > > > > > 
> > > > 
> > 
> 
>

Alexander Graf March 12, 2015, 2:59 p.m. UTC | #11

On 12.03.15 08:16, Frank Blaschka wrote:
> On Thu, Mar 12, 2015 at 11:50:02AM +0100, Frank Blaschka wrote:
>> On Thu, Mar 12, 2015 at 11:03:50AM +0100, Michael S. Tsirkin wrote:
>>> On Thu, Mar 12, 2015 at 10:54:24AM +0100, Frank Blaschka wrote:
>>>> On Wed, Mar 11, 2015 at 06:42:34PM +0100, Michael S. Tsirkin wrote:
>>>>> On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
>>>>>> On Tue, Mar 10, 2015 at 03:26:23PM +0100, Michael S. Tsirkin wrote:
>>>>>>> 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.
>>>>>>>
>>>>>> Absolutely yes. Have you seen my first proposal?
>>>>>> It basically exploits the normal pci bridge/bus/slot mechanism but need
>>>>>> a place to store s390 specific configuration attributes.
>>>>>>
>>>>>> The idea of a host bridge having this attributes and limit the bus
>>>>>> to one slot was an alternate design approach suggested by Alex.
>>>>>>
>>>>>> I like Alex's idea because:
>>>>>> 1) It reflects pretty well the actual nature of the pci system in real s390 hw
>>>>>> 2) It does not create an somehow "artifical" pci topology
>>>>>>
>>>>>
>>>>> I'll have to re-read but here's a thought: use your patch but
>>>>> remove host bridge hotplug support code.
>>>>> Stick a standard bridge with shpc support in the single slot
>>>>> behind your host bridge (existing pci-bridge-dev should do the trick,
>>>>> though not many people use it, so you might
>>>>> run into bugs, but fixing them is a good idea anyway).
>>>>> You can instanciate it automatically like Marcel's patches do
>>>>> for PXB.
>>>> Still don't undertsand so I try to summarize in my words please corrent me
>>>> if I got something wrong
>>>>
>>>> - create a standard host bridge
>>>> - change the s390-pcihost to be a pci 2 pci bridge
>>>
>>> Actually I suggested simply adding a pci 2 pci bridge behind
>>> s390-pcihost.
>>>
>>>> - now we can hotplug the s390-pcihost + hotplug a pci device to this
>>>>   s390-pcihost using standard pci hotplug mechanism
>>>
>>> My idea was to just hotplug a pci device behind the standard pci 2 pci
>>> bridge. don't support hotplugging bridge itself or s390-pcihost itself.
>>>
>>>> - we keep the 1 slot limit on the s390-pcihost. We need a place to
>>>>   store fid and uid information (see mail thread from my 1 proposal)
>>>
>>> Yes.
>>>
>>>> - If we need more than 32 pci functions we have to extend the primary pci bus
>>>>   via standart pci 2 pci bridges or add another standart host bridge
>>>>
>>>> Is this your suggestion?
>>>
>>> Almost, clarifications above.
>>>
>> OK, got your idea. Have to think about it and may do some prototyping. THX!
>>
> 
> hm, after thinking more about this I realized this is not working for us.
> Remember we need a place to store the fid and uid attributes. This place
> must be:
> 1) uid/fid per pci device
> 2) uid/fid in a hotplugable device
> 
> I have the feeling we are at the beginning again. Although I liked Alex's
> idea (host bridge containing uid/fid and having only 1 slot on the bus), it
> looks like we end up at my first proposal. This does not require any
> modification in base pci/bus code.
> 
> Thx to all of you for the discussion and suggestions.

I disagree with the assessment. The reason mst was opposed to do the
one-phb-per-device implementation (which is the closest we can get to
model things like real hardware FWIW) was that hotplug would work on the
s390 level rather than pci. I don't see how your first proposal fixes that.

Also Michael, PCI on s390 is very very special. You can't plug in
anything that does not come from IBM. There are no PCIe connectors -
instead you have IBM proprietary slots that only work with IBM approved
devices. So things like "we can plug in a PCI bridge" simply don't work
as well in that world.


Alex

Michael S. Tsirkin March 12, 2015, 3:18 p.m. UTC | #12

On Thu, Mar 12, 2015 at 02:16:07PM +0100, Frank Blaschka wrote:
> On Thu, Mar 12, 2015 at 11:50:02AM +0100, Frank Blaschka wrote:
> > On Thu, Mar 12, 2015 at 11:03:50AM +0100, Michael S. Tsirkin wrote:
> > > On Thu, Mar 12, 2015 at 10:54:24AM +0100, Frank Blaschka wrote:
> > > > On Wed, Mar 11, 2015 at 06:42:34PM +0100, Michael S. Tsirkin wrote:
> > > > > On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
> > > > > > On Tue, Mar 10, 2015 at 03:26:23PM +0100, Michael S. Tsirkin wrote:
> > > > > > > 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.
> > > > > > >
> > > > > > Absolutely yes. Have you seen my first proposal?
> > > > > > It basically exploits the normal pci bridge/bus/slot mechanism but need
> > > > > > a place to store s390 specific configuration attributes.
> > > > > >
> > > > > > The idea of a host bridge having this attributes and limit the bus
> > > > > > to one slot was an alternate design approach suggested by Alex.
> > > > > > 
> > > > > > I like Alex's idea because:
> > > > > > 1) It reflects pretty well the actual nature of the pci system in real s390 hw
> > > > > > 2) It does not create an somehow "artifical" pci topology
> > > > > > 
> > > > > 
> > > > > I'll have to re-read but here's a thought: use your patch but
> > > > > remove host bridge hotplug support code.
> > > > > Stick a standard bridge with shpc support in the single slot
> > > > > behind your host bridge (existing pci-bridge-dev should do the trick,
> > > > > though not many people use it, so you might
> > > > > run into bugs, but fixing them is a good idea anyway).
> > > > > You can instanciate it automatically like Marcel's patches do
> > > > > for PXB.
> > > > Still don't undertsand so I try to summarize in my words please corrent me
> > > > if I got something wrong
> > > > 
> > > > - create a standard host bridge
> > > > - change the s390-pcihost to be a pci 2 pci bridge
> > > 
> > > Actually I suggested simply adding a pci 2 pci bridge behind
> > > s390-pcihost.
> > > 
> > > > - now we can hotplug the s390-pcihost + hotplug a pci device to this
> > > >   s390-pcihost using standard pci hotplug mechanism
> > > 
> > > My idea was to just hotplug a pci device behind the standard pci 2 pci
> > > bridge. don't support hotplugging bridge itself or s390-pcihost itself.
> > > 
> > > > - we keep the 1 slot limit on the s390-pcihost. We need a place to
> > > >   store fid and uid information (see mail thread from my 1 proposal)
> > > 
> > > Yes.
> > > 
> > > > - If we need more than 32 pci functions we have to extend the primary pci bus
> > > >   via standart pci 2 pci bridges or add another standart host bridge
> > > > 
> > > > Is this your suggestion?
> > > 
> > > Almost, clarifications above.
> > >
> > OK, got your idea. Have to think about it and may do some prototyping. THX!
> >
> 
> hm, after thinking more about this I realized this is not working for us.
> Remember we need a place to store the fid and uid attributes. This place
> must be:
> 1) uid/fid per pci device
> 2) uid/fid in a hotplugable device
> 
> I have the feeling we are at the beginning again. Although I liked Alex's
> idea (host bridge containing uid/fid and having only 1 slot on the bus), it
> looks like we end up at my first proposal. This does not require any
> modification in base pci/bus code.
> 
> Thx to all of you for the discussion and suggestions.

It's up to you, but don't get discouraged if you do like
Alex's suggestion better after all.
I'm not nacking this approach, just trying to figure it all
out, and asking whether alternatives would work.

> > > > > 
> > > > > You get hotplug + multiple devices for free.
> > > > > 
> > > > > And the resulting topology is clearly something that *can* exist
> > > > > on bare-metal, since no one can prevent users sticking
> > > > > a pci bridge device in a pci slot.
> > > > 
> > > > Again on bar s390 metal we do not have bridge bus slot at all.
> > > > OS does not have any knowledge about it. So anything we try to model
> > > > is somehow artificial.
> > > 
> > > So I wonder: what does guest see if you do it like the above in QEMU?
> > > Do s390 guests even have ability to enumerate pci to pci bridges?
> > I think the answer is no. All a s390 guest can do is call a s390 
> > instruction to get a list of pci functions. There is no further
> > information about the function. The fact there might be a bridge/bus/slot
> > is completely abstracted in hw and not exposed to the OS.
> > 
> > I think the fact, the hw guaranties a pci function is completetly
> > independent, made the kernel folks to decide to enumarate the domain.
> > Looking at a pci address on the guest (or in general on s390 kernel)
> > you will never see a bus or slot != 0 (xxxx:00:00.0)
> > 
> > Anyhow what we are doing do model pci in qemu, the guest will only see
> > a list of pci functions and enumerate the domain to create a pci
> > address and that's it 
> > 
> > > 
> > > > > 
> > > > > Why do I prefer pci hotplug to s390 hotplug?
> > > > > s390 hotplug is really surprise removal. There's no way
> > > > 
> > > > Don't understand this neither. s390 linux kernel is well prepared
> > > > to receive s390 specific events to signal a pci function is
> > > > going away. Anyhow we end up I can generate this events
> > > > 
> > > > > to get ack from guest. Linux isn't very well equipped to
> > > > > handle that, we might be able to fix it with time but current
> > > > > virtio drivers aren't very happy.
> > > > > 
> > > > > >  
> > > > > > > > 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.
> > > > > > > 
> > > > > > Hm, I don't understand this in detail, can you elaborate a little bit more
> > > > > > on this?
> > > > > > 
> > > > > > For me it looks like this has the same issues like my first proposal. We
> > > > > > build a complete artifical pci topology in qemu, which has nothing to do with
> > > > > > the real hw. If we include pci 2 pci bridges this makes configuration even
> > > > > > more a nightmare for users.
> > > > > > 
> > > > > > Do you think detangle pci bus from bridge breaks some fundamental design?
> > > > > > If so, I would rather go with my first proposal than adding even more
> > > > > > complexity to implementation and configuration.
> > > > > > 
> > > > > > Thx, Frank
> > > > > > > 
> > > > > > > > Signed-off-by: Frank Blaschka <blaschka@linux.vnet.ibm.com>
> > > > > > > > ---
> > > > > > > >  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
> > > > > > > 
> > > > > 
> > > 
> > 
> >

Michael S. Tsirkin March 12, 2015, 3:22 p.m. UTC | #13

On Thu, Mar 12, 2015 at 09:59:59AM -0500, Alexander Graf wrote:
> 
> 
> On 12.03.15 08:16, Frank Blaschka wrote:
> > On Thu, Mar 12, 2015 at 11:50:02AM +0100, Frank Blaschka wrote:
> >> On Thu, Mar 12, 2015 at 11:03:50AM +0100, Michael S. Tsirkin wrote:
> >>> On Thu, Mar 12, 2015 at 10:54:24AM +0100, Frank Blaschka wrote:
> >>>> On Wed, Mar 11, 2015 at 06:42:34PM +0100, Michael S. Tsirkin wrote:
> >>>>> On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
> >>>>>> On Tue, Mar 10, 2015 at 03:26:23PM +0100, Michael S. Tsirkin wrote:
> >>>>>>> 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.
> >>>>>>>
> >>>>>> Absolutely yes. Have you seen my first proposal?
> >>>>>> It basically exploits the normal pci bridge/bus/slot mechanism but need
> >>>>>> a place to store s390 specific configuration attributes.
> >>>>>>
> >>>>>> The idea of a host bridge having this attributes and limit the bus
> >>>>>> to one slot was an alternate design approach suggested by Alex.
> >>>>>>
> >>>>>> I like Alex's idea because:
> >>>>>> 1) It reflects pretty well the actual nature of the pci system in real s390 hw
> >>>>>> 2) It does not create an somehow "artifical" pci topology
> >>>>>>
> >>>>>
> >>>>> I'll have to re-read but here's a thought: use your patch but
> >>>>> remove host bridge hotplug support code.
> >>>>> Stick a standard bridge with shpc support in the single slot
> >>>>> behind your host bridge (existing pci-bridge-dev should do the trick,
> >>>>> though not many people use it, so you might
> >>>>> run into bugs, but fixing them is a good idea anyway).
> >>>>> You can instanciate it automatically like Marcel's patches do
> >>>>> for PXB.
> >>>> Still don't undertsand so I try to summarize in my words please corrent me
> >>>> if I got something wrong
> >>>>
> >>>> - create a standard host bridge
> >>>> - change the s390-pcihost to be a pci 2 pci bridge
> >>>
> >>> Actually I suggested simply adding a pci 2 pci bridge behind
> >>> s390-pcihost.
> >>>
> >>>> - now we can hotplug the s390-pcihost + hotplug a pci device to this
> >>>>   s390-pcihost using standard pci hotplug mechanism
> >>>
> >>> My idea was to just hotplug a pci device behind the standard pci 2 pci
> >>> bridge. don't support hotplugging bridge itself or s390-pcihost itself.
> >>>
> >>>> - we keep the 1 slot limit on the s390-pcihost. We need a place to
> >>>>   store fid and uid information (see mail thread from my 1 proposal)
> >>>
> >>> Yes.
> >>>
> >>>> - If we need more than 32 pci functions we have to extend the primary pci bus
> >>>>   via standart pci 2 pci bridges or add another standart host bridge
> >>>>
> >>>> Is this your suggestion?
> >>>
> >>> Almost, clarifications above.
> >>>
> >> OK, got your idea. Have to think about it and may do some prototyping. THX!
> >>
> > 
> > hm, after thinking more about this I realized this is not working for us.
> > Remember we need a place to store the fid and uid attributes. This place
> > must be:
> > 1) uid/fid per pci device
> > 2) uid/fid in a hotplugable device
> > 
> > I have the feeling we are at the beginning again. Although I liked Alex's
> > idea (host bridge containing uid/fid and having only 1 slot on the bus), it
> > looks like we end up at my first proposal. This does not require any
> > modification in base pci/bus code.
> > 
> > Thx to all of you for the discussion and suggestions.
> 
> I disagree with the assessment. The reason mst was opposed to do the
> one-phb-per-device implementation (which is the closest we can get to
> model things like real hardware FWIW) was that hotplug would work on the
> s390 level rather than pci. I don't see how your first proposal fixes that.
> 
> Also Michael, PCI on s390 is very very special.

Yes, I'm trying to wrap my head around it all.
And is there hotplug support there on real hardware?


> You can't plug in
> anything that does not come from IBM. There are no PCIe connectors -
> instead you have IBM proprietary slots that only work with IBM approved
> devices. So things like "we can plug in a PCI bridge" simply don't work
> as well in that world.
> 
> 
> Alex

But interestingly, the usage example that Frank gave actually shows
e1000 and other non-IBM cards apparently working?
This kind of confuses me.

Alexander Graf March 17, 2015, 7:11 a.m. UTC | #14

On 12.03.15 16:22, Michael S. Tsirkin wrote:
> On Thu, Mar 12, 2015 at 09:59:59AM -0500, Alexander Graf wrote:
>>
>>
>> On 12.03.15 08:16, Frank Blaschka wrote:
>>> On Thu, Mar 12, 2015 at 11:50:02AM +0100, Frank Blaschka wrote:
>>>> On Thu, Mar 12, 2015 at 11:03:50AM +0100, Michael S. Tsirkin wrote:
>>>>> On Thu, Mar 12, 2015 at 10:54:24AM +0100, Frank Blaschka wrote:
>>>>>> On Wed, Mar 11, 2015 at 06:42:34PM +0100, Michael S. Tsirkin wrote:
>>>>>>> On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
>>>>>>>> On Tue, Mar 10, 2015 at 03:26:23PM +0100, Michael S. Tsirkin wrote:
>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>> Absolutely yes. Have you seen my first proposal?
>>>>>>>> It basically exploits the normal pci bridge/bus/slot mechanism but need
>>>>>>>> a place to store s390 specific configuration attributes.
>>>>>>>>
>>>>>>>> The idea of a host bridge having this attributes and limit the bus
>>>>>>>> to one slot was an alternate design approach suggested by Alex.
>>>>>>>>
>>>>>>>> I like Alex's idea because:
>>>>>>>> 1) It reflects pretty well the actual nature of the pci system in real s390 hw
>>>>>>>> 2) It does not create an somehow "artifical" pci topology
>>>>>>>>
>>>>>>>
>>>>>>> I'll have to re-read but here's a thought: use your patch but
>>>>>>> remove host bridge hotplug support code.
>>>>>>> Stick a standard bridge with shpc support in the single slot
>>>>>>> behind your host bridge (existing pci-bridge-dev should do the trick,
>>>>>>> though not many people use it, so you might
>>>>>>> run into bugs, but fixing them is a good idea anyway).
>>>>>>> You can instanciate it automatically like Marcel's patches do
>>>>>>> for PXB.
>>>>>> Still don't undertsand so I try to summarize in my words please corrent me
>>>>>> if I got something wrong
>>>>>>
>>>>>> - create a standard host bridge
>>>>>> - change the s390-pcihost to be a pci 2 pci bridge
>>>>>
>>>>> Actually I suggested simply adding a pci 2 pci bridge behind
>>>>> s390-pcihost.
>>>>>
>>>>>> - now we can hotplug the s390-pcihost + hotplug a pci device to this
>>>>>>   s390-pcihost using standard pci hotplug mechanism
>>>>>
>>>>> My idea was to just hotplug a pci device behind the standard pci 2 pci
>>>>> bridge. don't support hotplugging bridge itself or s390-pcihost itself.
>>>>>
>>>>>> - we keep the 1 slot limit on the s390-pcihost. We need a place to
>>>>>>   store fid and uid information (see mail thread from my 1 proposal)
>>>>>
>>>>> Yes.
>>>>>
>>>>>> - If we need more than 32 pci functions we have to extend the primary pci bus
>>>>>>   via standart pci 2 pci bridges or add another standart host bridge
>>>>>>
>>>>>> Is this your suggestion?
>>>>>
>>>>> Almost, clarifications above.
>>>>>
>>>> OK, got your idea. Have to think about it and may do some prototyping. THX!
>>>>
>>>
>>> hm, after thinking more about this I realized this is not working for us.
>>> Remember we need a place to store the fid and uid attributes. This place
>>> must be:
>>> 1) uid/fid per pci device
>>> 2) uid/fid in a hotplugable device
>>>
>>> I have the feeling we are at the beginning again. Although I liked Alex's
>>> idea (host bridge containing uid/fid and having only 1 slot on the bus), it
>>> looks like we end up at my first proposal. This does not require any
>>> modification in base pci/bus code.
>>>
>>> Thx to all of you for the discussion and suggestions.
>>
>> I disagree with the assessment. The reason mst was opposed to do the
>> one-phb-per-device implementation (which is the closest we can get to
>> model things like real hardware FWIW) was that hotplug would work on the
>> s390 level rather than pci. I don't see how your first proposal fixes that.
>>
>> Also Michael, PCI on s390 is very very special.
> 
> Yes, I'm trying to wrap my head around it all.
> And is there hotplug support there on real hardware?

I quite frankly don't know. Frank?

>> You can't plug in
>> anything that does not come from IBM. There are no PCIe connectors -
>> instead you have IBM proprietary slots that only work with IBM approved
>> devices. So things like "we can plug in a PCI bridge" simply don't work
>> as well in that world.
>>
>>
>> Alex
> 
> But interestingly, the usage example that Frank gave actually shows
> e1000 and other non-IBM cards apparently working?
> This kind of confuses me.

This is probably just by coincidence / luck. IIRC there are only 1 or 2
different cards you can buy from IBM that were ever tested on real
hardware. But then again there is no BOCHS VGA adapter in real hardware
either, yet it seems to work everywhere (except s390) ;). So the fact
that e1000 works is not incredibly surprising, yet I wouldn't rely on it.


Alex

Frank Blaschka March 17, 2015, 12:15 p.m. UTC | #15

On Tue, Mar 17, 2015 at 08:11:14AM +0100, Alexander Graf wrote:
> 
> 
> On 12.03.15 16:22, Michael S. Tsirkin wrote:
> > On Thu, Mar 12, 2015 at 09:59:59AM -0500, Alexander Graf wrote:
> >>
> >>
> >> On 12.03.15 08:16, Frank Blaschka wrote:
> >>> On Thu, Mar 12, 2015 at 11:50:02AM +0100, Frank Blaschka wrote:
> >>>> On Thu, Mar 12, 2015 at 11:03:50AM +0100, Michael S. Tsirkin wrote:
> >>>>> On Thu, Mar 12, 2015 at 10:54:24AM +0100, Frank Blaschka wrote:
> >>>>>> On Wed, Mar 11, 2015 at 06:42:34PM +0100, Michael S. Tsirkin wrote:
> >>>>>>> On Wed, Mar 11, 2015 at 03:38:44PM +0100, Frank Blaschka wrote:
> >>>>>>>> On Tue, Mar 10, 2015 at 03:26:23PM +0100, Michael S. Tsirkin wrote:
> >>>>>>>>> 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.
> >>>>>>>>>
> >>>>>>>> Absolutely yes. Have you seen my first proposal?
> >>>>>>>> It basically exploits the normal pci bridge/bus/slot mechanism but need
> >>>>>>>> a place to store s390 specific configuration attributes.
> >>>>>>>>
> >>>>>>>> The idea of a host bridge having this attributes and limit the bus
> >>>>>>>> to one slot was an alternate design approach suggested by Alex.
> >>>>>>>>
> >>>>>>>> I like Alex's idea because:
> >>>>>>>> 1) It reflects pretty well the actual nature of the pci system in real s390 hw
> >>>>>>>> 2) It does not create an somehow "artifical" pci topology
> >>>>>>>>
> >>>>>>>
> >>>>>>> I'll have to re-read but here's a thought: use your patch but
> >>>>>>> remove host bridge hotplug support code.
> >>>>>>> Stick a standard bridge with shpc support in the single slot
> >>>>>>> behind your host bridge (existing pci-bridge-dev should do the trick,
> >>>>>>> though not many people use it, so you might
> >>>>>>> run into bugs, but fixing them is a good idea anyway).
> >>>>>>> You can instanciate it automatically like Marcel's patches do
> >>>>>>> for PXB.
> >>>>>> Still don't undertsand so I try to summarize in my words please corrent me
> >>>>>> if I got something wrong
> >>>>>>
> >>>>>> - create a standard host bridge
> >>>>>> - change the s390-pcihost to be a pci 2 pci bridge
> >>>>>
> >>>>> Actually I suggested simply adding a pci 2 pci bridge behind
> >>>>> s390-pcihost.
> >>>>>
> >>>>>> - now we can hotplug the s390-pcihost + hotplug a pci device to this
> >>>>>>   s390-pcihost using standard pci hotplug mechanism
> >>>>>
> >>>>> My idea was to just hotplug a pci device behind the standard pci 2 pci
> >>>>> bridge. don't support hotplugging bridge itself or s390-pcihost itself.
> >>>>>
> >>>>>> - we keep the 1 slot limit on the s390-pcihost. We need a place to
> >>>>>>   store fid and uid information (see mail thread from my 1 proposal)
> >>>>>
> >>>>> Yes.
> >>>>>
> >>>>>> - If we need more than 32 pci functions we have to extend the primary pci bus
> >>>>>>   via standart pci 2 pci bridges or add another standart host bridge
> >>>>>>
> >>>>>> Is this your suggestion?
> >>>>>
> >>>>> Almost, clarifications above.
> >>>>>
> >>>> OK, got your idea. Have to think about it and may do some prototyping. THX!
> >>>>
> >>>
> >>> hm, after thinking more about this I realized this is not working for us.
> >>> Remember we need a place to store the fid and uid attributes. This place
> >>> must be:
> >>> 1) uid/fid per pci device
> >>> 2) uid/fid in a hotplugable device
> >>>
> >>> I have the feeling we are at the beginning again. Although I liked Alex's
> >>> idea (host bridge containing uid/fid and having only 1 slot on the bus), it
> >>> looks like we end up at my first proposal. This does not require any
> >>> modification in base pci/bus code.
> >>>
> >>> Thx to all of you for the discussion and suggestions.
> >>
> >> I disagree with the assessment. The reason mst was opposed to do the
> >> one-phb-per-device implementation (which is the closest we can get to
> >> model things like real hardware FWIW) was that hotplug would work on the
> >> s390 level rather than pci. I don't see how your first proposal fixes that.
> >>
> >> Also Michael, PCI on s390 is very very special.
> > 
> > Yes, I'm trying to wrap my head around it all.
> > And is there hotplug support there on real hardware?
> 
> I quite frankly don't know. Frank?
>
yes there is, but it might be different than you expect from traditional
PCI :-)
 
> >> You can't plug in
> >> anything that does not come from IBM. There are no PCIe connectors -
> >> instead you have IBM proprietary slots that only work with IBM approved
> >> devices. So things like "we can plug in a PCI bridge" simply don't work
> >> as well in that world.
> >>
> >>
> >> Alex
> > 
> > But interestingly, the usage example that Frank gave actually shows
> > e1000 and other non-IBM cards apparently working?
> > This kind of confuses me.
Sorry, I just use e1000 to give an example everybody can follow. Here is
the actual story:

1) In reality the only pci card I could get hands on was a mlx4
2) Linux kernel on s390 does not support fancy io stuff like VGA, USB,
   sound, ... in general
3) on s390 PCI has some restrictions like: no traditional IRQ (MSI/-X only),
   no IO BAR, no port IO, ...

For this you can attach an emulated card (like e1000) to qemu and the guest
can list it via lspci, but the card would not be able to work. At the end
I had 2 working setups:

1) virtio-pci-net
2) vfio (pass-through with mlx4 backend on the host)

> 
> This is probably just by coincidence / luck. IIRC there are only 1 or 2
> different cards you can buy from IBM that were ever tested on real
> hardware. But then again there is no BOCHS VGA adapter in real hardware
> either, yet it seems to work everywhere (except s390) ;). So the fact
> that e1000 works is not incredibly surprising, yet I wouldn't rely on it.
> 
> 
> Alex
>
Since I'm leaving the company in a couple of days I will not be able to bring
this to an end. That's why I want to handover the discussion to my
colleague Hong Bo Li now. He worked on s390 kvm/qemu pci development for more
than 1 year now and should be able to seamless take-over.

Thx Hong Bo!

Please give him the same kind support and help you provided to me. Also I want
to say thank-you and goodby. It was a pleasure to work with all of you ...

Hope to see you again one day,

Frank

[2/2,RFC] s390x/pci: rework pci infrastructure modeling

Commit Message

Comments

Patch