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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.11.2025 09:11, Jan Beulich wrote:
> 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.

Likely it would be best to build this into library members (ideally one per
function), such that unused ones wouldn't even be pulled in by the linker. I
meant to suggest to move to xen/arch/x86/lib/, but right now we only have
xen/lib/x86/, where I don't think this would be a good fit. I've brought this
up with the other x86 maintainers ...

Jan