Re: [PATCH v14 1/2] drivers/coresight: Add UltraSoc System Memory Buffer driver

[hejunhao <hejunhao3@huawei.com>]

On 2022/11/28 19:06, Suzuki K Poulose wrote:
> On 25/11/2022 14:26, hejunhao wrote:
>>
>> On 2022/11/24 21:45, Suzuki K Poulose wrote:
>>> On 24/11/2022 13:33, hejunhao wrote:
>>>>
>>>> On 2022/11/23 22:03, Suzuki K Poulose wrote:
>>>>> On 23/11/2022 12:38, Junhao He wrote:
>>>>>> From: Qi Liu <liuqi115@huawei.com>
>>>>>>
>>>>>> Add driver for UltraSoc SMB(System Memory Buffer) device.
>>>>>> SMB provides a way to buffer messages from ETM, and store
>>>>>> these "CPU instructions trace" in system memory.
>>>>>> The SMB device is identifier as ACPI HID "HISI03A1". Device
>>>>>> system memory address resources are allocated using the _CRS
>>>>>> method and buffer modes is the circular buffer mode.
>>>>>>
>>>>>> SMB is developed by UltraSoc technology, which is acquired by
>>>>>> Siemens, and we still use "UltraSoc" to name driver.
>>>>>>
>>>>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>>>>>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
>>>>>> Tested-by: JunHao He <hejunhao3@huawei.com>
>>>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>>>> ---
>>>>>>   drivers/hwtracing/coresight/Kconfig        |  12 +
>>>>>>   drivers/hwtracing/coresight/Makefile       |   1 +
>>>>>>   drivers/hwtracing/coresight/ultrasoc-smb.c | 658 
>>>>>> +++++++++++++++++++++
>>>>>>   drivers/hwtracing/coresight/ultrasoc-smb.h | 129 ++++
>>>>>>   4 files changed, 800 insertions(+)
>>>>>>   create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.c
>>>>>>   create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.h
>>>>>>
>>>>>
>>>>>> +static void smb_sync_perf_buffer(struct smb_drv_data *drvdata,
>>>>>> +                 struct cs_buffers *buf,
>>>>>> +                 unsigned long head,
>>>>>> +                 unsigned long data_size)
>>>>>> +{
>>>>>> +    struct smb_data_buffer *sdb = &drvdata->sdb;
>>>>>> +    char **dst_pages = (char **)buf->data_pages;
>>>>>> +    unsigned long to_copy;
>>>>>> +    long pg_idx, pg_offset;
>>>>>> +
>>>>>> +    pg_idx = head >> PAGE_SHIFT;
>>>>>> +    pg_offset = head & (PAGE_SIZE - 1);
>>>>>> +
>>>>>> +    while (data_size) {
>>>>>> +        unsigned long pg_space = PAGE_SIZE - pg_offset;
>>>>>> +
>>>>>> +        /* Copy parts of trace data when read pointer wrap 
>>>>>> around */
>>>>>> +        if (sdb->rd_offset + pg_space > sdb->buf_size)
>>>>>> +            to_copy = sdb->buf_size - sdb->rd_offset;
>>>>>> +        else
>>>>>> +            to_copy = min(data_size, pg_space);
>>>>>> +
>>>>>> +        memcpy(dst_pages[pg_idx] + pg_offset,
>>>>>> +                  sdb->buf_base + sdb->rd_offset, to_copy);
>>>>>> +
>>>>>> +        pg_offset += to_copy;
>>>>>> +        if (pg_offset >= PAGE_SIZE) {
>>>>>> +            pg_offset = 0;
>>>>>> +            pg_idx++;
>>>>>> +            pg_idx %= buf->nr_pages;
>>>>>> +        }
>>>>>> +        data_size -= to_copy;
>>>>>> +        sdb->rd_offset += to_copy;
>>>>>> +        sdb->rd_offset %= sdb->buf_size;
>>>>>> +    }
>>>>>> +
>>>>>> +    sdb->data_size = 0;
>>>>>
>>>>>
>>>>> --8>-- cut here --<8--
>>>>>
>>>>>> +    writel(sdb->start_addr + sdb->rd_offset,
>>>>>> +        drvdata->base + SMB_LB_RD_ADDR_REG);
>>>>>> +
>>>>>> +    /*
>>>>>> +     * Data remained in link cannot be purged when SMB is full, so
>>>>>> +     * synchronize the read pointer to write pointer, to make sure
>>>>>> +     * these remained data won't influence next trace.
>>>>>> +     */
>>>>>> +    if (sdb->full) {
>>>>>> +        smb_purge_data(drvdata);
>>>>>> +        writel(readl(drvdata->base + SMB_LB_WR_ADDR_REG),
>>>>>> +               drvdata->base + SMB_LB_RD_ADDR_REG);
>>>>>> +    }
>>>>>
>>>>> --<8-- end here --8>--
>>>>>
>>>>> As pointed out in the last review, we must do this step
>>>>> everytime for perf mode irrespective of whether the buffer
>>>>> was "FULL" or not.
>>>>>
>>>>> i.e, the above block should simply be:
>>>>>
>>>>>     if (sdb->full)
>>>>>         smb_purge_data(drvdata);
>>>>>
>>>>>     /*
>>>>>      * The uncollected Data must be discarded for perf,
>>>>>      * as it cannot be clubbed with next schedule. We
>>>>>      * any way TRUNCATE the buffer in this case.
>>>>>      */
>>>>>     writel(readl(drvdata->base + SMB_LB_WR_ADDR_REG),
>>>>>         drvdata->base + SMB_LB_RD_ADDR_REG);
>>>>>
>>>>> Suzuki
>>>>
>>>> Hi Suzuki,
>>>>
>>>> We need to update SMB_LB_RD_ADDR_REG register first, then
>>>> check the "full" flag, whether the register needs to be
>>>> updated again.
>>>
>>> Why ? sdb->full is not updated after the write to RD_ADDR_REG.
>>>
>> Hi Suzuki,
>>
>> Maybe using the code below is more appropriate.
>> i.e,
>>
>>      writel(sdb->start_addr + sdb->rd_offset,
>>          drvdata->base + SMB_LB_RD_ADDR_REG);
>>
>>      /*
>>       * The uncollected Data must be discarded for perf,
>>       * as it cannot be clubbed with next schedule. We
>>       * any way TRUNCATE the buffer in this case.
>>       */
>>      smb_update_data_size(drvdata);
>>      if (sdb->data_size)
>>          writel(readl(drvdata->base + SMB_LB_WR_ADDR_REG),
>>                  drvdata->base + SMB_LB_RD_ADDR_REG);
>
> That should work. However, my question is, we must set the
> RD_ADDR_REG to the WR_ADDR_REG in all cases. So, why not :
>
>     /*
>      * We must flush and discard any data left in the
>      * buffer for a perf session, to avoid mixing
>      * the data from sessions.
>      * We TRUNCATE the buffer anyways to indicate
>      * that the buffer was lost.
>      */
>     smb_purge_data();
>     writel(readl(drvdata->base + SMB_LB_WR_ADDR_REG),
>         drvdata->base + SMB_LB_RD_ADDR_REG);
>
> and avoid the extra write ?
>
Hi Suzuki,

In sysfs and perf mode, the code that handles buffer after dump
was rewritten in the next version.

Thanks,
Junhao.
>>>>
>>>> If we don`t update the value of SMB_LB_RD_ADDR_REG register
>>>> or reset buffer state, the buffer state will still be "full".
>>>> The buffer has not free area,so the data will still remain
>>>> in link.
>>>
>>> My issue here is with potentially "leaving the trace from a previous
>>> session for the next session". i.e., at the end of a run, we must 
>>> always
>>> make sure that the buffer is left empty (unlike the sysfs mode).
>>>
>>> e.g.,
>>>
>>> perf_session_x: RUN0: RD=0x0, WR=0x5000, HANDLE_SIZE=0x3000, 
>>> full=false.
>>> At the end of the above routine we will have :
>>>     RD=0x3000, WR=0x5000,
>>>
>>> and if a different perf session comes in, say perf_session_y, it 
>>> will consume trace written by "x" at 0x3000-0x50000, right ?
>>>
>>> This is fine in the sysfs mode as we expect the entire sysfs mode
>>> to be owned by a single session and is left to the user to split it.
>>> But for perf mode we cannot do that and thus must make sure we don't
>>> leak trace from one session to antoher.
>>>
>>> Suzuki
>>>
>>
>> In this cace:  RUN0: RD=0x0, WR=0x5000, HANDLE_SIZE=0x3000, full=false.
>> We will update the "rd_offset" in smb_update_buffer() function.
>> like this:
>>
>>      ...
>>      if (data_size > handle->size) {
>>          sdb->rd_offset += data_size - handle->size;
>>          sdb->rd_offset %= sdb->buf_size;
>>          ...
>>          }
>>      ...
>>
>>
>> So the rd_offset will advance to 0x2000 first,
>> then we dump the latest trace data (0x2000 - 0x5000) from "buf_base + 
>> rd_offset".
>
> Right, that makes sense.
>
>
> Thanks
> Suzuki
>
>
>
> .
>