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

[Fuad Tabba]

Hi Lorenzo,

On Tue, 20 Oct 2026 12:35:14 +0100, "Lorenzo Stoakes (ARM)"
<ljs@xxxxxxxxxx> 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.

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

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

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