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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Marc Zyngier <[email protected]>
>>>> ---
>>>> 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...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
