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

[Cédric Le Goater]

On 06/27/2018 12:22 PM, Andrea Bolognani wrote:
> 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.

objects have different states : allocated, initialized, realized
I guess "provision (memory)" is not the best choice for the first
state.

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

Yes. I had the idea to increase the number of PHBs with a machine
option later on if needed.

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

They don't exist on the PowerNV machine if you don't have a PHB3
device backing them.

> Is num_phbs the only way you can control how many PHBs a PowerNV 
> guest will have?

yes. Unless we make them user creatable but that's a discussion in
progress. I am not sure user creatable PHB3s are needed. We will
see.
 
> 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:

you need to use David's branch :

        https://github.com/dgibson/qemu/tree/ppc-for-3.0


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

yes. we don't want to change libvirt too much so this is a requirement 
to take into account.

> What I meant is that pSeries guests get a single PHB by default,

yes

> with additional ones being instantiable through -device; 

yes.

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

OK. So that's a +1 in favor of user creatable PHB3s devices 

>>> 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 :)
> 

Thanks for the feedback.

C.