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