Re: [PATCH v3] KVM: arm64: Sanitise ID_AA64MMFR3_EL1.S1P[IO]E

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Tue, Oct 20, 2026 at 08:30:56PM +0100, Fuad Tabba wrote:
> Hi Lorenzo,
>
> On Tue, 20 Oct 2026 12:35:14 +0100, "Lorenzo Stoakes (ARM)"
> <ljs@kernel.org> wrote:
> [...]
> >> Fixes: 70ed7238297fb ("KVM: arm64: Sanitise ID_AA64MMFR3_EL1")
> >> Fixes: d4a89e5aee23e ("KVM: arm64: Expose S1PIE to guests")
> >
> > I guess kernels that are missing 19868b67e3e7 and 2ce80f624032 will need
> > that stuff ported too?
>
> I think 2ce80f624032 would have to go back too, since without it a VMM
> can still write the ID registers after they've been sanitised.
> 19868b67e3e7 mostly provides the call site: the version I tested on
> 7.3-rc3 called kvm_idregs_enforce_dependencies() at the top of
> kvm_finalize_sys_regs() instead. Kernels before 6.16 don't have
> config.c, so the backport there would be a different patch. I'd leave
> the stable side to Marc though.

Ack :) I guess this is mostly 'stable looks painful' :)

>
> [...]
> > static const struct reg_feat_map_desc *idreg_feat_maps[] = {
> >         &id_aa64mmfr3_el1_desc,
> > +       &id_aa64pfr1_el1_desc,
> > };
> >
> > Right?
>
> That's how I'd expect it to look. Since
> kvm_idregs_enforce_dependencies() repeats until nothing changes,
> feat_gcs() could also test FEAT_S1PIE directly: by then, S1PIE has
> been cleared if its own dependencies are missing.

Great, so that clears I think the really main outstanding question for that
series. Really nice! :)

>
> [...]
> >> diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
> [...]
> >> @@ -59,6 +59,7 @@ struct reg_feat_map_desc {
> >>         const struct reg_bits_to_feat_map feat_map;
> >>         const struct reg_bits_to_feat_map *bit_feat_map;
> >>         const unsigned int                bit_feat_map_sz;
> >> +       const unsigned int                idreg_idx;
> >
> > This field feels a bit hazardous - 0 is a valid index and now things that
> > don't declare it are setting a valid value.
>
> idreg_idx is only read for the descriptors in idreg_feat_maps[], which
> all come from DECLARE_IDREG_FEAT_MAP(), so the 0 in the others is
> never used. Storing the encoding would work as well, I'll leave that
> one to Marc.

Yeah not used, but you're defaulting to a valid value, so it's got real
footgun potential.

>
> [...]
> >> +static bool idreg_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)
> >
> > To me this reads confusingly - idreg_apply_deps() would be clearer I think.
>
> Agreed.
>
> [...]
> >> +       for (int i = 0; i < r->bit_feat_map_sz; i++) {
> >> +               if ((r->bit_feat_map[i].flags & CALL_FUNC) &&
> >
> > What if it isn't a CALL_FUNC feature?
> >
> > Could this loop just be replaced with compute_resx_bits()?
>
> It's skipped, so a dependency written as a plain field match would be
> silently ignored. Both entries today are CALL_FUNC, but
> compute_resx_bits() handles every kind of entry and would also give
> the res1 line something to set. It'd mean moving idreg_deps() below
> it.
>
> Marc, compute_resx_bits(), the rename and the loop rework are all fine
> by me. If you agree, I can respin with them, or you can fold them in
> when applying, whichever is easier for you.
>
> Thanks,
> /fuad

Thanks! :)

--
Cheers, Lorenzo