Re: [PATCH 5/7] RISC-V: fix auipc-jalr addresses in patched alternatives

[Emil Renner Berthing <emil.renner.berthing@canonical.com>]

On Mon, 14 Nov 2022 at 13:15, Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Nov 14, 2022 at 12:38:39PM +0100, Heiko Stübner wrote:
> > Am Montag, 14. November 2022, 12:35:53 CET schrieb Andrew Jones:
> > > On Mon, Nov 14, 2022 at 11:57:29AM +0100, Emil Renner Berthing wrote:
> > > > On Thu, 10 Nov 2022 at 17:50, Heiko Stuebner <heiko@sntech.de> wrote:
> > > ...
> > > > > @@ -316,8 +384,15 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
> > > > >                 }
> > > > >
> > > > >                 tmp = (1U << alt->errata_id);
> > > > > -               if (cpu_req_feature & tmp)
> > > > > -                       patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len);
> > > > > +               if (cpu_req_feature & tmp) {
> > > > > +                       /* do the basic patching */
> > > > > +                       patch_text_nosync(alt->old_ptr, alt->alt_ptr,
> > > > > +                                         alt->alt_len);
> > > > > +
> > > > > +                       riscv_alternative_fix_auipc_jalr(alt->old_ptr,
> > > > > +                                                        alt->alt_len,
> > > > > +                                                        alt->old_ptr - alt->alt_ptr);
> > > >
> > > > Here you're casting a void pointer to an instruction to an unsigned
> > > > int pointer, but since we enable compressed instructions this may
> > > > result in an unaligned pointer. Using this pointer will work, but may
> > > > be slow. Eg. fault to m-mode to be patched up. We already do that in
> > > > other places in the arch/riscv, but I'd prefer not to add new
> > > > instances of this.
> > >
> > > Alternative instruction sequences (old and new) have compression disabled.
> >
> > That was my first thought as well, but I think Emil was talking more about the
> > placement of the alternative block inside the running kernel.
> >
> > i.e. I guess the starting point of an alternative sequence could also be unaligned.
>
> Oh, I see.
>
> >
> > Though I don't _yet_ see how an improvement could look like.
>
> I think we can patch the alternative macros to add alignment. Something
> like
>
> diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> index ec2f3f1b836f..3c330a9066f7 100644
> --- a/arch/riscv/include/asm/alternative-macros.h
> +++ b/arch/riscv/include/asm/alternative-macros.h
> @@ -20,6 +20,7 @@
>         ALT_ENTRY 886b, 888f, \vendor_id, \errata_id, 889f - 888f
>         .popsection
>         .subsection 1
> +       .balign 4
>  888 :
>         .option push
>         .option norvc
> @@ -34,6 +35,7 @@
>  .endm
>
>  .macro __ALTERNATIVE_CFG old_c, new_c, vendor_id, errata_id, enable
> +       .balign 4
>  886 :
>         .option push
>         .option norvc
> @@ -49,6 +51,7 @@
>
>  .macro __ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \
>                                   new_c_2, vendor_id_2, errata_id_2, enable_2
> +       .balign 4
>  886 :
>         .option push
>         .option norvc
> @@ -87,6 +90,7 @@
>         ALT_ENTRY("886b", "888f", __stringify(vendor_id), __stringify(errata_id), "889f - 888f") \
>         ".popsection\n"                                                 \
>         ".subsection 1\n"                                               \
> +       ".balign 4\n"                                                   \
>         "888 :\n"                                                       \
>         ".option push\n"                                                \
>         ".option norvc\n"                                               \
> @@ -100,6 +104,7 @@
>         ".endif\n"
>
>  #define __ALTERNATIVE_CFG(old_c, new_c, vendor_id, errata_id, enable)  \
> +       ".balign 4\n"                                                   \
>         "886 :\n"                                                       \
>         ".option push\n"                                                \
>         ".option norvc\n"                                               \
> @@ -116,6 +121,7 @@
>                                         enable_1,                       \
>                                    new_c_2, vendor_id_2, errata_id_2,   \
>                                         enable_2)                       \
> +       ".balign 4\n"                                                   \
>         "886 :\n"                                                       \
>         ".option push\n"                                                \
>         ".option norvc\n"                                               \

Why not just use accessors? Eg.

unsigned int riscv_instruction_at(void *p)
{
  u16 *parcel = p;
  return (unsigned int)parcel[0] | (unsigned int)parcel[1] << 16;
}

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv