Re: [PATCH v2 03/16] KVM: PPC: Book3S HV: XIVE: introduce a new capability KVM_CAP_PPC_IRQ_XIVE

From: "Cédric Le Goater" <clg@kaod.org>
To: Paul Mackerras <paulus@ozlabs.org>
Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org,
	David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH v2 03/16] KVM: PPC: Book3S HV: XIVE: introduce a new capability KVM_CAP_PPC_IRQ_XIVE
Date: Wed, 13 Mar 2019 09:34:53 +0100	[thread overview]
Message-ID: <a736c1b9-6406-2afa-d126-d0073fcd096e@kaod.org> (raw)
In-Reply-To: <20190225043551.GA20501@blackberry>

On 2/25/19 5:35 AM, Paul Mackerras wrote:
> On Fri, Feb 22, 2019 at 12:28:27PM +0100, Cédric Le Goater wrote:
>> The user interface exposes a new capability to let QEMU connect the
>> vCPU to the XIVE KVM device if required. The capability is only
>> advertised on a PowerNV Hypervisor as support for nested guests
>> (pseries KVM Hypervisor) is not yet available.
> 
> If a bisection happened to land on this commit, we would have KVM
> saying it had the ability to support guests using XIVE natively, but
> it wouldn't actually work since we don't have all the code that is in
> the following patches.

OK. I didn't think migration was a must-have for bisection. I will move
the enablement at end.

> Thus, in order to avoid breaking bisection, you should either add the
> capability now but have it always return false until the rest of the
> code is in place, or else defer the addition of the capability until
> the end of the patch series.

I will introduce the capability early in the patchset and return false
as you are proposing. It seems to be the best approach.

>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> index 8c69af10f91d..a38a643a24dd 100644
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -570,6 +570,12 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>  	case KVM_CAP_PPC_GET_CPU_CHAR:
>>  		r = 1;
>>  		break;
>> +#ifdef CONFIG_KVM_XIVE
>> +	case KVM_CAP_PPC_IRQ_XIVE:
>> +		/* only for PowerNV */
>> +		r = !!cpu_has_feature(CPU_FTR_HVMODE);
> 
> Shouldn't this be r = xive_enabled() && !!cpu_has_feature(CPU_FTR_HVMODE)

yes. we need the '__xive_enabled' toggle to be set also :/ 

It can set to off with the "xive=off" on the command line and on old P9 
skiboot. That could be simplified one day.

> (or alternatively r = xics_on_xive(), though that would be confusing
> to the reader)?

This is correct. I didn't want to use the xics_on_xive() which is not
the capability we are activating. 

I will keep the open-coded version.

> As it stands this would report true on POWER8, unless I'm missing
> something.

Ah yes. I forgot this combination also. 

This should not be too complex to fix.

Thanks,

C.