On 12.02.2026 17:21, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/guestcopy.c
> @@ -0,0 +1,112 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/domain_page.h>
> +#include <xen/page-size.h>
> +#include <xen/sched.h>
> +#include <xen/string.h>
> +
> +#include <asm/guest_access.h>
> +
> +#define COPY_from_guest (0U << 0)
> +#define COPY_to_guest (1U << 0)
> +#define COPY_ipa (0U << 1)
Like already asked elsewhere - is "ipa" a term commonly in use on RISC-V?
To me it's Arm terminology, which you don't want to copy as is.
Also, don't you prefer to use BIT() everywhere else?
> +#define COPY_linear (1U << 1)
> +
> +typedef union
> +{
> + struct
> + {
> + struct vcpu *v;
> + } gva;
> +
> + struct
> + {
> + struct domain *d;
> + } gpa;
> +} copy_info_t;
> +
> +#define GVA_INFO(vcpu) ((copy_info_t) { .gva = { vcpu } })
> +#define GPA_INFO(domain) ((copy_info_t) { .gpa = { domain } })
> +
> +static struct page_info *translate_get_page(copy_info_t info, uint64_t addr,
The caller has to pass in a domain here. I therefore recommend against
use of copy_info_t for this function. Or wait, this is misleading, as
the consuming part ...
> + bool linear, bool write)
> +{
> + p2m_type_t p2mt;
> + struct page_info *page;
> +
> + if ( linear )
> + BUG_ON("unimplemeted\n");
... of "linear" is missing here.
In any event, this one please shorter as:
BUG_ON(linear);
> + page = get_page_from_gfn(info.gpa.d, paddr_to_pfn(addr), &p2mt,
> P2M_ALLOC);
> +
> + if ( !page )
> + return NULL;
> +
> + if ( !p2m_is_ram(p2mt) )
> + {
> + put_page(page);
> + return NULL;
> + }
> +
> + return page;
> +}
The "write" function parameter also is unused, but there's no BUG_ON() for
that one? Imo the p2m_is_ram() check isn't thorough enough (on the Arm
original): p2m_ram_ro shouldn't be allowed when "write" is true. As soon
as you gain p2m_ram_ro on RISC-V, things will need updating here as well.
Perhaps best to leave a note.
> +static unsigned long copy_guest(void *buf, uint64_t addr, unsigned int len,
> + copy_info_t info, unsigned int flags)
Why an "unsigned long" return value when ...
> +{
> + unsigned int offset = PAGE_OFFSET(addr);
> +
> + BUILD_BUG_ON((sizeof(addr)) < sizeof(vaddr_t));
> + BUILD_BUG_ON((sizeof(addr)) < sizeof(paddr_t));
> +
> + while ( len )
> + {
> + void *p;
> + unsigned int size = min(len, (unsigned int)PAGE_SIZE - offset);
> + struct page_info *page;
> +
> + page = translate_get_page(info, addr, flags & COPY_linear,
> + flags & COPY_to_guest);
> + if ( page == NULL )
> + return len;
... only an "unsigned int" (or 0 further down) is returned? Same
question for copy_to_guest_phys() below then.
> + p = __map_domain_page(page);
> + p += offset;
> + if ( flags & COPY_to_guest )
> + {
> + /*
> + * buf will be NULL when the caller request to zero the
> + * guest memory.
> + */
> + if ( buf )
> + memcpy(p, buf, size);
> + else
> + memset(p, 0, size);
> + }
> + else
> + memcpy(buf, p, size);
> +
> + unmap_domain_page(p - offset);
> + put_page(page);
> + len -= size;
> + buf += size;
> + addr += size;
> +
> + /*
> + * After the first iteration, guest virtual address is correctly
> + * aligned to PAGE_SIZE.
> + */
> + offset = 0;
> + }
> +
> + return 0;
> +}
> +
> +unsigned long copy_to_guest_phys(struct domain *d,
> + paddr_t gpa,
> + void *buf,
> + unsigned int len)
May I suggest to make good use of line length, just like how copy_guest()
does?
Jan
