Messages in this thread | | | Subject | Re: [PATCH v14 1/2] drivers/coresight: Add UltraSoc System Memory Buffer driver | From | hejunhao <> | Date | Fri, 16 Dec 2022 17:30:05 +0800 |
| |
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 > > > > . >
| |