| Message ID | 20190107184331.8429-4-clg@kaod.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | KVM: PPC: Book3S HV: add XIVE native exploitation mode | expand |
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> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > 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; >
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? Do following patches ever add a path where the new checks could trigger, or is this just an excess of caution? (Your patch description should ideally have answered these questions for me.) Paul.
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. > 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. > (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.
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. >
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. >> >
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;
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(+)