Re: [Qemu-ppc] [PATCH 03/10] xics: Handle KVM ICP realize from the common code

[David Gibson]

On Fri, Feb 15, 2019 at 02:27:41PM +0100, Greg Kurz wrote:
> On Fri, 15 Feb 2019 14:09:53 +0100
> Cédric Le Goater <address@hidden> wrote:
> 
> > On 2/15/19 2:03 PM, Greg Kurz wrote:
> > > On Fri, 15 Feb 2019 13:54:02 +0100
> > > Cédric Le Goater <address@hidden> 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 <address@hidden>>
> > >>> ---
> > >>>  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.

I concur.  I'm not all that fussed really, and I think it would be
done best as a followup.

> 
> > C.
> > 
> > 
> > 
> > >   
> > >> Apart from that,
> > >>
> > >> Reviewed-by: Cédric Le Goater <address@hidden>
> > >>
> > >> 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 */
> > >>>     
> > >>  
> > >   
> > 
> 

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