* [PATCH] KVM: arm64: Clarify vcpu reset behaviour
@ 2021-04-06 12:58 Marc Zyngier
2021-04-07 15:59 ` Alexandru Elisei
2021-04-07 20:35 ` Will Deacon
0 siblings, 2 replies; 7+ messages in thread
From: Marc Zyngier @ 2021-04-06 12:58 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Will Deacon, kernel-team, ascull, tabba, dbrazdil, James Morse,
Suzuki K Poulose, Alexandru Elisei
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: arm64: Clarify vcpu reset behaviour
2021-04-06 12:58 [PATCH] KVM: arm64: Clarify vcpu reset behaviour Marc Zyngier
@ 2021-04-07 15:59 ` Alexandru Elisei
2021-04-07 16:26 ` Marc Zyngier
2021-04-07 20:35 ` Will Deacon
1 sibling, 1 reply; 7+ messages in thread
From: Alexandru Elisei @ 2021-04-07 15:59 UTC (permalink / raw)
To: Marc Zyngier, linux-arm-kernel
Cc: Will Deacon, kernel-team, ascull, tabba, dbrazdil, James Morse,
Suzuki K Poulose
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: arm64: Clarify vcpu reset behaviour
2021-04-07 15:59 ` Alexandru Elisei
@ 2021-04-07 16:26 ` Marc Zyngier
0 siblings, 0 replies; 7+ messages in thread
From: Marc Zyngier @ 2021-04-07 16:26 UTC (permalink / raw)
To: Alexandru Elisei
Cc: linux-arm-kernel, Will Deacon, kernel-team, ascull, tabba,
dbrazdil, James Morse, Suzuki K Poulose
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: arm64: Clarify vcpu reset behaviour
2021-04-06 12:58 [PATCH] KVM: arm64: Clarify vcpu reset behaviour Marc Zyngier
2021-04-07 15:59 ` Alexandru Elisei
@ 2021-04-07 20:35 ` Will Deacon
2021-04-08 10:36 ` Marc Zyngier
1 sibling, 1 reply; 7+ messages in thread
From: Will Deacon @ 2021-04-07 20:35 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, kernel-team, ascull, tabba, dbrazdil,
James Morse, Suzuki K Poulose, Alexandru Elisei
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: arm64: Clarify vcpu reset behaviour
2021-04-07 20:35 ` Will Deacon
@ 2021-04-08 10:36 ` Marc Zyngier
2021-04-08 10:53 ` Will Deacon
0 siblings, 1 reply; 7+ messages in thread
From: Marc Zyngier @ 2021-04-08 10:36 UTC (permalink / raw)
To: Will Deacon
Cc: linux-arm-kernel, kernel-team, ascull, tabba, dbrazdil,
James Morse, Suzuki K Poulose, Alexandru Elisei
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: arm64: Clarify vcpu reset behaviour
2021-04-08 10:36 ` Marc Zyngier
@ 2021-04-08 10:53 ` Will Deacon
2021-04-08 13:14 ` Marc Zyngier
0 siblings, 1 reply; 7+ messages in thread
From: Will Deacon @ 2021-04-08 10:53 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, kernel-team, ascull, tabba, dbrazdil,
James Morse, Suzuki K Poulose, Alexandru Elisei
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: arm64: Clarify vcpu reset behaviour
2021-04-08 10:53 ` Will Deacon
@ 2021-04-08 13:14 ` Marc Zyngier
0 siblings, 0 replies; 7+ messages in thread
From: Marc Zyngier @ 2021-04-08 13:14 UTC (permalink / raw)
To: Will Deacon
Cc: linux-arm-kernel, kernel-team, ascull, tabba, dbrazdil,
James Morse, Suzuki K Poulose, Alexandru Elisei
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-04-08 13:16 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06 12:58 [PATCH] KVM: arm64: Clarify vcpu reset behaviour Marc Zyngier
2021-04-07 15:59 ` Alexandru Elisei
2021-04-07 16:26 ` Marc Zyngier
2021-04-07 20:35 ` Will Deacon
2021-04-08 10:36 ` Marc Zyngier
2021-04-08 10:53 ` Will Deacon
2021-04-08 13:14 ` Marc Zyngier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).