Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[Cédric Le Goater]

On 06/08/2017 11:14 AM, Greg Kurz wrote:
> On Thu, 8 Jun 2017 11:53:29 +1000
> David Gibson <address@hidden> wrote:
> 
>> On Wed, Jun 07, 2017 at 10:55:07PM +0200, Greg Kurz wrote:
>>> On Wed, 7 Jun 2017 20:11:38 +0200
>>> Cédric Le Goater <address@hidden> wrote:
>>>   
>>>> On 06/07/2017 07:17 PM, Greg Kurz wrote:  
>>>>> Until recently, spapr used to allocate ICPState objects for the lifetime
>>>>> of the machine. They would only be associated to vCPUs in xics_cpu_setup()
>>>>> when plugging a CPU core.
>>>>>
>>>>> Now that ICPState objects have the same lifecycle as vCPUs, it is
>>>>> possible to associate them during realization.
>>>>>
>>>>> This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
>>>>> is passed as a property. Note that vCPU now needs to be realized first
>>>>> for the IRQs to be allocated. It also needs to resetted before ICPState
>>>>> realization in order to synchronize with KVM.
>>>>>
>>>>> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
>>>>> needed anymore and can be safely dropped.    
>>>>
>>>> I like the idea but I think the assignment of ->cs attribute should be 
>>>> moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by 
>>>> the kvm_vcpu_ioctl() calls. 
>>>>   
>>>
>>> Well, cs->cpu_index is also used for traces and to implement the 'info pic'
>>> monitor command.  
>>
>> Right.  I think it makes sense for the ICP to have a handle on it's
>> associated CPU, even if we don't actually use it in all cases right
>> now.  So I have no problem with the property being in all ICPs; I
>> think that will be cleaner than special casing xics_kvm.  Especially
>> if we have to un-special-case it sometime in future because we need
>> to access the CPU object for some reason we haven't thought of yet.
>>
> 
> We had discussed with Cedric about that actually but when I started
> to write code, I had the impression that I would have to do convoluted
> things to get rid of the CPU handle in ICP.

There is nothing complex and it would surely simplify cpu_setup(). 

But, the main argument is that it might be useful to other platforms.   
So there is not much value in removing it. I am OK with that. I am less
OK with using the index, even if it is useful for the migration state 
of ICP objects today. 

We could be tempted to use more of cpu_index, like we have done in 
the past, and that was really complex to untangle. Let's be cautious.

C.