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 <groug@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);
+        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 */

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 <groug@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()

Apart from that,

Reviewed-by: Cédric Le Goater <clg@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 */
> 

On Fri, 15 Feb 2019 13:54:02 +0100
Cédric Le Goater <clg@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 <groug@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...

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

On 2/15/19 2:03 PM, Greg Kurz wrote:
> On Fri, 15 Feb 2019 13:54:02 +0100
> Cédric Le Goater <clg@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 <groug@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. 

C.



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

On Fri, 15 Feb 2019 14:09:53 +0100
Cédric Le Goater <clg@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 <clg@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 <groug@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 <clg@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 */
> >>>     
> >>  
> >   
> 

On 2/15/19 2:27 PM, Greg Kurz wrote:
> On Fri, 15 Feb 2019 14:09:53 +0100
> Cédric Le Goater <clg@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 <clg@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 <groug@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.

yes. that's fine also.

C. 

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

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 <clg@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 <clg@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 <groug@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.

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 <clg@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 */
> > >>>     
> > >>  
> > >   
> > 
> 

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