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

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

On Wed, Oct 21, 2026 at 03:03:04PM +0100, Marc Zyngier wrote:
> On Tue, 20 Oct 2026 20:30:56 +0100,
> Fuad Tabba <fuad.tabba@linux.dev> wrote:
> >
> > >> @@ -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.
>
> I don't think we should baby-sit people who hack this code. If you put
> something that is not the output of IDREG_IDX(), you get to keep the
> pieces. If you use this out of context, it will blow up in your face.

That's fair enough, I did worry a bit about identifying the idreg variant
vs. not from a reg_feat_map_desc instance though?

> > >> +static bool idreg_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)
> > >> +       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.
>
> I really do not want to drag compute_resx_bits() into this. None of
> that stuff is RESx. It is bad enough that we use the same structures
> to represent different type of dependencies. I also want to add new
> ways to affect ID registers that really do not fit the RESx handling.

I wonder if it's worth breaking out the resx stuff into something separate
that both call? As a follow-up obviously.

>
> > 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.
>
> I don't care about the name, but I don't want the rest of the proposed
> changes.

Fine with me ;) the logic all looks fine so these were nits.

As you are indifferent on name, do you mind having the two functions one
calling another? That is a lot clearer to my eyes. But also not a
showstopper, obviously!

--
Cheers, Lorenzo