| 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
| ||
|
Message-ID: <23b05db2-5126-52a5-b845-b031e0471305@huawei.com>
Date: Fri, 16 Dec 2022 17:30:05 +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>
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/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@...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.
>>>
>>> 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
>
>
>
> .
>
Powered by blists - more mailing lists