At the moment the HCRX_EL2 system register is always initialized to
HCRX_GUEST_FLAGS when running a guest. Instead, choose the configuration
at vcpu reset time and save it in the vcpu struct, similarly to how
HCR_EL2 is set up. This will be needed in a subsequent change to
configure the register based on CPU features detected at runtime.
Signed-off-by: Kristina Martsenko <[email protected]>
---
arch/arm64/include/asm/kvm_emulate.h | 5 +++++
arch/arm64/include/asm/kvm_host.h | 1 +
arch/arm64/kvm/arm.c | 1 +
arch/arm64/kvm/hyp/include/hyp/switch.h | 2 +-
arch/arm64/kvm/hyp/nvhe/hyp-main.c | 1 +
arch/arm64/kvm/hyp/nvhe/pkvm.c | 1 +
6 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 3d6725ff0bf6..64ea27e6deb1 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -134,6 +134,11 @@ static inline void vcpu_ptrauth_disable(struct kvm_vcpu *vcpu)
vcpu->arch.hcr_el2 &= ~(HCR_API | HCR_APK);
}
+static inline void vcpu_reset_hcrx(struct kvm_vcpu *vcpu)
+{
+ vcpu->arch.hcrx_el2 = HCRX_GUEST_FLAGS;
+}
+
static inline unsigned long vcpu_get_vsesr(struct kvm_vcpu *vcpu)
{
return vcpu->arch.vsesr_el2;
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index af06ccb7ee34..2764748756a7 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -487,6 +487,7 @@ struct kvm_vcpu_arch {
/* Values of trap registers for the guest. */
u64 hcr_el2;
+ u64 hcrx_el2;
u64 mdcr_el2;
u64 cptr_el2;
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 4866b3f7b4ea..fa359fd6fca9 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1318,6 +1318,7 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
}
vcpu_reset_hcr(vcpu);
+ vcpu_reset_hcrx(vcpu);
vcpu->arch.cptr_el2 = kvm_get_reset_cptr_el2(vcpu);
/*
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index 9cfe6bd1dbe4..119b75344505 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -198,7 +198,7 @@ static inline void __activate_traps_common(struct kvm_vcpu *vcpu)
write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
if (cpus_have_final_cap(ARM64_HAS_HCX)) {
- u64 hcrx = HCRX_GUEST_FLAGS;
+ u64 hcrx = vcpu->arch.hcrx_el2;
if (vcpu_has_nv(vcpu) && !is_hyp_ctxt(vcpu)) {
u64 clr = 0, set = 0;
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index 857d9bc04fd4..26692083b80b 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -35,6 +35,7 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
hyp_vcpu->vcpu.arch.hw_mmu = host_vcpu->arch.hw_mmu;
hyp_vcpu->vcpu.arch.hcr_el2 = host_vcpu->arch.hcr_el2;
+ hyp_vcpu->vcpu.arch.hcrx_el2 = host_vcpu->arch.hcrx_el2;
hyp_vcpu->vcpu.arch.mdcr_el2 = host_vcpu->arch.mdcr_el2;
hyp_vcpu->vcpu.arch.cptr_el2 = host_vcpu->arch.cptr_el2;
diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
index 8033ef353a5d..8c8b37525dce 100644
--- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
+++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
@@ -188,6 +188,7 @@ static void pvm_init_trap_regs(struct kvm_vcpu *vcpu)
/* Clear res0 and set res1 bits to trap potential new features. */
vcpu->arch.hcr_el2 &= ~(HCR_RES0);
+ vcpu->arch.hcrx_el2 &= ~(HCRX_EL2_RES0);
vcpu->arch.mdcr_el2 &= ~(MDCR_EL2_RES0);
if (!has_hvhe()) {
vcpu->arch.cptr_el2 |= CPTR_NVHE_EL2_RES1;
--
2.25.1
Hi Kristina,
On Fri, 15 Sep 2023 13:48:38 +0100,
Kristina Martsenko <[email protected]> wrote:
>
> At the moment the HCRX_EL2 system register is always initialized to
> HCRX_GUEST_FLAGS when running a guest. Instead, choose the configuration
> at vcpu reset time and save it in the vcpu struct, similarly to how
> HCR_EL2 is set up. This will be needed in a subsequent change to
> configure the register based on CPU features detected at runtime.
>
> Signed-off-by: Kristina Martsenko <[email protected]>
> ---
> arch/arm64/include/asm/kvm_emulate.h | 5 +++++
> arch/arm64/include/asm/kvm_host.h | 1 +
> arch/arm64/kvm/arm.c | 1 +
> arch/arm64/kvm/hyp/include/hyp/switch.h | 2 +-
> arch/arm64/kvm/hyp/nvhe/hyp-main.c | 1 +
> arch/arm64/kvm/hyp/nvhe/pkvm.c | 1 +
> 6 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 3d6725ff0bf6..64ea27e6deb1 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -134,6 +134,11 @@ static inline void vcpu_ptrauth_disable(struct kvm_vcpu *vcpu)
> vcpu->arch.hcr_el2 &= ~(HCR_API | HCR_APK);
> }
>
> +static inline void vcpu_reset_hcrx(struct kvm_vcpu *vcpu)
> +{
> + vcpu->arch.hcrx_el2 = HCRX_GUEST_FLAGS;
> +}
> +
> static inline unsigned long vcpu_get_vsesr(struct kvm_vcpu *vcpu)
> {
> return vcpu->arch.vsesr_el2;
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index af06ccb7ee34..2764748756a7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -487,6 +487,7 @@ struct kvm_vcpu_arch {
>
> /* Values of trap registers for the guest. */
> u64 hcr_el2;
> + u64 hcrx_el2;
Do we really need this extra field? Yes, this is only an extra 64bit,
but they tend to accumulate...
Looking at patch #3, the change is related to this:
vcpu->arch.hcrx_el2 = HCRX_GUEST_FLAGS;
+
+ if (cpus_have_final_cap(ARM64_HAS_MOPS)) {
+ vcpu->arch.hcrx_el2 |= HCRX_EL2_MSCEn;
+ vcpu->arch.hcrx_el2 |= HCRX_EL2_MCE2;
+ }
meaning that this is a constant value for a given boot of the host.
At this stage, I'd rather you define HCRX_GUEST_FLAGS as:
#define HCRX_GUEST_FLAGS \
(HCRX_EL2_SMPME | HCRX_EL2_TCR2En | \
cpus_have_final_cap(ARM64_HAS_MOPS) ? \
(HCRX_EL2_MSCEn | HCRX_EL2_MCE2) : 0)
and drop the new field altogether, until we have something that
requires dynamic flipping of an HCRX_EL2 field.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On 18/09/2023 12:10, Marc Zyngier wrote:
> Hi Kristina,
Hi Marc,
> On Fri, 15 Sep 2023 13:48:38 +0100,
> Kristina Martsenko <[email protected]> wrote:
>>
>> At the moment the HCRX_EL2 system register is always initialized to
>> HCRX_GUEST_FLAGS when running a guest. Instead, choose the configuration
>> at vcpu reset time and save it in the vcpu struct, similarly to how
>> HCR_EL2 is set up. This will be needed in a subsequent change to
>> configure the register based on CPU features detected at runtime.
>>
>> Signed-off-by: Kristina Martsenko <[email protected]>
>> ---
>> arch/arm64/include/asm/kvm_emulate.h | 5 +++++
>> arch/arm64/include/asm/kvm_host.h | 1 +
>> arch/arm64/kvm/arm.c | 1 +
>> arch/arm64/kvm/hyp/include/hyp/switch.h | 2 +-
>> arch/arm64/kvm/hyp/nvhe/hyp-main.c | 1 +
>> arch/arm64/kvm/hyp/nvhe/pkvm.c | 1 +
>> 6 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
>> index 3d6725ff0bf6..64ea27e6deb1 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -134,6 +134,11 @@ static inline void vcpu_ptrauth_disable(struct kvm_vcpu *vcpu)
>> vcpu->arch.hcr_el2 &= ~(HCR_API | HCR_APK);
>> }
>>
>> +static inline void vcpu_reset_hcrx(struct kvm_vcpu *vcpu)
>> +{
>> + vcpu->arch.hcrx_el2 = HCRX_GUEST_FLAGS;
>> +}
>> +
>> static inline unsigned long vcpu_get_vsesr(struct kvm_vcpu *vcpu)
>> {
>> return vcpu->arch.vsesr_el2;
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index af06ccb7ee34..2764748756a7 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -487,6 +487,7 @@ struct kvm_vcpu_arch {
>>
>> /* Values of trap registers for the guest. */
>> u64 hcr_el2;
>> + u64 hcrx_el2;
>
> Do we really need this extra field? Yes, this is only an extra 64bit,
> but they tend to accumulate...
>
> Looking at patch #3, the change is related to this:
>
> vcpu->arch.hcrx_el2 = HCRX_GUEST_FLAGS;
> +
> + if (cpus_have_final_cap(ARM64_HAS_MOPS)) {
> + vcpu->arch.hcrx_el2 |= HCRX_EL2_MSCEn;
> + vcpu->arch.hcrx_el2 |= HCRX_EL2_MCE2;
> + }
>
> meaning that this is a constant value for a given boot of the host.
>
> At this stage, I'd rather you define HCRX_GUEST_FLAGS as:
>
> #define HCRX_GUEST_FLAGS \
> (HCRX_EL2_SMPME | HCRX_EL2_TCR2En | \
> cpus_have_final_cap(ARM64_HAS_MOPS) ? \
> (HCRX_EL2_MSCEn | HCRX_EL2_MCE2) : 0)
>
> and drop the new field altogether, until we have something that
> requires dynamic flipping of an HCRX_EL2 field.
Makes sense, the field isn't strictly required yet, I'll drop it in v2.
Thanks,
Kristina