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

[David Gibson]

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.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature