| Message ID | 20171019171553.24056-7-suzuki.poulose@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Suzuki, On 19/10/17 18:15, Suzuki K Poulose wrote: > Make the ETR SG table Circular buffer so that we could start > at any of the SG pages and use the entire buffer for tracing. > This can be achieved by : > > 1) Keeping an additional LINK pointer at the very end of the > SG table, i.e, after the LAST buffer entry, to point back to > the beginning of the first table. This will allow us to use > the buffer normally when we start the trace at offset 0 of > the buffer, as the LAST buffer entry hints the TMC-ETR and > it automatically wraps to the offset 0. > > 2) If we want to start at any other ETR SG page aligned offset, > we could : > a) Make the preceding page entry as LAST entry. > b) Make the original LAST entry a normal entry. > c) Use the table pointer to the "new" start offset as the > base of the table address. > This works as the TMC doesn't mandate that the page table > base address should be 4K page aligned. > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > drivers/hwtracing/coresight/coresight-tmc-etr.c | 159 +++++++++++++++++++++--- > 1 file changed, 139 insertions(+), 20 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index 4424eb67a54c..c171b244e12a 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c [...] > @@ -519,6 +534,107 @@ static void tmc_etr_sg_table_populate(struct etr_sg_table *etr_table) > /* Set up the last entry, which is always a data pointer */ > paddr = data_daddrs[dpidx] + spidx * ETR_SG_PAGE_SIZE; > *ptr++ = ETR_SG_ENTRY(paddr, ETR_SG_ET_LAST); > + /* followed by a circular link, back to the start of the table */ > + *ptr++ = ETR_SG_ENTRY(sg_table->table_daddr, ETR_SG_ET_LINK); > +} > + > +/* > + * tmc_etr_sg_offset_to_table_index : Translate a given data @offset > + * to the index of the page table "entry". Data pointers always have > + * a fixed location, with ETR_SG_PTRS_PER_PAGE - 1 entries in an > + * ETR_SG_PAGE and 1 link entry per (ETR_SG_PTRS_PER_PAGE -1) entries. > + */ > +static inline u32 > +tmc_etr_sg_offset_to_table_index(u64 offset) > +{ > + u64 sgpage_idx = offset >> ETR_SG_PAGE_SHIFT; > + > + return sgpage_idx + sgpage_idx / (ETR_SG_PTRS_PER_PAGE - 1); > +} > + > +/* > + * tmc_etr_sg_update_type: Update the type of a given entry in the > + * table to the requested entry. This is only used for data buffers > + * to toggle the "NORMAL" vs "LAST" buffer entries. > + */ > +static inline void tmc_etr_sg_update_type(sgte_t *entry, u32 type) > +{ > + WARN_ON(ETR_SG_ET(*entry) == ETR_SG_ET_LINK); > + WARN_ON(!ETR_SG_ET(*entry)); > + *entry &= ~ETR_SG_ET_MASK; > + *entry |= type; > +} > + > +/* > + * tmc_etr_sg_table_index_to_daddr: Return the hardware address to the table > + * entry @index. Use this address to let the table begin @index. > + */ > +static inline dma_addr_t > +tmc_etr_sg_table_index_to_daddr(struct tmc_sg_table *sg_table, u32 index) > +{ > + u32 sys_page_idx = index / ETR_SG_PTRS_PER_SYSPAGE; > + u32 sys_page_offset = index % ETR_SG_PTRS_PER_SYSPAGE; > + sgte_t *ptr; > + > + ptr = (sgte_t *)sg_table->table_pages.daddrs[sys_page_idx]; > + return (dma_addr_t)&ptr[sys_page_offset]; > +} > + > +/* > + * tmc_etr_sg_table_rotate : Rotate the SG circular buffer, moving > + * the "base" to a requested offset. We do so by : > + * > + * 1) Reset the current LAST buffer. > + * 2) Mark the "previous" buffer in the table to the "base" as LAST. > + * 3) Update the hwaddr to point to the table pointer for the buffer > + * which starts at "base". > + */ > +static int __maybe_unused > +tmc_etr_sg_table_rotate(struct etr_sg_table *etr_table, u64 base_offset) > +{ > + u32 last_entry, first_entry; > + u64 last_offset; > + struct tmc_sg_table *sg_table = etr_table->sg_table; > + sgte_t *table_ptr = sg_table->table_vaddr; > + ssize_t buf_size = tmc_sg_table_buf_size(sg_table); > + > + /* Offset should always be SG PAGE_SIZE aligned */ > + if (base_offset & (ETR_SG_PAGE_SIZE - 1)) { > + pr_debug("unaligned base offset %llx\n", base_offset); > + return -EINVAL; > + } > + /* Make sure the offset is within the range */ > + if (base_offset < 0 || base_offset > buf_size) { base_offset is unsigned, so the left operand of the '||' is useless (would've expected the compiler to emit a warning for this). > + base_offset = (base_offset + buf_size) % buf_size; > + pr_debug("Resetting offset to %llx\n", base_offset); > + } > + first_entry = tmc_etr_sg_offset_to_table_index(base_offset); > + if (first_entry == etr_table->first_entry) { > + pr_debug("Head is already at %llx, skipping\n", base_offset); > + return 0; > + } > + > + /* Last entry should be the previous one to the new "base" */ > + last_offset = ((base_offset - ETR_SG_PAGE_SIZE) + buf_size) % buf_size; > + last_entry = tmc_etr_sg_offset_to_table_index(last_offset); > + > + /* Reset the current Last page to Normal and new Last page to NORMAL */ Current Last page to NORMAL and new Last page to LAST? Cheers,
On 20/10/17 18:11, Julien Thierry wrote: >> +static int __maybe_unused >> +tmc_etr_sg_table_rotate(struct etr_sg_table *etr_table, u64 base_offset) >> +{ >> + u32 last_entry, first_entry; >> + u64 last_offset; >> + struct tmc_sg_table *sg_table = etr_table->sg_table; >> + sgte_t *table_ptr = sg_table->table_vaddr; >> + ssize_t buf_size = tmc_sg_table_buf_size(sg_table); >> + >> + /* Offset should always be SG PAGE_SIZE aligned */ >> + if (base_offset & (ETR_SG_PAGE_SIZE - 1)) { >> + pr_debug("unaligned base offset %llx\n", base_offset); >> + return -EINVAL; >> + } >> + /* Make sure the offset is within the range */ >> + if (base_offset < 0 || base_offset > buf_size) { > > base_offset is unsigned, so the left operand of the '||' is useless (would've expected the compiler to emit a warning for this). > >> + base_offset = (base_offset + buf_size) % buf_size; >> + pr_debug("Resetting offset to %llx\n", base_offset); >> + } >> + first_entry = tmc_etr_sg_offset_to_table_index(base_offset); >> + if (first_entry == etr_table->first_entry) { >> + pr_debug("Head is already at %llx, skipping\n", base_offset); >> + return 0; >> + } >> + >> + /* Last entry should be the previous one to the new "base" */ >> + last_offset = ((base_offset - ETR_SG_PAGE_SIZE) + buf_size) % buf_size; >> + last_entry = tmc_etr_sg_offset_to_table_index(last_offset); >> + >> + /* Reset the current Last page to Normal and new Last page to NORMAL */ > > Current Last page to NORMAL and new Last page to LAST? Thanks again, will fix them Cheers Suzuki
On Thu, Oct 19, 2017 at 06:15:42PM +0100, Suzuki K Poulose wrote: > Make the ETR SG table Circular buffer so that we could start > at any of the SG pages and use the entire buffer for tracing. > This can be achieved by : > > 1) Keeping an additional LINK pointer at the very end of the > SG table, i.e, after the LAST buffer entry, to point back to > the beginning of the first table. This will allow us to use > the buffer normally when we start the trace at offset 0 of > the buffer, as the LAST buffer entry hints the TMC-ETR and > it automatically wraps to the offset 0. > > 2) If we want to start at any other ETR SG page aligned offset, > we could : > a) Make the preceding page entry as LAST entry. > b) Make the original LAST entry a normal entry. > c) Use the table pointer to the "new" start offset as the > base of the table address. > This works as the TMC doesn't mandate that the page table > base address should be 4K page aligned. > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > drivers/hwtracing/coresight/coresight-tmc-etr.c | 159 +++++++++++++++++++++--- > 1 file changed, 139 insertions(+), 20 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index 4424eb67a54c..c171b244e12a 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -71,36 +71,41 @@ typedef u32 sgte_t; > * @sg_table: Generic SG Table holding the data/table pages. > * @hwaddr: hwaddress used by the TMC, which is the base > * address of the table. > + * @nr_entries: Total number of pointers in the table. > + * @first_entry: Index to the current "start" of the buffer. > + * @last_entry: Index to the last entry of the buffer. > */ > struct etr_sg_table { > struct tmc_sg_table *sg_table; > dma_addr_t hwaddr; > + u32 nr_entries; > + u32 first_entry; > + u32 last_entry; > }; > > /* > * tmc_etr_sg_table_entries: Total number of table entries required to map > * @nr_pages system pages. > * > - * We need to map @nr_pages * ETR_SG_PAGES_PER_SYSPAGE data pages. > + * We need to map @nr_pages * ETR_SG_PAGES_PER_SYSPAGE data pages and > + * an additional Link pointer for making it a Circular buffer. > * Each TMC page can map (ETR_SG_PTRS_PER_PAGE - 1) buffer pointers, > * with the last entry pointing to the page containing the table > - * entries. If we spill over to a new page for mapping 1 entry, > - * we could as well replace the link entry of the previous page > - * with the last entry. > + * entries. If we fill the last table in full with the pointers, (i.e, > + * nr_sgpages % (ETR_SG_PTRS_PER_PAGE - 1) == 0, we don't have to allocate > + * another table and hence skip the Link pointer. Also we could use the > + * link entry of the last page to make it circular. > */ > static inline unsigned long __attribute_const__ > tmc_etr_sg_table_entries(int nr_pages) > { > unsigned long nr_sgpages = nr_pages * ETR_SG_PAGES_PER_SYSPAGE; > unsigned long nr_sglinks = nr_sgpages / (ETR_SG_PTRS_PER_PAGE - 1); > - /* > - * If we spill over to a new page for 1 entry, we could as well > - * make it the LAST entry in the previous page, skipping the Link > - * address. > - */ > - if (nr_sglinks && (nr_sgpages % (ETR_SG_PTRS_PER_PAGE - 1) < 2)) > + > + if (nr_sglinks && !(nr_sgpages % (ETR_SG_PTRS_PER_PAGE - 1))) > nr_sglinks--; > - return nr_sgpages + nr_sglinks; > + /* Add an entry for the circular link */ > + return nr_sgpages + nr_sglinks + 1; > } > > /* > @@ -417,14 +422,21 @@ tmc_sg_daddr_to_vaddr(struct tmc_sg_table *sg_table, > /* Dump the given sg_table */ > static void tmc_etr_sg_table_dump(struct etr_sg_table *etr_table) > { > - sgte_t *ptr; > + sgte_t *ptr, *start; > int i = 0; > dma_addr_t addr; > struct tmc_sg_table *sg_table = etr_table->sg_table; > > - ptr = (sgte_t *)tmc_sg_daddr_to_vaddr(sg_table, > + start = (sgte_t *)tmc_sg_daddr_to_vaddr(sg_table, > etr_table->hwaddr, true); > - while (ptr) { > + if (!start) { > + pr_debug("ERROR: Failed to translate table base: 0x%llx\n", > + etr_table->hwaddr); > + return; > + } > + > + ptr = start; > + do { > addr = ETR_SG_ADDR(*ptr); > switch (ETR_SG_ET(*ptr)) { > case ETR_SG_ET_NORMAL: > @@ -436,14 +448,17 @@ static void tmc_etr_sg_table_dump(struct etr_sg_table *etr_table) > i, ptr, addr); > ptr = (sgte_t *)tmc_sg_daddr_to_vaddr(sg_table, > addr, true); > + if (!ptr) > + pr_debug("ERROR: Bad Link 0x%llx\n", addr); > break; > case ETR_SG_ET_LAST: > pr_debug("%05d: ### %p\t:[L] 0x%llx ###\n", > i, ptr, addr); > - return; > + ptr++; > + break; > } > i++; > - } > + } while (ptr && ptr != start); > pr_debug("******* End of Table *****\n"); > } > #endif > @@ -458,7 +473,7 @@ static void tmc_etr_sg_table_dump(struct etr_sg_table *etr_table) > static void tmc_etr_sg_table_populate(struct etr_sg_table *etr_table) > { > dma_addr_t paddr; > - int i, type, nr_entries; > + int i, type; > int tpidx = 0; /* index to the current system table_page */ > int sgtidx = 0; /* index to the sg_table within the current syspage */ > int sgtoffset = 0; /* offset to the next entry within the sg_table */ > @@ -469,16 +484,16 @@ static void tmc_etr_sg_table_populate(struct etr_sg_table *etr_table) > dma_addr_t *table_daddrs = sg_table->table_pages.daddrs; > dma_addr_t *data_daddrs = sg_table->data_pages.daddrs; > > - nr_entries = tmc_etr_sg_table_entries(sg_table->data_pages.nr_pages); > /* > * Use the contiguous virtual address of the table to update entries. > */ > ptr = sg_table->table_vaddr; > /* > - * Fill all the entries, except the last entry to avoid special > + * Fill all the entries, except the last two entries (i.e, the last > + * buffer and the circular link back to the base) to avoid special > * checks within the loop. > */ > - for (i = 0; i < nr_entries - 1; i++) { > + for (i = 0; i < etr_table->nr_entries - 2; i++) { > if (sgtoffset == ETR_SG_PTRS_PER_PAGE - 1) { > /* > * Last entry in a sg_table page is a link address to > @@ -519,6 +534,107 @@ static void tmc_etr_sg_table_populate(struct etr_sg_table *etr_table) > /* Set up the last entry, which is always a data pointer */ > paddr = data_daddrs[dpidx] + spidx * ETR_SG_PAGE_SIZE; > *ptr++ = ETR_SG_ENTRY(paddr, ETR_SG_ET_LAST); > + /* followed by a circular link, back to the start of the table */ > + *ptr++ = ETR_SG_ENTRY(sg_table->table_daddr, ETR_SG_ET_LINK); > +} > + > +/* > + * tmc_etr_sg_offset_to_table_index : Translate a given data @offset > + * to the index of the page table "entry". Data pointers always have > + * a fixed location, with ETR_SG_PTRS_PER_PAGE - 1 entries in an > + * ETR_SG_PAGE and 1 link entry per (ETR_SG_PTRS_PER_PAGE -1) entries. > + */ > +static inline u32 > +tmc_etr_sg_offset_to_table_index(u64 offset) > +{ > + u64 sgpage_idx = offset >> ETR_SG_PAGE_SHIFT; > + > + return sgpage_idx + sgpage_idx / (ETR_SG_PTRS_PER_PAGE - 1); > +} > + > +/* > + * tmc_etr_sg_update_type: Update the type of a given entry in the > + * table to the requested entry. This is only used for data buffers > + * to toggle the "NORMAL" vs "LAST" buffer entries. > + */ > +static inline void tmc_etr_sg_update_type(sgte_t *entry, u32 type) > +{ > + WARN_ON(ETR_SG_ET(*entry) == ETR_SG_ET_LINK); > + WARN_ON(!ETR_SG_ET(*entry)); > + *entry &= ~ETR_SG_ET_MASK; > + *entry |= type; > +} > + > +/* > + * tmc_etr_sg_table_index_to_daddr: Return the hardware address to the table > + * entry @index. Use this address to let the table begin @index. > + */ > +static inline dma_addr_t > +tmc_etr_sg_table_index_to_daddr(struct tmc_sg_table *sg_table, u32 index) > +{ > + u32 sys_page_idx = index / ETR_SG_PTRS_PER_SYSPAGE; > + u32 sys_page_offset = index % ETR_SG_PTRS_PER_SYSPAGE; > + sgte_t *ptr; > + > + ptr = (sgte_t *)sg_table->table_pages.daddrs[sys_page_idx]; > + return (dma_addr_t)&ptr[sys_page_offset]; > +} > + > +/* > + * tmc_etr_sg_table_rotate : Rotate the SG circular buffer, moving > + * the "base" to a requested offset. We do so by : > + * > + * 1) Reset the current LAST buffer. > + * 2) Mark the "previous" buffer in the table to the "base" as LAST. > + * 3) Update the hwaddr to point to the table pointer for the buffer > + * which starts at "base". > + */ > +static int __maybe_unused > +tmc_etr_sg_table_rotate(struct etr_sg_table *etr_table, u64 base_offset) > +{ > + u32 last_entry, first_entry; > + u64 last_offset; > + struct tmc_sg_table *sg_table = etr_table->sg_table; > + sgte_t *table_ptr = sg_table->table_vaddr; > + ssize_t buf_size = tmc_sg_table_buf_size(sg_table); > + > + /* Offset should always be SG PAGE_SIZE aligned */ > + if (base_offset & (ETR_SG_PAGE_SIZE - 1)) { > + pr_debug("unaligned base offset %llx\n", base_offset); > + return -EINVAL; > + } > + /* Make sure the offset is within the range */ > + if (base_offset < 0 || base_offset > buf_size) { > + base_offset = (base_offset + buf_size) % buf_size; > + pr_debug("Resetting offset to %llx\n", base_offset); > + } > + first_entry = tmc_etr_sg_offset_to_table_index(base_offset); > + if (first_entry == etr_table->first_entry) { > + pr_debug("Head is already at %llx, skipping\n", base_offset); > + return 0; > + } > + > + /* Last entry should be the previous one to the new "base" */ > + last_offset = ((base_offset - ETR_SG_PAGE_SIZE) + buf_size) % buf_size; > + last_entry = tmc_etr_sg_offset_to_table_index(last_offset); > + > + /* Reset the current Last page to Normal and new Last page to NORMAL */ > + tmc_etr_sg_update_type(&table_ptr[etr_table->last_entry], > + ETR_SG_ET_NORMAL); > + tmc_etr_sg_update_type(&table_ptr[last_entry], ETR_SG_ET_LAST); > + etr_table->hwaddr = tmc_etr_sg_table_index_to_daddr(sg_table, > + first_entry); > + etr_table->first_entry = first_entry; > + etr_table->last_entry = last_entry; > + pr_debug("table rotated to offset %llx-%llx, entries (%d - %d), dba: %llx\n", > + base_offset, last_offset, first_entry, last_entry, > + etr_table->hwaddr); The above line generates a warning when compiling for ARMv7. > + /* Sync the table for device */ > + tmc_sg_table_sync_table(sg_table); > +#ifdef ETR_SG_DEBUG > + tmc_etr_sg_table_dump(etr_table); > +#endif > + return 0; > } > > /* > @@ -552,6 +668,9 @@ tmc_init_etr_sg_table(struct device *dev, int node, > } > > etr_table->sg_table = sg_table; > + etr_table->nr_entries = nr_entries; > + etr_table->first_entry = 0; > + etr_table->last_entry = nr_entries - 2; > /* TMC should use table base address for DBA */ > etr_table->hwaddr = sg_table->table_daddr; > tmc_etr_sg_table_populate(etr_table); > -- > 2.13.6 >
On 01/11/17 23:47, Mathieu Poirier wrote: > On Thu, Oct 19, 2017 at 06:15:42PM +0100, Suzuki K Poulose wrote: >> Make the ETR SG table Circular buffer so that we could start >> at any of the SG pages and use the entire buffer for tracing. >> This can be achieved by : >> >> 1) Keeping an additional LINK pointer at the very end of the >> SG table, i.e, after the LAST buffer entry, to point back to >> the beginning of the first table. This will allow us to use >> the buffer normally when we start the trace at offset 0 of >> the buffer, as the LAST buffer entry hints the TMC-ETR and >> it automatically wraps to the offset 0. >> >> 2) If we want to start at any other ETR SG page aligned offset, >> we could : >> a) Make the preceding page entry as LAST entry. >> b) Make the original LAST entry a normal entry. >> c) Use the table pointer to the "new" start offset as the >> base of the table address. >> This works as the TMC doesn't mandate that the page table >> base address should be 4K page aligned. >> >> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> +static int __maybe_unused >> +tmc_etr_sg_table_rotate(struct etr_sg_table *etr_table, u64 base_offset) >> +{ >> + u32 last_entry, first_entry; >> + u64 last_offset; >> + struct tmc_sg_table *sg_table = etr_table->sg_table; >> + sgte_t *table_ptr = sg_table->table_vaddr; >> + ssize_t buf_size = tmc_sg_table_buf_size(sg_table); >> + >> + /* Offset should always be SG PAGE_SIZE aligned */ >> + if (base_offset & (ETR_SG_PAGE_SIZE - 1)) { >> + pr_debug("unaligned base offset %llx\n", base_offset); >> + return -EINVAL; >> + } >> + /* Make sure the offset is within the range */ >> + if (base_offset < 0 || base_offset > buf_size) { >> + base_offset = (base_offset + buf_size) % buf_size; >> + pr_debug("Resetting offset to %llx\n", base_offset); >> + } >> + first_entry = tmc_etr_sg_offset_to_table_index(base_offset); >> + if (first_entry == etr_table->first_entry) { >> + pr_debug("Head is already at %llx, skipping\n", base_offset); >> + return 0; >> + } >> + >> + /* Last entry should be the previous one to the new "base" */ >> + last_offset = ((base_offset - ETR_SG_PAGE_SIZE) + buf_size) % buf_size; >> + last_entry = tmc_etr_sg_offset_to_table_index(last_offset); >> + >> + /* Reset the current Last page to Normal and new Last page to NORMAL */ >> + tmc_etr_sg_update_type(&table_ptr[etr_table->last_entry], >> + ETR_SG_ET_NORMAL); >> + tmc_etr_sg_update_type(&table_ptr[last_entry], ETR_SG_ET_LAST); >> + etr_table->hwaddr = tmc_etr_sg_table_index_to_daddr(sg_table, >> + first_entry); >> + etr_table->first_entry = first_entry; >> + etr_table->last_entry = last_entry; >> + pr_debug("table rotated to offset %llx-%llx, entries (%d - %d), dba: %llx\n", >> + base_offset, last_offset, first_entry, last_entry, >> + etr_table->hwaddr); > > The above line generates a warning when compiling for ARMv7. Where you running with LPAE off ? That could probably be the case, where hwaddr could be 32bit or 64bit depending on whether LPAE is enabled. I will fix it. I have fixed some other warnings with ARMv7 with LPAE. Suzuki
On 2 November 2017 at 06:00, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: > On 01/11/17 23:47, Mathieu Poirier wrote: >> >> On Thu, Oct 19, 2017 at 06:15:42PM +0100, Suzuki K Poulose wrote: >>> >>> Make the ETR SG table Circular buffer so that we could start >>> at any of the SG pages and use the entire buffer for tracing. >>> This can be achieved by : >>> >>> 1) Keeping an additional LINK pointer at the very end of the >>> SG table, i.e, after the LAST buffer entry, to point back to >>> the beginning of the first table. This will allow us to use >>> the buffer normally when we start the trace at offset 0 of >>> the buffer, as the LAST buffer entry hints the TMC-ETR and >>> it automatically wraps to the offset 0. >>> >>> 2) If we want to start at any other ETR SG page aligned offset, >>> we could : >>> a) Make the preceding page entry as LAST entry. >>> b) Make the original LAST entry a normal entry. >>> c) Use the table pointer to the "new" start offset as the >>> base of the table address. >>> This works as the TMC doesn't mandate that the page table >>> base address should be 4K page aligned. >>> >>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >>> --- > > >>> +static int __maybe_unused >>> +tmc_etr_sg_table_rotate(struct etr_sg_table *etr_table, u64 base_offset) >>> +{ >>> + u32 last_entry, first_entry; >>> + u64 last_offset; >>> + struct tmc_sg_table *sg_table = etr_table->sg_table; >>> + sgte_t *table_ptr = sg_table->table_vaddr; >>> + ssize_t buf_size = tmc_sg_table_buf_size(sg_table); >>> + >>> + /* Offset should always be SG PAGE_SIZE aligned */ >>> + if (base_offset & (ETR_SG_PAGE_SIZE - 1)) { >>> + pr_debug("unaligned base offset %llx\n", base_offset); >>> + return -EINVAL; >>> + } >>> + /* Make sure the offset is within the range */ >>> + if (base_offset < 0 || base_offset > buf_size) { >>> + base_offset = (base_offset + buf_size) % buf_size; >>> + pr_debug("Resetting offset to %llx\n", base_offset); >>> + } >>> + first_entry = tmc_etr_sg_offset_to_table_index(base_offset); >>> + if (first_entry == etr_table->first_entry) { >>> + pr_debug("Head is already at %llx, skipping\n", >>> base_offset); >>> + return 0; >>> + } >>> + >>> + /* Last entry should be the previous one to the new "base" */ >>> + last_offset = ((base_offset - ETR_SG_PAGE_SIZE) + buf_size) % >>> buf_size; >>> + last_entry = tmc_etr_sg_offset_to_table_index(last_offset); >>> + >>> + /* Reset the current Last page to Normal and new Last page to >>> NORMAL */ >>> + tmc_etr_sg_update_type(&table_ptr[etr_table->last_entry], >>> + ETR_SG_ET_NORMAL); >>> + tmc_etr_sg_update_type(&table_ptr[last_entry], ETR_SG_ET_LAST); >>> + etr_table->hwaddr = tmc_etr_sg_table_index_to_daddr(sg_table, >>> + first_entry); >>> + etr_table->first_entry = first_entry; >>> + etr_table->last_entry = last_entry; >>> + pr_debug("table rotated to offset %llx-%llx, entries (%d - %d), >>> dba: %llx\n", >>> + base_offset, last_offset, first_entry, >>> last_entry, >>> + etr_table->hwaddr); >> >> >> The above line generates a warning when compiling for ARMv7. > > > Where you running with LPAE off ? That could probably be the case, > where hwaddr could be 32bit or 64bit depending on whether LPAE > is enabled. I will fix it. My original setup did not have LPAE configured but even when I do configure it I can generate the warnings. Compiler: arm-linux-gnueabi-gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609 Let me know if you want my .config file. > > I have fixed some other warnings with ARMv7 with LPAE. > > Suzuki
On Thu, Nov 02, 2017 at 08:40:16AM -0600, Mathieu Poirier wrote: > On 2 November 2017 at 06:00, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: > > On 01/11/17 23:47, Mathieu Poirier wrote: > >> > >> On Thu, Oct 19, 2017 at 06:15:42PM +0100, Suzuki K Poulose wrote: > >>> > >>> Make the ETR SG table Circular buffer so that we could start > >>> at any of the SG pages and use the entire buffer for tracing. > >>> This can be achieved by : > >>> > >>> 1) Keeping an additional LINK pointer at the very end of the > >>> SG table, i.e, after the LAST buffer entry, to point back to > >>> the beginning of the first table. This will allow us to use > >>> the buffer normally when we start the trace at offset 0 of > >>> the buffer, as the LAST buffer entry hints the TMC-ETR and > >>> it automatically wraps to the offset 0. > >>> > >>> 2) If we want to start at any other ETR SG page aligned offset, > >>> we could : > >>> a) Make the preceding page entry as LAST entry. > >>> b) Make the original LAST entry a normal entry. > >>> c) Use the table pointer to the "new" start offset as the > >>> base of the table address. > >>> This works as the TMC doesn't mandate that the page table > >>> base address should be 4K page aligned. > >>> > >>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > >>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > >>> --- > > > > > >>> +static int __maybe_unused > >>> +tmc_etr_sg_table_rotate(struct etr_sg_table *etr_table, u64 base_offset) > >>> +{ > >>> + u32 last_entry, first_entry; > >>> + u64 last_offset; > >>> + struct tmc_sg_table *sg_table = etr_table->sg_table; > >>> + sgte_t *table_ptr = sg_table->table_vaddr; > >>> + ssize_t buf_size = tmc_sg_table_buf_size(sg_table); > >>> + > >>> + /* Offset should always be SG PAGE_SIZE aligned */ > >>> + if (base_offset & (ETR_SG_PAGE_SIZE - 1)) { > >>> + pr_debug("unaligned base offset %llx\n", base_offset); > >>> + return -EINVAL; > >>> + } > >>> + /* Make sure the offset is within the range */ > >>> + if (base_offset < 0 || base_offset > buf_size) { > >>> + base_offset = (base_offset + buf_size) % buf_size; > >>> + pr_debug("Resetting offset to %llx\n", base_offset); > >>> + } > >>> + first_entry = tmc_etr_sg_offset_to_table_index(base_offset); > >>> + if (first_entry == etr_table->first_entry) { > >>> + pr_debug("Head is already at %llx, skipping\n", > >>> base_offset); > >>> + return 0; > >>> + } > >>> + > >>> + /* Last entry should be the previous one to the new "base" */ > >>> + last_offset = ((base_offset - ETR_SG_PAGE_SIZE) + buf_size) % > >>> buf_size; > >>> + last_entry = tmc_etr_sg_offset_to_table_index(last_offset); > >>> + > >>> + /* Reset the current Last page to Normal and new Last page to > >>> NORMAL */ > >>> + tmc_etr_sg_update_type(&table_ptr[etr_table->last_entry], > >>> + ETR_SG_ET_NORMAL); > >>> + tmc_etr_sg_update_type(&table_ptr[last_entry], ETR_SG_ET_LAST); > >>> + etr_table->hwaddr = tmc_etr_sg_table_index_to_daddr(sg_table, > >>> + first_entry); > >>> + etr_table->first_entry = first_entry; > >>> + etr_table->last_entry = last_entry; > >>> + pr_debug("table rotated to offset %llx-%llx, entries (%d - %d), > >>> dba: %llx\n", > >>> + base_offset, last_offset, first_entry, > >>> last_entry, > >>> + etr_table->hwaddr); > >> > >> > >> The above line generates a warning when compiling for ARMv7. > > > > > > Where you running with LPAE off ? That could probably be the case, > > where hwaddr could be 32bit or 64bit depending on whether LPAE > > is enabled. I will fix it. > > My original setup did not have LPAE configured but even when I do > configure it I can generate the warnings. > > Compiler: > > arm-linux-gnueabi-gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609 > > Let me know if you want my .config file. For those who don't know... (it seems it's an all too common mistake). In the printf format definition, the flag can be used to determine the data type for the format. Of those flags, the two which are relevant here are: l printf expects a "long" or "unsigned long" type. ll printf expects a "long long" or "unsigned long long" type. The size of "long" or "long long" is ABI dependent. Typically, on 32-bit platforms, "long" is 32-bit and "long long" is 64-bit. On 64-bit platforms, "long" is 64-bit and "long long" is 128-bit. Moreover, ABIs can mandate alignment requirements for these data types. This can cause problems if you do something like: u64 var = 1; printk("foo %lx\n", var); If a 32-bit architecture mandates that arguments are passed in ascending 32-bit registers from r0, and 64-bit arguments are to be passed in an even,odd register pair, then the above printk() is a problem. The format specifies a "long" type, which is 32-bit type, printk() will expect the value in r1 for the above, but the compiler has arranged to pass "var" in r2,r3. So the end result is that the above prints an undefined value - whatever happens to be in r1 at the time. Don't laugh, exactly this problem is in kexec-tools right now! The kernel data types (and C99 data types) that are typed using the bit width map to the standard C types. What this means is that a "u64" might be "long" if building on a 64-bit platform, or "long long" if building on a 32-bit platform: include/uapi/asm-generic/int-ll64.h:typedef unsigned long long __u64; include/uapi/asm-generic/int-l64.h:typedef unsigned long __u64; This means if you try and pass a u64 integer variable to printf, you really can't use either of the "l" or "ll" flags, because you don't know which you should be using. The guidance at the top of Documentation/printk-formats.txt concerning s64/u64 is basically incorrect: Integer types ============= :: If variable is of Type, use printk format specifier: ------------------------------------------------------------ s64 %lld or %llx u64 %llu or %llx The only way around this is: 1) to cast to the appropriate non-bitwidth defined type and use it's format flag (eg, unsigned long long if you need at least 64-bit precision, and use "ll" in the format. Yes, on 64-bit it means you get 128-bit values, but that's a small price to pay for stuff working correctly.) 2) we do as done in userspace, and define a PRI64 which can be inserted into the format, but this is messy (iow, eg, "%"PRI64"x"). I personally find this rather horrid, and I suspect it'll trip other kernel developers sanity filters too.
On Thu, Oct 19, 2017 at 06:15:42PM +0100, Suzuki K Poulose wrote: > Make the ETR SG table Circular buffer so that we could start > at any of the SG pages and use the entire buffer for tracing. > This can be achieved by : > > 1) Keeping an additional LINK pointer at the very end of the > SG table, i.e, after the LAST buffer entry, to point back to > the beginning of the first table. This will allow us to use > the buffer normally when we start the trace at offset 0 of > the buffer, as the LAST buffer entry hints the TMC-ETR and > it automatically wraps to the offset 0. > > 2) If we want to start at any other ETR SG page aligned offset, > we could : > a) Make the preceding page entry as LAST entry. > b) Make the original LAST entry a normal entry. > c) Use the table pointer to the "new" start offset as the > base of the table address. > This works as the TMC doesn't mandate that the page table > base address should be 4K page aligned. > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > drivers/hwtracing/coresight/coresight-tmc-etr.c | 159 +++++++++++++++++++++--- > 1 file changed, 139 insertions(+), 20 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index 4424eb67a54c..c171b244e12a 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -71,36 +71,41 @@ typedef u32 sgte_t; > * @sg_table: Generic SG Table holding the data/table pages. > * @hwaddr: hwaddress used by the TMC, which is the base > * address of the table. > + * @nr_entries: Total number of pointers in the table. > + * @first_entry: Index to the current "start" of the buffer. > + * @last_entry: Index to the last entry of the buffer. > */ > struct etr_sg_table { > struct tmc_sg_table *sg_table; > dma_addr_t hwaddr; > + u32 nr_entries; > + u32 first_entry; > + u32 last_entry; > }; > > /* > * tmc_etr_sg_table_entries: Total number of table entries required to map > * @nr_pages system pages. > * > - * We need to map @nr_pages * ETR_SG_PAGES_PER_SYSPAGE data pages. > + * We need to map @nr_pages * ETR_SG_PAGES_PER_SYSPAGE data pages and > + * an additional Link pointer for making it a Circular buffer. > * Each TMC page can map (ETR_SG_PTRS_PER_PAGE - 1) buffer pointers, > * with the last entry pointing to the page containing the table > - * entries. If we spill over to a new page for mapping 1 entry, > - * we could as well replace the link entry of the previous page > - * with the last entry. > + * entries. If we fill the last table in full with the pointers, (i.e, > + * nr_sgpages % (ETR_SG_PTRS_PER_PAGE - 1) == 0, we don't have to allocate > + * another table and hence skip the Link pointer. Also we could use the > + * link entry of the last page to make it circular. > */ > static inline unsigned long __attribute_const__ > tmc_etr_sg_table_entries(int nr_pages) > { > unsigned long nr_sgpages = nr_pages * ETR_SG_PAGES_PER_SYSPAGE; > unsigned long nr_sglinks = nr_sgpages / (ETR_SG_PTRS_PER_PAGE - 1); > - /* > - * If we spill over to a new page for 1 entry, we could as well > - * make it the LAST entry in the previous page, skipping the Link > - * address. > - */ > - if (nr_sglinks && (nr_sgpages % (ETR_SG_PTRS_PER_PAGE - 1) < 2)) > + > + if (nr_sglinks && !(nr_sgpages % (ETR_SG_PTRS_PER_PAGE - 1))) > nr_sglinks--; > - return nr_sgpages + nr_sglinks; > + /* Add an entry for the circular link */ > + return nr_sgpages + nr_sglinks + 1; > } > > /* > @@ -417,14 +422,21 @@ tmc_sg_daddr_to_vaddr(struct tmc_sg_table *sg_table, > /* Dump the given sg_table */ > static void tmc_etr_sg_table_dump(struct etr_sg_table *etr_table) > { > - sgte_t *ptr; > + sgte_t *ptr, *start; > int i = 0; > dma_addr_t addr; > struct tmc_sg_table *sg_table = etr_table->sg_table; > > - ptr = (sgte_t *)tmc_sg_daddr_to_vaddr(sg_table, > + start = (sgte_t *)tmc_sg_daddr_to_vaddr(sg_table, > etr_table->hwaddr, true); > - while (ptr) { > + if (!start) { > + pr_debug("ERROR: Failed to translate table base: 0x%llx\n", > + etr_table->hwaddr); > + return; > + } > + > + ptr = start; > + do { > addr = ETR_SG_ADDR(*ptr); > switch (ETR_SG_ET(*ptr)) { > case ETR_SG_ET_NORMAL: > @@ -436,14 +448,17 @@ static void tmc_etr_sg_table_dump(struct etr_sg_table *etr_table) > i, ptr, addr); > ptr = (sgte_t *)tmc_sg_daddr_to_vaddr(sg_table, > addr, true); > + if (!ptr) > + pr_debug("ERROR: Bad Link 0x%llx\n", addr); > break; > case ETR_SG_ET_LAST: > pr_debug("%05d: ### %p\t:[L] 0x%llx ###\n", > i, ptr, addr); > - return; > + ptr++; > + break; > } > i++; > - } > + } while (ptr && ptr != start); > pr_debug("******* End of Table *****\n"); > } > #endif > @@ -458,7 +473,7 @@ static void tmc_etr_sg_table_dump(struct etr_sg_table *etr_table) > static void tmc_etr_sg_table_populate(struct etr_sg_table *etr_table) > { > dma_addr_t paddr; > - int i, type, nr_entries; > + int i, type; > int tpidx = 0; /* index to the current system table_page */ > int sgtidx = 0; /* index to the sg_table within the current syspage */ > int sgtoffset = 0; /* offset to the next entry within the sg_table */ > @@ -469,16 +484,16 @@ static void tmc_etr_sg_table_populate(struct etr_sg_table *etr_table) > dma_addr_t *table_daddrs = sg_table->table_pages.daddrs; > dma_addr_t *data_daddrs = sg_table->data_pages.daddrs; > > - nr_entries = tmc_etr_sg_table_entries(sg_table->data_pages.nr_pages); > /* > * Use the contiguous virtual address of the table to update entries. > */ > ptr = sg_table->table_vaddr; > /* > - * Fill all the entries, except the last entry to avoid special > + * Fill all the entries, except the last two entries (i.e, the last > + * buffer and the circular link back to the base) to avoid special > * checks within the loop. > */ > - for (i = 0; i < nr_entries - 1; i++) { > + for (i = 0; i < etr_table->nr_entries - 2; i++) { > if (sgtoffset == ETR_SG_PTRS_PER_PAGE - 1) { > /* > * Last entry in a sg_table page is a link address to > @@ -519,6 +534,107 @@ static void tmc_etr_sg_table_populate(struct etr_sg_table *etr_table) > /* Set up the last entry, which is always a data pointer */ > paddr = data_daddrs[dpidx] + spidx * ETR_SG_PAGE_SIZE; > *ptr++ = ETR_SG_ENTRY(paddr, ETR_SG_ET_LAST); > + /* followed by a circular link, back to the start of the table */ > + *ptr++ = ETR_SG_ENTRY(sg_table->table_daddr, ETR_SG_ET_LINK); > +} > + > +/* > + * tmc_etr_sg_offset_to_table_index : Translate a given data @offset > + * to the index of the page table "entry". Data pointers always have > + * a fixed location, with ETR_SG_PTRS_PER_PAGE - 1 entries in an > + * ETR_SG_PAGE and 1 link entry per (ETR_SG_PTRS_PER_PAGE -1) entries. > + */ > +static inline u32 > +tmc_etr_sg_offset_to_table_index(u64 offset) > +{ > + u64 sgpage_idx = offset >> ETR_SG_PAGE_SHIFT; > + > + return sgpage_idx + sgpage_idx / (ETR_SG_PTRS_PER_PAGE - 1); > +} This function is the source of a bizarre linking error when compiling [14/17] on armv7 as pasted here: UPD include/generated/compile.h CC init/version.o AR init/built-in.o AR built-in.o LD vmlinux.o MODPOST vmlinux.o drivers/hwtracing/coresight/coresight-tmc-etr.o: In function `tmc_etr_sg_offset_to_table_index': /home/mpoirier/work/linaro/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-tmc-etr.c:553: undefined reference to `__aeabi_uldivmod' /home/mpoirier/work/linaro/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-tmc-etr.c:551: undefined reference to `__aeabi_uldivmod' /home/mpoirier/work/linaro/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-tmc-etr.c:553: undefined reference to `__aeabi_uldivmod' drivers/hwtracing/coresight/coresight-tmc-etr.o: In function `tmc_etr_sg_table_rotate': /home/mpoirier/work/linaro/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-tmc-etr.c:609: undefined reference to `__aeabi_uldivmod' Please see if you can reproduce on your side. Thanks, Mathieu > + > +/* > + * tmc_etr_sg_update_type: Update the type of a given entry in the > + * table to the requested entry. This is only used for data buffers > + * to toggle the "NORMAL" vs "LAST" buffer entries. > + */ > +static inline void tmc_etr_sg_update_type(sgte_t *entry, u32 type) > +{ > + WARN_ON(ETR_SG_ET(*entry) == ETR_SG_ET_LINK); > + WARN_ON(!ETR_SG_ET(*entry)); > + *entry &= ~ETR_SG_ET_MASK; > + *entry |= type; > +} > + > +/* > + * tmc_etr_sg_table_index_to_daddr: Return the hardware address to the table > + * entry @index. Use this address to let the table begin @index. > + */ > +static inline dma_addr_t > +tmc_etr_sg_table_index_to_daddr(struct tmc_sg_table *sg_table, u32 index) > +{ > + u32 sys_page_idx = index / ETR_SG_PTRS_PER_SYSPAGE; > + u32 sys_page_offset = index % ETR_SG_PTRS_PER_SYSPAGE; > + sgte_t *ptr; > + > + ptr = (sgte_t *)sg_table->table_pages.daddrs[sys_page_idx]; > + return (dma_addr_t)&ptr[sys_page_offset]; > +} > + > +/* > + * tmc_etr_sg_table_rotate : Rotate the SG circular buffer, moving > + * the "base" to a requested offset. We do so by : > + * > + * 1) Reset the current LAST buffer. > + * 2) Mark the "previous" buffer in the table to the "base" as LAST. > + * 3) Update the hwaddr to point to the table pointer for the buffer > + * which starts at "base". > + */ > +static int __maybe_unused > +tmc_etr_sg_table_rotate(struct etr_sg_table *etr_table, u64 base_offset) > +{ > + u32 last_entry, first_entry; > + u64 last_offset; > + struct tmc_sg_table *sg_table = etr_table->sg_table; > + sgte_t *table_ptr = sg_table->table_vaddr; > + ssize_t buf_size = tmc_sg_table_buf_size(sg_table); > + > + /* Offset should always be SG PAGE_SIZE aligned */ > + if (base_offset & (ETR_SG_PAGE_SIZE - 1)) { > + pr_debug("unaligned base offset %llx\n", base_offset); > + return -EINVAL; > + } > + /* Make sure the offset is within the range */ > + if (base_offset < 0 || base_offset > buf_size) { > + base_offset = (base_offset + buf_size) % buf_size; > + pr_debug("Resetting offset to %llx\n", base_offset); > + } > + first_entry = tmc_etr_sg_offset_to_table_index(base_offset); > + if (first_entry == etr_table->first_entry) { > + pr_debug("Head is already at %llx, skipping\n", base_offset); > + return 0; > + } > + > + /* Last entry should be the previous one to the new "base" */ > + last_offset = ((base_offset - ETR_SG_PAGE_SIZE) + buf_size) % buf_size; > + last_entry = tmc_etr_sg_offset_to_table_index(last_offset); > + > + /* Reset the current Last page to Normal and new Last page to NORMAL */ > + tmc_etr_sg_update_type(&table_ptr[etr_table->last_entry], > + ETR_SG_ET_NORMAL); > + tmc_etr_sg_update_type(&table_ptr[last_entry], ETR_SG_ET_LAST); > + etr_table->hwaddr = tmc_etr_sg_table_index_to_daddr(sg_table, > + first_entry); > + etr_table->first_entry = first_entry; > + etr_table->last_entry = last_entry; > + pr_debug("table rotated to offset %llx-%llx, entries (%d - %d), dba: %llx\n", > + base_offset, last_offset, first_entry, last_entry, > + etr_table->hwaddr); > + /* Sync the table for device */ > + tmc_sg_table_sync_table(sg_table); > +#ifdef ETR_SG_DEBUG > + tmc_etr_sg_table_dump(etr_table); > +#endif > + return 0; > } > > /* > @@ -552,6 +668,9 @@ tmc_init_etr_sg_table(struct device *dev, int node, > } > > etr_table->sg_table = sg_table; > + etr_table->nr_entries = nr_entries; > + etr_table->first_entry = 0; > + etr_table->last_entry = nr_entries - 2; > /* TMC should use table base address for DBA */ > etr_table->hwaddr = sg_table->table_daddr; > tmc_etr_sg_table_populate(etr_table); > -- > 2.13.6 >
On 06/11/17 19:07, Mathieu Poirier wrote: > On Thu, Oct 19, 2017 at 06:15:42PM +0100, Suzuki K Poulose wrote: ... >> +/* >> + * tmc_etr_sg_offset_to_table_index : Translate a given data @offset >> + * to the index of the page table "entry". Data pointers always have >> + * a fixed location, with ETR_SG_PTRS_PER_PAGE - 1 entries in an >> + * ETR_SG_PAGE and 1 link entry per (ETR_SG_PTRS_PER_PAGE -1) entries. >> + */ >> +static inline u32 >> +tmc_etr_sg_offset_to_table_index(u64 offset) >> +{ >> + u64 sgpage_idx = offset >> ETR_SG_PAGE_SHIFT; >> + >> + return sgpage_idx + sgpage_idx / (ETR_SG_PTRS_PER_PAGE - 1); >> +} > > This function is the source of a bizarre linking error when compiling [14/17] on > armv7 as pasted here: > > UPD include/generated/compile.h > CC init/version.o > AR init/built-in.o > AR built-in.o > LD vmlinux.o > MODPOST vmlinux.o > drivers/hwtracing/coresight/coresight-tmc-etr.o: In function > `tmc_etr_sg_offset_to_table_index': > /home/mpoirier/work/linaro/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-tmc-etr.c:553: > undefined reference to `__aeabi_uldivmod' > /home/mpoirier/work/linaro/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-tmc-etr.c:551: > undefined reference to `__aeabi_uldivmod' > /home/mpoirier/work/linaro/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-tmc-etr.c:553: > undefined reference to `__aeabi_uldivmod' > drivers/hwtracing/coresight/coresight-tmc-etr.o: In function > `tmc_etr_sg_table_rotate': > /home/mpoirier/work/linaro/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-tmc-etr.c:609: > undefined reference to `__aeabi_uldivmod' > > Please see if you can reproduce on your side. Uh ! I had gcc-7, which didn't complain about it. But if switch to 4.9, it does. It looks like division of 64bit entity on arm32 is triggering it. We don't need this to be u64 above, as it is the page_idx and could simply switch to "unsigned long" rather than explicitly using div64 helpers. The following change fixes the issue for me. Please could you check if it solves the problem for you ? @@ -551,7 +553,7 @@ static void tmc_etr_sg_table_populate(struct etr_sg_table *etr_table) static inline u32 tmc_etr_sg_offset_to_table_index(u64 offset) { - u64 sgpage_idx = offset >> ETR_SG_PAGE_SHIFT; + unsigned long sgpage_idx = offset >> ETR_SG_PAGE_SHIFT; return sgpage_idx + sgpage_idx / (ETR_SG_PTRS_PER_PAGE - 1); } Thanks for the testing ! Suzuki
On 7 November 2017 at 03:36, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: > On 06/11/17 19:07, Mathieu Poirier wrote: >> >> On Thu, Oct 19, 2017 at 06:15:42PM +0100, Suzuki K Poulose wrote: > > > ... > >>> +/* >>> + * tmc_etr_sg_offset_to_table_index : Translate a given data @offset >>> + * to the index of the page table "entry". Data pointers always have >>> + * a fixed location, with ETR_SG_PTRS_PER_PAGE - 1 entries in an >>> + * ETR_SG_PAGE and 1 link entry per (ETR_SG_PTRS_PER_PAGE -1) entries. >>> + */ >>> +static inline u32 >>> +tmc_etr_sg_offset_to_table_index(u64 offset) >>> +{ >>> + u64 sgpage_idx = offset >> ETR_SG_PAGE_SHIFT; >>> + >>> + return sgpage_idx + sgpage_idx / (ETR_SG_PTRS_PER_PAGE - 1); >>> +} >> >> >> This function is the source of a bizarre linking error when compiling >> [14/17] on >> armv7 as pasted here: >> >> UPD include/generated/compile.h >> CC init/version.o >> AR init/built-in.o >> AR built-in.o >> LD vmlinux.o >> MODPOST vmlinux.o >> drivers/hwtracing/coresight/coresight-tmc-etr.o: In function >> `tmc_etr_sg_offset_to_table_index': >> >> /home/mpoirier/work/linaro/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-tmc-etr.c:553: >> undefined reference to `__aeabi_uldivmod' >> >> /home/mpoirier/work/linaro/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-tmc-etr.c:551: >> undefined reference to `__aeabi_uldivmod' >> >> /home/mpoirier/work/linaro/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-tmc-etr.c:553: >> undefined reference to `__aeabi_uldivmod' >> drivers/hwtracing/coresight/coresight-tmc-etr.o: In function >> `tmc_etr_sg_table_rotate': >> >> /home/mpoirier/work/linaro/coresight/kernel-maint/drivers/hwtracing/coresight/coresight-tmc-etr.c:609: >> undefined reference to `__aeabi_uldivmod' >> >> Please see if you can reproduce on your side. > > > Uh ! I had gcc-7, which didn't complain about it. But if switch to 4.9, it > does. > It looks like division of 64bit entity on arm32 is triggering it. We don't > need > this to be u64 above, as it is the page_idx and could simply switch to > "unsigned long" > rather than explicitly using div64 helpers. > > The following change fixes the issue for me. Please could you check if it > solves the problem > for you ? Unfortunately it doesn't. Mathieu > > > @@ -551,7 +553,7 @@ static void tmc_etr_sg_table_populate(struct > etr_sg_table *etr_table) > static inline u32 > tmc_etr_sg_offset_to_table_index(u64 offset) > { > - u64 sgpage_idx = offset >> ETR_SG_PAGE_SHIFT; > + unsigned long sgpage_idx = offset >> ETR_SG_PAGE_SHIFT; > return sgpage_idx + sgpage_idx / (ETR_SG_PTRS_PER_PAGE - 1); > } > > > > Thanks for the testing ! > > Suzuki
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 4424eb67a54c..c171b244e12a 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -71,36 +71,41 @@ typedef u32 sgte_t; * @sg_table: Generic SG Table holding the data/table pages. * @hwaddr: hwaddress used by the TMC, which is the base * address of the table. + * @nr_entries: Total number of pointers in the table. + * @first_entry: Index to the current "start" of the buffer. + * @last_entry: Index to the last entry of the buffer. */ struct etr_sg_table { struct tmc_sg_table *sg_table; dma_addr_t hwaddr; + u32 nr_entries; + u32 first_entry; + u32 last_entry; }; /* * tmc_etr_sg_table_entries: Total number of table entries required to map * @nr_pages system pages. * - * We need to map @nr_pages * ETR_SG_PAGES_PER_SYSPAGE data pages. + * We need to map @nr_pages * ETR_SG_PAGES_PER_SYSPAGE data pages and + * an additional Link pointer for making it a Circular buffer. * Each TMC page can map (ETR_SG_PTRS_PER_PAGE - 1) buffer pointers, * with the last entry pointing to the page containing the table - * entries. If we spill over to a new page for mapping 1 entry, - * we could as well replace the link entry of the previous page - * with the last entry. + * entries. If we fill the last table in full with the pointers, (i.e, + * nr_sgpages % (ETR_SG_PTRS_PER_PAGE - 1) == 0, we don't have to allocate + * another table and hence skip the Link pointer. Also we could use the + * link entry of the last page to make it circular. */ static inline unsigned long __attribute_const__ tmc_etr_sg_table_entries(int nr_pages) { unsigned long nr_sgpages = nr_pages * ETR_SG_PAGES_PER_SYSPAGE; unsigned long nr_sglinks = nr_sgpages / (ETR_SG_PTRS_PER_PAGE - 1); - /* - * If we spill over to a new page for 1 entry, we could as well - * make it the LAST entry in the previous page, skipping the Link - * address. - */ - if (nr_sglinks && (nr_sgpages % (ETR_SG_PTRS_PER_PAGE - 1) < 2)) + + if (nr_sglinks && !(nr_sgpages % (ETR_SG_PTRS_PER_PAGE - 1))) nr_sglinks--; - return nr_sgpages + nr_sglinks; + /* Add an entry for the circular link */ + return nr_sgpages + nr_sglinks + 1; } /* @@ -417,14 +422,21 @@ tmc_sg_daddr_to_vaddr(struct tmc_sg_table *sg_table, /* Dump the given sg_table */ static void tmc_etr_sg_table_dump(struct etr_sg_table *etr_table) { - sgte_t *ptr; + sgte_t *ptr, *start; int i = 0; dma_addr_t addr; struct tmc_sg_table *sg_table = etr_table->sg_table; - ptr = (sgte_t *)tmc_sg_daddr_to_vaddr(sg_table, + start = (sgte_t *)tmc_sg_daddr_to_vaddr(sg_table, etr_table->hwaddr, true); - while (ptr) { + if (!start) { + pr_debug("ERROR: Failed to translate table base: 0x%llx\n", + etr_table->hwaddr); + return; + } + + ptr = start; + do { addr = ETR_SG_ADDR(*ptr); switch (ETR_SG_ET(*ptr)) { case ETR_SG_ET_NORMAL: @@ -436,14 +448,17 @@ static void tmc_etr_sg_table_dump(struct etr_sg_table *etr_table) i, ptr, addr); ptr = (sgte_t *)tmc_sg_daddr_to_vaddr(sg_table, addr, true); + if (!ptr) + pr_debug("ERROR: Bad Link 0x%llx\n", addr); break; case ETR_SG_ET_LAST: pr_debug("%05d: ### %p\t:[L] 0x%llx ###\n", i, ptr, addr); - return; + ptr++; + break; } i++; - } + } while (ptr && ptr != start); pr_debug("******* End of Table *****\n"); } #endif @@ -458,7 +473,7 @@ static void tmc_etr_sg_table_dump(struct etr_sg_table *etr_table) static void tmc_etr_sg_table_populate(struct etr_sg_table *etr_table) { dma_addr_t paddr; - int i, type, nr_entries; + int i, type; int tpidx = 0; /* index to the current system table_page */ int sgtidx = 0; /* index to the sg_table within the current syspage */ int sgtoffset = 0; /* offset to the next entry within the sg_table */ @@ -469,16 +484,16 @@ static void tmc_etr_sg_table_populate(struct etr_sg_table *etr_table) dma_addr_t *table_daddrs = sg_table->table_pages.daddrs; dma_addr_t *data_daddrs = sg_table->data_pages.daddrs; - nr_entries = tmc_etr_sg_table_entries(sg_table->data_pages.nr_pages); /* * Use the contiguous virtual address of the table to update entries. */ ptr = sg_table->table_vaddr; /* - * Fill all the entries, except the last entry to avoid special + * Fill all the entries, except the last two entries (i.e, the last + * buffer and the circular link back to the base) to avoid special * checks within the loop. */ - for (i = 0; i < nr_entries - 1; i++) { + for (i = 0; i < etr_table->nr_entries - 2; i++) { if (sgtoffset == ETR_SG_PTRS_PER_PAGE - 1) { /* * Last entry in a sg_table page is a link address to @@ -519,6 +534,107 @@ static void tmc_etr_sg_table_populate(struct etr_sg_table *etr_table) /* Set up the last entry, which is always a data pointer */ paddr = data_daddrs[dpidx] + spidx * ETR_SG_PAGE_SIZE; *ptr++ = ETR_SG_ENTRY(paddr, ETR_SG_ET_LAST); + /* followed by a circular link, back to the start of the table */ + *ptr++ = ETR_SG_ENTRY(sg_table->table_daddr, ETR_SG_ET_LINK); +} + +/* + * tmc_etr_sg_offset_to_table_index : Translate a given data @offset + * to the index of the page table "entry". Data pointers always have + * a fixed location, with ETR_SG_PTRS_PER_PAGE - 1 entries in an + * ETR_SG_PAGE and 1 link entry per (ETR_SG_PTRS_PER_PAGE -1) entries. + */ +static inline u32 +tmc_etr_sg_offset_to_table_index(u64 offset) +{ + u64 sgpage_idx = offset >> ETR_SG_PAGE_SHIFT; + + return sgpage_idx + sgpage_idx / (ETR_SG_PTRS_PER_PAGE - 1); +} + +/* + * tmc_etr_sg_update_type: Update the type of a given entry in the + * table to the requested entry. This is only used for data buffers + * to toggle the "NORMAL" vs "LAST" buffer entries. + */ +static inline void tmc_etr_sg_update_type(sgte_t *entry, u32 type) +{ + WARN_ON(ETR_SG_ET(*entry) == ETR_SG_ET_LINK); + WARN_ON(!ETR_SG_ET(*entry)); + *entry &= ~ETR_SG_ET_MASK; + *entry |= type; +} + +/* + * tmc_etr_sg_table_index_to_daddr: Return the hardware address to the table + * entry @index. Use this address to let the table begin @index. + */ +static inline dma_addr_t +tmc_etr_sg_table_index_to_daddr(struct tmc_sg_table *sg_table, u32 index) +{ + u32 sys_page_idx = index / ETR_SG_PTRS_PER_SYSPAGE; + u32 sys_page_offset = index % ETR_SG_PTRS_PER_SYSPAGE; + sgte_t *ptr; + + ptr = (sgte_t *)sg_table->table_pages.daddrs[sys_page_idx]; + return (dma_addr_t)&ptr[sys_page_offset]; +} + +/* + * tmc_etr_sg_table_rotate : Rotate the SG circular buffer, moving + * the "base" to a requested offset. We do so by : + * + * 1) Reset the current LAST buffer. + * 2) Mark the "previous" buffer in the table to the "base" as LAST. + * 3) Update the hwaddr to point to the table pointer for the buffer + * which starts at "base". + */ +static int __maybe_unused +tmc_etr_sg_table_rotate(struct etr_sg_table *etr_table, u64 base_offset) +{ + u32 last_entry, first_entry; + u64 last_offset; + struct tmc_sg_table *sg_table = etr_table->sg_table; + sgte_t *table_ptr = sg_table->table_vaddr; + ssize_t buf_size = tmc_sg_table_buf_size(sg_table); + + /* Offset should always be SG PAGE_SIZE aligned */ + if (base_offset & (ETR_SG_PAGE_SIZE - 1)) { + pr_debug("unaligned base offset %llx\n", base_offset); + return -EINVAL; + } + /* Make sure the offset is within the range */ + if (base_offset < 0 || base_offset > buf_size) { + base_offset = (base_offset + buf_size) % buf_size; + pr_debug("Resetting offset to %llx\n", base_offset); + } + first_entry = tmc_etr_sg_offset_to_table_index(base_offset); + if (first_entry == etr_table->first_entry) { + pr_debug("Head is already at %llx, skipping\n", base_offset); + return 0; + } + + /* Last entry should be the previous one to the new "base" */ + last_offset = ((base_offset - ETR_SG_PAGE_SIZE) + buf_size) % buf_size; + last_entry = tmc_etr_sg_offset_to_table_index(last_offset); + + /* Reset the current Last page to Normal and new Last page to NORMAL */ + tmc_etr_sg_update_type(&table_ptr[etr_table->last_entry], + ETR_SG_ET_NORMAL); + tmc_etr_sg_update_type(&table_ptr[last_entry], ETR_SG_ET_LAST); + etr_table->hwaddr = tmc_etr_sg_table_index_to_daddr(sg_table, + first_entry); + etr_table->first_entry = first_entry; + etr_table->last_entry = last_entry; + pr_debug("table rotated to offset %llx-%llx, entries (%d - %d), dba: %llx\n", + base_offset, last_offset, first_entry, last_entry, + etr_table->hwaddr); + /* Sync the table for device */ + tmc_sg_table_sync_table(sg_table); +#ifdef ETR_SG_DEBUG + tmc_etr_sg_table_dump(etr_table); +#endif + return 0; } /* @@ -552,6 +668,9 @@ tmc_init_etr_sg_table(struct device *dev, int node, } etr_table->sg_table = sg_table; + etr_table->nr_entries = nr_entries; + etr_table->first_entry = 0; + etr_table->last_entry = nr_entries - 2; /* TMC should use table base address for DBA */ etr_table->hwaddr = sg_table->table_daddr; tmc_etr_sg_table_populate(etr_table);
Make the ETR SG table Circular buffer so that we could start at any of the SG pages and use the entire buffer for tracing. This can be achieved by : 1) Keeping an additional LINK pointer at the very end of the SG table, i.e, after the LAST buffer entry, to point back to the beginning of the first table. This will allow us to use the buffer normally when we start the trace at offset 0 of the buffer, as the LAST buffer entry hints the TMC-ETR and it automatically wraps to the offset 0. 2) If we want to start at any other ETR SG page aligned offset, we could : a) Make the preceding page entry as LAST entry. b) Make the original LAST entry a normal entry. c) Use the table pointer to the "new" start offset as the base of the table address. This works as the TMC doesn't mandate that the page table base address should be 4K page aligned. Cc: Mathieu Poirier <mathieu.poirier@linaro.org> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> --- drivers/hwtracing/coresight/coresight-tmc-etr.c | 159 +++++++++++++++++++++--- 1 file changed, 139 insertions(+), 20 deletions(-)