On Fri, Feb 14, 2025 at 3:55 AM Leo Yan [off-list ref] wrote:

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

[...]

quoted

quoted

quoted

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

quoted

quoted

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.

We have to read BRBCR to see if it is enabled as PAUSED is unknown out
of reset and the driver may not exist to initialize it. Either way, it
is a register read and write, so same overhead for both.

Rob