mbox series

[0/3] Fixes for tval/tinst when using sbi_trap_redirect()

Message ID TYYP286MB1439F38753D033042F83C8E0C6A79@TYYP286MB1439.JPNP286.PROD.OUTLOOK.COM
Headers show
Series Fixes for tval/tinst when using sbi_trap_redirect() | expand

Message

dramforever June 9, 2022, 7:06 a.m. UTC 
I went through usages of sbi_trap_redirect() and saw some cases where
after redirecting, stval/htinst could be set to incorrect values. I
tried fixing what I noticed.

Note that these patches are *UNTESTED*.

- For the htinst fixes, no current implementation (QEMU or Spike) of the
  hypervisor extension writes mtinst/htinst to non-zero values, so the
  code paths are unused.
- The sbi_get_insn() fix requires a rare occasion where the instruction
  is fetched normally then unmapped from memory before OpenSBI can load
  it again, and I have not been reproduce.

Please bear in mind when reviewing.

dramforever (3):
  include: sbi: Add mtinst/htinst psuedoinstructions
  lib: sbi: Fixup tinst for exceptions in sbi_misaligned_*()
  lib: sbi: Fix tval and tinst for sbi_get_insn()

 include/sbi/riscv_encoding.h  | 20 ++++++++++++++++++++
 lib/sbi/sbi_misaligned_ldst.c | 16 ++++++++++++++++
 lib/sbi/sbi_unpriv.c          |  8 +++++---
 3 files changed, 41 insertions(+), 3 deletions(-)

Comments

Anup Patel June 9, 2022, 8:51 a.m. UTC | #1 
On Thu, Jun 9, 2022 at 12:37 PM dramforever <dramforever@live.com> wrote:
>
> I went through usages of sbi_trap_redirect() and saw some cases where
> after redirecting, stval/htinst could be set to incorrect values. I
> tried fixing what I noticed.

Overall, most fixes are in the right direction. I will review it soon.

>
> Note that these patches are *UNTESTED*.
>
> - For the htinst fixes, no current implementation (QEMU or Spike) of the
>   hypervisor extension writes mtinst/htinst to non-zero values, so the
>   code paths are unused.

Can you test this series with QEMU nested virtualization fixes which
include mtinst/htinst emulation ?
https://lore.kernel.org/all/20220609033011.752714-1-apatel@ventanamicro.com/

> - The sbi_get_insn() fix requires a rare occasion where the instruction
>   is fetched normally then unmapped from memory before OpenSBI can load
>   it again, and I have not been reproduce.

Yes, this case can happen but it is very rare and difficult to reproduce.

Regards,
Anup

>
> Please bear in mind when reviewing.
>
> dramforever (3):
>   include: sbi: Add mtinst/htinst psuedoinstructions
>   lib: sbi: Fixup tinst for exceptions in sbi_misaligned_*()
>   lib: sbi: Fix tval and tinst for sbi_get_insn()
>
>  include/sbi/riscv_encoding.h  | 20 ++++++++++++++++++++
>  lib/sbi/sbi_misaligned_ldst.c | 16 ++++++++++++++++
>  lib/sbi/sbi_unpriv.c          |  8 +++++---
>  3 files changed, 41 insertions(+), 3 deletions(-)
>
> --
> 2.36.0
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
dramforever June 9, 2022, 12:06 p.m. UTC | #2 
>> Note that these patches are *UNTESTED*.
>>
>> - For the htinst fixes, no current implementation (QEMU or Spike) of the
>>   hypervisor extension writes mtinst/htinst to non-zero values, so the
>>   code paths are unused.
> Can you test this series with QEMU nested virtualization fixes which
> include mtinst/htinst emulation ?
> https://lore.kernel.org/all/20220609033011.752714-1-apatel@ventanamicro.com/
I just realized that QEMU does not raise misaligned exceptions, so even with
these patches we still couldn't test the sbi_misaligned_*() patch. Do you have
suggestions on how we can test misaligned load/store emulation?
Anup Patel June 9, 2022, 12:13 p.m. UTC | #3 
On Thu, Jun 9, 2022 at 5:36 PM dram <dramforever@live.com> wrote:
>
>
> >> Note that these patches are *UNTESTED*.
> >>
> >> - For the htinst fixes, no current implementation (QEMU or Spike) of the
> >>   hypervisor extension writes mtinst/htinst to non-zero values, so the
> >>   code paths are unused.
> > Can you test this series with QEMU nested virtualization fixes which
> > include mtinst/htinst emulation ?
> > https://lore.kernel.org/all/20220609033011.752714-1-apatel@ventanamicro.com/
> I just realized that QEMU does not raise misaligned exceptions, so even with
> these patches we still couldn't test the sbi_misaligned_*() patch. Do you have
> suggestions on how we can test misaligned load/store emulation?

Spike does raise misaligned load/store exceptions so we need to add
htinst/mtinst support in Spike to test this.

Regards,
Anup

>
>