| 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: <8e3b1957-33a7-83ed-6d62-1294cdb679e4@huawei.com>
Date: Fri, 25 Nov 2022 22:26:29 +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/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);
Best regards,
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".
Best regards,
Junhao.
>
>>
>> Thanks.
>> HeJunhao.
>>
>>> _______________________________________________
>>> CoreSight mailing list -- coresight@...ts.linaro.org
>>> To unsubscribe send an email to coresight-leave@...ts.linaro.org
>>>
>>> .
>>>
>>
>
> _______________________________________________
> CoreSight mailing list -- coresight@...ts.linaro.org
> To unsubscribe send an email to coresight-leave@...ts.linaro.org
Powered by blists - more mailing lists