Hi Marc,
On 7/26/23 09:23, Marc Zyngier wrote:
> On Tue, 25 Jul 2023 17:39:52 +0100,
> Eric Auger <eric.auger@xxxxxxxxxx> wrote:
>> Hi Marc,
>>
>> On 7/12/23 16:57, Marc Zyngier wrote:
>>> As we're about to majorly extend the handling of FGT registers,
>>> restructure the code to actually save/restore the registers
>>> as required. This is made easy thanks to the previous addition
>>> of the EL2 registers, allowing us to use the host context for
>>> this purpose.
>>>
>>> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
>>> ---
>>> arch/arm64/include/asm/kvm_arm.h | 21 ++++++++++
>>> arch/arm64/kvm/hyp/include/hyp/switch.h | 55 +++++++++++++------------
>>> 2 files changed, 49 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>>> index 028049b147df..85908aa18908 100644
>>> --- a/arch/arm64/include/asm/kvm_arm.h
>>> +++ b/arch/arm64/include/asm/kvm_arm.h
>>> @@ -333,6 +333,27 @@
>>> BIT(18) | \
>>> GENMASK(16, 15))
>>>
>>> +/*
>>> + * FGT register definitions
>>> + *
>>> + * RES0 and polarity masks as of DDI0487J.a, to be updated as needed.
>>> + * We're not using the generated masks as they are usually ahead of
>>> + * the published ARM ARM, which we use as a reference.
>>> + *
>>> + * Once we get to a point where the two describe the same thing, we'll
>>> + * merge the definitions. One day.
>>> + */
>>> +#define __HFGRTR_EL2_RES0 (GENMASK(63, 56) | GENMASK(53, 51))
>>> +#define __HFGRTR_EL2_MASK GENMASK(49, 0)
>>> +#define __HFGRTR_EL2_nMASK (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(55, 54) | BIT(50))
>>> +
>>> /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>>> #define HPFAR_MASK (~UL(0xf))
>>> /*
>>> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
>>> index 4bddb8541bec..9781e79a5127 100644
>>> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
>>> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
>>> @@ -70,20 +70,19 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
>>> }
>>> }
>>>
>>> -static inline bool __hfgxtr_traps_required(void)
>>> -{
>>> - if (cpus_have_final_cap(ARM64_SME))
>>> - return true;
>>> -
>>> - if (cpus_have_final_cap(ARM64_WORKAROUND_AMPERE_AC03_CPU_38))
>>> - return true;
>>>
>>> - return false;
>>> -}
>>>
>>> -static inline void __activate_traps_hfgxtr(void)
>>> +static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu)
>>> {
>>> + struct kvm_cpu_context *hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt;
>>> u64 r_clr = 0, w_clr = 0, r_set = 0, w_set = 0, tmp;
>>> + u64 r_val, w_val;
>>> +
>>> + if (!cpus_have_final_cap(ARM64_HAS_FGT))
>>> + return;
>>> +
>>> + ctxt_sys_reg(hctxt, HFGRTR_EL2) = read_sysreg_s(SYS_HFGRTR_EL2);
>>> + ctxt_sys_reg(hctxt, HFGWTR_EL2) = read_sysreg_s(SYS_HFGWTR_EL2);
>>>
>>> if (cpus_have_final_cap(ARM64_SME)) {
>>> tmp = HFGxTR_EL2_nSMPRI_EL1_MASK | HFGxTR_EL2_nTPIDR2_EL0_MASK;
>>> @@ -98,26 +97,30 @@ static inline void __activate_traps_hfgxtr(void)
>>> if (cpus_have_final_cap(ARM64_WORKAROUND_AMPERE_AC03_CPU_38))
>>> w_set |= HFGxTR_EL2_TCR_EL1_MASK;
>>>
>>> - sysreg_clear_set_s(SYS_HFGRTR_EL2, r_clr, r_set);
>>> - sysreg_clear_set_s(SYS_HFGWTR_EL2, w_clr, w_set);
>>> +
>>> + r_val = __HFGRTR_EL2_nMASK & ~HFGxTR_EL2_nACCDATA_EL1;
>> I don't get why you do
>>
>> & ~HFGxTR_EL2_nACCDATA_EL1 as this latter also has a negative polarity.
>>
>> Please could you explain what is special with this bit/add a comment?
> Nothing is really special with this bit.
>
> But it is currently always cleared (we blindly write a big fat zero),
> and I wanted to explicitly show all the instructions for which we
> enable trapping for (ACCDATA_EL1 being the only one that is currently
> documented in the ARM ARM, although there are more already).
>
> So the construct I came up with is the above, initialising the
> register value with the nMASK bits (i.e. not trapping the
> corresponding instructions), and then clearing the bit for the stuff
> we want to trap. Maybe adding something like:
>
> /* Default to no trapping anything but ACCDATA_EL1 */
>
> would help?
thank you for the explanation. Yes it does.
Eric
>
> Thanks,
>
> M.
>
