Re: [PATCH v2 25/39] xen/riscv: add guest load emulation for trapped MMIO accesses

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

> Implement emulate_load() on top of the decoding interface introduced by
> the previous patch: fetch the trapped instruction, decode it, dispatch
> the access to a registered MMIO handler via do_mmio(), write the result
> back into the destination register and step over the instruction.
> 
> Xen dispatches MMIO synchronously to an in-hypervisor handler, so unlike
> KVM RISC-V there is no userspace exit/return step and no equivalent of
> the kvm_io_bus_read() / KVM_EXIT_MMIO / kvm_riscv_vcpu_mmio_return()
> split; the result is consumed in place.
> 
> Sign extension is done here rather than in the handlers: a signed load
> is normalized by a shift pair, so a handler need only report the value
> it read.
> 
> At the moment vINTC is the only backend registered with the MMIO
> dispatch, so in practice this only covers vINTC traps. An access which
> no handler claims currently crashes the domain; injecting an access
> fault into the guest instead is left for later.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>
> diff --git a/xen/arch/riscv/emulate.c b/xen/arch/riscv/emulate.c
> index 81a50643a5..e52f285180 100644
> --- a/xen/arch/riscv/emulate.c
> +++ b/xen/arch/riscv/emulate.c
> @@ -5,7 +5,6 @@
>   */
>  
>  #include <xen/bug.h>
> -#include <xen/compiler.h>
>  #include <xen/errno.h>
>  #include <xen/sched.h>
>  #include <xen/types.h>
> @@ -15,6 +14,7 @@
>  #include <asm/current.h>
>  #include <asm/emulate.h>
>  #include <asm/guest_access.h>
> +#include <asm/mmio.h>
>  #include <asm/processor.h>
>  #include <asm/riscv_encoding.h>
>  #include <asm/traps.h>
> @@ -65,8 +65,7 @@ static bool is_load_guest_page_fault(unsigned long scause)
>      return scause == CAUSE_LOAD_GUEST_PAGE_FAULT;
>  }
>  
> -static __maybe_unused void advance_pc(struct cpu_user_regs *regs,
> -                                      unsigned int step)
> +static void advance_pc(struct cpu_user_regs *regs, unsigned int step)
>  {
>      regs->sepc += step;
>  }
> @@ -88,7 +87,7 @@ static __maybe_unused void advance_pc(struct cpu_user_regs 
> *regs,
>   * IS_ENABLED() can't be used here as HSTATUS_VSXL is defined for
>   * __riscv_xlen == 64 only, the field not existing on RV32 in the first 
> place.
>   */
> -static __maybe_unused unsigned int guest_xlen(const struct cpu_user_regs 
> *regs)
> +static unsigned int guest_xlen(const struct cpu_user_regs *regs)
>  {
>  #ifdef CONFIG_RISCV_32
>      return 32;
> @@ -179,8 +178,7 @@ static void resolve_faulting_gpa(struct guest_fault *gf)
>   * Relies on x0..x31 being laid out at the start of struct cpu_user_regs in
>   * architectural register-number order; see the comment there.
>   */
> -static __maybe_unused unsigned long *guest_gpr(struct cpu_user_regs *regs,
> -                                               unsigned int reg)
Could we unified this function with regs_get_gpr()?

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>