From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Subject: Re: [PATCH 03/19] KVM: PPC: Book3S HV: check the IRQ controller type Date: Mon, 4 Feb 2019 11:16:16 +0100 Message-ID: References: <20190107184331.8429-1-clg@kaod.org> <20190107184331.8429-4-clg@kaod.org> <20190122045642.GB15124@blackberry> <29eab965-3e98-714c-7686-f6caa59f3672@kaod.org> <20190204005056.GC2593@umbus.fritz.box> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org To: David Gibson Return-path: In-Reply-To: <20190204005056.GC2593@umbus.fritz.box> 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/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 >>>> --- >>>> 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. >> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Date: Mon, 04 Feb 2019 10:16:16 +0000 Subject: Re: [PATCH 03/19] KVM: PPC: Book3S HV: check the IRQ controller type Message-Id: List-Id: References: <20190107184331.8429-1-clg@kaod.org> <20190107184331.8429-4-clg@kaod.org> <20190122045642.GB15124@blackberry> <29eab965-3e98-714c-7686-f6caa59f3672@kaod.org> <20190204005056.GC2593@umbus.fritz.box> In-Reply-To: <20190204005056.GC2593@umbus.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: David Gibson Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org On 2/4/19 1:50 AM, David Gibson wrote: > On Wed, Jan 23, 2019 at 05:24:13PM +0100, C=E9dric Le Goater wrote: >> On 1/22/19 5:56 AM, Paul Mackerras wrote: >>> On Mon, Jan 07, 2019 at 07:43:15PM +0100, C=E9dric 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=E9dric Le Goater >>>> --- >>>> 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 =3D vcpu->arch.xive_vcpu; >>>> =20 >>>> + if (!kvmppc_xics_enabled(vcpu)) >>>> + return -EPERM; >>>> + >>>> if (!xc) >>>> return 0; >>>> =20 >>>> @@ -835,6 +838,9 @@ int kvmppc_xive_set_icp(struct kvm_vcpu *vcpu, u64= icpval) >>>> u8 cppr, mfrr; >>>> u32 xisr; >>>> =20 >>>> + 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?=20 >> >> It would require some custom QEMU doing silly things : create the XICS=20 >> KVM device, and then call kvm_get_one_reg(KVM_REG_PPC_ICP_STATE) or=20 >> kvm_set_one_reg(icp->cs, KVM_REG_PPC_ICP_STATE) without connecting the >> vCPU to its presenter.=20 >> >> Today, you get a ENOENT. >=20 > TBH, ENOENT seems fine to me. >=20 >>> Do following patches ever add a path where the new checks could trigger= ,=20 >>> or is this just an excess of caution?=20 >> >> 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= =20 >> and call KVM CPU ioctls of the KVM XIVE device, or the opposite. >=20 > 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. >=20 >> >>> (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.= =20 >> >> Thanks, >> >> C. >> >=20