Re: [PATCH V2 10/11] coresight: sink: Add TRBE driver

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 1/15/21 6:13 PM, Suzuki K Poulose wrote:
> On 1/15/21 5:29 AM, Anshuman Khandual wrote:
>>
>>
>> On 1/13/21 8:58 PM, Suzuki K Poulose wrote:
>>> Hi Anshuman,
>>>
>>> The driver looks overall good to me. Please find some minor comments below

Sure.

>>>
>>> On 1/13/21 4:18 AM, Anshuman Khandual wrote:
>>>> Trace Buffer Extension (TRBE) implements a trace buffer per CPU which is
>>>> accessible via the system registers. The TRBE supports different addressing
>>>> modes including CPU virtual address and buffer modes including the circular
>>>> buffer mode. The TRBE buffer is addressed by a base pointer (TRBBASER_EL1),
>>>> an write pointer (TRBPTR_EL1) and a limit pointer (TRBLIMITR_EL1). But the
>>>> access to the trace buffer could be prohibited by a higher exception level
>>>> (EL3 or EL2), indicated by TRBIDR_EL1.P. The TRBE can also generate a CPU
>>>> private interrupt (PPI) on address translation errors and when the buffer
>>>> is full. Overall implementation here is inspired from the Arm SPE driver.
>>>>
>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>> Cc: Mike Leach <mike.leach@linaro.org>
>>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> 
> ...
> 
>>>> +
>>>> +/*
>>>> + * TRBE Buffer Management
>>>> + *
>>>> + * The TRBE buffer spans from the base pointer till the limit pointer. When enabled,
>>>> + * it starts writing trace data from the write pointer onward till the limit pointer.
>>>
>>>
>>>> + * When the write pointer reaches the address just before the limit pointer, it gets
>>>> + * wrapped around again to the base pointer. This is called a TRBE wrap event, which
>>>> + * generates a maintenance interrupt when operated in WRAP or STOP mode.
>>>
>>> According to the TRM, it is FILL mode, instead of STOP. So please change the above to:
>>>
>>> "operated in WRAP or FILL mode".

Changed.

>>
>> Updated.
>>
>>>
>>>
>>>>      The write
>>>> + * pointer again starts writing trace data from the base pointer until just before
>>>> + * the limit pointer before getting wrapped again with an IRQ and this process just
>>>> + * goes on as long as the TRBE is enabled.
>>>
>>> This could be dropped as it applies to WRAP/CIRCULAR buffer mode, which we don't use.
>>
>> Probably this could be changed a bit to match the FILL mode. Because it is essential
>> to describe the continuous nature of the buffer operation, even in the FILL mode.
>>
>>   * After TRBE
>>   * IRQ gets handled and enabled again, write pointer again starts writing trace data
>>   * from the base pointer until just before the limit pointer before getting wrapped
>>   * again with an IRQ and this process just goes on as long as the TRBE is enabled.
>>
> 
> The above doesn't parse well and kind of repeats the operation of TRBE which is
> already explained above. How about :
> 
>>>> + * When the write pointer reaches the address just before the limit pointer, it gets
>>>> + * wrapped around again to the base pointer. This is called a TRBE wrap event, which
>>>> + * generates a maintenance interrupt when operated in WRAP or STOP mode.
> 
> This driver uses FILL mode, where the TRBE stops the trace collection at wrap event.
> The IRQ handler updates the AUX buffer and re-enables the TRBE with updated WRITE and
> LIMIT pointers.

Updated.

> 
> 
>>>> +
>>>> +static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
>>>> +                   struct perf_event *event, void **pages,
>>>> +                   int nr_pages, bool snapshot)
>>>> +{
>>>> +    struct trbe_buf *buf;
>>>> +    struct page **pglist;
>>>> +    int i;
>>>> +
>>>> +    if ((nr_pages < 2) || (snapshot && (nr_pages & 1)))
>>>
>>> This restriction on snapshot could be removed now, since we use the
>>> full buffer.
>>
>> Dropped only the second condition here i.e (snapshot && (nr_pages & 1).
>> Just wondering if the aux buffer could work with a single page so that
>> the first condition can also be dropped.
> 
> I think it is good to keep the restriction of 2 pages, as the WRITE_PTR
> and the LIMIT_PTR must be page aligned. With a single page, you can't do
> much, with writing into a partially filled buffer. This may be added
> as a comment to explain the restriction.

Added the above comment.

> 
>>>> +
>>>> +static enum trbe_fault_action trbe_get_fault_act(struct perf_output_handle *handle)
>>>> +{
>>>> +    int ec = get_trbe_ec();
>>>> +    int bsc = get_trbe_bsc();
>>>> +
>>>> +    WARN_ON(is_trbe_running());
>>>> +    if (is_trbe_trg() || is_trbe_abort())
>>>
>>> We seem to be reading the TRBSR every single in these helpers. Could we optimise them
>>> by passing the register value in ?
>>
>> The same goes for get_trbe_ec() and get_trbe_bsc() as well. Probably all
>> TRBSR field probing helpers should be modified to accept a TRBSR register
>> value instead.
>>
>>>
>>> i.e
>>>      u64 trbsr = get_trbe_status();
>>>
>>>      WARN_ON(is_trbe_runnign(trbsr))
>>>      if (is_trbe_trg(trbsr) || is_trbe_abort(trbsr))
>>>
>>> For is_trbe_wrap() too
>>
>> Yes.
>>
>>>
> 
> 
>>>
>>> We should skip the driver init, if the kernel is unmapped at EL0,
>>> as the TRBE can't safely write to the kernel virtual addressed
>>> buffer when the CPU is running at EL0. This is unlikely, but we
>>> should cover that case.
>>
>> This should be sufficient or it needs a pr_err() as well ?
>>
> 
> Please add a pr_err() message to indicate why this failed. Otherwise
> the user could be left with no clue.

Sure, will add the following before exiting the TRBE init.

pr_err("TRBE wouldn't work if kernel gets unmapped at EL0\n")