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@xxxxxxxxxx> >>>> >>>> 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@xxxxxxxxxx> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx> >>>> --- >>>> 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. -- Jazz is not dead. It just smells funny...
