| Message ID | dae753618491b2a6e42f7ed3f24190d0dc13fe3f.1740754166.git.Slavisa.Petrovic@rt-rk.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/riscv: copy_to_guest/copy_from_guest functionality. | expand |
On 28/02/2025 2:59 pm, Milan Djokic wrote: > From: Slavisa Petrovic <Slavisa.Petrovic@rt-rk.com> > > 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@rt-rk.com> > Signed-off-by: Slavisa Petrovic <Slavisa.Petrovic@rt-rk.com> > --- > 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. Several things. First, it's probably worth setting you up with Gitlab access, seeing as that's the first step of RISC-V CI. Second, where can I read about the semantics of h{l,s}v? That looks alarmingly like a virtual address, and there's a world supply of corner cases that can come with it, including incorrect translations. Also, I very desperately want RISC-V (and PPC) not to inherit 2-decade-old x86-ISMs which we're currently trying to remove, because starting without them is 10x easier than to fix them after the fact. > 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: For assembly code, please use the linkage macros. See 7015f337a217 as an example. However, as far as I can tell, the only interesting thing h{s,l}v.b, at which point a simple piece of inline asm is surely easier, and would simplify guestcopy.c too. Exception table entries are perfectly easy to do in inline asm. See _ASM_EXTABLE() in x86 for an example. ~Andrew
On 2/28/25 3:59 PM, Milan Djokic wrote: > From: Slavisa Petrovic<Slavisa.Petrovic@rt-rk.com> > > 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@rt-rk.com> > Signed-off-by: Slavisa Petrovic<Slavisa.Petrovic@rt-rk.com> > --- > Tested on qemu with xcp-ng latest branchhttps://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. > > $(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) > 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@gmail.com/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. > > 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()? ~ Oleksii > +} > + > +unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned len) > +{ > + return copy_from((void *)to, (vaddr_t)from, len); > +} > diff --git a/xen/arch/riscv/include/asm/guest_access.h b/xen/arch/riscv/include/asm/guest_access.h > index 7cd51fbbde..4fcf3fbf68 100644 > --- a/xen/arch/riscv/include/asm/guest_access.h > +++ b/xen/arch/riscv/include/asm/guest_access.h > @@ -2,6 +2,11 @@ > #ifndef ASM__RISCV__GUEST_ACCESS_H > #define ASM__RISCV__GUEST_ACCESS_H > > +#include <xen/types.h> > + > +extern size_t _copy_to(uint64_t dest, const void *src, size_t len); > +extern size_t _copy_from(void *dest, uint64_t src, size_t len); > + > unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len); > unsigned long raw_copy_from_guest(void *to, const void *from, unsigned len); > unsigned long raw_clear_guest(void *to, unsigned int len);
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 $(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 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)) 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; +} + +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); +} + +unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned len) +{ + return copy_from((void *)to, (vaddr_t)from, len); +} diff --git a/xen/arch/riscv/include/asm/guest_access.h b/xen/arch/riscv/include/asm/guest_access.h index 7cd51fbbde..4fcf3fbf68 100644 --- a/xen/arch/riscv/include/asm/guest_access.h +++ b/xen/arch/riscv/include/asm/guest_access.h @@ -2,6 +2,11 @@ #ifndef ASM__RISCV__GUEST_ACCESS_H #define ASM__RISCV__GUEST_ACCESS_H +#include <xen/types.h> + +extern size_t _copy_to(uint64_t dest, const void *src, size_t len); +extern size_t _copy_from(void *dest, uint64_t src, size_t len); + unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len); unsigned long raw_copy_from_guest(void *to, const void *from, unsigned len); unsigned long raw_clear_guest(void *to, unsigned int len);