Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1323132 > unrolled thread

Re: [PATCH v2 10/21] arm64: KVM: VHE: Split save/restore of sysregs shared between EL1 and EL2

Started byChristoffer Dall <christoffer.dall@linaro.org>
First post2016-02-01 15:00 +0100
Last post2016-02-02 21:10 +0100
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 10/21] arm64: KVM: VHE: Split save/restore of sysregs  shared between EL1 and EL2 Christoffer Dall <christoffer.dall@linaro.org> - 2016-02-01 15:00 +0100
    Re: [PATCH v2 10/21] arm64: KVM: VHE: Split save/restore of sysregs  shared between EL1 and EL2 Marc Zyngier <marc.zyngier@arm.com> - 2016-02-02 10:50 +0100
      Re: [PATCH v2 10/21] arm64: KVM: VHE: Split save/restore of sysregs  shared between EL1 and EL2 Christoffer Dall <christoffer.dall@linaro.org> - 2016-02-02 16:50 +0100
        Re: [PATCH v2 10/21] arm64: KVM: VHE: Split save/restore of sysregs  shared between EL1 and EL2 Marc Zyngier <marc.zyngier@arm.com> - 2016-02-02 17:20 +0100
          Re: [PATCH v2 10/21] arm64: KVM: VHE: Split save/restore of sysregs  shared between EL1 and EL2 Christoffer Dall <christoffer.dall@linaro.org> - 2016-02-02 21:10 +0100

#1323132 — Re: [PATCH v2 10/21] arm64: KVM: VHE: Split save/restore of sysregs shared between EL1 and EL2

FromChristoffer Dall <christoffer.dall@linaro.org>
Date2016-02-01 15:00 +0100
SubjectRe: [PATCH v2 10/21] arm64: KVM: VHE: Split save/restore of sysregs shared between EL1 and EL2
Message-ID<qXnbk-3DE-7@gated-at.bofh.it>
On Mon, Jan 25, 2016 at 03:53:44PM +0000, Marc Zyngier wrote:
> A handful of system registers are still shared between EL1 and EL2,
> even while using VHE. These are tpidr*_el[01], actlr_el1, sp0, elr,
> and spsr.

So by shared registers you mean registers that do both have an EL0/1
version as well as an EL2 version, but where accesses aren't rewritten
transparently?

also, by sp0 do you mean sp_el0, and by elr you mean elr_el1, and by
spsr you mean spsr_el1 ?


> 
> In order to facilitate the introduction of a VHE-specific sysreg
> save/restore, make move the access to these registers to their
> own save/restore functions.
> 
> No functionnal change.

Otherwise:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/hyp/sysreg-sr.c | 48 +++++++++++++++++++++++++++++-------------
>  1 file changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index bd5b543..61bad17 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -23,13 +23,29 @@
>  
>  #include "hyp.h"
>  
> -/* ctxt is already in the HYP VA space */
> +/*
> + * Non-VHE: Both host and guest must save everything.
> + *
> + * VHE: Host must save tpidr*_el[01], actlr_el1, sp0, pc, pstate, and
> + * guest must save everything.
> + */
> +
> +static void __hyp_text __sysreg_save_common_state(struct kvm_cpu_context *ctxt)
> +{
> +	ctxt->sys_regs[ACTLR_EL1]	= read_sysreg(actlr_el1);
> +	ctxt->sys_regs[TPIDR_EL0]	= read_sysreg(tpidr_el0);
> +	ctxt->sys_regs[TPIDRRO_EL0]	= read_sysreg(tpidrro_el0);
> +	ctxt->sys_regs[TPIDR_EL1]	= read_sysreg(tpidr_el1);
> +	ctxt->gp_regs.regs.sp		= read_sysreg(sp_el0);
> +	ctxt->gp_regs.regs.pc		= read_sysreg(elr_el2);
> +	ctxt->gp_regs.regs.pstate	= read_sysreg(spsr_el2);
> +}
> +
>  static void __hyp_text __sysreg_save_state(struct kvm_cpu_context *ctxt)
>  {
>  	ctxt->sys_regs[MPIDR_EL1]	= read_sysreg(vmpidr_el2);
>  	ctxt->sys_regs[CSSELR_EL1]	= read_sysreg(csselr_el1);
>  	ctxt->sys_regs[SCTLR_EL1]	= read_sysreg(sctlr_el1);
> -	ctxt->sys_regs[ACTLR_EL1]	= read_sysreg(actlr_el1);
>  	ctxt->sys_regs[CPACR_EL1]	= read_sysreg(cpacr_el1);
>  	ctxt->sys_regs[TTBR0_EL1]	= read_sysreg(ttbr0_el1);
>  	ctxt->sys_regs[TTBR1_EL1]	= read_sysreg(ttbr1_el1);
> @@ -41,17 +57,11 @@ static void __hyp_text __sysreg_save_state(struct kvm_cpu_context *ctxt)
>  	ctxt->sys_regs[MAIR_EL1]	= read_sysreg(mair_el1);
>  	ctxt->sys_regs[VBAR_EL1]	= read_sysreg(vbar_el1);
>  	ctxt->sys_regs[CONTEXTIDR_EL1]	= read_sysreg(contextidr_el1);
> -	ctxt->sys_regs[TPIDR_EL0]	= read_sysreg(tpidr_el0);
> -	ctxt->sys_regs[TPIDRRO_EL0]	= read_sysreg(tpidrro_el0);
> -	ctxt->sys_regs[TPIDR_EL1]	= read_sysreg(tpidr_el1);
>  	ctxt->sys_regs[AMAIR_EL1]	= read_sysreg(amair_el1);
>  	ctxt->sys_regs[CNTKCTL_EL1]	= read_sysreg(cntkctl_el1);
>  	ctxt->sys_regs[PAR_EL1]		= read_sysreg(par_el1);
>  	ctxt->sys_regs[MDSCR_EL1]	= read_sysreg(mdscr_el1);
>  
> -	ctxt->gp_regs.regs.sp		= read_sysreg(sp_el0);
> -	ctxt->gp_regs.regs.pc		= read_sysreg(elr_el2);
> -	ctxt->gp_regs.regs.pstate	= read_sysreg(spsr_el2);
>  	ctxt->gp_regs.sp_el1		= read_sysreg(sp_el1);
>  	ctxt->gp_regs.elr_el1		= read_sysreg(elr_el1);
>  	ctxt->gp_regs.spsr[KVM_SPSR_EL1]= read_sysreg(spsr_el1);
> @@ -60,11 +70,24 @@ static void __hyp_text __sysreg_save_state(struct kvm_cpu_context *ctxt)
>  void __hyp_text __sysreg_save_host_state(struct kvm_cpu_context *ctxt)
>  {
>  	__sysreg_save_state(ctxt);
> +	__sysreg_save_common_state(ctxt);
>  }
>  
>  void __hyp_text __sysreg_save_guest_state(struct kvm_cpu_context *ctxt)
>  {
>  	__sysreg_save_state(ctxt);
> +	__sysreg_save_common_state(ctxt);
> +}
> +
> +static void __hyp_text __sysreg_restore_common_state(struct kvm_cpu_context *ctxt)
> +{
> +	write_sysreg(ctxt->sys_regs[ACTLR_EL1],	  actlr_el1);
> +	write_sysreg(ctxt->sys_regs[TPIDR_EL0],	  tpidr_el0);
> +	write_sysreg(ctxt->sys_regs[TPIDRRO_EL0], tpidrro_el0);
> +	write_sysreg(ctxt->sys_regs[TPIDR_EL1],	  tpidr_el1);
> +	write_sysreg(ctxt->gp_regs.regs.sp,	  sp_el0);
> +	write_sysreg(ctxt->gp_regs.regs.pc,	  elr_el2);
> +	write_sysreg(ctxt->gp_regs.regs.pstate,	  spsr_el2);
>  }
>  
>  static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt)
> @@ -72,7 +95,6 @@ static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt)
>  	write_sysreg(ctxt->sys_regs[MPIDR_EL1],	  vmpidr_el2);
>  	write_sysreg(ctxt->sys_regs[CSSELR_EL1],  csselr_el1);
>  	write_sysreg(ctxt->sys_regs[SCTLR_EL1],	  sctlr_el1);
> -	write_sysreg(ctxt->sys_regs[ACTLR_EL1],	  actlr_el1);
>  	write_sysreg(ctxt->sys_regs[CPACR_EL1],	  cpacr_el1);
>  	write_sysreg(ctxt->sys_regs[TTBR0_EL1],	  ttbr0_el1);
>  	write_sysreg(ctxt->sys_regs[TTBR1_EL1],	  ttbr1_el1);
> @@ -84,17 +106,11 @@ static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt)
>  	write_sysreg(ctxt->sys_regs[MAIR_EL1],	  mair_el1);
>  	write_sysreg(ctxt->sys_regs[VBAR_EL1],	  vbar_el1);
>  	write_sysreg(ctxt->sys_regs[CONTEXTIDR_EL1], contextidr_el1);
> -	write_sysreg(ctxt->sys_regs[TPIDR_EL0],	  tpidr_el0);
> -	write_sysreg(ctxt->sys_regs[TPIDRRO_EL0], tpidrro_el0);
> -	write_sysreg(ctxt->sys_regs[TPIDR_EL1],	  tpidr_el1);
>  	write_sysreg(ctxt->sys_regs[AMAIR_EL1],	  amair_el1);
>  	write_sysreg(ctxt->sys_regs[CNTKCTL_EL1], cntkctl_el1);
>  	write_sysreg(ctxt->sys_regs[PAR_EL1],	  par_el1);
>  	write_sysreg(ctxt->sys_regs[MDSCR_EL1],	  mdscr_el1);
>  
> -	write_sysreg(ctxt->gp_regs.regs.sp,	sp_el0);
> -	write_sysreg(ctxt->gp_regs.regs.pc,	elr_el2);
> -	write_sysreg(ctxt->gp_regs.regs.pstate,	spsr_el2);
>  	write_sysreg(ctxt->gp_regs.sp_el1,	sp_el1);
>  	write_sysreg(ctxt->gp_regs.elr_el1,	elr_el1);
>  	write_sysreg(ctxt->gp_regs.spsr[KVM_SPSR_EL1], spsr_el1);
> @@ -103,11 +119,13 @@ static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt)
>  void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt)
>  {
>  	__sysreg_restore_state(ctxt);
> +	__sysreg_restore_common_state(ctxt);
>  }
>  
>  void __hyp_text __sysreg_restore_guest_state(struct kvm_cpu_context *ctxt)
>  {
>  	__sysreg_restore_state(ctxt);
> +	__sysreg_restore_common_state(ctxt);
>  }
>  
>  void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
> -- 
> 2.1.4
>

