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

[Cédric Le Goater]

On 2/15/19 2:27 PM, 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.

yes. that's fine also.

C. 

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