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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 24 Nov 2022 21:33:01 +0800
From:   hejunhao <hejunhao3@...wei.com>
To:     Suzuki K Poulose <suzuki.poulose@....com>,
        <mathieu.poirier@...aro.org>, <mike.leach@...aro.org>,
        <leo.yan@...aro.org>, <jonathan.cameron@...wei.com>,
        <john.garry@...wei.com>
CC:     <coresight@...ts.linaro.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-doc@...r.kernel.org>, <lpieralisi@...nel.org>,
        <linuxarm@...wei.com>, <yangyicong@...wei.com>,
        <liuqi6124@...il.com>, <f.fangjian@...wei.com>,
        <prime.zeng@...ilicon.com>
Subject: Re: [PATCH v14 1/2] drivers/coresight: Add UltraSoc System Memory
 Buffer driver


On 2022/11/23 22:03, Suzuki K Poulose wrote:
> On 23/11/2022 12:38, Junhao He wrote:
>> From: Qi Liu <liuqi115@...wei.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@...wei.com>
>> Signed-off-by: Junhao He <hejunhao3@...wei.com>
>> Tested-by: JunHao He <hejunhao3@...wei.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.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.

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.

Thanks.
HeJunhao.

> _______________________________________________
> CoreSight mailing list -- coresight@...ts.linaro.org
> To unsubscribe send an email to coresight-leave@...ts.linaro.org
>
> .
>