diff mbox series

lib: sbi_misaligned_ldst: Add handling of C.LHU/C.LH and C.SH

Message ID 20240226055644.881161-1-nylon.chen@sifive.com
State Accepted
Headers show
Series lib: sbi_misaligned_ldst: Add handling of C.LHU/C.LH and C.SH | expand

Commit Message

Nylon Chen Feb. 26, 2024, 5:56 a.m. UTC 
From: "Nylon Chen" <nylon.chen@sifive.com>

Added exception handling for compressed instructions C.LHU, C.LH, and
C.SH from the zcb extension to the sbi_misaligned_ldst library.

Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
---
 include/sbi/riscv_encoding.h  |  7 +++++++
 lib/sbi/sbi_misaligned_ldst.c | 10 ++++++++++
 2 files changed, 17 insertions(+)

Comments

Anup Patel March 4, 2024, 5:12 a.m. UTC | #1 
On Mon, Feb 26, 2024 at 11:27 AM Nylon Chen <nylon.chen@sifive.com> wrote:
>
> From: "Nylon Chen" <nylon.chen@sifive.com>
>
> Added exception handling for compressed instructions C.LHU, C.LH, and
> C.SH from the zcb extension to the sbi_misaligned_ldst library.
>
> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> ---
>  include/sbi/riscv_encoding.h  |  7 +++++++
>  lib/sbi/sbi_misaligned_ldst.c | 10 ++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
> index e74cc0d..3d77e26 100644
> --- a/include/sbi/riscv_encoding.h
> +++ b/include/sbi/riscv_encoding.h
> @@ -837,6 +837,13 @@
>  #define INSN_MATCH_C_FSWSP             0xe002
>  #define INSN_MASK_C_FSWSP              0xe003
>
> +#define INSN_MATCH_C_LHU               0x8400
> +#define INSN_MASK_C_LHU                0xfc43
> +#define INSN_MATCH_C_LH                0x8440
> +#define INSN_MASK_C_LH                 0xfc43
> +#define INSN_MATCH_C_SH                0x8c00
> +#define INSN_MASK_C_SH                 0xfc43
> +
>  #define INSN_MASK_WFI                  0xffffff00
>  #define INSN_MATCH_WFI                 0x10500000
>
> diff --git a/lib/sbi/sbi_misaligned_ldst.c b/lib/sbi/sbi_misaligned_ldst.c
> index aa512de..9ca225a 100644
> --- a/lib/sbi/sbi_misaligned_ldst.c
> +++ b/lib/sbi/sbi_misaligned_ldst.c
> @@ -123,6 +123,13 @@ int sbi_misaligned_load_handler(ulong addr, ulong tval2, ulong tinst,
>                 len = 4;
>  #endif
>  #endif
> +       } else if ((insn & INSN_MASK_C_LHU) == INSN_MATCH_C_LHU) {
> +               len = 2;
> +               insn = RVC_RS2S(insn) << SH_RD;
> +       } else if ((insn & INSN_MASK_C_LH) == INSN_MATCH_C_LH) {
> +               len = 2;
> +               shift = 8 * (sizeof(ulong) - len);
> +               insn = RVC_RS2S(insn) << SH_RD;
>         } else {
>                 uptrap.epc = regs->mepc;
>                 uptrap.cause = CAUSE_MISALIGNED_LOAD;
> @@ -237,6 +244,9 @@ int sbi_misaligned_store_handler(ulong addr, ulong tval2, ulong tinst,
>                 val.data_ulong = GET_F32_RS2C(insn, regs);
>  #endif
>  #endif
> +       } else if ((insn & INSN_MASK_C_SH) == INSN_MATCH_C_SH) {
> +               len             = 2;
> +               val.data_ulong = GET_RS2S(insn, regs);

Don't need to handle "c.sb" instruction ??

>         } else {
>                 uptrap.epc = regs->mepc;
>                 uptrap.cause = CAUSE_MISALIGNED_STORE;
> --
> 2.34.1
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

Regards,
Anup
Nylon Chen March 4, 2024, 6:19 a.m. UTC | #2 
Anup Patel <apatel@ventanamicro.com> 於 2024年3月4日 週一 下午1:12寫道：
>
> On Mon, Feb 26, 2024 at 11:27 AM Nylon Chen <nylon.chen@sifive.com> wrote:
> >
> > From: "Nylon Chen" <nylon.chen@sifive.com>
> >
> > Added exception handling for compressed instructions C.LHU, C.LH, and
> > C.SH from the zcb extension to the sbi_misaligned_ldst library.
> >
> > Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> > ---
> >  include/sbi/riscv_encoding.h  |  7 +++++++
> >  lib/sbi/sbi_misaligned_ldst.c | 10 ++++++++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
> > index e74cc0d..3d77e26 100644
> > --- a/include/sbi/riscv_encoding.h
> > +++ b/include/sbi/riscv_encoding.h
> > @@ -837,6 +837,13 @@
> >  #define INSN_MATCH_C_FSWSP             0xe002
> >  #define INSN_MASK_C_FSWSP              0xe003
> >
> > +#define INSN_MATCH_C_LHU               0x8400
> > +#define INSN_MASK_C_LHU                0xfc43
> > +#define INSN_MATCH_C_LH                0x8440
> > +#define INSN_MASK_C_LH                 0xfc43
> > +#define INSN_MATCH_C_SH                0x8c00
> > +#define INSN_MASK_C_SH                 0xfc43
> > +
> >  #define INSN_MASK_WFI                  0xffffff00
> >  #define INSN_MATCH_WFI                 0x10500000
> >
> > diff --git a/lib/sbi/sbi_misaligned_ldst.c b/lib/sbi/sbi_misaligned_ldst.c
> > index aa512de..9ca225a 100644
> > --- a/lib/sbi/sbi_misaligned_ldst.c
> > +++ b/lib/sbi/sbi_misaligned_ldst.c
> > @@ -123,6 +123,13 @@ int sbi_misaligned_load_handler(ulong addr, ulong tval2, ulong tinst,
> >                 len = 4;
> >  #endif
> >  #endif
> > +       } else if ((insn & INSN_MASK_C_LHU) == INSN_MATCH_C_LHU) {
> > +               len = 2;
> > +               insn = RVC_RS2S(insn) << SH_RD;
> > +       } else if ((insn & INSN_MASK_C_LH) == INSN_MATCH_C_LH) {
> > +               len = 2;
> > +               shift = 8 * (sizeof(ulong) - len);
> > +               insn = RVC_RS2S(insn) << SH_RD;
> >         } else {
> >                 uptrap.epc = regs->mepc;
> >                 uptrap.cause = CAUSE_MISALIGNED_LOAD;
> > @@ -237,6 +244,9 @@ int sbi_misaligned_store_handler(ulong addr, ulong tval2, ulong tinst,
> >                 val.data_ulong = GET_F32_RS2C(insn, regs);
> >  #endif
> >  #endif
> > +       } else if ((insn & INSN_MASK_C_SH) == INSN_MATCH_C_SH) {
> > +               len             = 2;
> > +               val.data_ulong = GET_RS2S(insn, regs);
>
> Don't need to handle "c.sb" instruction ??
Hi Anup, thanks for your feedback.

in my opinion, because it's a byte so I don't think we'll encounter
any misaligned issues.

I also wrote the following test program to specifically test c.sb, and
indeed, it doesn't require any exception handler.

/* Check whether there is an address misalignment issue in the store
byte instruction of zcb externsion. */
void checkStoreB(unsigned char* memory, size_t offset, unsigned char
value_byte) {
    asm volatile (
        "c.sb %[value_byte], 0(%[addr])"
        :
        : [value_byte] "r" (value_byte), [addr] "r" (memory + offset)
        : "memory"
    );

    printf("Store (c.sb) operation at address: 0x%lx\n", (unsigned
long)(memory + offset));

    if (memcmp(memory + offset, &value_byte, sizeof(unsigned char)) == 0) {
        printf("Store (c.sb) operation is correct: 0x%X\n", value_byte);
    } else {
        printf("Store (c.sb) operation may have address misalignment
issues.\n");
    }
}

int main() {
    size_t size = 10;
    unsigned char* memory = (unsigned char*)malloc(size);

    if (memory == NULL) {
        fprintf(stderr, "Memory allocation failed.\n");
        return 1;
    }

    size_t offset = 1;

    checkStoreB(memory, offset, 0x42);
...
}

>
> >         } else {
> >                 uptrap.epc = regs->mepc;
> >                 uptrap.cause = CAUSE_MISALIGNED_STORE;
> > --
> > 2.34.1
> >
> >
> > --
> > opensbi mailing list
> > opensbi@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
>
> Regards,
> Anup
Anup Patel March 4, 2024, 7:33 a.m. UTC | #3 
On Mon, Mar 4, 2024 at 11:50 AM Nylon Chen <nylon7717@gmail.com> wrote:
>
> Anup Patel <apatel@ventanamicro.com> 於 2024年3月4日 週一 下午1:12寫道：
> >
> > On Mon, Feb 26, 2024 at 11:27 AM Nylon Chen <nylon.chen@sifive.com> wrote:
> > >
> > > From: "Nylon Chen" <nylon.chen@sifive.com>
> > >
> > > Added exception handling for compressed instructions C.LHU, C.LH, and
> > > C.SH from the zcb extension to the sbi_misaligned_ldst library.
> > >
> > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> > > ---
> > >  include/sbi/riscv_encoding.h  |  7 +++++++
> > >  lib/sbi/sbi_misaligned_ldst.c | 10 ++++++++++
> > >  2 files changed, 17 insertions(+)
> > >
> > > diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
> > > index e74cc0d..3d77e26 100644
> > > --- a/include/sbi/riscv_encoding.h
> > > +++ b/include/sbi/riscv_encoding.h
> > > @@ -837,6 +837,13 @@
> > >  #define INSN_MATCH_C_FSWSP             0xe002
> > >  #define INSN_MASK_C_FSWSP              0xe003
> > >
> > > +#define INSN_MATCH_C_LHU               0x8400
> > > +#define INSN_MASK_C_LHU                0xfc43
> > > +#define INSN_MATCH_C_LH                0x8440
> > > +#define INSN_MASK_C_LH                 0xfc43
> > > +#define INSN_MATCH_C_SH                0x8c00
> > > +#define INSN_MASK_C_SH                 0xfc43
> > > +
> > >  #define INSN_MASK_WFI                  0xffffff00
> > >  #define INSN_MATCH_WFI                 0x10500000
> > >
> > > diff --git a/lib/sbi/sbi_misaligned_ldst.c b/lib/sbi/sbi_misaligned_ldst.c
> > > index aa512de..9ca225a 100644
> > > --- a/lib/sbi/sbi_misaligned_ldst.c
> > > +++ b/lib/sbi/sbi_misaligned_ldst.c
> > > @@ -123,6 +123,13 @@ int sbi_misaligned_load_handler(ulong addr, ulong tval2, ulong tinst,
> > >                 len = 4;
> > >  #endif
> > >  #endif
> > > +       } else if ((insn & INSN_MASK_C_LHU) == INSN_MATCH_C_LHU) {
> > > +               len = 2;
> > > +               insn = RVC_RS2S(insn) << SH_RD;
> > > +       } else if ((insn & INSN_MASK_C_LH) == INSN_MATCH_C_LH) {
> > > +               len = 2;
> > > +               shift = 8 * (sizeof(ulong) - len);
> > > +               insn = RVC_RS2S(insn) << SH_RD;
> > >         } else {
> > >                 uptrap.epc = regs->mepc;
> > >                 uptrap.cause = CAUSE_MISALIGNED_LOAD;
> > > @@ -237,6 +244,9 @@ int sbi_misaligned_store_handler(ulong addr, ulong tval2, ulong tinst,
> > >                 val.data_ulong = GET_F32_RS2C(insn, regs);
> > >  #endif
> > >  #endif
> > > +       } else if ((insn & INSN_MASK_C_SH) == INSN_MATCH_C_SH) {
> > > +               len             = 2;
> > > +               val.data_ulong = GET_RS2S(insn, regs);
> >
> > Don't need to handle "c.sb" instruction ??
> Hi Anup, thanks for your feedback.
>
> in my opinion, because it's a byte so I don't think we'll encounter
> any misaligned issues.
>
> I also wrote the following test program to specifically test c.sb, and
> indeed, it doesn't require any exception handler.

Ahh, yes. We certainly don't need for the misaligned load/store handling.

There is a separate series from Bo Gan where we are re-purposing the
misaligned load/store handling for emulating load/store upon access
fault. I got mixed-up between two separate patches.

Regards,
Anup

>
> /* Check whether there is an address misalignment issue in the store
> byte instruction of zcb externsion. */
> void checkStoreB(unsigned char* memory, size_t offset, unsigned char
> value_byte) {
>     asm volatile (
>         "c.sb %[value_byte], 0(%[addr])"
>         :
>         : [value_byte] "r" (value_byte), [addr] "r" (memory + offset)
>         : "memory"
>     );
>
>     printf("Store (c.sb) operation at address: 0x%lx\n", (unsigned
> long)(memory + offset));
>
>     if (memcmp(memory + offset, &value_byte, sizeof(unsigned char)) == 0) {
>         printf("Store (c.sb) operation is correct: 0x%X\n", value_byte);
>     } else {
>         printf("Store (c.sb) operation may have address misalignment
> issues.\n");
>     }
> }
>
> int main() {
>     size_t size = 10;
>     unsigned char* memory = (unsigned char*)malloc(size);
>
>     if (memory == NULL) {
>         fprintf(stderr, "Memory allocation failed.\n");
>         return 1;
>     }
>
>     size_t offset = 1;
>
>     checkStoreB(memory, offset, 0x42);
> ...
> }
>
> >
> > >         } else {
> > >                 uptrap.epc = regs->mepc;
> > >                 uptrap.cause = CAUSE_MISALIGNED_STORE;
> > > --
> > > 2.34.1
> > >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
> >
> > Regards,
> > Anup
>
>
>
> --
>
> ==========================================
> sifive system software  team 陳伯綸
>
> Cell phone:  0989057854
> E-mail: nylon7717@gmail.com
> ==========================================
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
Anup Patel March 6, 2024, 5:46 a.m. UTC | #4 
On Mon, Feb 26, 2024 at 11:26 AM Nylon Chen <nylon.chen@sifive.com> wrote:
>
> From: "Nylon Chen" <nylon.chen@sifive.com>
>
> Added exception handling for compressed instructions C.LHU, C.LH, and
> C.SH from the zcb extension to the sbi_misaligned_ldst library.
>
> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>

LGTM.

Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
>  include/sbi/riscv_encoding.h  |  7 +++++++
>  lib/sbi/sbi_misaligned_ldst.c | 10 ++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
> index e74cc0d..3d77e26 100644
> --- a/include/sbi/riscv_encoding.h
> +++ b/include/sbi/riscv_encoding.h
> @@ -837,6 +837,13 @@
>  #define INSN_MATCH_C_FSWSP             0xe002
>  #define INSN_MASK_C_FSWSP              0xe003
>
> +#define INSN_MATCH_C_LHU               0x8400
> +#define INSN_MASK_C_LHU                0xfc43
> +#define INSN_MATCH_C_LH                0x8440
> +#define INSN_MASK_C_LH                 0xfc43
> +#define INSN_MATCH_C_SH                0x8c00
> +#define INSN_MASK_C_SH                 0xfc43
> +
>  #define INSN_MASK_WFI                  0xffffff00
>  #define INSN_MATCH_WFI                 0x10500000
>
> diff --git a/lib/sbi/sbi_misaligned_ldst.c b/lib/sbi/sbi_misaligned_ldst.c
> index aa512de..9ca225a 100644
> --- a/lib/sbi/sbi_misaligned_ldst.c
> +++ b/lib/sbi/sbi_misaligned_ldst.c
> @@ -123,6 +123,13 @@ int sbi_misaligned_load_handler(ulong addr, ulong tval2, ulong tinst,
>                 len = 4;
>  #endif
>  #endif
> +       } else if ((insn & INSN_MASK_C_LHU) == INSN_MATCH_C_LHU) {
> +               len = 2;
> +               insn = RVC_RS2S(insn) << SH_RD;
> +       } else if ((insn & INSN_MASK_C_LH) == INSN_MATCH_C_LH) {
> +               len = 2;
> +               shift = 8 * (sizeof(ulong) - len);
> +               insn = RVC_RS2S(insn) << SH_RD;
>         } else {
>                 uptrap.epc = regs->mepc;
>                 uptrap.cause = CAUSE_MISALIGNED_LOAD;
> @@ -237,6 +244,9 @@ int sbi_misaligned_store_handler(ulong addr, ulong tval2, ulong tinst,
>                 val.data_ulong = GET_F32_RS2C(insn, regs);
>  #endif
>  #endif
> +       } else if ((insn & INSN_MASK_C_SH) == INSN_MATCH_C_SH) {
> +               len             = 2;
> +               val.data_ulong = GET_RS2S(insn, regs);
>         } else {
>                 uptrap.epc = regs->mepc;
>                 uptrap.cause = CAUSE_MISALIGNED_STORE;
> --
> 2.34.1
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
diff mbox series

Patch

diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
index e74cc0d..3d77e26 100644
--- a/include/sbi/riscv_encoding.h
+++ b/include/sbi/riscv_encoding.h
@@ -837,6 +837,13 @@ 
 #define INSN_MATCH_C_FSWSP		0xe002
 #define INSN_MASK_C_FSWSP		0xe003
 
+#define INSN_MATCH_C_LHU		0x8400
+#define INSN_MASK_C_LHU		0xfc43
+#define INSN_MATCH_C_LH		0x8440
+#define INSN_MASK_C_LH			0xfc43
+#define INSN_MATCH_C_SH		0x8c00
+#define INSN_MASK_C_SH			0xfc43
+
 #define INSN_MASK_WFI			0xffffff00
 #define INSN_MATCH_WFI			0x10500000
 
diff --git a/lib/sbi/sbi_misaligned_ldst.c b/lib/sbi/sbi_misaligned_ldst.c
index aa512de..9ca225a 100644
--- a/lib/sbi/sbi_misaligned_ldst.c
+++ b/lib/sbi/sbi_misaligned_ldst.c
@@ -123,6 +123,13 @@  int sbi_misaligned_load_handler(ulong addr, ulong tval2, ulong tinst,
 		len = 4;
 #endif
 #endif
+	} else if ((insn & INSN_MASK_C_LHU) == INSN_MATCH_C_LHU) {
+		len = 2;
+		insn = RVC_RS2S(insn) << SH_RD;
+	} else if ((insn & INSN_MASK_C_LH) == INSN_MATCH_C_LH) {
+		len = 2;
+		shift = 8 * (sizeof(ulong) - len);
+		insn = RVC_RS2S(insn) << SH_RD;
 	} else {
 		uptrap.epc = regs->mepc;
 		uptrap.cause = CAUSE_MISALIGNED_LOAD;
@@ -237,6 +244,9 @@  int sbi_misaligned_store_handler(ulong addr, ulong tval2, ulong tinst,
 		val.data_ulong = GET_F32_RS2C(insn, regs);
 #endif
 #endif
+	} else if ((insn & INSN_MASK_C_SH) == INSN_MATCH_C_SH) {
+		len		= 2;
+		val.data_ulong = GET_RS2S(insn, regs);
 	} else {
 		uptrap.epc = regs->mepc;
 		uptrap.cause = CAUSE_MISALIGNED_STORE;