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

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

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):

	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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel