On Fri, 24 Nov 2023 16:34:51 +0000,
Joey Gouly <joey.gouly@xxxxxxx> wrote:
>
> Define the new system registers that POE introduces and context switch them.
I would really like to see a discussion on the respective lifetimes of
these two registers (see below).
>
> Signed-off-by: Joey Gouly <joey.gouly@xxxxxxx>
> Cc: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: Oliver Upton <oliver.upton@xxxxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> ---
> arch/arm64/include/asm/kvm_arm.h | 4 ++--
> arch/arm64/include/asm/kvm_host.h | 4 ++++
> arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 10 ++++++++++
> arch/arm64/kvm/sys_regs.c | 2 ++
> 4 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index b85f46a73e21..597470e0b87b 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -346,14 +346,14 @@
> */
> #define __HFGRTR_EL2_RES0 (GENMASK(63, 56) | GENMASK(53, 51))
> #define __HFGRTR_EL2_MASK GENMASK(49, 0)
> -#define __HFGRTR_EL2_nMASK (GENMASK(58, 57) | GENMASK(55, 54) | BIT(50))
> +#define __HFGRTR_EL2_nMASK (GENMASK(60, 57) | GENMASK(55, 54) | BIT(50))
>
> #define __HFGWTR_EL2_RES0 (GENMASK(63, 56) | GENMASK(53, 51) | \
> BIT(46) | BIT(42) | BIT(40) | BIT(28) | \
> GENMASK(26, 25) | BIT(21) | BIT(18) | \
> GENMASK(15, 14) | GENMASK(10, 9) | BIT(2))
> #define __HFGWTR_EL2_MASK GENMASK(49, 0)
> -#define __HFGWTR_EL2_nMASK (GENMASK(58, 57) | GENMASK(55, 54) | BIT(50))
> +#define __HFGWTR_EL2_nMASK (GENMASK(60, 57) | GENMASK(55, 54) | BIT(50))
>
> #define __HFGITR_EL2_RES0 GENMASK(63, 57)
> #define __HFGITR_EL2_MASK GENMASK(54, 0)
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 824f29f04916..fa9ebd8fce40 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -401,6 +401,10 @@ enum vcpu_sysreg {
> PIR_EL1, /* Permission Indirection Register 1 (EL1) */
> PIRE0_EL1, /* Permission Indirection Register 0 (EL1) */
>
> + /* Permission Overlay Extension registers */
> + POR_EL1, /* Permission Overlay Register 1 (EL1) */
> + POR_EL0, /* Permission Overlay Register 0 (EL0) */
> +
> /* 32bit specific registers. */
> DACR32_EL2, /* Domain Access Control Register */
> IFSR32_EL2, /* Instruction Fault Status Register */
> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index bb6b571ec627..22f07ee43e7e 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -19,6 +19,9 @@
> static inline void __sysreg_save_common_state(struct kvm_cpu_context *ctxt)
> {
> ctxt_sys_reg(ctxt, MDSCR_EL1) = read_sysreg(mdscr_el1);
> +
> + if (system_supports_poe())
> + ctxt_sys_reg(ctxt, POR_EL0) = read_sysreg_s(SYS_POR_EL0);
So this is saved as eagerly as it gets. Why? If it only affects EL0,
it can be saved/restored in a much lazier way.
> }
>
> static inline void __sysreg_save_user_state(struct kvm_cpu_context *ctxt)
> @@ -59,6 +62,8 @@ static inline void __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
> ctxt_sys_reg(ctxt, PIR_EL1) = read_sysreg_el1(SYS_PIR);
> ctxt_sys_reg(ctxt, PIRE0_EL1) = read_sysreg_el1(SYS_PIRE0);
And the fact that you only touch PIRE0_EL1 here seems to be a good
indication that the above can be relaxed.
> }
> + if (system_supports_poe())
nit: missing new line before the if().
> + ctxt_sys_reg(ctxt, POR_EL1) = read_sysreg_el1(SYS_POR);
> ctxt_sys_reg(ctxt, PAR_EL1) = read_sysreg_par();
> ctxt_sys_reg(ctxt, TPIDR_EL1) = read_sysreg(tpidr_el1);
>
> @@ -89,6 +94,9 @@ static inline void __sysreg_save_el2_return_state(struct kvm_cpu_context *ctxt)
> static inline void __sysreg_restore_common_state(struct kvm_cpu_context *ctxt)
> {
> write_sysreg(ctxt_sys_reg(ctxt, MDSCR_EL1), mdscr_el1);
> +
> + if (system_supports_poe())
> + write_sysreg_s(ctxt_sys_reg(ctxt, POR_EL0), SYS_POR_EL0);
Same thing here about the eager restore.
> }
>
> static inline void __sysreg_restore_user_state(struct kvm_cpu_context *ctxt)
> @@ -135,6 +143,8 @@ static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
> write_sysreg_el1(ctxt_sys_reg(ctxt, PIR_EL1), SYS_PIR);
> write_sysreg_el1(ctxt_sys_reg(ctxt, PIRE0_EL1), SYS_PIRE0);
> }
> + if (system_supports_poe())
new line.
> + write_sysreg_el1(ctxt_sys_reg(ctxt, POR_EL1), SYS_POR);
> write_sysreg(ctxt_sys_reg(ctxt, PAR_EL1), par_el1);
> write_sysreg(ctxt_sys_reg(ctxt, TPIDR_EL1), tpidr_el1);
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 4735e1b37fb3..a54e5eadbf29 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2269,6 +2269,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> { SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 },
> { SYS_DESC(SYS_PIRE0_EL1), NULL, reset_unknown, PIRE0_EL1 },
> { SYS_DESC(SYS_PIR_EL1), NULL, reset_unknown, PIR_EL1 },
> + { SYS_DESC(SYS_POR_EL1), NULL, reset_unknown, POR_EL1 },
> { SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 },
>
> { SYS_DESC(SYS_LORSA_EL1), trap_loregion },
> @@ -2352,6 +2353,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> .access = access_pmovs, .reg = PMOVSSET_EL0,
> .get_user = get_pmreg, .set_user = set_pmreg },
>
> + { SYS_DESC(SYS_POR_EL0), NULL, reset_unknown, POR_EL0 },
> { SYS_DESC(SYS_TPIDR_EL0), NULL, reset_unknown, TPIDR_EL0 },
> { SYS_DESC(SYS_TPIDRRO_EL0), NULL, reset_unknown, TPIDRRO_EL0 },
> { SYS_DESC(SYS_TPIDR2_EL0), undef_access },
Another thing that is missing is the trap routing for NV in
emulated-nested.c. Please fill in the various tables there.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
