[PATCH v3 14/17] KVM: arm64: Macros for setting/clearing FGT bits

[Fuad Tabba]

Hi Marc,

On Mon, Dec 18, 2023 at 9:40 AM Marc Zyngier <maz at kernel.org> wrote:
>
> On Thu, 14 Dec 2023 10:01:54 +0000,
> Fuad Tabba <tabba at 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 at 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.