Re: [PATCH v2 06/12] KVM: arm64: Update and fix FGT register masks

[Joey Gouly <joey.gouly@arm.com>]

On Thu, Dec 07, 2023 at 03:06:44PM +0000, Fuad Tabba wrote:
> Hi Joey,
> 
> On Thu, Dec 7, 2023 at 3:00 PM Joey Gouly <joey.gouly@arm.com> wrote:
> >
> > Hello Fuad,
> >
> > On Wed, Dec 06, 2023 at 10:04:56AM +0000, Fuad Tabba wrote:
> > > New trap bits have been defined in the 2023-09 Arm Architecture
> > > System Registers xml specification [*]. Moreover, the existing
> > > definitions of some of the mask and the RES0 bits overlap, which
> > > could be wrong, confusing, or both.
> > >
> > > Update the bits to represent the latest spec (as of this patch,
> > > 2023-09), and ensure that the existing bits are consistent.
> > >
> > > Subsequent patches will use the generated RES0 fields instead of
> > > specifying them manually. This patch keeps the manual encoding of
> > > the bits to make it easier to review the series.
> > >
> > > [*] https://developer.arm.com/downloads/-/exploration-tools
> > >
> > > Fixes: 0fd76865006d ("KVM: arm64: Add nPIR{E0}_EL1 to HFG traps")
> > > Signed-off-by: Fuad Tabba <tabba@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 7de0a7062625..b0dc3249d5cd 100644
> > > --- a/arch/arm64/include/asm/kvm_arm.h
> > > +++ b/arch/arm64/include/asm/kvm_arm.h
> > > @@ -344,30 +344,39 @@
> > >   * 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)
> > >  #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))
> >
> > By adding all these bits to *_nMASK, we're allowing a guest to access registers
> > which KVM doesn't (currently) deal with.  For example if I apply this patch
> > series, a guest can access S2POR_EL1, previously it would print something like:
> >
> >         kvm [80]: Unsupported guest sys_reg access at: ffffc42969c1f270 [600000c5]
> >          { Op0( 3), Op1( 0), CRn(10), CRm( 2), Op2( 5), func_read },
> >
> > After applying this patch series, the guest can read S2POR_EL1.
> >
> > We don't expose S2POE to the guest through ID_AA64MMFR3_EL1, so a well behaved
> > guest shouldn't access it, but there's nothing stopping it.
> >
> > My question is, is this intended? Or do we need to update the following code
> > (and comment!) to trap all the stuff we don't currently handle (along with
> > ACCDATA_EL1):
> 
> Thanks for pointing this out, and no, it's not intended. For
> consistency, I think it might be good to update the code, as you
> suggest, to prevent these accesses. If you and the other reviewers are
> happy with that solution I can respin with your suggested fix. I have
> a couple of other minor fixes as well for the series.
> 

Thanks for clarifying that.

To me it seems weird to use the generated bits directly, and I think that's why
the code seems confusing?

I think we should stop using __HFGRTR_EL2_nMASK directly in that function, and
build it up from '0'. This also matches, I think, with Marc Z wanting to move
towards setting these things based on features [1], so maybe something like:

	u64 r_val = 0;

	if (cpus_have_final_cap(ARM64_HAS_S1PIE))
		r_set |= HFGxTR_EL2_nPIR_EL1 | HFGxTR_EL2_nPIREO_EL1;

	r_val |= r_set;
	r_val &= ~r_clr;

	write_sysreg_s(r_val, SYS_HFGRTR_EL2);

That way we just explicitly set which bits we don't want to trap on. We have
some weird behaviour right now were we set nSMPRI_EL1 to 1, unless you have
FEAT_SME where we set it to 0.. but if you don't have FEAT_SME it's RES0
anyway? The above approach would fix that.

Thanks,
Joey

[1] https://lore.kernel.org/linux-arm-kernel/87bkb285ud.wl-maz@kernel.org/ (last paragraph)

> 
> Cheers,
> /fuad
> 
> 
> >
> >         static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu)
> >         {
> >
> >                 ..
> >
> >                 /* The default is not to trap anything but ACCDATA_EL1 */
> >                 r_val = __HFGRTR_EL2_nMASK & ~HFGxTR_EL2_nACCDATA_EL1;
> >                 r_val |= r_set;
> >                 r_val &= ~r_clr;
> >
> >
> > Thanks,
> > Joey
> >
> > >
> > > -#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)
> > >
> > >  #define __HAFGRTR_EL2_RES0   (GENMASK(63, 50) | GENMASK(16, 5))
> > > @@ -375,9 +384,9 @@
> > >  #define __HAFGRTR_EL2_nMASK  0UL
> > >
> > >  /* 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))
> > > --
> > > 2.43.0.rc2.451.g8631bc7472-goog
> > >
> > >