Re: [PATCH v1 2/6] xen/riscv: implement copy_to_guest_phys()

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 2/16/26 3:57 PM, Jan Beulich wrote:
> 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.

As we discussed in another patch thread, IPA isn't really used for RISC-V
and I will rename it to GPA.

>
> Also, don't you prefer to use BIT() everywhere else?

Yes, BIT() would be better for consistency.


>
>> +#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.

Yes, for this once cases it will be used vcpu as an argument passed by "copy_info_t info".
I will add the comment above suggested below BUG_ON(linear).

Btw, I think it makes sense to change linear to GVA to be more close to RISC-V spec?

>
> 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.

I will apply your changes from suggested for Arm patch (Arm: tighten
translate_get_page()) so write will be used and also no extra updates will
be needed here.


>
>> +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.

Agree, unsigned int should be enough.

>
>> +        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?

Sure, I will do that.

Thanks.

~ Oleksii