Re: [PATCH 03/19] KVM: PPC: Book3S HV: check the IRQ controller type

["Cédric Le Goater" <clg@kaod.org>]

On 2/4/19 1:50 AM, David Gibson wrote:
> On Wed, Jan 23, 2019 at 05:24:13PM +0100, Cédric Le Goater wrote:
>> On 1/22/19 5:56 AM, Paul Mackerras wrote:
>>> On Mon, Jan 07, 2019 at 07:43:15PM +0100, Cédric Le Goater wrote:
>>>> We will have different KVM devices for interrupts, one for the
>>>> XICS-over-XIVE mode and one for the XIVE native exploitation
>>>> mode. Let's add some checks to make sure we are not mixing the
>>>> interfaces in KVM.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>  arch/powerpc/kvm/book3s_xive.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
>>>> index f78d002f0fe0..8a4fa45f07f8 100644
>>>> --- a/arch/powerpc/kvm/book3s_xive.c
>>>> +++ b/arch/powerpc/kvm/book3s_xive.c
>>>> @@ -819,6 +819,9 @@ u64 kvmppc_xive_get_icp(struct kvm_vcpu *vcpu)
>>>>  {
>>>>  	struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>>>>  
>>>> +	if (!kvmppc_xics_enabled(vcpu))
>>>> +		return -EPERM;
>>>> +
>>>>  	if (!xc)
>>>>  		return 0;
>>>>  
>>>> @@ -835,6 +838,9 @@ int kvmppc_xive_set_icp(struct kvm_vcpu *vcpu, u64 icpval)
>>>>  	u8 cppr, mfrr;
>>>>  	u32 xisr;
>>>>  
>>>> +	if (!kvmppc_xics_enabled(vcpu))
>>>> +		return -EPERM;
>>>> +
>>>>  	if (!xc || !xive)
>>>>  		return -ENOENT;
>>>
>>> I can't see how these new checks could ever trigger in the code as it
>>> stands.  Is there a way at present? 
>>
>> It would require some custom QEMU doing silly things : create the XICS 
>> KVM device, and then call kvm_get_one_reg(KVM_REG_PPC_ICP_STATE) or 
>> kvm_set_one_reg(icp->cs, KVM_REG_PPC_ICP_STATE) without connecting the
>> vCPU to its presenter. 
>>
>> Today, you get a ENOENT.
> 
> TBH, ENOENT seems fine to me.
> 
>>> Do following patches ever add a path where the new checks could trigger, 
>>> or is this just an excess of caution? 
>>
>> With the following patches, QEMU could to do something even more silly,
>> which is to mix the interrupt mode interfaces : create a KVM XICS device    
>> and call KVM CPU ioctls of the KVM XIVE device, or the opposite.
> 
> AFAICT, like above, that won't really differ from calling the XIVE CPU
> ioctl()s when no irqchip is set up at all, and should be covered by
> just a !xive check.

we can drop that patch. It does not bring much.

Thanks,

C.

> 
>>
>>> (Your patch description should ideally have answered these questions > for me.)
>>
>> Yes. I also think that I introduced this patch to early in the series.
>> It make more sense when the XICS and the XIVE KVM devices are available.  
>>
>> Thanks,
>>
>> C.
>>
>