Re: [RFC patch 4/4] riscv: sifive: apply errata "cip-453" patch

[Vincent Chen <vincent.chen@sifive.com>]

On Thu, Mar 11, 2021 at 5:58 AM Ruinland ChuanTzu Tsai
<ruinland@andestech.com> wrote:
>
> Hi Vincent,
>
> Thanks for introducing the alternative mechanism to RISC-V, with which
> vendors could provide fixes for each erratum in a more elegant way.
>
> Somehow, I'm a bit sketchy about these parts of your proposal :
>
> >     /* Exception vector table */
> >  ENTRY(excp_vect_table)
> >     RISCV_PTR do_trap_insn_misaligned
> > -   RISCV_PTR do_trap_insn_fault
> > +   ALTERNATIVE(__stringify(RISCV_PTR do_trap_insn_fault),
> > +           __stringify(RISCV_PTR do_trap_insn_fault_trampoline),
> > +           SIFIVE_VENDOR_ID, ERRATA_CIP_453, CONFIG_ERRATA_SIFIVE_CIP_453)
> >     RISCV_PTR do_trap_insn_illegal
> >     RISCV_PTR do_trap_break
> >     RISCV_PTR do_trap_load_misaligned
> > @@ -461,7 +466,10 @@ ENTRY(excp_vect_table)
> >     RISCV_PTR do_trap_ecall_s
> >     RISCV_PTR do_trap_unknown
> >     RISCV_PTR do_trap_ecall_m
> > -   RISCV_PTR do_page_fault   /* instruction page fault */
> > +   /* instruciton page fault */
> > +   ALTERNATIVE(__stringify(RISCV_PTR do_page_fault),
> > +           __stringify(RISCV_PTR do_page_fault_trampoline),
> > +           SIFIVE_VENDOR_ID, ERRATA_CIP_453, CONFIG_ERRATA_SIFIVE_CIP_453)
> >     RISCV_PTR do_page_fault   /* load page fault */
>
> As far as I can tell, `ALTERNATIVE(...)` seems a bit like a mixture of
> ARM's version of ALTERNATIVE and `alternative_insn`. However, ARM's
> ALTERNATIVE takes a vardatic macro and yours here doesn't, which makes
> me wonder if another vendor needs to patch the same location as yours,
> will they be able to multiplex the same probe ?
>
This is a good question. For this case, I think the current
ALTERNATIVE(...) can not be used.
In my thoughts, we need to define a new MACRO like

#define ALTERNATIVE2("old inst", "vendor-1's new inst","vendor-1
id","errata_id","CONFIG_ERRATA_vendor-1" , "vendor-2's new
inst","vendor-2 id","errata_id","CONFIG_ERRATA_vendor-2" ) \
       "886 :\n"                                                       \
       oldinsn "\n"                                                    \
       ALT("vendor-1's new inst", "vendor-1
id","errata_id","CONFIG_ERRATA_vendor-1") \
       ALT("vendor-2's new inst", "vendor-2
id","errata_id","CONFIG_ERRATA_vendor-2")

where ALT is defined as below:
+#define ALT (altinsn, vendor_id, errata_id, enable) \
+       ".if " __stringify(enable) " == 1\n"                            \
+       "887 :\n"                                                       \
+       ".pushsection .alternative, \"a\"\n"                            \
+       ALT_ENTRY("886b", "888f", __stringify(vendor_id),
__stringify(errata_id), "889f - 888f") \
+       ".popsection\n"                                                 \
+       ".subsection 1\n"                                               \
+       "888 :\n"                                                       \
+       altinsn "\n"                                                    \
+       "889 :\n"                                                       \
+       ".previous\n"                                                   \
+       ".org   . - (887b - 886b) + (889b - 888b)\n"                    \
+       ".org   . - (889b - 888b) + (887b - 886b)\n"                    \,
+       ".endif\n"
+
By using ALTERNATIVE2, vendor-1 and vendor-2 (more precisely, CPU-1
and CPU-2) can replace the same instruction with different errata
patches. I will create a sample code in my next version patch for
vendor reference.

>
> Secondly, I think it's a bit intrusive to patch directly on exception
> vector table here.
>
> I'm not sure about whether it's possible to introduce your alternative
> probe inside do_trap_insn_fault() & do_page_fault(), do the inspection
> the reason of trap (e.g. instruction/load/store page fault) there and
> then, perform the software workaround.
>
> If that's not feasible, maybe we shall make a new macro with a name
> like "RISCV_TRAP_ENTRY" which encompass the alternative probes ?
>
The do_page_fault() will deal with all page fault exceptions such as
load/store page fault. However, our errata is only for the instruction
page fault. Therefore, not only we need an additional if condition to
distinguish the incoming exception type, but also all page fault will
suffer the performance impact from the if conditions. Therefore, I
decided to replace the exception handler directly.

I think your idea is good. I will follow your suggestions to create a
new macro for them to improve the readability.


> Last but not least, is it possible that in the near future,
> `alternative_if` macro family from ARM could be ported to RISC-V ?
>
No, it is not in my current plan.


Thank you for your feedback.


> Best regards,
> Ruinland
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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