Re: [PATCH v1 1/3] KVM: arm64: Add ESR class to the hyp_enter hyp event

[Vincent Donnefort <vdonnefort@google.com>]

On Fri, Oct 09, 2026 at 12:30:56PM +0100, Fuad Tabba wrote:
> Hi Vincent,
> 
> On Fri, 09 Oct 2026 09:45:27 +0100, Vincent Donnefort
> <vdonnefort@google.com> wrote:
> 
> [...]
> > To distinguish between a trap from host and from guest, add a "from="
> > field, which has 3 possibilities: "host", "vcpu" or "firmware". This
> > value can be deducted based on the existing vcpu field.
> 
> nit: "deduced". Also, "firmware" comes from the reason rather than
> from the vcpu field.
> 
> [...]
> > diff --git a/arch/arm64/include/asm/kvm_hypevents.h b/arch/arm64/include/asm/kvm_hypevents.h
> 
> [...]
> > @@ -10,15 +10,10 @@
> >  #ifndef __HYP_ENTER_EXIT_REASON
> >  #define __HYP_ENTER_EXIT_REASON
> >  enum hyp_enter_exit_reason {
> > -        HYP_REASON_SMC,
> > -        HYP_REASON_HVC,
> > -        HYP_REASON_SYS,
> > +        HYP_REASON_SMC = ESR_ELx_EC_MAX + 1,
> 
> This header now uses ESR_ELx_EC_MAX, so it should include <asm/esr.h>
> rather than rely on its includers.
> 
> >          HYP_REASON_PSCI,
> > -        HYP_REASON_HOST_ABORT,
> > -        HYP_REASON_GUEST_EXIT,
> > -        HYP_REASON_ERET_HOST,
> > -        HYP_REASON_ERET_GUEST,
> > -        HYP_REASON_UNKNOWN        /* Must be last */
> > +        HYP_REASON_IRQ,
> > +        HYP_REASON_ERET,
> >  };
> 
> Aren't fixed trace points ABI? This changes the raw values of the
> reason field (1 was hvc and is now WFx) and the strings it prints.
> Keeping the old values and recording the EC in a new field would avoid
> that.

The event format is described precisely in events/hypervisor/hyp_enter/format.
Any tooling should use that, so it doesn't seem like ABI to me. Also, this is
behind NVHE_EL2_DEBUG. The only precedent I know is in kernel/sched/ where there
are only tracepoints to avoid having trace events. 

Finally, we do not expose (yet :)) the raw interface. So this part is definitely
not ABI.

I tried to avoid having the EC separately to keep the event as small as
possible.

> 
> [...]
> > diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> [...]
> > @@ -213,7 +213,7 @@ static void __noreturn __kvm_host_psci_cpu_entry(unsigned long pc, unsigned long
> >          write_sysreg_el1(INIT_SCTLR_EL1_MMU_OFF, SYS_SCTLR);
> >          write_sysreg(INIT_PSTATE_EL1, SPSR_EL2);
> >
> > -        trace_hyp_exit(host_ctxt, HYP_REASON_PSCI);
> > +        trace_hyp_exit(host_ctxt, HYP_REASON_ERET);
> >          __host_enter(host_ctxt);
> 
> This also replaces the PSCI, ERET_HOST and ERET_GUEST exit reasons
> with a single ERET, which the commit message doesn't mention.

ERET_HOST is now "to=host reason=eret" and ERET_GUEST "to=guest reason=ERET".

This one here could be considered a bug fix and I could make it a separate
patch?

> 
> [...]
> > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> 
> [...]
> > @@ -324,13 +324,15 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
> >          __debug_switch_to_guest(vcpu);
> >
> >          do {
> > -                trace_hyp_exit(host_ctxt, HYP_REASON_ERET_GUEST);
> > +                trace_hyp_exit(host_ctxt, HYP_REASON_ERET);
> >
> >                  /* Jump in the fire! */
> >                  exit_code = __guest_enter(vcpu);
> >
> >                  /* And we're baaack! */
> > -                trace_hyp_enter(host_ctxt, HYP_REASON_GUEST_EXIT);
> > +                trace_hyp_enter(host_ctxt,
> > +                                ARM_EXCEPTION_CODE(exit_code) == ARM_EXCEPTION_IRQ ?
> > +                                HYP_REASON_IRQ : ESR_ELx_EC(read_sysreg_el2(SYS_ESR)));
> >          } while (fixup_guest_exit(vcpu, &exit_code));
> 
> Could ESR_EL2 be read only when the event is enabled? The read happens
> even when tracing is disabled or compiled out, so every non-IRQ guest
> exit now does an extra one: with NVHE_EL2_DEBUG=n, __kvm_vcpu_run()
> goes from two reads of ESR_EL2 to three.

Hum, that's a good point. I was thinking about having a HYP_REASON_ESR that when
sent actually read the ESR. But then handle_trap() would read it twice, unless
this one still gives the esr directly.

Alternatively, I thought of moving the esr_el2 = read_sysreg_el2(SYS_ESR) out of
__fixup_guest_exit(). But it didn't look nice.

WDYS?

> 
> [...]
> > diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
> [...]
> >  static const char *__hyp_enter_exit_reason_str(u8 reason)
> >  {
> [...]
> > +        if (reason <= ESR_ELx_EC_MAX) {
> > +                int i;
> > +
> > +                for (i = 0; i < ARRAY_SIZE(class); i++) {
> > +                        if (class[i].mask == reason)
> > +                                return class[i].name;
> > +                }
> > +
> > +                return "UNKNOWN_ESR";
> > +        }
> 
> Could this print the EC value when it isn't in the table?
> kvm_arm_exception_class has no entry for ILL, BTI, SME or GCS among
> others, so an ARM_EXCEPTION_IL exit (EC 0x0E) shows up as UNKNOWN_ESR.

I can extend kvm_arm_exception_class. Having a dynamic format string is not
possible here.

> 
> Cheers,
> /fuad

-- 
Vincent