Re: [PATCH v19 10/11] KVM: arm64: nvhe: Disable branch generation in nVHE guests

[Leo Yan <leo.yan@arm.com>]

On Thu, Feb 13, 2025 at 05:16:45PM -0600, Rob Herring wrote:

[...]

> > > +static void __debug_save_brbe(u64 *brbcr_el1)
> > > +{
> > > +       *brbcr_el1 = 0;
> > > +
> > > +       /* Check if the BRBE is enabled */
> > > +       if (!(read_sysreg_el1(SYS_BRBCR) & (BRBCR_ELx_E0BRE | BRBCR_ELx_ExBRE)))
> > > +               return;
> > > +
> > > +       /*
> > > +        * Prohibit branch record generation while we are in guest.
> > > +        * Since access to BRBCR_EL1 is trapped, the guest can't
> > > +        * modify the filtering set by the host.
> > > +        */
> > > +       *brbcr_el1 = read_sysreg_el1(SYS_BRBCR);
> > > +       write_sysreg_el1(0, SYS_BRBCR);
> > > +}
> >
> > Should flush branch record and use isb() before exit host kernel?
> 
> I don't think so. The isb()'s in the other cases appear to be related
> to ordering WRT memory buffers. BRBE is just registers. I would assume
> that there's some barrier before we switch to the guest.

Given BRBCR is a system register, my understanding is the followd ISB
can ensure the writing BRBCR has finished and take effect.  As a result,
it is promised that the branch record has been stopped.

However, with isb() it is not necessarily to say the branch records have
been flushed to the buffer.  The purpose at here is just to stop record.
The BRBE driver will take care the flush issue when it reads records.

I agreed that it is likely barriers in the followed switch flow can assure
the writing BRBCR to take effect.  It might be good to add a comment for
easier maintenance.

> > I see inconsistence between the function above and BRBE's disable
> > function. Here it clears E0BRE / ExBRE bits for disabling BRBE, but the
> > BRBE driver sets the PAUSED bit in BRBFCR_EL1 for disabling BRBE.
> 
> Indeed. This works, but the enabled check won't work. I'm going to add
> clearing BRBCR to brbe_disable(), and this part will stay the same.

Seems to me, a right logic would be:

- In BRBE driver, the brbe_disable() function should clear E0BRE and
  ExBRE bits in BRBCR.  It can make sure the BRBE is totally disabled
  when a perf session is terminated.

- For a kvm context switching, it is good to use PAUSED bit.  If a host
  is branch record enabled, this is a light way for temporarily pause
  branch record for the switched VM.

Thanks,
Leo