Re: [Qemu-devel] [PATCH RFC 1/1] s390x/pci: Extend pci representation by new zpci device

[Frank Blaschka]

On Wed, Mar 04, 2015 at 04:58:25PM +0100, Frank Blaschka wrote:
> On Wed, Mar 04, 2015 at 04:25:07PM +0100, Alexander Graf wrote:
> > 
> > 
> > On 04.03.15 16:07, Frank Blaschka wrote:
> > > On Wed, Mar 04, 2015 at 03:49:15PM +0100, Alexander Graf wrote:
> > >>
> > >>
> > >> On 04.03.15 14:44, Frank Blaschka wrote:
> > >>> On Tue, Mar 03, 2015 at 09:38:37PM +0100, Alexander Graf wrote:
> > >>>>
> > >>>>
> > >>>> On 03.03.15 14:25, Frank Blaschka wrote:
> > >>>>> On Tue, Mar 03, 2015 at 10:33:05AM +0100, Alexander Graf wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>>> Am 03.03.2015 um 09:06 schrieb Frank Blaschka <address@hidden>:
> > >>>>>>>
> > >>>>>>>> On Thu, Feb 26, 2015 at 04:34:06PM +0100, Alexander Graf wrote:
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>> On 26.02.15 16:27, Frank Blaschka wrote:
> > >>>>>>>>>> On Thu, Feb 26, 2015 at 03:39:15PM +0100, Alexander Graf wrote:
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>> On 26.02.15 12:59, Frank Blaschka wrote:
> > >>>>>>>>>>> This patch extends the current s390 pci implementation to
> > >>>>>>>>>>> provide more flexibility in configuration of s390 specific
> > >>>>>>>>>>> device handling. For this we had to introduce a new facility
> > >>>>>>>>>>> (and bus) to hold devices representing information actually
> > >>>>>>>>>>> provided by s390 firmware and I/O configuration.
> > >>>>>>>>>>>
> > >>>>>>>>>>> On s390 the physical structure of the pci system (bridge, bus, 
> > >>>>>>>>>>> slot)
> > >>>>>>>>>>> in not shown to the OS. For this the pci bridge and bus created
> > >>>>>>>>>>> in qemu can also not be shown to the guest. The new zpci device 
> > >>>>>>>>>>> class
> > >>>>>>>>>>> represents this abstract view on the bare pci function and 
> > >>>>>>>>>>> allows to
> > >>>>>>>>>>> provide s390 specific configuration attributes for it.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Sample qemu configuration:
> > >>>>>>>>>>> -device e1000,id=zpci1
> > >>>>>>>>>>> -device ne2k_pci,id=zpci2
> > >>>>>>>>>>> -device zpci,fid=2,uid=1248,pci_id=zpci1
> > >>>>>>>>>>> -device zpci,fid=17,uid=2244,pci_id=zpci2
> > >>>>>>>>>>>
> > >>>>>>>>>>> A zpci device references the corresponding PCI device via 
> > >>>>>>>>>>> device id.
> > >>>>>>>>>>> The new design allows to define multiple host bridges and 
> > >>>>>>>>>>> support more
> > >>>>>>>>>>> pci devices.
> > >>>>>>>>>>
> > >>>>>>>>>> Isn't this reverse? Shouldn't it rather be
> > >>>>>>>>>>
> > >>>>>>>>>>  -device zpci,...,id=zpci1
> > >>>>>>>>>>  -device e1000,bus=zpci1.0
> > >>>>>>>>>>
> > >>>>>>>>>> with a limit on each virtual zpci bus to only support one device?
> > >>>>>>>>>
> > >>>>>>>>> Do you mean something like having multiple host bridges 
> > >>>>>>>>> (providing a pci bus
> > >>>>>>>>> each) and limit the bus to just one device?
> > >>>>>>>>>
> > >>>>>>>>> -device s390-pcihost,fid=16,uid=1234
> > >>>>>>>>> -device s390-pcihost,fid=17,uid=5678
> > >>>>>>>>> -device e1000,bus=pci.0
> > >>>>>>>>> -device ne2k_pci,bus=pci.1
> > >>>>>>>>>
> > >>>>>>>>> We also discussed this option but we don't like the idea to put 
> > >>>>>>>>> attributes
> > >>>>>>>>> belong to the pci device to the host bridge.
> > >>>>>>>>
> > >>>>>>>> I guess I'm not grasping something obvious here :). What exactly 
> > >>>>>>>> are the
> > >>>>>>>> attributes again?
> > >>>>>>> Sorry for the late response, I was on vacation the last couple days.
> > >>>>>>>
> > >>>>>>> The fid and uid values are provided by microcode/io layer on the 
> > >>>>>>> real hardware.
> > >>>>>>
> > >>>>>> So they are arbitrary numbers? What uniqueness constraints do we 
> > >>>>>> have on them?
> > >>>>> fid and uid must be unique within the same qemu. At a first look the 
> > >>>>> numbers are
> > >>>>> arbitrary but our configuration folks want explicitly define a 
> > >>>>> particular fid and uid
> > >>>>> to better support migration and pass-through scenarios.
> > >>>>
> > >>>> Well, at the end of the day you want to make sure they're identical on
> > >>>> both sides, yes.
> > >>>>
> > >>>>>> IIUC you can only have a single pcie device behind a virtual "bus" 
> > >>>>>> anyway, so what if we just calculate uid and fid from the bus id?
> > >>>>> I think this similar to the current implementation. We use the slot 
> > >>>>> (idea for the future was
> > >>>>> bus + slot) to generate uid and fid. But this is not flexible enough. 
> > >>>>> As I said, our
> > >>>>> configuration folks want to be able to specify fid and uid for the 
> > >>>>> device.
> > >>>>
> > >>>> I don't see how this is different from what PPC does with its LIOBN
> > >>>> which is a property of the PHB.
> > >>>>
> > >>>>
> > >>>> Alex
> > >>>>
> > >>>
> > >>> I played arround with the idea of having multiple host bridges and this 
> > >>> worked well
> > >>> at least for static (non hotplug) configuration. In case I want to 
> > >>> hotplug a host
> > >>> bridge I got following error:
> > >>>
> > >>> (qemu) device_add s390-pcihost,fid=8,uid=9
> > >>> Bus 'main-system-bus' does not support hotplugging
> > >>>
> > >>> Is there anything I have to enable to support this?
> > >>>
> > >>> I have: has_dynamic_sysbus = 1 and 
> > >>> cannot_instantiate_with_device_add_yet = false
> > >>> but this seems not to help for the hotplug case.
> > >>
> > >> Having s390 devices reside on sysbus is probably a bad idea. Instead,
> > >> they should be on an s390 specific bus which then can implement hotplug
> > >> easily.
> > >>
> > >>
> > >> Alex
> > >>
> > > 
> > > Hm now I get lost ...
> > > 
> > > Do you suggest we should implement a s390 specific device (which is not 
> > > derived from
> > > TYPE_PCI_HOST_BRIDGE) but implements a pci bus so we can attach a pci 
> > > device to this
> > > device?  
> > 
> > Ugh, PCI_HOST_BRIDGE is a sysbus device. Awesome.
> > 
> > Conceptually your PCI bridge is not a sysbus device, since it doesn't
> > live on a flat MMIO + legacy IRQ routing bus. Instead, it lives on its
> > own thing that handles MMIO and IRQs via special backdoor interfaces.
> >
> well spoken :-)
> 
> > How much of the PCI_HOST_BRIDGE device are you actually using? Would it
> > be a lot of effort to have another s390 specific device that exposes a
> > PCIBus, but is not of type PCI_HOST_BRIDGE (and thus sysbus)?
> > 
> I do not use much functionality of the PCI_HOST_BRIDGE but I was not able
> to put a pci bus on a device != PCI_HOST_BRIDGE. If I recall it correctly
> the pci bus code wants to collect all bridges in a list.
> I have to do some more research to find out if it is possible to change
> this ...
>

