On 06/08/2017 10:45 AM, Greg Kurz wrote:
> On Thu, 8 Jun 2017 12:01:12 +1000
> David Gibson <da...@gibson.dropbear.id.au> wrote:
>
>> On Wed, Jun 07, 2017 at 07:17:09PM +0200, 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.
>>
>> Ok, what enforces those ordering constraints?
>>
>
> I'm not sure about what you're asking... I had to re-order because
> xics_cpu_setup() used to be called after the vCPU is realized and
> put in PAPR mode.
>
> Especially, we have these lines:
>
> switch (PPC_INPUT(env)) {
> case PPC_FLAGS_INPUT_POWER7:
> icp->output = env->irq_inputs[POWER7_INPUT_INT];
> break;
>
> case PPC_FLAGS_INPUT_970:
> icp->output = env->irq_inputs[PPC970_INPUT_INT];
> break;
>
> They depend on env->irq_inputs being allocated. This happens on the
>
> spapr_cpu_core_realize_child()
> {
> ...
> object_property_set_bool(child, true, "realized", &local_err);
> object_property_set_bool()
> object_property_set_qobject()
> object_property_set()
> property_set_bool()
> device_set_realized()
> ppc_cpu_realizefn()
> init_ppc_proc()
> init_proc_POWER8()
> ppcPOWER7_irq_init()
>
> and, when using xics_kvm:
>
> icp_kvm_cpu_setup()
> {
> ...
> ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd,
> vcpu_id);
>
> This ioctl returns EINVAL because it fails the kvmppc_sanity_check() in KVM.
>
> It depends on QEMU enabling KVM_CAP_PPC_PAPR for the vCPU, which happens in:
>
> spapr_cpu_core_realize_child()
> {
> ...
> spapr_cpu_init(spapr, cpu, &local_err);
> cpu_ppc_set_papr()
> kvmppc_set_papr()
>
>>> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
>>> needed anymore and can be safely dropped.
>>>
>>> Signed-off-by: Greg Kurz <gr...@kaod.org>
>>
>>
>> Looks pretty good, though a couple nits below.
>>
>
> Thanks. I'm also thinking about going one step further and converting
> xics_kvm to use the icpc->realize() handler instead of icpc->cpu_setup().
> Makes sense ?
It should be fine I think, now that a reset handler is defined.
C.
>>> ---
>>> hw/intc/xics.c | 76
>>> ++++++++++++++++++++---------------------------
>>> hw/ppc/pnv_core.c | 16 ++++------
>>> hw/ppc/spapr_cpu_core.c | 21 ++++++-------
>>> include/hw/ppc/xics.h | 2 -
>>> 4 files changed, 48 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>>> index ec73f02144c9..330441ff7fe8 100644
>>> --- a/hw/intc/xics.c
>>> +++ b/hw/intc/xics.c
>>> @@ -38,50 +38,6 @@
>>> #include "monitor/monitor.h"
>>> #include "hw/intc/intc.h"
>>>
>>> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
>>> -{
>>> - CPUState *cs = CPU(cpu);
>>> - ICPState *icp = ICP(cpu->intc);
>>> -
>>> - assert(icp);
>>> - assert(cs == icp->cs);
>>> -
>>> - icp->output = NULL;
>>> - icp->cs = NULL;
>>> -}
>>> -
>>> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
>>> -{
>>> - CPUState *cs = CPU(cpu);
>>> - CPUPPCState *env = &cpu->env;
>>> - ICPStateClass *icpc;
>>> -
>>> - assert(icp);
>>> -
>>> - cpu->intc = OBJECT(icp);
>>> - icp->cs = cs;
>>> -
>>> - icpc = ICP_GET_CLASS(icp);
>>> - if (icpc->cpu_setup) {
>>> - icpc->cpu_setup(icp, cpu);
>>> - }
>>> -
>>> - switch (PPC_INPUT(env)) {
>>> - case PPC_FLAGS_INPUT_POWER7:
>>> - icp->output = env->irq_inputs[POWER7_INPUT_INT];
>>> - break;
>>> -
>>> - case PPC_FLAGS_INPUT_970:
>>> - icp->output = env->irq_inputs[PPC970_INPUT_INT];
>>> - break;
>>> -
>>> - default:
>>> - error_report("XICS interrupt controller does not support this CPU "
>>> - "bus model");
>>> - abort();
>>> - }
>>> -}
>>> -
>>> void icp_pic_print_info(ICPState *icp, Monitor *mon)
>>> {
>>> int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
>>> @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
>>> {
>>> ICPState *icp = ICP(dev);
>>> ICPStateClass *icpc = ICP_GET_CLASS(dev);
>>> + PowerPCCPU *cpu;
>>> + CPUPPCState *env;
>>> Object *obj;
>>> Error *err = NULL;
>>>
>>> @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
>>>
>>> icp->xics = XICS_FABRIC(obj);
>>>
>>> + obj = object_property_get_link(OBJECT(dev), "cs", &err);
>>
>> I don't like the name "cs" for an externally visible property. "cpu"
>> would be better.
>>
>
> Right. I'll fix this.
>
>>> + if (!obj) {
>>> + error_setg(errp, "%s: required link 'xics' not found: %s",
>>
>> Copy/paste mistake in this message.
>>
>
> Oops :)
>
>>> + __func__, error_get_pretty(err));
>>> + return;
>>> + }
>>> +
>>> + cpu = POWERPC_CPU(obj);
>>> + cpu->intc = OBJECT(icp);
>>> + icp->cs = CPU(obj);
>>> +
>>> + if (icpc->cpu_setup) {
>>> + icpc->cpu_setup(icp, cpu);
>>> + }
>>> +
>>> + env = &cpu->env;
>>> + switch (PPC_INPUT(env)) {
>>> + case PPC_FLAGS_INPUT_POWER7:
>>> + icp->output = env->irq_inputs[POWER7_INPUT_INT];
>>> + break;
>>> +
>>> + case PPC_FLAGS_INPUT_970:
>>> + icp->output = env->irq_inputs[PPC970_INPUT_INT];
>>> + break;
>>> +
>>> + default:
>>> + error_setg(errp, "XICS interrupt controller does not support this
>>> CPU bus model");
>>> + return;
>>> + }
>>> +
>>> if (icpc->realize) {
>>> icpc->realize(dev, errp);
>>> }
>>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
>>> index e8a9a94d5a24..1393005e76f3 100644
>>> --- a/hw/ppc/pnv_core.c
>>> +++ b/hw/ppc/pnv_core.c
>>> @@ -118,19 +118,19 @@ static void pnv_core_realize_child(Object *child,
>>> XICSFabric *xi, Error **errp)
>>> PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> Object *obj;
>>>
>>> - obj = object_new(TYPE_PNV_ICP);
>>> - object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
>>> - object_unref(obj);
>>> - object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
>>> - object_property_set_bool(obj, true, "realized", &local_err);
>>> + object_property_set_bool(child, true, "realized", &local_err);
>>> if (local_err) {
>>> error_propagate(errp, local_err);
>>> return;
>>> }
>>>
>>> - object_property_set_bool(child, true, "realized", &local_err);
>>> + obj = object_new(TYPE_PNV_ICP);
>>> + object_property_add_child(child, "icp", obj, NULL);
>>> + object_unref(obj);
>>> + object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
>>> + object_property_add_const_link(obj, "cs", child, &error_abort);
>>> + object_property_set_bool(obj, true, "realized", &local_err);
>>> if (local_err) {
>>> - object_unparent(obj);
>>> error_propagate(errp, local_err);
>>> return;
>>> }
>>> @@ -141,8 +141,6 @@ static void pnv_core_realize_child(Object *child,
>>> XICSFabric *xi, Error **errp)
>>> error_propagate(errp, local_err);
>>> return;
>>> }
>>> -
>>> - xics_cpu_setup(xi, cpu, ICP(obj));
>>> }
>>>
>>> static void pnv_core_realize(DeviceState *dev, Error **errp)
>>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>>> index 029a14120edd..9a6259525953 100644
>>> --- a/hw/ppc/spapr_cpu_core.c
>>> +++ b/hw/ppc/spapr_cpu_core.c
>>> @@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque)
>>>
>>> static void spapr_cpu_destroy(PowerPCCPU *cpu)
>>> {
>>> - sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>> -
>>> - xics_cpu_destroy(XICS_FABRIC(spapr), cpu);
>>> qemu_unregister_reset(spapr_cpu_reset, cpu);
>>> }
>>>
>>> @@ -140,28 +137,28 @@ static void spapr_cpu_core_realize_child(Object
>>> *child, Error **errp)
>>> sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>> CPUState *cs = CPU(child);
>>> PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> - Object *obj;
>>> + Object *obj = NULL;
>>>
>>> - obj = object_new(spapr->icp_type);
>>> - object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
>>> - object_unref(obj);
>>> - object_property_add_const_link(obj, "xics", OBJECT(spapr),
>>> &error_abort);
>>> - object_property_set_bool(obj, true, "realized", &local_err);
>>> + object_property_set_bool(child, true, "realized", &local_err);
>>> if (local_err) {
>>> goto error;
>>> }
>>>
>>> - object_property_set_bool(child, true, "realized", &local_err);
>>> + spapr_cpu_init(spapr, cpu, &local_err);
>>> if (local_err) {
>>> goto error;
>>> }
>>>
>>> - spapr_cpu_init(spapr, cpu, &local_err);
>>> + obj = object_new(spapr->icp_type);
>>> + object_property_add_child(child, "icp", obj, &error_abort);
>>> + object_unref(obj);
>>> + object_property_add_const_link(obj, "xics", OBJECT(spapr),
>>> &error_abort);
>>> + object_property_add_const_link(obj, "cs", child, &error_abort);
>>> + object_property_set_bool(obj, true, "realized", &local_err);
>>> if (local_err) {
>>> goto error;
>>> }
>>>
>>> - xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
>>> return;
>>>
>>> error:
>>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>>> index 40a506eacfb4..05767a15be9a 100644
>>> --- a/include/hw/ppc/xics.h
>>> +++ b/include/hw/ppc/xics.h
>>> @@ -183,8 +183,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uint32_t
>>> phandle);
>>>
>>> qemu_irq xics_get_qirq(XICSFabric *xi, int irq);
>>> ICPState *xics_icp_get(XICSFabric *xi, int server);
>>> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
>>> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
>>>
>>> /* Internal XICS interfaces */
>>> void icp_set_cppr(ICPState *icp, uint8_t cppr);
>>>
>>
>