[toc] | [next] | [standalone]

#1323912

FromMarc Zyngier <marc.zyngier@arm.com>
Date2016-02-02 10:50 +0100
Message-ID<qXFKW-BP-5@gated-at.bofh.it>
In reply to#1323132
On 01/02/16 13:54, Christoffer Dall wrote:
> On Mon, Jan 25, 2016 at 03:53:44PM +0000, Marc Zyngier wrote:
>> A handful of system registers are still shared between EL1 and EL2,
>> even while using VHE. These are tpidr*_el[01], actlr_el1, sp0, elr,
>> and spsr.
> 
> So by shared registers you mean registers that do both have an EL0/1
> version as well as an EL2 version, but where accesses aren't rewritten
> transparently?

No, I mean that these registers do *not* have a separate banked version.
There is only a single set of registers, which have to be save/restored
the old way.

> 
> also, by sp0 do you mean sp_el0, and by elr you mean elr_el1, and by
> spsr you mean spsr_el1 ?

sp0 -> sp_el0 indeed. elr and spsr really are the guest PC and PSTATE,
so I should really reword this commit message, it is utterly confusing.

> 
>>
>> In order to facilitate the introduction of a VHE-specific sysreg
>> save/restore, make move the access to these registers to their
>> own save/restore functions.
>>
>> No functionnal change.
> 
> Otherwise:
> 
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1324211

FromChristoffer Dall <christoffer.dall@linaro.org>
Date2016-02-02 16:50 +0100
Message-ID<qXLnl-55B-25@gated-at.bofh.it>
In reply to#1323912
On Tue, Feb 02, 2016 at 09:46:05AM +0000, Marc Zyngier wrote:
> On 01/02/16 13:54, Christoffer Dall wrote:
> > On Mon, Jan 25, 2016 at 03:53:44PM +0000, Marc Zyngier wrote:
> >> A handful of system registers are still shared between EL1 and EL2,
> >> even while using VHE. These are tpidr*_el[01], actlr_el1, sp0, elr,
> >> and spsr.
> > 
> > So by shared registers you mean registers that do both have an EL0/1
> > version as well as an EL2 version, but where accesses aren't rewritten
> > transparently?
> 
> No, I mean that these registers do *not* have a separate banked version.
> There is only a single set of registers, which have to be save/restored
> the old way.

huh, ARMv8 clearly specifies the existence of TPIDR_EL0, TPIDR_EL1, and
TPIDR_EL2, for example.

I cannot seem to find anywhere in the VHE spec that says that the
TPIDR_EL2 goes away.  I'm confused now.

> 
> > 
> > also, by sp0 do you mean sp_el0, and by elr you mean elr_el1, and by
> > spsr you mean spsr_el1 ?
> 
> sp0 -> sp_el0 indeed. elr and spsr really are the guest PC and PSTATE,
> so I should really reword this commit message, it is utterly confusing.
> 
I guess I don't understand the definition of a 'shared' register given
your comments here...

Thanks,
-Christoffer

[toc] | [prev] | [next] | [standalone]

#1324239