I have implemented your idea (I can provide the patch later if you want) but had
to change pci code to allow to have a pci bus without a host bridge. I don't
know if this makes sense at all or if this breaks some general concept. Since
my code does not use any functionality of the host bridge following patch seems
to be sufficient for me.

Can anybody with more experience in qemu pci and host bridge code comment on 
this?

Thx!

---
 hw/pci/pci.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -253,9 +253,11 @@ static void pcibus_reset(BusState *qbus)

 static void pci_host_bus_register(PCIBus *bus, DeviceState *parent)
 {
-    PCIHostState *host_bridge = PCI_HOST_BRIDGE(parent);
-
-    QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next);
+    PCIHostState *host_bridge = (PCIHostState *)object_dynamic_cast(
+                                OBJECT(parent), TYPE_PCI_HOST_BRIDGE);
+    if (host_bridge) {
+        QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next);
+    }
 }

 PCIBus *pci_find_primary_bus(void)
@@ -288,14 +290,20 @@ PCIBus *pci_device_root_bus(const PCIDev
 const char *pci_root_bus_path(PCIDevice *dev)
 {
     PCIBus *rootbus = pci_device_root_bus(dev);
-    PCIHostState *host_bridge = PCI_HOST_BRIDGE(rootbus->qbus.parent);
-    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
+    PCIHostState *host_bridge;
+    PCIHostBridgeClass *hc;
+
+    host_bridge = (PCIHostState *)object_dynamic_cast(
+                  OBJECT(rootbus->qbus.parent), TYPE_PCI_HOST_BRIDGE);

     assert(!rootbus->parent_dev);
-    assert(host_bridge->bus == rootbus);

-    if (hc->root_bus_path) {
-        return (*hc->root_bus_path)(host_bridge, rootbus);
+    if (host_bridge) {
+        assert(host_bridge->bus == rootbus);
+        hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
+        if (hc->root_bus_path) {
+            return (*hc->root_bus_path)(host_bridge, rootbus);
+        }
     }

     return rootbus->qbus.name;
> 
>