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

[Greg Kurz]

On Thu, 8 Jun 2017 11:25:52 +0200
Cédric Le Goater <address@hidden> wrote:

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

I agree. Maybe this could be hidden in a icpc->get_index() handler ?

> C.

pgpn_72qbTrTA.pgp
Description: OpenPGP digital signature