| Message ID | 20190621093843.220980-7-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: ARMv8.3 Nested Virtualization support | expand |
On 21/06/2019 10:37, Marc Zyngier wrote: > From: Christoffer Dall <christoffer.dall@linaro.org> > > We were not allowing userspace to set a more privileged mode for the VCPU > than EL1, but we should allow this when nested virtualization is enabled > for the VCPU. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/kvm/guest.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index 3ae2f82fca46..4c35b5d51e21 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -37,6 +37,7 @@ > #include <asm/kvm_emulate.h> > #include <asm/kvm_coproc.h> > #include <asm/kvm_host.h> > +#include <asm/kvm_nested.h> > #include <asm/sigcontext.h> > > #include "trace.h" > @@ -194,6 +195,11 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > if (vcpu_el1_is_32bit(vcpu)) > return -EINVAL; > break; > + case PSR_MODE_EL2h: > + case PSR_MODE_EL2t: > + if (vcpu_el1_is_32bit(vcpu) || !nested_virt_in_use(vcpu)) This condition reads a bit weirdly. Why do we care about anything else than !nested_virt_in_use() ? If nested virt is not in use then obviously we return the error. If nested virt is in use then why do we care about EL1? Or should this test read as "highest_el_is_32bit" ? Thanks,
On 21/06/2019 14:24, Julien Thierry wrote: > > > On 21/06/2019 10:37, Marc Zyngier wrote: >> From: Christoffer Dall <christoffer.dall@linaro.org> >> >> We were not allowing userspace to set a more privileged mode for the VCPU >> than EL1, but we should allow this when nested virtualization is enabled >> for the VCPU. >> >> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> arch/arm64/kvm/guest.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c >> index 3ae2f82fca46..4c35b5d51e21 100644 >> --- a/arch/arm64/kvm/guest.c >> +++ b/arch/arm64/kvm/guest.c >> @@ -37,6 +37,7 @@ >> #include <asm/kvm_emulate.h> >> #include <asm/kvm_coproc.h> >> #include <asm/kvm_host.h> >> +#include <asm/kvm_nested.h> >> #include <asm/sigcontext.h> >> >> #include "trace.h" >> @@ -194,6 +195,11 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) >> if (vcpu_el1_is_32bit(vcpu)) >> return -EINVAL; >> break; >> + case PSR_MODE_EL2h: >> + case PSR_MODE_EL2t: >> + if (vcpu_el1_is_32bit(vcpu) || !nested_virt_in_use(vcpu)) > > This condition reads a bit weirdly. Why do we care about anything else > than !nested_virt_in_use() ? > > If nested virt is not in use then obviously we return the error. > > If nested virt is in use then why do we care about EL1? Or should this > test read as "highest_el_is_32bit" ? There are multiple things at play here: - MODE_EL2x is not a valid 32bit mode - The architecture forbids nested virt with 32bit EL2 The code above is a simplification of these two conditions. But certainly we can do a bit better, as kvm_reset_cpu() doesn't really check that we don't create a vcpu with both 32bit+NV. These two bits should really be exclusive. M.
On Fri, Jun 21, 2019 at 02:50:08PM +0100, Marc Zyngier wrote: > On 21/06/2019 14:24, Julien Thierry wrote: > > > > > > On 21/06/2019 10:37, Marc Zyngier wrote: > >> From: Christoffer Dall <christoffer.dall@linaro.org> > >> > >> We were not allowing userspace to set a more privileged mode for the VCPU > >> than EL1, but we should allow this when nested virtualization is enabled > >> for the VCPU. > >> > >> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >> --- > >> arch/arm64/kvm/guest.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > >> index 3ae2f82fca46..4c35b5d51e21 100644 > >> --- a/arch/arm64/kvm/guest.c > >> +++ b/arch/arm64/kvm/guest.c > >> @@ -37,6 +37,7 @@ > >> #include <asm/kvm_emulate.h> > >> #include <asm/kvm_coproc.h> > >> #include <asm/kvm_host.h> > >> +#include <asm/kvm_nested.h> > >> #include <asm/sigcontext.h> > >> > >> #include "trace.h" > >> @@ -194,6 +195,11 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > >> if (vcpu_el1_is_32bit(vcpu)) > >> return -EINVAL; > >> break; > >> + case PSR_MODE_EL2h: > >> + case PSR_MODE_EL2t: > >> + if (vcpu_el1_is_32bit(vcpu) || !nested_virt_in_use(vcpu)) > > > > This condition reads a bit weirdly. Why do we care about anything else > > than !nested_virt_in_use() ? > > > > If nested virt is not in use then obviously we return the error. > > > > If nested virt is in use then why do we care about EL1? Or should this > > test read as "highest_el_is_32bit" ? > > There are multiple things at play here: > > - MODE_EL2x is not a valid 32bit mode > - The architecture forbids nested virt with 32bit EL2 > > The code above is a simplification of these two conditions. But > certainly we can do a bit better, as kvm_reset_cpu() doesn't really > check that we don't create a vcpu with both 32bit+NV. These two bits > should really be exclusive. This code is safe for now because KVM_VCPU_MAX_FEATURES <= KVM_ARM_VCPU_NESTED_VIRT, right, i.e., nested_virt_in_use() cannot be true? This makes me a little uneasy, but I think that's paranoia talking: we want bisectably, but no sane person should ship with just half of this series. So I guess this is fine. We could stick something like if (WARN_ON(...)) return false; in nested_virt_in_use() and then remove it in the final patch, but it's probably overkill. Cheers ---Dave
On 24/06/2019 13:48, Dave Martin wrote: > On Fri, Jun 21, 2019 at 02:50:08PM +0100, Marc Zyngier wrote: >> On 21/06/2019 14:24, Julien Thierry wrote: >>> >>> >>> On 21/06/2019 10:37, Marc Zyngier wrote: >>>> From: Christoffer Dall <christoffer.dall@linaro.org> >>>> >>>> We were not allowing userspace to set a more privileged mode for the VCPU >>>> than EL1, but we should allow this when nested virtualization is enabled >>>> for the VCPU. >>>> >>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>> --- >>>> arch/arm64/kvm/guest.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c >>>> index 3ae2f82fca46..4c35b5d51e21 100644 >>>> --- a/arch/arm64/kvm/guest.c >>>> +++ b/arch/arm64/kvm/guest.c >>>> @@ -37,6 +37,7 @@ >>>> #include <asm/kvm_emulate.h> >>>> #include <asm/kvm_coproc.h> >>>> #include <asm/kvm_host.h> >>>> +#include <asm/kvm_nested.h> >>>> #include <asm/sigcontext.h> >>>> >>>> #include "trace.h" >>>> @@ -194,6 +195,11 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) >>>> if (vcpu_el1_is_32bit(vcpu)) >>>> return -EINVAL; >>>> break; >>>> + case PSR_MODE_EL2h: >>>> + case PSR_MODE_EL2t: >>>> + if (vcpu_el1_is_32bit(vcpu) || !nested_virt_in_use(vcpu)) >>> >>> This condition reads a bit weirdly. Why do we care about anything else >>> than !nested_virt_in_use() ? >>> >>> If nested virt is not in use then obviously we return the error. >>> >>> If nested virt is in use then why do we care about EL1? Or should this >>> test read as "highest_el_is_32bit" ? >> >> There are multiple things at play here: >> >> - MODE_EL2x is not a valid 32bit mode >> - The architecture forbids nested virt with 32bit EL2 >> >> The code above is a simplification of these two conditions. But >> certainly we can do a bit better, as kvm_reset_cpu() doesn't really >> check that we don't create a vcpu with both 32bit+NV. These two bits >> should really be exclusive. > > This code is safe for now because KVM_VCPU_MAX_FEATURES <= > KVM_ARM_VCPU_NESTED_VIRT, right, i.e., nested_virt_in_use() cannot be > true? > > This makes me a little uneasy, but I think that's paranoia talking: we > want bisectably, but no sane person should ship with just half of this > series. So I guess this is fine. > > We could stick something like > > if (WARN_ON(...)) > return false; > > in nested_virt_in_use() and then remove it in the final patch, but it's > probably overkill. The only case I can imagine something going wrong is if this series is only applied halfway, and another series bumps the maximum feature to something that includes NV. I guess your suggestion would solve that. Thanks, M.
On Wed, Jul 03, 2019 at 10:21:57AM +0100, Marc Zyngier wrote: > On 24/06/2019 13:48, Dave Martin wrote: > > On Fri, Jun 21, 2019 at 02:50:08PM +0100, Marc Zyngier wrote: > >> On 21/06/2019 14:24, Julien Thierry wrote: > >>> > >>> > >>> On 21/06/2019 10:37, Marc Zyngier wrote: > >>>> From: Christoffer Dall <christoffer.dall@linaro.org> > >>>> > >>>> We were not allowing userspace to set a more privileged mode for the VCPU > >>>> than EL1, but we should allow this when nested virtualization is enabled > >>>> for the VCPU. > >>>> > >>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >>>> --- > >>>> arch/arm64/kvm/guest.c | 6 ++++++ > >>>> 1 file changed, 6 insertions(+) > >>>> > >>>> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > >>>> index 3ae2f82fca46..4c35b5d51e21 100644 > >>>> --- a/arch/arm64/kvm/guest.c > >>>> +++ b/arch/arm64/kvm/guest.c > >>>> @@ -37,6 +37,7 @@ > >>>> #include <asm/kvm_emulate.h> > >>>> #include <asm/kvm_coproc.h> > >>>> #include <asm/kvm_host.h> > >>>> +#include <asm/kvm_nested.h> > >>>> #include <asm/sigcontext.h> > >>>> > >>>> #include "trace.h" > >>>> @@ -194,6 +195,11 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > >>>> if (vcpu_el1_is_32bit(vcpu)) > >>>> return -EINVAL; > >>>> break; > >>>> + case PSR_MODE_EL2h: > >>>> + case PSR_MODE_EL2t: > >>>> + if (vcpu_el1_is_32bit(vcpu) || !nested_virt_in_use(vcpu)) > >>> > >>> This condition reads a bit weirdly. Why do we care about anything else > >>> than !nested_virt_in_use() ? > >>> > >>> If nested virt is not in use then obviously we return the error. > >>> > >>> If nested virt is in use then why do we care about EL1? Or should this > >>> test read as "highest_el_is_32bit" ? > >> > >> There are multiple things at play here: > >> > >> - MODE_EL2x is not a valid 32bit mode > >> - The architecture forbids nested virt with 32bit EL2 > >> > >> The code above is a simplification of these two conditions. But > >> certainly we can do a bit better, as kvm_reset_cpu() doesn't really > >> check that we don't create a vcpu with both 32bit+NV. These two bits > >> should really be exclusive. > > > > This code is safe for now because KVM_VCPU_MAX_FEATURES <= > > KVM_ARM_VCPU_NESTED_VIRT, right, i.e., nested_virt_in_use() cannot be > > true? > > > > This makes me a little uneasy, but I think that's paranoia talking: we > > want bisectably, but no sane person should ship with just half of this > > series. So I guess this is fine. > > > > We could stick something like > > > > if (WARN_ON(...)) > > return false; > > > > in nested_virt_in_use() and then remove it in the final patch, but it's > > probably overkill. > > The only case I can imagine something going wrong is if this series is > only applied halfway, and another series bumps the maximum feature to > something that includes NV. I guess your suggestion would solve that. I won't lose sleep over it either way. Cheers ---Dave
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index 3ae2f82fca46..4c35b5d51e21 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -37,6 +37,7 @@ #include <asm/kvm_emulate.h> #include <asm/kvm_coproc.h> #include <asm/kvm_host.h> +#include <asm/kvm_nested.h> #include <asm/sigcontext.h> #include "trace.h" @@ -194,6 +195,11 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) if (vcpu_el1_is_32bit(vcpu)) return -EINVAL; break; + case PSR_MODE_EL2h: + case PSR_MODE_EL2t: + if (vcpu_el1_is_32bit(vcpu) || !nested_virt_in_use(vcpu)) + return -EINVAL; + break; default: err = -EINVAL; goto out;