Re: [PATCH 20/20] KVM: arm64: Add debugfs file dumping computed RESx values

[Marc Zyngier <maz@kernel.org>]

On Mon, 02 Feb 2026 08:59:45 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> Hi Marc,
> 
> On Mon, 26 Jan 2026 at 12:17, Marc Zyngier <maz@kernel.org> wrote:
> >
> > Computing RESx values is hard. Verifying that they are correct is
> > harder. Add a debugfs file called "resx" that will dump all the RESx
> > values for a given VM.
> >
> > I found it useful, maybe you will too.
> 
> I'm sure I will :)
> 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  1 +
> >  arch/arm64/kvm/sys_regs.c         | 98 +++++++++++++++++++++++++++++++
> >  2 files changed, 99 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index c82b071ade2a5..54072f6ec9d4b 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -375,6 +375,7 @@ struct kvm_arch {
> >
> >         /* Iterator for idreg debugfs */
> >         u8      idreg_debugfs_iter;
> > +       u16     sr_resx_iter;
> 
> Storing `sr_resx_iter` in `struct kvm_arch` effectively makes this
> debugfs file exclusive (returning -EBUSY on contention). Standard
> `seq_file` implementations should be stateless, using the `loff_t
> *pos` argument to track the index. This allows multiple users to read
> the file simultaneously without blocking each other.

Yup, that's a good point. I guess I've lazily reimplemented a square
wheel...

> 
> >
> >         /* Hypercall features firmware registers' descriptor */
> >         struct kvm_smccc_features smccc_feat;
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 88a57ca36d96c..f3f92b489b588 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -5090,12 +5090,110 @@ static const struct seq_operations idregs_debug_sops = {
> >
> >  DEFINE_SEQ_ATTRIBUTE(idregs_debug);
> >
> > +static const struct sys_reg_desc *sr_resx_find(struct kvm *kvm, u16 pos)
> > +{
> > +       unsigned long i, sr_idx = 0;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
> > +               const struct sys_reg_desc *r = &sys_reg_descs[i];
> > +
> > +               if (r->reg < __SANITISED_REG_START__)
> > +                       continue;
> > +
> > +               if (sr_idx == pos)
> > +                       return r;
> > +
> > +               sr_idx++;
> > +       }
> > +
> > +       return NULL;
> > +}
> > +
> > +static void *sr_resx_start(struct seq_file *s, loff_t *pos)
> > +{
> > +       struct kvm *kvm = s->private;
> > +       u16 *iter;
> > +
> > +       guard(mutex)(&kvm->arch.config_lock);
> 
> My understanding of `guard()` is that it releases the lock as soon as
> the current scope ends (i.e., when `sr_resx_start() `returns). If the
> intention was to protect the iteration, it seems like `sr_resx_next()`
> and `sr_resx_show()` would end up running unprotected. That said,
> converting this to a standard `seq_file` implementation should remove
> the need for locking altogether.
> 
> I guess you based your code on the existing code for the idregs
> debugfs. I had a look at that, and at vgic-debug, and I think they
> both can be simplified and made more robust [1]. I also have a diff
> that converts this to use `seq_file`. It's pretty similar to what I
> have for idregs in the series I sent out [2]. Let me know if you'd
> like me to share it.

Yes please. We might as well do the right thing, and I can fold that
into my current series with you as a co-author.

Thanks,

	M.

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