FromMarc Zyngier <marc.zyngier@arm.com>
Date2016-02-02 17:20 +0100
Message-ID<qXLQm-5Be-15@gated-at.bofh.it>
In reply to#1324211
On 02/02/16 15:46, Christoffer Dall wrote:
> On Tue, Feb 02, 2016 at 09:46:05AM +0000, Marc Zyngier wrote:
>> On 01/02/16 13:54, Christoffer Dall wrote:
>>> On Mon, Jan 25, 2016 at 03:53:44PM +0000, Marc Zyngier wrote:
>>>> A handful of system registers are still shared between EL1 and EL2,
>>>> even while using VHE. These are tpidr*_el[01], actlr_el1, sp0, elr,
>>>> and spsr.
>>>
>>> So by shared registers you mean registers that do both have an EL0/1
>>> version as well as an EL2 version, but where accesses aren't rewritten
>>> transparently?
>>
>> No, I mean that these registers do *not* have a separate banked version.
>> There is only a single set of registers, which have to be save/restored
>> the old way.
> 
> huh, ARMv8 clearly specifies the existence of TPIDR_EL0, TPIDR_EL1, and
> TPIDR_EL2, for example.
> 
> I cannot seem to find anywhere in the VHE spec that says that the
> TPIDR_EL2 goes away.  I'm confused now.

Nothing goes away, but these registers do not get renamed either. For
example, TPIDR_EL1 doesn't magically access TPIDR_EL2 when running at
EL2+VHE, and there is no TPIDR_EL12 accessor either.

So TPIDR_EL1 is effectively "shared" between host and guest, and must be
save/restored (note that the host kernel still uses TIPDR_EL1 even when
running with VHE, and that KVM still uses TPIDR_EL2 to cache the current
vcpu).

>>
>>>
>>> also, by sp0 do you mean sp_el0, and by elr you mean elr_el1, and by
>>> spsr you mean spsr_el1 ?
>>
>> sp0 -> sp_el0 indeed. elr and spsr really are the guest PC and PSTATE,
>> so I should really reword this commit message, it is utterly confusing.
>>
> I guess I don't understand the definition of a 'shared' register given
> your comments here...

Does this make it clearer?

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1324582

FromChristoffer Dall <christoffer.dall@linaro.org>
Date2016-02-02 21:10 +0100
Message-ID<qXPqW-8iU-11@gated-at.bofh.it>
In reply to#1324239
On Tue, Feb 02, 2016 at 04:19:44PM +0000, Marc Zyngier wrote:
> On 02/02/16 15:46, Christoffer Dall wrote:
> > On Tue, Feb 02, 2016 at 09:46:05AM +0000, Marc Zyngier wrote:
> >> On 01/02/16 13:54, Christoffer Dall wrote:
> >>> On Mon, Jan 25, 2016 at 03:53:44PM +0000, Marc Zyngier wrote:
> >>>> A handful of system registers are still shared between EL1 and EL2,
> >>>> even while using VHE. These are tpidr*_el[01], actlr_el1, sp0, elr,
> >>>> and spsr.
> >>>
> >>> So by shared registers you mean registers that do both have an EL0/1
> >>> version as well as an EL2 version, but where accesses aren't rewritten
> >>> transparently?
> >>
> >> No, I mean that these registers do *not* have a separate banked version.
> >> There is only a single set of registers, which have to be save/restored
> >> the old way.
> > 
> > huh, ARMv8 clearly specifies the existence of TPIDR_EL0, TPIDR_EL1, and
> > TPIDR_EL2, for example.
> > 
> > I cannot seem to find anywhere in the VHE spec that says that the
> > TPIDR_EL2 goes away.  I'm confused now.
> 
> Nothing goes away, but these registers do not get renamed either. For
> example, TPIDR_EL1 doesn't magically access TPIDR_EL2 when running at
> EL2+VHE, and there is no TPIDR_EL12 accessor either.
> 
> So TPIDR_EL1 is effectively "shared" between host and guest, and must be
> save/restored (note that the host kernel still uses TIPDR_EL1 even when
> running with VHE, and that KVM still uses TPIDR_EL2 to cache the current
> vcpu).
> 

ok, I can understand as long as we're saying a register is shared
between the host and the guest, but it was the "registers are shared
between EL1 and EL2" that threw me off.

> >>
> >>>
> >>> also, by sp0 do you mean sp_el0, and by elr you mean elr_el1, and by
> >>> spsr you mean spsr_el1 ?
> >>
> >> sp0 -> sp_el0 indeed. elr and spsr really are the guest PC and PSTATE,
> >> so I should really reword this commit message, it is utterly confusing.
> >>
> > I guess I don't understand the definition of a 'shared' register given
> > your comments here...
> 
> Does this make it clearer?
> 
yes.  You could change the host to path it when using VHE to use
TPIDR_EL2 if you wanted and store the vcpu pointer on the stack while
running the guest, but there's probably no real benefit of doing so.

I'll be shutting up now...

Thanks,
-Christoffer

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web