Re: [PATCH 13/20] KVM: arm64: Move RESx into individual register descriptors

[Marc Zyngier <maz@kernel.org>]

On Thu, 29 Jan 2026 16:29:39 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> Hi Marc,
> 
> On Mon, 26 Jan 2026 at 12:17, Marc Zyngier <maz@kernel.org> wrote:
> >
> > Instead of hacking the RES1 bits at runtime, move them into the
> > register descriptors. This makes it significantly nicer.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/config.c | 36 +++++++++++++++++++++++++++++-------
> >  1 file changed, 29 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
> > index 7063fffc22799..d5871758f1fcc 100644
> > --- a/arch/arm64/kvm/config.c
> > +++ b/arch/arm64/kvm/config.c
> > @@ -30,6 +30,7 @@ struct reg_bits_to_feat_map {
> >  #define        RES0_WHEN_E2H1  BIT(7)  /* RES0 when E2H=1 and not supported */
> >  #define        RES1_WHEN_E2H0  BIT(8)  /* RES1 when E2H=0 and not supported */
> >  #define        RES1_WHEN_E2H1  BIT(9)  /* RES1 when E2H=1 and not supported */
> > +#define        FORCE_RESx      BIT(10) /* Unconditional RESx */
> >
> >         unsigned long   flags;
> >
> > @@ -107,6 +108,11 @@ struct reg_feat_map_desc {
> >   */
> >  #define NEEDS_FEAT(m, ...)     NEEDS_FEAT_FLAG(m, 0, __VA_ARGS__)
> >
> > +/* Declare fixed RESx bits */
> > +#define FORCE_RES0(m)          NEEDS_FEAT_FLAG(m, FORCE_RESx, enforce_resx)
> > +#define FORCE_RES1(m)          NEEDS_FEAT_FLAG(m, FORCE_RESx | AS_RES1, \
> > +                                               enforce_resx)
> > +
> >  /*
> >   * Declare the dependency between a non-FGT register, a set of
> >   * feature, and the set of individual bits it contains. This generates
> 
> nit: features
> 
> > @@ -230,6 +236,15 @@ struct reg_feat_map_desc {
> >  #define FEAT_HCX               ID_AA64MMFR1_EL1, HCX, IMP
> >  #define FEAT_S2PIE             ID_AA64MMFR3_EL1, S2PIE, IMP
> >
> > +static bool enforce_resx(struct kvm *kvm)
> > +{
> > +       /*
> > +        * Returning false here means that the RESx bits will be always
> > +        * addded to the fixed set bit. Yes, this is counter-intuitive.
> 
> nit: added
> 
> > +        */
> > +       return false;
> > +}
> 
> I see what you're doing here, but it took me a while to get it and
> convince myself that there aren't any bugs (my self couldn't find any
> bugs, but I wouldn't trust him that much). You already introduce a new
> flag, FORCE_RESx. Why not just check that directly in the
> compute_resx_bits() loop, before the check for CALL_FUNC?
> 
> + if (map[i].flags & FORCE_RESx)
> +     match = false;
> + else if (map[i].flags & CALL_FUNC)
> ...
> 
> The way it is now, to understand FORCE_RES0, you must trace a flag, a
> macro expansion, and a function pointer, just to set a boolean to
> false.

With that scheme, you'd write something like:

+#define FORCE_RES0(m)          NEEDS_FEAT_FLAG(m, FORCE_RESx)

This construct would need a new __NEEDS_FEAT_0() macro that doesn't
take any argument other than flags. Something like below (untested).

	M.

diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
index 9485e1f2dc0b7..364bdd1e5be51 100644
--- a/arch/arm64/kvm/config.c
+++ b/arch/arm64/kvm/config.c
@@ -79,6 +79,12 @@ struct reg_feat_map_desc {
 		.match = (fun),				\
 	}
 
+#define __NEEDS_FEAT_0(m, f, w, ...)			\
+	{						\
+		.w	= (m),				\
+		.flags = (f),				\
+	}
+
 #define __NEEDS_FEAT_FLAG(m, f, w, ...)			\
 	CONCATENATE(__NEEDS_FEAT_, COUNT_ARGS(__VA_ARGS__))(m, f, w, __VA_ARGS__)
 
@@ -95,9 +101,8 @@ struct reg_feat_map_desc {
 #define NEEDS_FEAT(m, ...)	NEEDS_FEAT_FLAG(m, 0, __VA_ARGS__)
 
 /* Declare fixed RESx bits */
-#define FORCE_RES0(m)		NEEDS_FEAT_FLAG(m, FORCE_RESx, enforce_resx)
-#define FORCE_RES1(m)		NEEDS_FEAT_FLAG(m, FORCE_RESx | AS_RES1, \
-						enforce_resx)
+#define FORCE_RES0(m)		NEEDS_FEAT_FLAG(m, FORCE_RESx)
+#define FORCE_RES1(m)		NEEDS_FEAT_FLAG(m, FORCE_RESx | AS_RES1)
 
 /*
  * Declare the dependency between a non-FGT register, a set of
@@ -221,15 +226,6 @@ struct reg_feat_map_desc {
 #define FEAT_HCX		ID_AA64MMFR1_EL1, HCX, IMP
 #define FEAT_S2PIE		ID_AA64MMFR3_EL1, S2PIE, IMP
 
-static bool enforce_resx(struct kvm *kvm)
-{
-	/*
-	 * Returning false here means that the RESx bits will be always
-	 * addded to the fixed set bit. Yes, this is counter-intuitive.
-	 */
-	return false;
-}
-
 static bool not_feat_aa64el3(struct kvm *kvm)
 {
 	return !kvm_has_feat(kvm, FEAT_AA64EL3);
@@ -996,7 +992,7 @@ static const struct reg_bits_to_feat_map hcr_feat_map[] = {
 	NEEDS_FEAT(HCR_EL2_TWEDEL	|
 		   HCR_EL2_TWEDEn,
 		   FEAT_TWED),
-	NEEDS_FEAT_FLAG(HCR_EL2_E2H, RES1_WHEN_E2H1, enforce_resx),
+	NEEDS_FEAT_FLAG(HCR_EL2_E2H, RES1_WHEN_E2H1 | FORCE_RESx),
 	FORCE_RES0(HCR_EL2_RES0),
 	FORCE_RES1(HCR_EL2_RES1),
 };
@@ -1362,7 +1358,9 @@ struct resx compute_resx_bits(struct kvm *kvm,
 		if (map[i].flags & exclude)
 			continue;
 
-		if (map[i].flags & CALL_FUNC)
+		if (map[i].flags & FORCE_RESx)
+			match = false;
+		else if (map[i].flags & CALL_FUNC)
 			match = map[i].match(kvm);
 		else
 			match = idreg_feat_match(kvm, &map[i]);

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