On Fri, 15 Feb 2019 14:09:53 +0100
Cédric Le Goater <c...@kaod.org> wrote:
> On 2/15/19 2:03 PM, Greg Kurz wrote:
> > On Fri, 15 Feb 2019 13:54:02 +0100
> > Cédric Le Goater <c...@kaod.org> wrote:
> >
> >> On 2/15/19 12:40 PM, Greg Kurz wrote:
> >>> The realization of KVM ICP currently follows the parent_realize logic,
> >>> which is a bit overkill here. Also we want to get rid of the KVM ICP
> >>> class. Explicitely call icp_kvm_realize() from the base ICP realize
> >>> function.
> >>>
> >>> Note that ICPStateClass::parent_realize is retained because powernv
> >>> needs it.
> >>>
> >>> Signed-off-by: Greg Kurz <gr...@kaod.org>>
> >>> ---
> >>> hw/intc/xics.c | 8 ++++++++
> >>> hw/intc/xics_kvm.c | 10 +---------
> >>> include/hw/ppc/xics.h | 1 +
> >>> 3 files changed, 10 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> >>> index 822d367e6388..acd63ab5e0b9 100644
> >>> --- a/hw/intc/xics.c
> >>> +++ b/hw/intc/xics.c
> >>> @@ -349,6 +349,14 @@ static void icp_realize(DeviceState *dev, Error
> >>> **errp)
> >>> return;
> >>> }
> >>>
> >>> + if (kvm_irqchip_in_kernel()) {
> >>> + icp_kvm_realize(dev, &err);
> >>
> >> While we are at changing things, I would prefix all the KVM
> >> backends routine with kvmppc_*. so that icp_kvm_realize()
> >> becomes kvmppc_icp_realize()
> >>
> >
> > Well... kvmppc_* routines have historically been sitting under
> > target/ppc so I'm not sure we want to use the same prefix
> > elsewhere...
>
> Well, they could also be moved there but I think what is important
> is that the kvmppc_* routine should be used under the kvm_enabled()
> flag.
>
> Those under target/ppc have and extra dummy stub provided for the
> !kvm_enabled() case.
>
Well, I don't really care but if we go this way (David?), I'd rather do it
globally in a followup patch.
> C.
>
>
>
> >
> >> Apart from that,
> >>
> >> Reviewed-by: Cédric Le Goater <c...@kaod.org>
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >>
> >>> + if (err) {
> >>> + error_propagate(errp, err);
> >>> + return;
> >>> + }
> >>> + }
> >>> +
> >>> qemu_register_reset(icp_reset_handler, dev);
> >>> vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> >>> }
> >>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> >>> index 80321e9b75ab..4eebced516b6 100644
> >>> --- a/hw/intc/xics_kvm.c
> >>> +++ b/hw/intc/xics_kvm.c
> >>> @@ -115,11 +115,9 @@ int icp_set_kvm_state(ICPState *icp)
> >>> return 0;
> >>> }
> >>>
> >>> -static void icp_kvm_realize(DeviceState *dev, Error **errp)
> >>> +void icp_kvm_realize(DeviceState *dev, Error **errp)
> >>> {
> >>> ICPState *icp = ICP(dev);
> >>> - ICPStateClass *icpc = ICP_GET_CLASS(icp);
> >>> - Error *local_err = NULL;
> >>> CPUState *cs;
> >>> KVMEnabledICP *enabled_icp;
> >>> unsigned long vcpu_id;
> >>> @@ -129,12 +127,6 @@ static void icp_kvm_realize(DeviceState *dev, Error
> >>> **errp)
> >>> abort();
> >>> }
> >>>
> >>> - icpc->parent_realize(dev, &local_err);
> >>> - if (local_err) {
> >>> - error_propagate(errp, local_err);
> >>> - return;
> >>> - }
> >>> -
> >>> cs = icp->cs;
> >>> vcpu_id = kvm_arch_vcpu_id(cs);
> >>>
> >>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> >>> index e33282a576d0..ab61dc24010a 100644
> >>> --- a/include/hw/ppc/xics.h
> >>> +++ b/include/hw/ppc/xics.h
> >>> @@ -202,5 +202,6 @@ Object *icp_create(Object *cpu, const char *type,
> >>> XICSFabric *xi,
> >>> void icp_get_kvm_state(ICPState *icp);
> >>> int icp_set_kvm_state(ICPState *icp);
> >>> void icp_synchronize_state(ICPState *icp);
> >>> +void icp_kvm_realize(DeviceState *dev, Error **errp);
> >>>
> >>> #endif /* XICS_H */
> >>>
> >>
> >
>
