On 06/08/2017 11:14 AM, Greg Kurz wrote: > On Thu, 8 Jun 2017 11:53:29 +1000 > David Gibson <da...@gibson.dropbear.id.au> 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 <c...@kaod.org> 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.