| Message ID | 20190325001425.29483-6-jniethe5@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve usability of skiboot traces | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (b392d785eb49630b9f00fef8d17944ed82b2c1fe) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | fail | Signed-off-by missing |
On Mon, Mar 25, 2019 at 11:17 AM Jordan Niethe <jniethe5@gmail.com> wrote: > > The trace buffers are currently aligned to 4K. Later when we will mmap > the buffers, they must be page aligned. Most Power systems have a 64K > page size. On systems with a 4K page size, 64K aligned will still be > page aligned so they would not be effected by this change. It is the > trace_info struct that is aligned, meaning the tracebuf within it will > not be. Change the physical address exposed to the kernel from the > tracebuf_struct to the trace_info struct that contains it. This way an > aligned address exposed however the lock within the trace_info struct is > now exposed too. > --- > core/trace.c | 13 ++++++------- > include/trace.h | 2 +- > skiboot.lds.S | 6 +++--- > 3 files changed, 10 insertions(+), 11 deletions(-) > > diff --git a/core/trace.c b/core/trace.c > index 64a64bd7c35c..8e231b32eaa4 100644 > --- a/core/trace.c > +++ b/core/trace.c > @@ -191,7 +191,7 @@ static void trace_add_desc(struct trace_info *t, uint64_t size) > } > debug_descriptor.num_traces++; > > - debug_descriptor.trace_phys[i] = (uint64_t)&t->tb; > + debug_descriptor.trace_phys[i] = (uint64_t)t; > debug_descriptor.trace_tce[i] = 0; /* populated later */ > debug_descriptor.trace_size[i] = size; > } > @@ -204,23 +204,22 @@ void init_trace_buffers(void) > uint64_t size; > > /* Boot the boot trace in the debug descriptor */ > - trace_add_desc(any, sizeof(boot_tracebuf.buf)); > + trace_add_desc(any, sizeof(boot_tracebuf)); > > /* Allocate a trace buffer for each primary cpu. */ > for_each_cpu(t) { > if (t->is_secondary) > continue; > > - /* Use a 4K alignment for TCE mapping */ > - size = ALIGN_UP(sizeof(*t->trace) + tracebuf_extra(), 0x1000); > - t->trace = local_alloc(t->chip_id, size, 0x1000); > + /* Use a 64K alignment for TCE mapping */ > + size = ALIGN_UP(sizeof(*t->trace) + tracebuf_extra(), 0x10000); > + t->trace = local_alloc(t->chip_id, size, 0x10000); > if (t->trace) { > any = t->trace; > memset(t->trace, 0, size); > init_lock(&t->trace->lock); > t->trace->tb.max_size = cpu_to_be32(MAX_SIZE); > - trace_add_desc(any, sizeof(t->trace->tb) + > - tracebuf_extra()); > + trace_add_desc(any, size); > } else > prerror("TRACE: cpu 0x%x allocation failed\n", t->pir); > } > diff --git a/include/trace.h b/include/trace.h > index da43572e2d06..2b65e9075591 100644 > --- a/include/trace.h > +++ b/include/trace.h > @@ -29,7 +29,7 @@ struct cpu_thread; > void init_boot_tracebuf(struct cpu_thread *boot_cpu); > > struct trace_info { > - /* Lock for writers. */ > + /* Lock for writers. Exposed to kernel. */ > struct lock lock; > /* Exposed to kernel. */ > struct tracebuf tb; > diff --git a/skiboot.lds.S b/skiboot.lds.S > index 8d09b40e601c..596ee8d78abf 100644 > --- a/skiboot.lds.S > +++ b/skiboot.lds.S > @@ -142,11 +142,11 @@ SECTIONS > * to reside in their own pages for the sake of TCE > * mappings > */ > - . = ALIGN(0x1000); > + . = ALIGN(0x10000); > *(.data.memcons); Does the alignment of the data.memcons section need to change? Having the .align before data.boot_trace forces will insert padding as required and the .align afterwards ensures that no unrelated stuff from .data is included in the page, but I don't see why this needs to change. > - . = ALIGN(0x1000); > + . = ALIGN(0x10000); > *(.data.boot_trace); > - . = ALIGN(0x1000); > + . = ALIGN(0x10000); > *(.data*) > *(.force.data) > *(.toc1) > -- > 2.20.1 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
diff --git a/core/trace.c b/core/trace.c index 64a64bd7c35c..8e231b32eaa4 100644 --- a/core/trace.c +++ b/core/trace.c @@ -191,7 +191,7 @@ static void trace_add_desc(struct trace_info *t, uint64_t size) } debug_descriptor.num_traces++; - debug_descriptor.trace_phys[i] = (uint64_t)&t->tb; + debug_descriptor.trace_phys[i] = (uint64_t)t; debug_descriptor.trace_tce[i] = 0; /* populated later */ debug_descriptor.trace_size[i] = size; } @@ -204,23 +204,22 @@ void init_trace_buffers(void) uint64_t size; /* Boot the boot trace in the debug descriptor */ - trace_add_desc(any, sizeof(boot_tracebuf.buf)); + trace_add_desc(any, sizeof(boot_tracebuf)); /* Allocate a trace buffer for each primary cpu. */ for_each_cpu(t) { if (t->is_secondary) continue; - /* Use a 4K alignment for TCE mapping */ - size = ALIGN_UP(sizeof(*t->trace) + tracebuf_extra(), 0x1000); - t->trace = local_alloc(t->chip_id, size, 0x1000); + /* Use a 64K alignment for TCE mapping */ + size = ALIGN_UP(sizeof(*t->trace) + tracebuf_extra(), 0x10000); + t->trace = local_alloc(t->chip_id, size, 0x10000); if (t->trace) { any = t->trace; memset(t->trace, 0, size); init_lock(&t->trace->lock); t->trace->tb.max_size = cpu_to_be32(MAX_SIZE); - trace_add_desc(any, sizeof(t->trace->tb) + - tracebuf_extra()); + trace_add_desc(any, size); } else prerror("TRACE: cpu 0x%x allocation failed\n", t->pir); } diff --git a/include/trace.h b/include/trace.h index da43572e2d06..2b65e9075591 100644 --- a/include/trace.h +++ b/include/trace.h @@ -29,7 +29,7 @@ struct cpu_thread; void init_boot_tracebuf(struct cpu_thread *boot_cpu); struct trace_info { - /* Lock for writers. */ + /* Lock for writers. Exposed to kernel. */ struct lock lock; /* Exposed to kernel. */ struct tracebuf tb; diff --git a/skiboot.lds.S b/skiboot.lds.S index 8d09b40e601c..596ee8d78abf 100644 --- a/skiboot.lds.S +++ b/skiboot.lds.S @@ -142,11 +142,11 @@ SECTIONS * to reside in their own pages for the sake of TCE * mappings */ - . = ALIGN(0x1000); + . = ALIGN(0x10000); *(.data.memcons); - . = ALIGN(0x1000); + . = ALIGN(0x10000); *(.data.boot_trace); - . = ALIGN(0x1000); + . = ALIGN(0x10000); *(.data*) *(.force.data) *(.toc1)