Re: [External] Re: [PATCH] RISC-V: KVM: Delegate illegal instruction fault

[Xu Lu]

Hi Radim,

On Mon, Jun 23, 2025 at 6:04 PM Radim Krčmář <rkrcmar@xxxxxxxxxxxxxxxx> wrote:
>
> 2025-06-22T18:11:49+08:00, Xu Lu <luxu.kernel@xxxxxxxxxxxxx>:
> > Hi Radim,
> >
> > On Fri, Jun 20, 2025 at 8:04 PM Radim Krčmář <rkrcmar@xxxxxxxxxxxxxxxx> wrote:
> >>
> >> 2025-06-20T17:17:20+08:00, Xu Lu <luxu.kernel@xxxxxxxxxxxxx>:
> >> > Delegate illegal instruction fault to VS mode in default to avoid such
> >> > exceptions being trapped to HS and redirected back to VS.
> >> >
> >> > Signed-off-by: Xu Lu <luxu.kernel@xxxxxxxxxxxxx>
> >> > ---
> >> > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> >> > @@ -48,6 +48,7 @@
> >> > +                                      BIT(EXC_INST_ILLEGAL)    | \
> >>
> >> You should also remove the dead code in kvm_riscv_vcpu_exit.
> >
> > I only want to delegate it by default. And KVM may still want to
> > delegate different exceptions for different VMs like what it does for
> > EXC_BREAKPOINT.
>
> (I think we could easily reintroduce the code if KVM wants to do that in
>  the future.  I also think that it's bad that this patch is doing an
>  observable change without userspace involvement -- the counting of KVM
>  SBI PMU events, but others will probably disagree with me on this.)
>
> >                 So maybe it is better to reserve these codes?
>
> Possibly, the current is acceptable if you have considered the
> implications on PMU events.

So maybe it comes back to our discussion on the difference between vs
insn fault and illegal insn fault again~ In my personal opinion, it
seems to be a waste of CPU resources to trap illegal instruction to
HS-mode hypervisor, which does nothing but redirect it back to VS-mode
guest kernel. I think it is OK (and maybe it should) to record 0
illegal instruction exits in KVM PMU. If someone wants illegal insn to
trigger an vcpu exit, then an ioctl can be provided to remove the
delegation like what KVM_SET_GUEST_DEBUG does.

>
> >> And why not delegate the others as well?
> >> (EXC_LOAD_MISALIGNED, EXC_STORE_MISALIGNED, EXC_LOAD_ACCESS,
> >>  EXC_STORE_ACCESS, and EXC_INST_ACCESS.)
> >
> > Thanks for the reminder. I will have a test and resend the patch if it works.
>
> The misaligned exceptions are already being worked on, so don't waste
> your time on them, sorry.

Thanks for the reminder too. I did not consider this before. I will
leave the MISALIGNED faults alone.

Best Regards,

Xu Lu