Hi Leo, James,

On Tue, Jul 22, 2025 at 03:46:11PM +0100, James Clark wrote:

On 21/07/2025 4:21 pm, Leo Yan wrote:

quoted

On Mon, Jul 21, 2025 at 02:20:15PM +0100, James Clark wrote:

[...]

quoted

quoted

quoted

Thought about this some more.

Before:

arm_spe_pmu_buf_get_fault_act:
    <drain buffer>
    ISB
arm_spe_perf_aux_output_begin:
    PMBLIMITR_EL1.E = 1
ISB
PMBSR_EL1.S = 0
ERET

Now:

PMBLIMITR_EL1 = 0
ISB

PMBSR_EL1.S = 0
arm_spe_perf_aux_output_begin:
    ISB
    PMBLIMITR_EL1.E = 1
ERET

I don't see much of a difference between the two sequences - the point after
which we can be sure that profiling is enabled remains the ERET from the
exception return.  The only difference is that, before this change, the ERET
synchronized clearing the PMBSR_EL1.S bit, now it synchronizes setting the
PMBLIMITR_EL1.E bit.

Thoughts?

To make the discussion easier, I'll focus on the trace enabling flow
in this reply.

My understanding of a sane flow would be:

    arm_spe_pmu_irq_handler() {
      arm_spe_perf_aux_output_begin() {
            SYS_PMBPTR_EL1 = base;

            ISB // Synchronization between SPE register setting and
                // enabling profiling buffer.
            PMBLIMITR_EL1.E = 1;
      }
      ISB // Context synchronization event to ensure visibility to SPE
    }

    ... start trace ... (Bottom half, e.g., softirq, etc)

    ERET

In the current code, arm_spe_perf_aux_output_begin() is followed by an
ISB, which serves as a context synchronization event to ensure
visibility to the SPE. After that, it ensures that the trace unit will
function correctly.

But I think Alex's point is that in the existing code the thing that finally
enables trace (PMBSR_EL1.S = 0) isn't followed by an isb(), only an ERET. So
the new flow isn't any different in that regard.

Thanks for explanation.

quoted

quoted

I understand that the Software Usage PKLXF recommends using an ERET as
the synchronization point. However, between enabling the profiling
buffer and the ERET, the kernel might execute other operations (e.g.,
softirqs, tasklets, etc.).

Isn't preemption disabled? Surely that's not possible. Even if something did
run it wouldn't be anything that touches the SPE registers, and we're sure
there's an isb() between setting up the buffer and the final PMBLIMITR_EL1.E
= 1

Yes, bottom half runs in preemtion disabled state. See:

   el1_interrupt() -> __el1_irq() -> irq_exit_rcu() -> invoke_softirq()

In some cases, sotfirq and tasklet might even cause long latency (I
believe it can be milliseconds or even longer), this is why ftrace
"irqsoff" tracer is used to profile latency caused by irq off state.

Bottom half does not modify SPE registsers, but it can be a long road
between enabling SPE trace and ERET.

quoted

quoted

Therefore, it seems to me that using ERET as the synchronization point
may be too late. This is why I think we should keep an ISB after
arm_spe_perf_aux_output_begin().

Wouldn't that make the ERET too late even in the current code then? But I
think we're agreeing there's no issue there?

I would say ERET is too late for current code as well.

Thanks,
Leo

Thanks for the explanation and the analysis. I think we were looking at the
patch from different point of views: I was interested in not changing the
current behaviour, you were saying that the existing behaviour can be improved
upon.

Ok I get it now. The point is that there is stuff in between the return in
the SPE IRQ handler and the actual ERET. If someone is interested in
sampling the kernel then they might miss sampling a small amount of that.

It's not a correctness thing, just reducing potential gaps in the samples. I
can add another commit to add it, but it doesn't look like it would be a
fixes commit either, just an improvement. And the same issue applies to the
existing code too.

I agree here, this looks on an improvement on existing behaviour. I would keep
it as a patch separate from this series, as it's not a fix, and it's not related
to to the rules from DEN0154.

Thanks,
Alex