[U-Boot] [PATCH 12/19] riscv: Do some basic architecture level cpu initialization

[Auer, Lukas]

Hi Bin,

On Fri, 2018-11-30 at 17:48 +0800, Bin Meng wrote:
> Hi Lukas,
> 
> On Fri, Nov 16, 2018 at 7:10 AM Auer, Lukas
> <lukas.auer at aisec.fraunhofer.de> wrote:
> > 
> > Hi Bin,
> > 
> > On Tue, 2018-11-13 at 00:22 -0800, Bin Meng wrote:
> > > Implement arch_cpu_init() to do some basic architecture level cpu
> > > initialization, like FPU enable, etc.
> > > 
> > > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> > > ---
> > > 
> > >  arch/riscv/cpu/cpu.c | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> > > index d9f820c..4e508cf 100644
> > > --- a/arch/riscv/cpu/cpu.c
> > > +++ b/arch/riscv/cpu/cpu.c
> > > @@ -5,6 +5,7 @@
> > > 
> > >  #include <common.h>
> > >  #include <asm/csr.h>
> > > +#include <asm/encoding.h>
> > > 
> > >  /*
> > >   * prior_stage_fdt_address must be stored in the data section
> > > since
> > > it is used
> > > @@ -53,3 +54,23 @@ int print_cpuinfo(void)
> > > 
> > >       return 0;
> > >  }
> > > +
> > > +int arch_cpu_init(void)
> > > +{
> > > +     /* Enable FPU */
> > > +     if (supports_extension('d') || supports_extension('f')) {
> > > +             csr_write(mstatus, MSTATUS_FS);
> > 
> > This should use csr_set(), so that we don't overwrite the other
> > mstatus
> > entries.
> > 
> 
> Ah, yes!
> 
> > > +             csr_write(fcsr, 0);
> > 
> > BBL also clears the floating point registers before clearing fcsr.
> > Coreboot does neither of these, I am not sure if they are required.
> > 
> 
> It's not required. I believe this is just for sanitizing these
> registers.
> 

Ok, makes sense.

> > > +     }
> > > +
> > > +     /* Enable user/supervisor use of perf counters */
> > > +     if (supports_extension('s'))
> > > +             csr_write(scounteren, -1);
> > > +     csr_write(mcounteren, -1);
> > 
> > Should we really enable all counters, and for both user and
> > supervisor
> > code? I would tend to enable only the ones we need. How about only
> > enabling the cycle, time, and instret counters (the rest are not
> > currently used in the kernel) and only for supervisor code (so in
> > mcounteren)?
> > 
> 
> Yes, not all of them are used by current kernel, but they might be
> used in the future. To enable all of them is not a bad idea, unless
> turning it on brings some consequences.
> 

Hardware performance counters can be used as part of side-channel
attacks. It is therefore a good idea to disable them by default. Here,
I would restrict them to accesses from the supervisor mode. If needed,
software running in supervisor mode can still enable access for the
user mode.

Thanks,
Lukas

> > > +
> > > +     /* Disable paging */
> > > +     if (supports_extension('s'))
> > > +             csr_write(sptbr, 0);
> > 
> > sptbr has been renamed to satp.
> > 
> 
> Good catch! Will fix in v2.
> 
> Regards,
> Bin