[PATCH v2 06/12] KVM: arm64: Update and fix FGT register masks
Fuad Tabba
tabba at google.com
Fri Dec 8 00:23:05 PST 2023
Hi Joey,
On Thu, Dec 7, 2023 at 5:12 PM Joey Gouly <joey.gouly at arm.com> wrote:
>
> 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 at 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 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 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.
I don't have a strong opinion about that, but I am inclined to agree
with you. Let's also hear what Marc Zygier (since he's the author of
that part) opinion is, and I'm happy either way.
Cheers,
/fuad
> 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
> > > >
> > > >
More information about the linux-arm-kernel
mailing list