[Skiboot] [PATCH 05/15] core/trace: Change buffer alignment from 4K to 64K

[Oliver]

On Mon, Mar 25, 2019 at 11:17 AM Jordan Niethe <jniethe5 at 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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot