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