[PATCH 1/5] lib: sbi: Allow specifying start mode to sbi_hsm_hart_start() API

[Atish Patra]

On Mon, Sep 14, 2020 at 12:44 AM Anup Patel <Anup.Patel at wdc.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Atish Patra <atishp at atishpatra.org>
> > Sent: 11 September 2020 06:29
> > To: Anup Patel <Anup.Patel at wdc.com>
> > Cc: Atish Patra <Atish.Patra at wdc.com>; Alistair Francis
> > <Alistair.Francis at wdc.com>; Anup Patel <anup at brainfault.org>; OpenSBI
> > <opensbi at lists.infradead.org>
> > Subject: Re: [PATCH 1/5] lib: sbi: Allow specifying start mode to
> > sbi_hsm_hart_start() API
> >
> > On Wed, Sep 9, 2020 at 7:51 AM Anup Patel <anup.patel at wdc.com> wrote:
> > >
> > > The sbi_scratch already has provision to specify the next stage mode
> > > so we can leverage this to specify start mode to sbi_hsm_hart_start().
> > >
> > > In future, this will be useful in providing SBI calls to U-mode on
> > > embedded cores where we M-mode and U-mode but no S-mode.
> > >
> > > Signed-off-by: Anup Patel <anup.patel at wdc.com>
> > > ---
> > >  include/sbi/sbi_hsm.h   | 2 +-
> > >  lib/sbi/sbi_ecall_hsm.c | 6 +++++-
> > >  lib/sbi/sbi_hsm.c       | 6 +++++-
> > >  3 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/sbi/sbi_hsm.h b/include/sbi/sbi_hsm.h index
> > > 57d41ff..18d129b 100644
> > > --- a/include/sbi/sbi_hsm.h
> > > +++ b/include/sbi/sbi_hsm.h
> > > @@ -25,7 +25,7 @@ int sbi_hsm_init(struct sbi_scratch *scratch, u32
> > > hartid, bool cold_boot);  void __noreturn sbi_hsm_exit(struct
> > > sbi_scratch *scratch);
> > >
> > >  int sbi_hsm_hart_start(struct sbi_scratch *scratch, u32 hartid,
> > > -                      ulong saddr, ulong priv);
> > > +                      ulong saddr, ulong smode, ulong priv);
> > >  int sbi_hsm_hart_stop(struct sbi_scratch *scratch, bool exitnow);
> > > int sbi_hsm_hart_get_state(u32 hartid);  int
> > > sbi_hsm_hart_state_to_status(int state); diff --git
> > > a/lib/sbi/sbi_ecall_hsm.c b/lib/sbi/sbi_ecall_hsm.c index
> > > 028bf68..992c93a 100644
> > > --- a/lib/sbi/sbi_ecall_hsm.c
> > > +++ b/lib/sbi/sbi_ecall_hsm.c
> > > @@ -19,12 +19,16 @@ static int sbi_ecall_hsm_handler(unsigned long
> > extid, unsigned long funcid,
> > >                                  unsigned long *args, unsigned long *out_val,
> > >                                  struct sbi_trap_info *out_trap)  {
> > > +       ulong smode;
> > >         int ret = 0, hstate;
> > >         struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
> > >
> > >         switch (funcid) {
> > >         case SBI_EXT_HSM_HART_START:
> > > -               ret = sbi_hsm_hart_start(scratch, args[0], args[1], args[2]);
> > > +               smode = csr_read(CSR_MSTATUS);
> > > +               smode = (smode & MSTATUS_MPP) >> MSTATUS_MPP_SHIFT;
> > > +               ret = sbi_hsm_hart_start(scratch, args[0], args[1],
> > > +                                        smode, args[2]);
> > >                 break;
> > >         case SBI_EXT_HSM_HART_STOP:
> > >                 ret = sbi_hsm_hart_stop(scratch, TRUE); diff --git
> > > a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c index 013647a..b430793 100644
> > > --- a/lib/sbi/sbi_hsm.c
> > > +++ b/lib/sbi/sbi_hsm.c
> > > @@ -203,7 +203,7 @@ fail_exit:
> > >  }
> > >
> > >  int sbi_hsm_hart_start(struct sbi_scratch *scratch, u32 hartid,
> > > -                      ulong saddr, ulong priv)
> > > +                      ulong saddr, ulong smode, ulong priv)
> > >  {
> > >         int rc;
> > >         unsigned long init_count;
> > > @@ -212,6 +212,9 @@ int sbi_hsm_hart_start(struct sbi_scratch *scratch,
> > u32 hartid,
> > >         struct sbi_hsm_data *hdata;
> > >         const struct sbi_platform *plat = sbi_platform_ptr(scratch);
> > >
> > > +       if (smode != PRV_M && smode != PRV_S && smode != PRV_U)
> > > +               return SBI_EINVAL;
> > > +
> > >         rscratch = sbi_hartid_to_scratch(hartid);
> > >         if (!rscratch)
> > >                 return SBI_EINVAL;
> > > @@ -236,6 +239,7 @@ int sbi_hsm_hart_start(struct sbi_scratch *scratch,
> > u32 hartid,
> > >         init_count = sbi_init_count(hartid);
> > >         rscratch->next_arg1 = priv;
> > >         rscratch->next_addr = saddr;
> > > +       rscratch->next_mode = smode;
> > >
> > >         if (sbi_platform_has_hart_hotplug(plat) ||
> > >            (sbi_platform_has_hart_secondary_boot(plat) &&
> > > !init_count)) {
> > > --
> > > 2.25.1
> > >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
> >
> > The change as such looks okay to me. But if it is done inside an internal
> > function instead of changing sbi_hsm_hart_start,we can avoid confusion.
> > We should keep the function names/arguments same as the spec if possible.
>
> The purpose of adding parameter in sbi_hsm_hart_start(), was to have
> capability to start boot HART of all non-root domains from init_coldboot().
> That's why this series is a preparatory series for OpenSBI domain support.
>

Got it. We are getting the priv mode of the caller hart for ecall use
case but domains
will get that information domain information. That's why the mode has
to be extracted before
sbi_hsm_hart_start not inside it. Thanks for the explanation.

> The sbi_ecall_hsm.c is the one which follows SBI spec but sbi_hsm.c is our
> Internal library for HSM functionality. (Similar example is sbi_ecall_replace.c
> and sbi_tlb.c)
>
> Regards,
> Anup


Reviewed-by: Atish Patra <atish.patra at wdc.com>

-- 
Regards,
Atish