Re: [PATCH] xen/riscv: copy_to_guest/copy_from_guest functionality.

[Milan Đokić <milandjokic1995@xxxxxxxxx>]

Hello Oleksii,
On Fri, Feb 28, 2025 at 5:03 PM Oleksii Kurochko
<oleksii.kurochko@xxxxxxxxx> wrote:
>
>
> On 2/28/25 3:59 PM, Milan Djokic wrote:
>
> From: Slavisa Petrovic <Slavisa.Petrovic@xxxxxxxxx>
>
> This patch implements copy_to_guest/copy_from_guest functions for RISC-V.
> These functions are designed to facilitate data exchange between guest and 
> hypervisor.
>
> Signed-off-by: Milan Djokic <Milan.Djokic@xxxxxxxxx>
> Signed-off-by: Slavisa Petrovic <Slavisa.Petrovic@xxxxxxxxx>
> ---
> Tested on qemu with xcp-ng latest branch 
> https://gitlab.com/xen-project/people/olkur/xen/-/merge_requests/6
> Full description on how to setup test environment can be found in attached PR 
> link (Linux kernel patching to support copy_from_guest/copy_to_guest for 
> RISC-V).
> Linux kernel patch shall be upstreamed after these changes are merged.
> ---
>  xen/arch/riscv/Makefile                   |  1 +
>  xen/arch/riscv/addr_translation.S         | 63 +++++++++++++++++++++++
>  xen/arch/riscv/arch.mk                    |  6 ++-
>  xen/arch/riscv/guestcopy.c                | 43 ++++++++++++++++
>  xen/arch/riscv/include/asm/guest_access.h |  5 ++
>  5 files changed, 117 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/riscv/addr_translation.S
>  create mode 100644 xen/arch/riscv/guestcopy.c
>
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index a5eb2aed4b..1bd61cc993 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -10,6 +10,7 @@ obj-y += smp.o
>  obj-y += stubs.o
>  obj-y += traps.o
>  obj-y += vm_event.o
> +obj-y += addr_translation.o
>
> It should be sorted in alphabetical order.
>
Will be updated in next version

>
>  $(TARGET): $(TARGET)-syms
>   $(OBJCOPY) -O binary -S $< $@
> diff --git a/xen/arch/riscv/addr_translation.S 
> b/xen/arch/riscv/addr_translation.S
> new file mode 100644
> index 0000000000..7e94774b47
> --- /dev/null
> +++ b/xen/arch/riscv/addr_translation.S
> @@ -0,0 +1,63 @@
> +#include <asm/riscv_encoding.h>
> +#include <asm/asm.h>
> +
> +.macro add_extable lbl
> +.pushsection .extable, "a"     /* Start .extable section */
> +.balign      8                 /* Align to 8 bytes */
> +.quad        \lbl              /* Add label address to extable */
> +.popsection                    /* End section */
> +.endm
> +
> +.section .text
> +
> +/*
> + * size_t _copy_to(uint64_t dest, const void *src, size_t len)
> + *
> + * a0 - dest
> + * a1 - src
> + * a2 - len
> + *
> + */
> +
> +.global _copy_to
> +_copy_to:
> +    la    t0, copy_done             /* Load address of return label */
> +    mv    t2, zero                  /* Initialize counter to zero */
> +loop_check:
> +    beq   t2, a2, copy_done         /* Check if all bytes are copied */
> +    lb    t3, (a1)                  /* Load byte from source (guest address) 
> */
> +store_byte:
> +    hsv.b t3, (a0)                  /* Store byte to destination (host 
> address) */
> +    add_extable store_byte          /* Add exception table for this location 
> */
> +    addi  a0, a0, 1                 /* Increment destination pointer */
> +    addi  a1, a1, 1                 /* Increment source pointer */
> +    addi  t2, t2, 1                 /* Increment byte counter */
> +    j     loop_check                /* Loop back if not done */
> +
> +/*
> + * size_t _copy_from(void *dest, uint64_t src, size_t len)
> + *
> + * a0 - dest
> + * a1 - src
> + * a2 - len
> + *
> + */
> +
> +.global _copy_from
> +_copy_from:
> +    la    t0, copy_done
> +    mv    t2, zero
> +loop_check_from:
> +    beq   t2, a2, copy_done
> +load_byte:
> +    hlv.b t3, (a1)                  /* Load byte from guest address */
> +    add_extable load_byte
> +    sb    t3, (a0)
> +    addi  a0, a0, 1
> +    addi  a1, a1, 1
> +    addi  t2, t2, 1
> +    j     loop_check_from
> +
> +copy_done:
> +    mv    a0, t2                    /* Return number of bytes copied */
> +    ret
>
> Can't we done this functions fully in C? (it doesn't something mandatory)
>
Yes, we have changed this part to be completely handled in C

> diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk
> index 17827c302c..f4098f5b5e 100644
> --- a/xen/arch/riscv/arch.mk
> +++ b/xen/arch/riscv/arch.mk
> @@ -23,13 +23,17 @@ $(eval $(1) := \
>   $(call as-insn,$(CC) $(riscv-generic-flags)_$(1),$(value $(1)-insn),_$(1)))
>  endef
>
> +h-extension-name := $(call cc-ifversion,-lt,1301, hh, h)
> +$(h-extension-name)-insn := "hfence.gvma"
> +$(call check-extension,$(h-extension-name))
> +
>  zbb-insn := "andn t0$(comma)t0$(comma)t0"
>  $(call check-extension,zbb)
>
>  zihintpause-insn := "pause"
>  $(call check-extension,zihintpause)
>
> -extensions := $(zbb) $(zihintpause)
> +extensions := $(value $(h-extension-name)) $(zbb) $(zihintpause)
>
>  extensions := $(subst $(space),,$(extensions))
>
> This patch should take into account another one patch series ( 
> https://lore.kernel.org/xen-devel/cover.1740071755.git.oleksii.kurochko@xxxxxxxxx/T/#t)
> update for which I am going to sent today.
>
> Also, these changes would be better to move into separate commit with 
> explanation why what is so specific with 1301 and why it is needed to 
> introduce
> 'hh'.
> I believe that these changes were taken based on my patch: 
> https://gitlab.com/xen-project/people/olkur/xen/-/commit/154f75e943f1668dbf2c7be0f0fdff5b30132e89
> Probably it will make sense just to get it and rebase on top of mentioned 
> above patch series.
>
Yes, it is based on your patch. Sorry, I was not aware that you
already have an active patch series which contains this part.
In that case we'll wait for your patch series to be merged first. And
we'll split it into 2 commits where first one will only introduce h
extension handling in arch.mk and the second one with
copy_from/to_guest functionallity

>
> diff --git a/xen/arch/riscv/guestcopy.c b/xen/arch/riscv/guestcopy.c
> new file mode 100644
> index 0000000000..0325956845
> --- /dev/null
> +++ b/xen/arch/riscv/guestcopy.c
> @@ -0,0 +1,43 @@
> +#include <xen/bug.h>
> +#include <xen/domain_page.h>
> +#include <xen/errno.h>
> +#include <xen/sched.h>
> +
> +#include <asm/csr.h>
> +#include <asm/guest_access.h>
> +#include <asm/system.h>
> +#include <asm/traps.h>
> +
> +unsigned long copy_to(uint64_t dest, void* src, size_t len)
> +{
> +    size_t bytes;
> +
> +    bytes = _copy_to(dest, src, len);
> +
> +    if (bytes == len)
> +        return 0;
> +    else
> +        return -ENOSYS;
> +}
>
> Why do we have a different prototypes with copy_from() below? If we will have
> void *dest then ...
>
> +
> +unsigned long copy_from(void *dest, uint64_t src, size_t len)
> +{
> +    size_t bytes;
> +
> +    bytes = _copy_from(dest, src, len);
> +
> +    if (bytes == len)
> +        return 0;
> +    else
> +        return -ENOSYS;
> +}
> +
> +unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
> +{
> +    return copy_to((vaddr_t)to, (void *)from, len);
>
> ... we won't need cast for `to` and wo we really need cast for copy_to()? Why 
> `const` is
> dropped when passed to copy_to()?
>
These are just mismatches (prototype and const drop), fixed for the
next version.

BR,
Milan