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

[Alexander Graf <agraf@suse.de>]

On 06.03.15 11:34, Frank Blaschka wrote:
> 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 <blaschka@linux.vnet.ibm.com>:
>>>>>>>>>>
>>>>>>>>>>> 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?

I think it's reasonable to detangle the Sysbus PCI bridges from PCIBuses.

Michael, what do you think?


Alex

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