[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Cédric Le Goater
Subject: Re: [Qemu-devel] [PATCH] ppc/pnv: Add model for Power8 PHB3 PCIe Host bridge
Date: Wed, 27 Jun 2018 14:18:14 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2

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

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


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


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

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]