Re: [libvirt] [Qemu-ppc] [RFC PATCH qemu] spapr_pci: Create PCI-express root bus by default

On 16/11/16 01:02, Andrea Bolognani wrote: > On Tue, 2016-11-01 at 13:46 +1100, David Gibson wrote: >> On Mon, Oct 31, 2016 at 03:10:23PM +1100, Alexey Kardashevskiy wrote: >>> >>> On 31/10/16 13:53, David Gibson wrote: >>>> >>>> On Fri, Oct 28, 2016 at 12:07:12PM +0200, Greg Kurz wrote: >>>>> >>>>> On Fri, 28 Oct 2016 18:56:40 +1100 >>>>> Alexey Kardashevskiy <aik(a)ozlabs.ru&gt; wrote: >>>>> >>>>>> >>>>>> At the moment sPAPR PHB creates a root buf of TYPE_PCI_BUS type. >>>>>> This means that vfio-pci devices attached to it (and this is >>>>>> a default behaviour) hide PCIe extended capabilities as >>>>>> the bus does not pass a pci_bus_is_express(pdev->bus) check. >>>>>> >>>>>> This changes adds a default PCI bus type property to sPAPR PHB >>>>>> and uses TYPE_PCIE_BUS if none passed; older machines get TYPE_PCI_BUS >>>>>> for backward compatibility as a bus type is used in the bus name >>>>>> so the root bus name becomes "pcie.0" instead of "pci.0". >>>>>> >>>>>> Signed-off-by: Alexey Kardashevskiy <aik(a)ozlabs.ru&gt; >>>>>> --- >>>>>> >>>>>> What can possibly go wrong with such change of a name? >>>>>> From devices prospective, I cannot see any. >>>>>> >>>>>> libvirt might get upset as "pci.0" will not be available, >>>>>> will it make sense to create pcie.0 as a root bus and always >>>>>> add a PCIe->PCI bridge and name its bus "pci.0"? >>>>>> >>>>>> Or create root bus from TYPE_PCIE_BUS and force name to "pci.0"? >>>>>> pci_register_bus() can do this. >>>>>> >>>>>> >>>>>> --- >>>>>> hw/ppc/spapr.c | 5 +++++ >>>>>> hw/ppc/spapr_pci.c | 5 ++++- >>>>>> include/hw/pci-host/spapr.h | 1 + >>>>>> 3 files changed, 10 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>>>>> index 0b3820b..a268511 100644 >>>>>> --- a/hw/ppc/spapr.c >>>>>> +++ b/hw/ppc/spapr.c >>>>>> @@ -2541,6 +2541,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true); >>>>>> .driver = TYPE_SPAPR_PCI_HOST_BRIDGE, \ >>>>>> .property = "mem64_win_size", \ >>>>>> .value = "0", \ >>>>>> + }, \ >>>>>> + { \ >>>>>> + .driver = TYPE_SPAPR_PCI_HOST_BRIDGE, \ >>>>>> + .property = "root_bus_type", \ >>>>>> + .value = TYPE_PCI_BUS, \ >>>>>> }, >>>>>> >>>>>> static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index, >>>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>>>>> index 7cde30e..2fa1f22 100644 >>>>>> --- a/hw/ppc/spapr_pci.c >>>>>> +++ b/hw/ppc/spapr_pci.c >>>>>> @@ -1434,7 +1434,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >>>>>> bus = pci_register_bus(dev, NULL, >>>>>> pci_spapr_set_irq, pci_spapr_map_irq, sphb, >>>>>> &sphb->memspace, &sphb->iospace, >>>>>> - PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS); >>>>>> + PCI_DEVFN(0, 0), PCI_NUM_PINS, >>>>>> + sphb->root_bus_type ? sphb->root_bus_type : >>>>>> + TYPE_PCIE_BUS); >>>>> >>>>> Shouldn't we ensure that sphb->root_bus_type is either TYPE_PCIE_BUS or >>>>> TYPE_PCI_BUS ? >>>> >>>> Yes, I think so. In fact, I think it would be better to make the >>>> property a boolean that just selects PCI-E, rather than this which >>>> exposes qemu (semi-)internal type names on the comamnd line. >>> >>> Sure, a "pcie-root" boolean property should do. >>> >>> However this is not my main concern, I rather wonder if we have to have >>> pci.0 when we pick PCIe for the root. >> >> Right. >> >> I've added Andrea Bologna to the CC list to get a libvirt perspective. > > Thanks for doing so: changes such as this one can have quite > an impact on the upper layers of the stack, so the earliest > libvirt is involved in the discussion the better. > > I'm going to go a step further and cross-post to libvir-list > in order to give other libvirt contributors a chance to chime > in too. > >> Andrea, >> >> To summarise the issue here: >> * As I've said before the PAPR spec kinda-sorta abstracts the >> difference between vanilla PCI and PCI-E >> * However, because within qemu we're declaring the bus as PCI that >> means some PCI-E devices aren't working right >> * In particular it means that PCI-E extended config space isn't >> available >> >> The proposal is to change (on newer machine types) the spapr PHB code >> to declare a PCI-E bus instead. AIUI this still won't make the root >> complex guest visible (which it's not supposed to be under PAPR), and >> the guest shouldn't see a difference in most cases - it will still see >> the PAPR abstracted PCIish bus, but will now be able to get extended >> config space. >> >> The possible problem from a libvirt perspective is that doing this in >> the simplest way in qemu would change the name of the default bus from >> pci.0 to pcie.0. We have two suggested ways to mitigate this: >> 1) Automatically create a PCI-E to PCI bridge, so that new machine >> types will have both a pcie.0 and pci.0 bus >> 2) Force the name of the bus to be pci.0, even though it's treated >> as PCI-E in other ways. >> >> We're trying to work out exactly what will and won't cause trouble for >> libvirt. > > Option 2) is definitely a no-no, as we don't want to be piling > up even more hacks and architecture-specific code: the PCI > Express Root Bus should be called pcie.0, just as it is on q35 > and mach-virt machine types. > > Option 1) doesn't look too bad, but devices that are added > automatically by QEMU are an issue since we need to hardcode > knowledge of them into libvirt if we want the rest of the PCI > address allocation logic to handle them correctly. > > Moreover libvirt now has the ability of building a legacy PCI > topology without user intervention, if needed to plug in > legacy devices, on machines that have a PCI Express Root Bus, > which makes the additional bridge fully redundant... > > ... or at least it would, if we actually had a proper > PCIe-to-PCI bridge; AFAIK, though, the closest we have is the > i82801b11-bridge that is Intel-specific despite having so far > been abused as a generic PCIe-to-PCI bridge. I'm not even > sure whether it would work at all on ppc64. > > Moving from legacy PCI to PCI Express would definitely be an > improvement, in my opinion. As mentioned, that's already the > case for at least two other architectures, so the more we can > standardize on that, the better. > > That said, considering that a big part of the PCI address > allocation logic is based off whether the specific machine > type exposes a legay PCI Root Bus or a PCI Express Root Bus, > libvirt will need a way to be able to tell which one is which. > > Version checks are pretty much out of the question, as they > fail as soon as downstream releases enter the picture. A > few ways we could deal with the situation: > > 1) switch to PCI Express on newer machine types, and > expose some sort of capability through QMP so that > libvirt can know about the switch > > 2) switch between legacy PCI and PCI Express based on a > machine type option. libvirt would be able to find out > whether the option is available or not, and default to > either > > <controller type='pci' model='pci-root'/> > > or > > <controller type='pci' model='pcie-root'/> > > based on that. In order to support multiple PHBs > properly, those would have to be switchable with an > option as well > > 3) create an entirely new machine type, eg. pseries-pcie > or whatever someone with the ability to come up with > decent names can suggest :) That would make ppc64 > similar to x86, where i440fx and q35 have different > root buses. libvirt would learn about the new machine > type, know that it has a PCI Express Root Bus, and > behave accordingly > > Option 1) would break horribly with existing libvirt > versions, and so would Option 2) if we default to using How exactly 1) will break libvirt? Migrating from pseries-2.7 to pseries-2.8 does not work anyway, and machines are allowed to behave different from version to version, what distinct difference will using "pseries-pcie-X.Y" make? I believe after we introduced the very first pseries-pcie-X.Y, we will just stop adding new pseries-X.Y.

> PCI Express. Option 2) with default to legacy PCI and > option 3) would work just fine with existing libvirt > versions AFAICT, but wouldn't of course expose the new > capabilities. > > Option 3) is probably the one that will be less confusing > to users; we might even decide to take the chance and fix > other small annoyances with the current pseries machine > type, if there's any. On the other hand, it might very well > be considered to be too big a hammer for such a small nail.