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

[Marc Zyngier <maz@kernel.org>]

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.

> 
> [...]
> >> +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.

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.

> 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.

Thanks,

	M.

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