[PATCH v1 1/6] KVM: arm64: Update and fix FGT register masks

[Fuad Tabba]

Hi Marc,

On Tue, Dec 5, 2023 at 11:20 AM Marc Zyngier <maz at kernel.org> wrote:
>
> On Tue, 05 Dec 2023 10:22:43 +0000,
> Fuad Tabba <tabba at google.com> wrote:
> >
> > New trap bits have been defined in the latest Arm Architecture
> > System Registers xml specification [*]. Moreover, the existing
> > definitions of some of the mask and the res0 bits overlap, which
>
> nit: s/res0/RES0/

Ack

> > could be wrong, confusing, or potentially both.
> >
> > Update the bits to represent the latest spec (as of this patch),
> > and ensure that the existing bits are consistent.
> >
> > [*] https://developer.arm.com/downloads/-/exploration-tools
>
> Please indicate which version of the XML you are referring to (it
> changes on average every 3 months).

Ack

> >
> > Fixes: 0fd76865006d ("KVM: arm64: Add nPIR{E0}_EL1 to HFG traps")
> > Signed-off-by: Fuad Tabba <tabba at google.com>
> > ---
> >  arch/arm64/include/asm/kvm_arm.h | 39 ++++++++++++++++++++------------
> >  1 file changed, 24 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > index b85f46a73e21..b1061647e837 100644
> > --- a/arch/arm64/include/asm/kvm_arm.h
> > +++ b/arch/arm64/include/asm/kvm_arm.h
> > @@ -344,36 +344,45 @@
> >   * 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_RES0    BIT(51)
>
> If we are now using the current architectural definition for RES0, can
> we please move over to the generated values, as indicated in the above
> comment?

I'd missed the generated definitions. I thought that the comment was
referring to the read and write version of the registers. Will fix
that.

> >  #define __HFGRTR_EL2_MASK    GENMASK(49, 0)
> > -#define __HFGRTR_EL2_nMASK   (GENMASK(58, 57) | GENMASK(55, 54) | BIT(50))
> > +#define __HFGRTR_EL2_nMASK   (GENMASK(63, 52) | 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) |  \
> > +#define __HFGWTR_EL2_RES0    (BIT(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_MASK    (GENMASK(49, 47) | GENMASK(45, 43) | \
> > +                              BIT(41) | GENMASK(39, 29) | BIT(27) | \
> > +                              GENMASK(24, 22) | GENMASK(20, 19) | \
> > +                              GENMASK(17, 16) | GENMASK(13, 11) | \
> > +                              GENMASK(8, 3) | GENMASK(1, 0))
> > +#define __HFGWTR_EL2_nMASK   (GENMASK(63, 52) | BIT(50))
> >
> > -#define __HFGITR_EL2_RES0    GENMASK(63, 57)
> > -#define __HFGITR_EL2_MASK    GENMASK(54, 0)
> > -#define __HFGITR_EL2_nMASK   GENMASK(56, 55)
> > +#define __HFGITR_EL2_RES0    (BIT(63) | BIT(61))
> > +#define __HFGITR_EL2_MASK    (BIT(62) | BIT(60) | GENMASK(54, 0))
> > +#define __HFGITR_EL2_nMASK   GENMASK(59, 55)
> >
> >  #define __HDFGRTR_EL2_RES0   (BIT(49) | BIT(42) | GENMASK(39, 38) |  \
> >                                GENMASK(21, 20) | BIT(8))
> > -#define __HDFGRTR_EL2_MASK   ~__HDFGRTR_EL2_nMASK
> > +#define __HDFGRTR_EL2_MASK   (BIT(63) | GENMASK(58, 50) | GENMASK(48, 43) | \
> > +                              GENMASK(41, 40) | GENMASK(37, 22) | \
> > +                              GENMASK(19, 9) | GENMASK(7, 0))
> >  #define __HDFGRTR_EL2_nMASK  GENMASK(62, 59)
> >
> >  #define __HDFGWTR_EL2_RES0   (BIT(63) | GENMASK(59, 58) | BIT(51) | BIT(47) | \
> >                                BIT(43) | GENMASK(40, 38) | BIT(34) | BIT(30) | \
> >                                BIT(22) | BIT(9) | BIT(6))
> > -#define __HDFGWTR_EL2_MASK   ~__HDFGWTR_EL2_nMASK
> > +#define __HDFGWTR_EL2_MASK   (GENMASK(57, 52) | GENMASK(50, 48) | \
> > +                              GENMASK(46, 44) | GENMASK(42, 41) | \
> > +                              GENMASK(37, 35) | GENMASK(33, 31) | \
> > +                              GENMASK(29, 23) | GENMASK(21, 10) | \
> > +                              GENMASK(8, 7) | GENMASK(5, 0))
> >  #define __HDFGWTR_EL2_nMASK  GENMASK(62, 60)
> >
> >  /* Similar definitions for HCRX_EL2 */
> > -#define __HCRX_EL2_RES0              (GENMASK(63, 16) | GENMASK(13, 12))
> > -#define __HCRX_EL2_MASK              (0)
> > -#define __HCRX_EL2_nMASK     (GENMASK(15, 14) | GENMASK(4, 0))
> > +#define __HCRX_EL2_RES0         (GENMASK(63, 25) | GENMASK(13, 12))
> > +#define __HCRX_EL2_MASK              (BIT(6))
> > +#define __HCRX_EL2_nMASK     (GENMASK(24, 14) | GENMASK(11, 7) | GENMASK(5, 0))
> >
> >  /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
> >  #define HPFAR_MASK   (~UL(0xf))
>
> The main driver for not following the latest and greatest version
> published in the architecture (including the FGT2 stuff) was to limit
> the amount of work we need to do in NV.
>
> Can you please update the FGT tables in emulated-nested.c so that we
> have a matching description of the added bits? This also overlaps with
> the work Joey is doing on POE, so there is scope for collaboration
> between you two here.

Will do.

Cheers,
/fuad

> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.