From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= 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 Message-ID: References: <20190222112840.25000-1-clg@kaod.org> <20190222112840.25000-4-clg@kaod.org> <20190225043551.GA20501@blackberry> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, David Gibson To: Paul Mackerras Return-path: In-Reply-To: <20190225043551.GA20501@blackberry> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" List-Id: kvm.vger.kernel.org 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Date: Wed, 13 Mar 2019 08:34:53 +0000 Subject: Re: [PATCH v2 03/16] KVM: PPC: Book3S HV: XIVE: introduce a new capability KVM_CAP_PPC_IRQ_XIVE Message-Id: List-Id: References: <20190222112840.25000-1-clg@kaod.org> <20190222112840.25000-4-clg@kaod.org> <20190225043551.GA20501@blackberry> In-Reply-To: <20190225043551.GA20501@blackberry> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Paul Mackerras Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, David Gibson On 2/25/19 5:35 AM, Paul Mackerras wrote: > On Fri, Feb 22, 2019 at 12:28:27PM +0100, C=C3=A9dric 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. >=20 > 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, l= ong ext) >> case KVM_CAP_PPC_GET_CPU_CHAR: >> r =3D 1; >> break; >> +#ifdef CONFIG_KVM_XIVE >> + case KVM_CAP_PPC_IRQ_XIVE: >> + /* only for PowerNV */ >> + r =3D !!cpu_has_feature(CPU_FTR_HVMODE); >=20 > Shouldn't this be r =3D xive_enabled() && !!cpu_has_feature(CPU_FTR_HVMOD= E) yes. we need the '__xive_enabled' toggle to be set also :/=20 It can set to off with the "xive=3Doff" on the command line and on old P9=20 skiboot. That could be simplified one day. > (or alternatively r =3D 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.=20 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.=20 This should not be too complex to fix. Thanks, C.