| Message ID | 20231214100158.2305400-15-tabba@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Fixes to fine grain traps and pKVM traps | expand |
Hi, On Thu, Dec 14, 2023 at 10:02 AM Fuad Tabba <tabba@google.com> wrote: > > There's a lot of boilerplate code for setting and clearing FGT > bits when activating guest traps. Refactor it into macros. These > macros will also be used in future patch series. > > No functional change intended. > > Signed-off-by: Fuad Tabba <tabba@google.com> And in relation to my email in "PATCH v3 09/17" [*] on HAFGRTR_EL2 being present only when FEAT_AMUv1 is implemented and FEAT_FGT: diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index 58e450180f68..071d6db299f5 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -174,7 +174,9 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) update_fgt_traps(HFGITR_EL2); update_fgt_traps(HDFGRTR_EL2); update_fgt_traps(HDFGWTR_EL2); - update_fgt_traps(HAFGRTR_EL2); + + if (cpu_has_amu()) + update_fgt_traps(HAFGRTR_EL2); } static inline void __deactivate_traps_hfgxtr(struct kvm_vcpu *vcpu) * https://lore.kernel.org/all/CA+EHjTwrcXjOdo7JG-6fr0YpDq2EtMoTQxngq5H-ELLdjyQG=A@mail.gmail.com/ Cheers, /fuad > --- > arch/arm64/kvm/hyp/include/hyp/switch.h | 60 +++++++++---------------- > 1 file changed, 21 insertions(+), 39 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > index 17ce40f5b006..e223fc0d5193 100644 > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > @@ -79,6 +79,23 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) > clr |= ~hfg & __ ## reg ## _nMASK; \ > } while(0) > > +#define update_fgt_traps_cs(reg, clr, set) \ > + do { \ > + struct kvm_cpu_context *hctxt = \ > + &this_cpu_ptr(&kvm_host_data)->host_ctxt; \ > + u64 val, c = 0, s = 0; \ > + \ > + ctxt_sys_reg(hctxt, reg) = read_sysreg_s(SYS_ ## reg); \ > + compute_clr_set(vcpu, reg, c, s); \ > + val = __ ## reg ## _nMASK; \ > + val |= (s | set); \ > + val &= ~(c | clr); \ > + write_sysreg_s(val, SYS_ ## reg); \ > + } while(0) > + > +#define update_fgt_traps(reg) \ > + update_fgt_traps_cs(reg, 0, 0) > + > /* > * Validate the fine grain trap masks. > * Check that the masks do not overlap and that all bits are accounted for. > @@ -146,45 +163,10 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) > if (!vcpu_has_nv(vcpu) || is_hyp_ctxt(vcpu)) > return; > > - ctxt_sys_reg(hctxt, HFGITR_EL2) = read_sysreg_s(SYS_HFGITR_EL2); > - > - r_set = r_clr = 0; > - compute_clr_set(vcpu, HFGITR_EL2, r_clr, r_set); > - r_val = __HFGITR_EL2_nMASK; > - r_val |= r_set; > - r_val &= ~r_clr; > - > - write_sysreg_s(r_val, SYS_HFGITR_EL2); > - > - ctxt_sys_reg(hctxt, HDFGRTR_EL2) = read_sysreg_s(SYS_HDFGRTR_EL2); > - ctxt_sys_reg(hctxt, HDFGWTR_EL2) = read_sysreg_s(SYS_HDFGWTR_EL2); > - > - r_clr = r_set = w_clr = w_set = 0; > - > - compute_clr_set(vcpu, HDFGRTR_EL2, r_clr, r_set); > - compute_clr_set(vcpu, HDFGWTR_EL2, w_clr, w_set); > - > - r_val = __HDFGRTR_EL2_nMASK; > - r_val |= r_set; > - r_val &= ~r_clr; > - > - w_val = __HDFGWTR_EL2_nMASK; > - w_val |= w_set; > - w_val &= ~w_clr; > - > - write_sysreg_s(r_val, SYS_HDFGRTR_EL2); > - write_sysreg_s(w_val, SYS_HDFGWTR_EL2); > - > - ctxt_sys_reg(hctxt, HAFGRTR_EL2) = read_sysreg_s(SYS_HAFGRTR_EL2); > - > - r_clr = r_set = 0; > - compute_clr_set(vcpu, HAFGRTR_EL2, r_clr, r_set); > - > - r_val = __HAFGRTR_EL2_nMASK; > - r_val |= r_set; > - r_val &= ~r_clr; > - > - write_sysreg_s(r_val, SYS_HAFGRTR_EL2); > + update_fgt_traps(HFGITR_EL2); > + update_fgt_traps(HDFGRTR_EL2); > + update_fgt_traps(HDFGWTR_EL2); > + update_fgt_traps(HAFGRTR_EL2); > } > > static inline void __deactivate_traps_hfgxtr(struct kvm_vcpu *vcpu) > -- > 2.43.0.472.g3155946c3a-goog >
On Thu, 14 Dec 2023 10:01:54 +0000, Fuad Tabba <tabba@google.com> wrote: > > There's a lot of boilerplate code for setting and clearing FGT > bits when activating guest traps. Refactor it into macros. These > macros will also be used in future patch series. > > No functional change intended. > > Signed-off-by: Fuad Tabba <tabba@google.com> > --- > arch/arm64/kvm/hyp/include/hyp/switch.h | 60 +++++++++---------------- > 1 file changed, 21 insertions(+), 39 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > index 17ce40f5b006..e223fc0d5193 100644 > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > @@ -79,6 +79,23 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) > clr |= ~hfg & __ ## reg ## _nMASK; \ > } while(0) > > +#define update_fgt_traps_cs(reg, clr, set) \ > + do { \ > + struct kvm_cpu_context *hctxt = \ > + &this_cpu_ptr(&kvm_host_data)->host_ctxt; \ > + u64 val, c = 0, s = 0; \ > + \ > + ctxt_sys_reg(hctxt, reg) = read_sysreg_s(SYS_ ## reg); \ > + compute_clr_set(vcpu, reg, c, s); \ You are referring to a variable name that is in the scope of the macro user, and not the macro itself. It is so fragile it isn't funny. Why don't you simply pass the vcpu as a parameter to the function? Another thing is that this read/write can be expensive. How about not doing anything when there is no change to the value of the sysreg? I'll see if I can come up with something. M.
Hi Marc, On Mon, Dec 18, 2023 at 9:40 AM Marc Zyngier <maz@kernel.org> wrote: > > On Thu, 14 Dec 2023 10:01:54 +0000, > Fuad Tabba <tabba@google.com> wrote: > > > > There's a lot of boilerplate code for setting and clearing FGT > > bits when activating guest traps. Refactor it into macros. These > > macros will also be used in future patch series. > > > > No functional change intended. > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > --- > > arch/arm64/kvm/hyp/include/hyp/switch.h | 60 +++++++++---------------- > > 1 file changed, 21 insertions(+), 39 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > > index 17ce40f5b006..e223fc0d5193 100644 > > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > > @@ -79,6 +79,23 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) > > clr |= ~hfg & __ ## reg ## _nMASK; \ > > } while(0) > > > > +#define update_fgt_traps_cs(reg, clr, set) \ > > + do { \ > > + struct kvm_cpu_context *hctxt = \ > > + &this_cpu_ptr(&kvm_host_data)->host_ctxt; \ > > + u64 val, c = 0, s = 0; \ > > + \ > > + ctxt_sys_reg(hctxt, reg) = read_sysreg_s(SYS_ ## reg); \ > > + compute_clr_set(vcpu, reg, c, s); \ > > You are referring to a variable name that is in the scope of the macro > user, and not the macro itself. It is so fragile it isn't funny. > > Why don't you simply pass the vcpu as a parameter to the function? > > Another thing is that this read/write can be expensive. How about not > doing anything when there is no change to the value of the sysreg? What do you think of this (spacing will be fixed): #define update_fgt_traps_cs(vcpu, reg, clr, set) \ do { \ struct kvm_cpu_context *hctxt = \ &this_cpu_ptr(&kvm_host_data)->host_ctxt; \ u64 val, c = 0, s = 0; \ \ ctxt_sys_reg(hctxt, reg) = read_sysreg_s(SYS_ ## reg); \ compute_clr_set(vcpu, reg, c, s); \ val = __ ## reg ## _nMASK; \ val |= (s | set); \ val &= ~(c | clr); \ if (ctxt_sys_reg(hctxt, reg) != val) \ write_sysreg_s(val, SYS_ ## reg); \ } while(0) If it looks good to you, I'll fix it on the respin. Thanks, /fuad > > I'll see if I can come up with something. > > M. > > -- > Without deviation from the norm, progress is not possible.
On Mon, 18 Dec 2023 09:56:32 +0000, Fuad Tabba <tabba@google.com> wrote: > > Hi Marc, > > On Mon, Dec 18, 2023 at 9:40 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Thu, 14 Dec 2023 10:01:54 +0000, > > Fuad Tabba <tabba@google.com> wrote: > > > > > > There's a lot of boilerplate code for setting and clearing FGT > > > bits when activating guest traps. Refactor it into macros. These > > > macros will also be used in future patch series. > > > > > > No functional change intended. > > > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > > --- > > > arch/arm64/kvm/hyp/include/hyp/switch.h | 60 +++++++++---------------- > > > 1 file changed, 21 insertions(+), 39 deletions(-) > > > > > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > > > index 17ce40f5b006..e223fc0d5193 100644 > > > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > > > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > > > @@ -79,6 +79,23 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) > > > clr |= ~hfg & __ ## reg ## _nMASK; \ > > > } while(0) > > > > > > +#define update_fgt_traps_cs(reg, clr, set) \ > > > + do { \ > > > + struct kvm_cpu_context *hctxt = \ > > > + &this_cpu_ptr(&kvm_host_data)->host_ctxt; \ > > > + u64 val, c = 0, s = 0; \ > > > + \ > > > + ctxt_sys_reg(hctxt, reg) = read_sysreg_s(SYS_ ## reg); \ > > > + compute_clr_set(vcpu, reg, c, s); \ > > > > You are referring to a variable name that is in the scope of the macro > > user, and not the macro itself. It is so fragile it isn't funny. > > > > Why don't you simply pass the vcpu as a parameter to the function? > > > > Another thing is that this read/write can be expensive. How about not > > doing anything when there is no change to the value of the sysreg? > > What do you think of this (spacing will be fixed): > > #define update_fgt_traps_cs(vcpu, reg, clr, set) \ > do { > \ > struct kvm_cpu_context *hctxt = > \ > &this_cpu_ptr(&kvm_host_data)->host_ctxt; \ > u64 val, c = 0, s = 0; > \ > > \ > ctxt_sys_reg(hctxt, reg) = read_sysreg_s(SYS_ ## reg); \ > compute_clr_set(vcpu, reg, c, s); > \ > val = __ ## reg ## _nMASK; > \ > val |= (s | set); > \ > val &= ~(c | clr); > \ > if (ctxt_sys_reg(hctxt, reg) != val) > \ > write_sysreg_s(val, SYS_ ## reg); > \ > } while(0) > > If it looks good to you, I'll fix it on the respin. This is what I have on top at the moment: diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index 27077e1f7de2..d56fef44dc31 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -79,22 +79,26 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) clr |= ~hfg & __ ## reg ## _nMASK; \ } while(0) -#define update_fgt_traps_cs(reg, clr, set) \ +#define update_fgt_traps_cs(vcpu, reg, clr, set) \ do { \ struct kvm_cpu_context *hctxt = \ &this_cpu_ptr(&kvm_host_data)->host_ctxt; \ - u64 val, c = 0, s = 0; \ + u64 c = 0, s = 0; \ \ ctxt_sys_reg(hctxt, reg) = read_sysreg_s(SYS_ ## reg); \ compute_clr_set(vcpu, reg, c, s); \ - val = __ ## reg ## _nMASK; \ - val |= (s | set); \ - val &= ~(c | clr); \ - write_sysreg_s(val, SYS_ ## reg); \ + s |= set; \ + c |= clr; \ + if (c || s) { \ + u64 val = __ ## reg ## _nMASK; \ + val |= s; \ + val &= ~c; \ + write_sysreg_s(val, SYS_ ## reg); \ + } \ } while(0) -#define update_fgt_traps(reg) \ - update_fgt_traps_cs(reg, 0, 0) +#define update_fgt_traps(vcpu, reg) \ + update_fgt_traps_cs(vcpu, reg, 0, 0) /* * Validate the fine grain trap masks. @@ -171,9 +175,9 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) if (!vcpu_has_nv(vcpu) || is_hyp_ctxt(vcpu)) return; - update_fgt_traps(HFGITR_EL2); - update_fgt_traps(HDFGRTR_EL2); - update_fgt_traps(HDFGWTR_EL2); + update_fgt_traps(vcpu, HFGITR_EL2); + update_fgt_traps(vcpu, HDFGRTR_EL2); + update_fgt_traps(vcpu, HDFGWTR_EL2); if (cpu_has_amu()) update_fgt_traps(vcpu, HAFGRTR_EL2); If that seems sensible to you, I'll fold it in, as I'd like to do without a respin if we can avoid it. ` M.
Hi Marc, On Mon, Dec 18, 2023 at 11:12 AM Marc Zyngier <maz@kernel.org> wrote: ... > if (cpu_has_amu()) > update_fgt_traps(vcpu, HAFGRTR_EL2); > > If that seems sensible to you, I'll fold it in, as I'd like to do > without a respin if we can avoid it. Looks good to me. Thanks Marc! /fuad > > ` M. > > -- > Without deviation from the norm, progress is not possible.
On Mon, 18 Dec 2023 11:17:55 +0000, Fuad Tabba <tabba@google.com> wrote: > > Hi Marc, > > On Mon, Dec 18, 2023 at 11:12 AM Marc Zyngier <maz@kernel.org> wrote: > ... > > if (cpu_has_amu()) > > update_fgt_traps(vcpu, HAFGRTR_EL2); > > > > If that seems sensible to you, I'll fold it in, as I'd like to do > > without a respin if we can avoid it. > > Looks good to me. Thanks Marc! Please have a look at [1] to see what I have done (AMU fixes and my suggestion for the FGT macros), and let me know if that's OK with you. I've given it a go on my M2 with NV (only a couple of minor conflicts) and nothing caught fire. Thanks, M. [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/fgt-rework
Hi Marc, On Mon, Dec 18, 2023 at 12:25 PM Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 18 Dec 2023 11:17:55 +0000, > Fuad Tabba <tabba@google.com> wrote: > > > > Hi Marc, > > > > On Mon, Dec 18, 2023 at 11:12 AM Marc Zyngier <maz@kernel.org> wrote: > > ... > > > if (cpu_has_amu()) > > > update_fgt_traps(vcpu, HAFGRTR_EL2); > > > > > > If that seems sensible to you, I'll fold it in, as I'd like to do > > > without a respin if we can avoid it. > > > > Looks good to me. Thanks Marc! > > Please have a look at [1] to see what I have done (AMU fixes and my > suggestion for the FGT macros), and let me know if that's OK with you. > > I've given it a go on my M2 with NV (only a couple of minor conflicts) > and nothing caught fire. Both the AMU fixes and the FGT macros look good to me. Thanks for this! Cheers, /fuad > > Thanks, > > M. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/fgt-rework > > -- > Without deviation from the norm, progress is not possible.
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index 17ce40f5b006..e223fc0d5193 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -79,6 +79,23 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) clr |= ~hfg & __ ## reg ## _nMASK; \ } while(0) +#define update_fgt_traps_cs(reg, clr, set) \ + do { \ + struct kvm_cpu_context *hctxt = \ + &this_cpu_ptr(&kvm_host_data)->host_ctxt; \ + u64 val, c = 0, s = 0; \ + \ + ctxt_sys_reg(hctxt, reg) = read_sysreg_s(SYS_ ## reg); \ + compute_clr_set(vcpu, reg, c, s); \ + val = __ ## reg ## _nMASK; \ + val |= (s | set); \ + val &= ~(c | clr); \ + write_sysreg_s(val, SYS_ ## reg); \ + } while(0) + +#define update_fgt_traps(reg) \ + update_fgt_traps_cs(reg, 0, 0) + /* * Validate the fine grain trap masks. * Check that the masks do not overlap and that all bits are accounted for. @@ -146,45 +163,10 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) if (!vcpu_has_nv(vcpu) || is_hyp_ctxt(vcpu)) return; - ctxt_sys_reg(hctxt, HFGITR_EL2) = read_sysreg_s(SYS_HFGITR_EL2); - - r_set = r_clr = 0; - compute_clr_set(vcpu, HFGITR_EL2, r_clr, r_set); - r_val = __HFGITR_EL2_nMASK; - r_val |= r_set; - r_val &= ~r_clr; - - write_sysreg_s(r_val, SYS_HFGITR_EL2); - - ctxt_sys_reg(hctxt, HDFGRTR_EL2) = read_sysreg_s(SYS_HDFGRTR_EL2); - ctxt_sys_reg(hctxt, HDFGWTR_EL2) = read_sysreg_s(SYS_HDFGWTR_EL2); - - r_clr = r_set = w_clr = w_set = 0; - - compute_clr_set(vcpu, HDFGRTR_EL2, r_clr, r_set); - compute_clr_set(vcpu, HDFGWTR_EL2, w_clr, w_set); - - r_val = __HDFGRTR_EL2_nMASK; - r_val |= r_set; - r_val &= ~r_clr; - - w_val = __HDFGWTR_EL2_nMASK; - w_val |= w_set; - w_val &= ~w_clr; - - write_sysreg_s(r_val, SYS_HDFGRTR_EL2); - write_sysreg_s(w_val, SYS_HDFGWTR_EL2); - - ctxt_sys_reg(hctxt, HAFGRTR_EL2) = read_sysreg_s(SYS_HAFGRTR_EL2); - - r_clr = r_set = 0; - compute_clr_set(vcpu, HAFGRTR_EL2, r_clr, r_set); - - r_val = __HAFGRTR_EL2_nMASK; - r_val |= r_set; - r_val &= ~r_clr; - - write_sysreg_s(r_val, SYS_HAFGRTR_EL2); + update_fgt_traps(HFGITR_EL2); + update_fgt_traps(HDFGRTR_EL2); + update_fgt_traps(HDFGWTR_EL2); + update_fgt_traps(HAFGRTR_EL2); } static inline void __deactivate_traps_hfgxtr(struct kvm_vcpu *vcpu)
There's a lot of boilerplate code for setting and clearing FGT bits when activating guest traps. Refactor it into macros. These macros will also be used in future patch series. No functional change intended. Signed-off-by: Fuad Tabba <tabba@google.com> --- arch/arm64/kvm/hyp/include/hyp/switch.h | 60 +++++++++---------------- 1 file changed, 21 insertions(+), 39 deletions(-)