Re: [PATCH v3 01/14] powerpc: Enable Prefixed Instructions

[Nicholas Piggin <npiggin@gmail.com>]

Jordan Niethe's on February 28, 2020 12:52 pm:
> On Wed, Feb 26, 2020 at 5:50 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Jordan Niethe's on February 26, 2020 2:07 pm:
>> > From: Alistair Popple <alistair@popple.id.au>
>> >
>> > Prefix instructions have their own FSCR bit which needs to enabled via
>> > a CPU feature. The kernel will save the FSCR for problem state but it
>> > needs to be enabled initially.
>> >
>> > Signed-off-by: Alistair Popple <alistair@popple.id.au>
>> > ---
>> >  arch/powerpc/include/asm/reg.h    |  3 +++
>> >  arch/powerpc/kernel/dt_cpu_ftrs.c | 23 +++++++++++++++++++++++
>> >  2 files changed, 26 insertions(+)
>> >
>> > diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
>> > index 1aa46dff0957..c7758c2ccc5f 100644
>> > --- a/arch/powerpc/include/asm/reg.h
>> > +++ b/arch/powerpc/include/asm/reg.h
>> > @@ -397,6 +397,7 @@
>> >  #define SPRN_RWMR    0x375   /* Region-Weighting Mode Register */
>> >
>> >  /* HFSCR and FSCR bit numbers are the same */
>> > +#define FSCR_PREFIX_LG       13      /* Enable Prefix Instructions */
>> >  #define FSCR_SCV_LG  12      /* Enable System Call Vectored */
>> >  #define FSCR_MSGP_LG 10      /* Enable MSGP */
>> >  #define FSCR_TAR_LG  8       /* Enable Target Address Register */
>> > @@ -408,11 +409,13 @@
>> >  #define FSCR_VECVSX_LG       1       /* Enable VMX/VSX  */
>> >  #define FSCR_FP_LG   0       /* Enable Floating Point */
>> >  #define SPRN_FSCR    0x099   /* Facility Status & Control Register */
>> > +#define   FSCR_PREFIX        __MASK(FSCR_PREFIX_LG)
>>
>> When you add a new FSCR, there's a couple more things to do, check
>> out traps.c.
>>
>> >  #define   FSCR_SCV   __MASK(FSCR_SCV_LG)
>> >  #define   FSCR_TAR   __MASK(FSCR_TAR_LG)
>> >  #define   FSCR_EBB   __MASK(FSCR_EBB_LG)
>> >  #define   FSCR_DSCR  __MASK(FSCR_DSCR_LG)
>> >  #define SPRN_HFSCR   0xbe    /* HV=1 Facility Status & Control Register */
>> > +#define   HFSCR_PREFIX       __MASK(FSCR_PREFIX_LG)
>> >  #define   HFSCR_MSGP __MASK(FSCR_MSGP_LG)
>> >  #define   HFSCR_TAR  __MASK(FSCR_TAR_LG)
>> >  #define   HFSCR_EBB  __MASK(FSCR_EBB_LG)
>> > diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > index 182b4047c1ef..396f2c6c588e 100644
>> > --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > @@ -553,6 +553,28 @@ static int __init feat_enable_large_ci(struct dt_cpu_feature *f)
>> >       return 1;
>> >  }
>> >
>> > +static int __init feat_enable_prefix(struct dt_cpu_feature *f)
>> > +{
>> > +     u64 fscr, hfscr;
>> > +
>> > +     if (f->usable_privilege & USABLE_HV) {
>> > +             hfscr = mfspr(SPRN_HFSCR);
>> > +             hfscr |= HFSCR_PREFIX;
>> > +             mtspr(SPRN_HFSCR, hfscr);
>> > +     }
>> > +
>> > +     if (f->usable_privilege & USABLE_OS) {
>> > +             fscr = mfspr(SPRN_FSCR);
>> > +             fscr |= FSCR_PREFIX;
>> > +             mtspr(SPRN_FSCR, fscr);
>> > +
>> > +             if (f->usable_privilege & USABLE_PR)
>> > +                     current->thread.fscr |= FSCR_PREFIX;
>> > +     }
>> > +
>> > +     return 1;
>> > +}
>>
>> It would be good to be able to just use the default feature matching
>> for this, if possible? Do we not do the right thing with
>> init_thread.fscr?
> The default feature matching do you mean feat_enable()?
> I just tested using that again, within feat_enable() I can print and
> see that the [h]fscr gets the bits set for enabling prefixed
> instructions. However once I get to the shell and start xmon, the fscr
> bit for prefixed instructions can be seen to be unset. What we are
> doing in feat_enable_prefix() avoids that problem. So it seems maybe
> something is not quite right with the init_thread.fscr. I will look
> into further.

Okay it probably gets overwritten somewhere.

But hmm, the dt_cpu_ftrs parsing should be picking those up and setting
them by default I would think (or maybe not because you don't expect
FSCR bit if OS support is not required).

All the other FSCR bits are done similarly to this AFAIKS:

 https://patchwork.ozlabs.org/patch/1244476/

I would do that for now rather than digging into it too far, but we
should look at cleaning that up and doing something more like what
you have here.

>> >  struct dt_cpu_feature_match {
>> >       const char *name;
>> >       int (*enable)(struct dt_cpu_feature *f);
>> > @@ -626,6 +648,7 @@ static struct dt_cpu_feature_match __initdata
>> >       {"vector-binary128", feat_enable, 0},
>> >       {"vector-binary16", feat_enable, 0},
>> >       {"wait-v3", feat_enable, 0},
>> > +     {"prefix-instructions", feat_enable_prefix, 0},
>>
>> That's reasonable to make that a feature, will it specify a minimum
>> base set of prefix instructions or just that prefix instructions
>> with the prefix/suffix arrangement exist?
> This was just going to be that they exist.
>>
>> You may not need "-instructions" on the end, none of the other
>> instructions do.
> Good point.
>>
>> I would maybe just hold off upstreaming the dt_cpu_ftrs changes for
>> a bit. We have to do a pass over new CPU feature device tree, and
>> some compatibility questions have come up recently.
>>
>> If you wouldn't mind just adding the new [H]FSCR bits and faults
>> upstream for now, that would be good.
> No problem.

Thanks,
Nick