Re: [Qemu-devel] [PATCH] ppc/pnv: Add model for Power8 PHB3 PCIe Host bridge

[Andrea Bolognani]

On Tue, 2018-06-26 at 19:02 +0200, Cédric Le Goater wrote:
> On 06/26/2018 05:57 PM, Andrea Bolognani wrote:
> > On Tue, 2018-06-26 at 15:59 +0200, Cédric Le Goater wrote:
> > > This is a model of the PCIe host bridge found on Power8 chips,
> > > including PowerBus logic interface, IOMMU support, PCIe root complex,
> > > XICS MSI and LSI interrupt sources.
> > > 
> > > 4 PHBs are provisioned under the Power8 chip model to fit hardware but
> > > only one is currently initialized.
> > 
> > What's the advantage in creating 4 PHBs instead of a single one,
> 
> The Power8 chip comes in different flavors: Venice, Murano, Naple, 
> each having a different number of PHBs. We don't need to initialize 
> them all to plug only a couple of devices (net, storage, usbs) 
> 
> When time comes, we might want to test some more complex configurations
> or extend the modeling with CAPI support. That's why we have a :
> 
>       #define PNV_MAX_CHIP_PHB 4
>           PnvPHB3      phbs[PNV_MAX_CHIP_PHB];
> 
> under the chip, and a 'num_phbs' attribute to increase the number
> of controllers. It still needs to be tested but that's the goal.

I was a bit confused about the difference between "provisioning"
and "initializing" a PHB, I guess.

Now that I've looked at the code, my (very uneducated) reading is
that you're allocating memory for 4 PHBs along with the chip, but
only actually initializing num_phbs of them, with 1 being the
default.

I have no idea what this implies when it comes to adding PCI
controller to the guest, though. If I run a guest with num_phbs=1,
are ids pci.1..pci.3 reserved even though the corresponding PHBs
have not been initialized? Is num_phbs the only way you can control
how many PHBs a PowerNV guest will have?

I would play around and try to figure out all the kinks on my own,
but I can't actually compile QEMU with this patch applied:

  hw/pci-host/pnv_phb3_msi.c: In function ‘phb3_msi_reset’:
  hw/pci-host/pnv_phb3_msi.c:229:11: error: ‘ICSStateClass’ {aka ‘struct 
ICSStateClass’} has no member named ‘parent_reset’; did you mean ‘parent_class’?
       icsc->parent_reset(dev);
             ^~~~~~~~~~~~
             parent_class
  hw/pci-host/pnv_phb3_msi.c: In function ‘phb3_msi_realize’:
  hw/pci-host/pnv_phb3_msi.c:260:11: error: ‘ICSStateClass’ {aka ‘struct 
ICSStateClass’} has no member named ‘parent_realize’; did you mean 
‘parent_class’?
       icsc->parent_realize(dev, &local_err);
             ^~~~~~~~~~~~~~
             parent_class
  hw/pci-host/pnv_phb3_msi.c: In function ‘phb3_msi_class_init’:
  hw/pci-host/pnv_phb3_msi.c:293:43: error: ‘ICSStateClass’ {aka ‘struct 
ICSStateClass’} has no member named ‘parent_realize’; did you mean 
‘parent_class’?
                                       &isc->parent_realize);
                                             ^~~~~~~~~~~~~~
                                             parent_class
  hw/pci-host/pnv_phb3_msi.c:295:41: error: ‘ICSStateClass’ {aka ‘struct 
ICSStateClass’} has no member named ‘parent_reset’; did you mean ‘parent_class’?
                                     &isc->parent_reset);
                                           ^~~~~~~~~~~~
                                           parent_class

> > like we already do for pSeries guests? 
> 
> I didn't follow that discussion but this is "another" kind of PHB.
> This one models the baremetal controller as found on OpenPOWER and
> IBM Power machines. pSeries has a virtual PHB.

I understand that, and of course libvirt will need to learn about
this new type of PHB and make sure both pSeries and PowerNV guests
get the correct one assigned to them.

What I meant is that pSeries guests get a single PHB by default,
with additional ones being instantiable through -device; this is
also consistent with how PCI controllers are added to other guest
types including pc, q35 and aarch64/virt, so it would be really
nice if PowerNV behaved the same way.

> > As it is, this will confuse the heck out of libvirt's PCI address > 
> > allocation algorithm :)
> 
> The pci bus name should be directly related to the PHB index. But
> I agree we need to be careful. That's why you are in cc: :)

Thanks, I really appreciate you keeping me in the loop :)

-- 
Andrea Bolognani / Red Hat / Virtualization