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.
