Re: [PATCH] KVM: arm64: Clarify vcpu reset behaviour

[Marc Zyngier <maz@kernel.org>]

On Wed, 07 Apr 2021 16:59:58 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On 4/6/21 1:58 PM, Marc Zyngier wrote:
> > Although the KVM_ARM_VCPU_INIT documentation mention that the
> > registers are reset to their "initial values", it doesn't
> > describe what these values are.
> >
> > Describe this state explicitly.
> 
> This is a good idea.

Yeah, it turns out this is something we need for pKVM, because the
reset state is slightly different.

> 
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  Documentation/virt/kvm/api.rst | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> > index 38e327d4b479..e2237e4e10ba 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -3115,6 +3115,16 @@ optional features it should have.  This will cause a reset of the cpu
> >  registers to their initial values.  If this is not called, KVM_RUN will
> >  return ENOEXEC for that vcpu.
> >  
> > +The initial values are defined as:
> > +	- Processor state:
> > +		* AArch64: EL1h, D, A, I and F bits set
> 
> That value matches the macro VCPU_RESET_STATE_EL1.
> 
> > +		* Aarch32: SVC, D, A, I and F bits set
> VCPU_RESET_STATE_SVC doesn't have a D bit; according to ARM DDI 0487G.a, page
> G1-5965, CPSR doesn't have a D bit either (I might be reading it wrong).

Meh, copy paste. Thanks for that,

> > +	- General Purpose registers, including PC and SP: set to 0
> 
> They are zero'ed explicitly in kvm_reset_vcpu().
> 
> > +	- FPSIMD/NEON registers: set to 0
> 
> I haven't been able to find where they are initialized explicitly, I
> assume they aren't. They are zero because the kvm_vcpu struct is
> allocated via kmem_cache_zalloc() in kvm_vm_ioctl_create_vcpu(), so
> this is correct.

Indeed. However, this outlines an interesting buglet. Userspace is
allowed to call KVM_ARM_VCPU_INIT at any point, and get the vcpu back
to the reset state.

The lack of explicit wipeout of the FPSIMD file is on its own a bug
which crept in with e47c2055c6 ("KVM: arm64: Make struct kvm_regs
userspace-only"), when kvm_cpu_context started to use user_pt_regs
directly.

I'll fix that separately, thanks for pushing me to have a closer look.

> 
> > +	- SVE registers: set to 0
> 
> They are zero'ed explicitly in kvm_vcpu_reset_sve().
> 
> > +	- System registers: Reset to their architecturally defined
> > +	  values as for a warm reset to EL1 (resp. SVC)
> 
> This is done in kvm_reset_vcpu() -> kvm_reset_sys_regs(), which from
> what I can tell does the right thing, but I haven't checked each and
> every register.
> 
> Assuming the D bit for CPSR/SPSR was a typo and with it removed:
> 
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks for that. I'll send the other fix separately.

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel