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.

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

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