[PATCH v2 06/12] KVM: arm64: Update and fix FGT register masks
Fuad Tabba
tabba at google.com
Thu Dec 7 07:06:44 PST 2023
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.
What do you think?
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
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
More information about the linux-arm-kernel
mailing list