Re: [XEN][PATCH v4] xen/x86: guest_access: optimize raw_x_guest() for PV and HVM combinations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.11.2025 20:36, Grygorii Strashko wrote:
> Hi Jan
> 
> On 18.11.25 15:45, Jan Beulich wrote:
>> On 18.11.2025 14:08, Grygorii Strashko wrote:
>>> On 17.11.25 18:43, Jan Beulich wrote:
>>>> On 14.11.2025 15:01, Grygorii Strashko wrote:
>>>>> --- a/xen/arch/x86/pv/Makefile
>>>>> +++ b/xen/arch/x86/pv/Makefile
>>>>> @@ -14,6 +14,10 @@ obj-y += ro-page-fault.o
>>>>>    obj-$(CONFIG_PV_SHIM) += shim.o
>>>>>    obj-$(CONFIG_TRACEBUFFER) += trace.o
>>>>>    obj-y += traps.o
>>>>> +obj-$(CONFIG_PV) += usercopy.o
>>>>
>>>> Just obj-y with the movement.
>>>>
>>>> However, is the movement (and was the adding of $(CONFIG_PV) in the earlier
>>>> version) actually correct? The file also produces 
>>>> copy_{from,to}_unsafe_ll(),
>>>> which aren't PV-specific. This may be only a latent issue right now, as we
>>>> have only a single use site of copy_from_unsafe(), but those functions need
>>>> to remain available. (We may want to arrange for them to be removed when
>>>> linking, as long as they're not referenced. But that's a separate topic.)
>>>
>>> It is confusing that none of build cfg combinations have failed
>>> (HVM=y PV=n, HVM=n PV=n) :(
>>>
>>> copy_to_unsafe_ll()
>>> - called from copy_to_unsafe()
>>> - copy_to_unsafe() has no users (unreachable, MISRA 2.1?)
>>>
>>> copy_from_unsafe_ll()
>>> - called from copy_from_unsafe()
>>> - copy_from_unsafe() called from one place do_invalid_op() with
>>>     copy_from_unsafe(,, n = sizeof(bug_insn)).
>>>     Due to __builtin_constant_p(n) check the copy_from_unsafe() call
>>>     optimized by compiler to
>>>     get_unsafe_size(*(uint16_t *)to, from, 2, UA_DROP, ret, 2);
>>>
>>> as result copy_from_unsafe_ll() is unreachable also (?).
>>
>> Yes, these likely all want to become library-like, so they are linked in only
>> when actually referenced.
>>
>>> If those function are not subject to be removed, the
>>>    usercopy.c can't be moved in "x86/pv", Right?
>>
>> That's my take, yes.
>>
>>> Making copy_{from,to}_unsafe_ll() available for !PV means
>>> rewriting usercopy.c in some way, Right?
>>
>> "Re-writing" is probably too much, but some adjustments would be needed if
>> you want to keep the "unsafe" functions but compile out the "guest" ones.
>> It may be possible to compile the file twice, once from x86/pv/ and once
>> from x86/, replacing the self-#include near the bottom of the file. The
>> former would then produce the "guest" functions, the latter the "unsafe"
>> ones.
> 
> Below is the difference I came up with, will it work?

That'll be on you to make sure, but ...

> --- /dev/null
> +++ b/xen/arch/x86/usercopy.c
> @@ -0,0 +1,77 @@
> +/*
> + * User address space access functions.
> + *
> + * Copyright 1997 Andi Kleen <ak@xxxxxx>
> + * Copyright 1997 Linus Torvalds
> + * Copyright 2002 Andi Kleen <ak@xxxxxxx>
> + */
> +
> +#include <xen/lib.h>
> +#include <xen/sched.h>
> +#include <asm/uaccess.h>
> +
> +# define GUARD UA_DROP
> +# define copy_to_guest_ll copy_to_unsafe_ll
> +# define copy_from_guest_ll copy_from_unsafe_ll
> +# undef __user
> +# define __user
> +
> +unsigned int copy_to_guest_ll(void __user *to, const void *from, unsigned 
> int n)
> +{
> +    GUARD(unsigned dummy);
> +
> +    stac();
> +    asm_inline volatile (
> +        GUARD(
> +        "    guest_access_mask_ptr %[to], %q[scratch1], %q[scratch2]\n"
> +        )
> +        "1:  rep movsb\n"
> +        "2:\n"
> +        _ASM_EXTABLE(1b, 2b)
> +        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from)
> +          GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy))
> +        :: "memory" );
> +    clac();
> +
> +    return n;
> +}
> +
> +unsigned int copy_from_guest_ll(void *to, const void __user *from, unsigned 
> int n)
> +{
> +    unsigned dummy;
> +
> +    stac();
> +    asm_inline volatile (
> +        GUARD(
> +        "    guest_access_mask_ptr %[from], %q[scratch1], %q[scratch2]\n"
> +        )
> +        "1:  rep movsb\n"
> +        "2:\n"
> +        ".section .fixup,\"ax\"\n"
> +        "6:  mov  %[cnt], %k[from]\n"
> +        "    xchg %%eax, %[aux]\n"
> +        "    xor  %%eax, %%eax\n"
> +        "    rep stosb\n"
> +        "    xchg %[aux], %%eax\n"
> +        "    mov  %k[from], %[cnt]\n"
> +        "    jmp 2b\n"
> +        ".previous\n"
> +        _ASM_EXTABLE(1b, 6b)
> +        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from),
> +          [aux] "=&r" (dummy)
> +          GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy))
> +        :: "memory" );
> +    clac();
> +
> +    return n;
> +}

... we don't want to have two instances of that code in the code base. One file
should #include the other, after putting in place suitable #define-s. Which
direction the #include should work I'm not entirely certain:
- pv/usercopy.c including usercopy.c means usercopy.c then ends up misnamed,
- usercopy.c including pv/usercopy.c means a "common" file includes a more
  specialized (PV-only) one.

Jan