Re: [Qemu-devel] [PATCH v5 0/9] ppc/pnv: interrupt controller (POWER8)

[Cédric Le Goater]

On 04/05/2017 07:51 AM, David Gibson wrote:
> On Mon, Apr 03, 2017 at 09:45:56AM +0200, Cédric Le Goater wrote:
>> Hello,
>>
>> Here is a series adding support for the interrupt controller as found
>> on a POWER8 system. POWER9 uses a different interrupt controller
>> called XIVE, still to be worked on.
>>
>> The initial patches are more cleanups of the XICS layer which move the
>> IRQ 'server' number mapping under the machine handlers.
>>
>> A new PnvICPState object based on MMIOs, which is specific to PowerNV,
>> is introduced in XICS. These ICP objects are created for each thread
>> of a core and linked to the associated PowerPCCPU object.
>>
>> Finally, to make use of the XICS layer, the PowerNV machine is
>> extended with a QOM XICSFabric interface and with a global memory
>> region acting as the Interrupt Management area.
>>
>>
>> To test, grab a kernel and a rootfs image here :
>>
>>   
>> https://openpower.xyz/job/openpower-op-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/images/zImage.epapr
>>   
>> https://openpower.xyz/job/openpower-op-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/images/rootfs.cpio.xz
>>
>> The full patchset is available here :
>>
>>    https://github.com/legoater/qemu/commits/powernv-ipmi-2.9
>>
>> Thanks,
> 
> I've applied the series to my ppc-for-2.10 branch.  A couple of things
> to think about for follow up cleanups:
> 
> 1) The spapr->nr_servers field has very little remaining use; can you
>    go a bit further and remove it entirely?

yes. I have a patch for it already, reshuffling the xics_system_init
routine again to make it look a little better : 

        
https://github.com/legoater/qemu/commit/94e90b4b660d521ad4806a5496dd89f351995810
 
> 2) At the moment you're creating (and realizing) the ICP objects 

yes. This is because the number of ICPState objects depends 
on 'nr_threads' which is only known at realize time.

We could also define an array of icps in the core:

        PnvICPState icps[MAX_THREADS]

initialize them in an init routine and only realize a subset 
depending on 'nr_threads'. 

This is possible because the size of TYPE_PNV_ICP and TYPE_ICP
are the same. 

But, the current method is safer I think. 

> just  before you realize the core objects. 

yes. this is to not complete the core realization if the ICP 
creation fails. Else the core would be fully realized before 
the ICP, which feels incorrect as the ICP is an attribute 
of the core now. 

> I'm not entirely sure if
> creating new objects at realize time is QOMishly correct.  Maybe do
> some more enquiries to see if it is or not, and if not update.

Yes.

Thanks,

C.