[PATCH] KVM: arm64: Clarify vcpu reset behaviour

[PATCH] KVM: arm64: Clarify vcpu reset behaviour

[Marc Zyngier]

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.

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
+		* Aarch32: SVC, D, A, I and F bits set
+	- General Purpose registers, including PC and SP: set to 0
+	- FPSIMD/NEON registers: set to 0
+	- SVE registers: set to 0
+	- System registers: Reset to their architecturally defined
+	  values as for a warm reset to EL1 (resp. SVC)
+
 Note that because some registers reflect machine topology, all vcpus
 should be created before this ioctl is invoked.
 
-- 
2.29.2


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

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

[Alexandru Elisei]

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.

>
> 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).
> +	- 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.

> +	- 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,

Alex

> +
>  Note that because some registers reflect machine topology, all vcpus
>  should be created before this ioctl is invoked.
>  

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

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

[Marc Zyngier]

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

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

[Will Deacon]

On Tue, Apr 06, 2021 at 01:58:41PM +0100, 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.
> 
> 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
> +		* Aarch32: SVC, D, A, I and F bits set

nit, but capitalisation should be "AArch32"

Is "SVC" sufficient, or do we also need to describe Arm vs Thumb?

Will

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

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

[Marc Zyngier]

On 2021-04-07 21:35, Will Deacon wrote:
> On Tue, Apr 06, 2021 at 01:58:41PM +0100, 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.
>> 
>> 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
>> +		* Aarch32: SVC, D, A, I and F bits set
> 
> nit, but capitalisation should be "AArch32"
> 
> Is "SVC" sufficient, or do we also need to describe Arm vs Thumb?

SVC doesn't describe the instruction set, but we don't say that PSR.T
is set, which implies ARM.

I can add something like "All other bits are set to 0", if that makes it
clearer.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

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

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

[Will Deacon]

On Thu, Apr 08, 2021 at 11:36:34AM +0100, Marc Zyngier wrote:
> On 2021-04-07 21:35, Will Deacon wrote:
> > On Tue, Apr 06, 2021 at 01:58:41PM +0100, 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.
> > > 
> > > 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
> > > +		* Aarch32: SVC, D, A, I and F bits set
> > 
> > nit, but capitalisation should be "AArch32"
> > 
> > Is "SVC" sufficient, or do we also need to describe Arm vs Thumb?
> 
> SVC doesn't describe the instruction set, but we don't say that PSR.T
> is set, which implies ARM.

Ah yes, thanks.

> I can add something like "All other bits are set to 0", if that makes it
> clearer.

I think that would be worth adding. With that and the minor typo fix:

Acked-by: Will Deacon <will@kernel.org>

Will

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

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

[Marc Zyngier]

On Thu, 08 Apr 2021 11:53:35 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> On Thu, Apr 08, 2021 at 11:36:34AM +0100, Marc Zyngier wrote:
> > On 2021-04-07 21:35, Will Deacon wrote:
> > > On Tue, Apr 06, 2021 at 01:58:41PM +0100, 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.
> > > > 
> > > > 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
> > > > +		* Aarch32: SVC, D, A, I and F bits set
> > > 
> > > nit, but capitalisation should be "AArch32"
> > > 
> > > Is "SVC" sufficient, or do we also need to describe Arm vs Thumb?
> > 
> > SVC doesn't describe the instruction set, but we don't say that PSR.T
> > is set, which implies ARM.
> 
> Ah yes, thanks.
> 
> > I can add something like "All other bits are set to 0", if that makes it
> > clearer.
> 
> I think that would be worth adding. With that and the minor typo fix:
> 
> Acked-by: Will Deacon <will@kernel.org>

Now fixed.

Thanks,

	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