On Thu, Nov 17, 2016 at 01:02:57PM +1100, Alexey Kardashevskiy wrote:
> 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@xxxxxxxxx> 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@xxxxxxxxx>
> >>>>>> ---
> >>>>>>
> >>>>>> 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.
IIUC, it's because when libvirt wants to attach a PCI device, it will
just try to attach it to bus pci.0, which will no longer exist.
> > 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.
>
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
