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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.11.2025 19:17, Grygorii Strashko wrote:
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -71,7 +71,7 @@ obj-y += time.o
>  obj-y += traps-setup.o
>  obj-y += traps.o
>  obj-$(CONFIG_INTEL) += tsx.o
> -obj-y += usercopy.o
> +obj-$(CONFIG_PV) += usercopy.o

Imo, if this was indeed doable (see below) the file would rather want moving
to pv/.

> --- a/xen/arch/x86/include/asm/guest_access.h
> +++ b/xen/arch/x86/include/asm/guest_access.h
> @@ -13,26 +13,64 @@
>  #include <asm/hvm/guest_access.h>
>  
>  /* Raw access functions: no type checking. */
> -#define raw_copy_to_guest(dst, src, len)        \
> -    (is_hvm_vcpu(current) ?                     \
> -     copy_to_user_hvm((dst), (src), (len)) :    \
> -     copy_to_guest_pv(dst, src, len))
> -#define raw_copy_from_guest(dst, src, len)      \
> -    (is_hvm_vcpu(current) ?                     \
> -     copy_from_user_hvm((dst), (src), (len)) :  \
> -     copy_from_guest_pv(dst, src, len))
> -#define raw_clear_guest(dst,  len)              \
> -    (is_hvm_vcpu(current) ?                     \
> -     clear_user_hvm((dst), (len)) :             \
> -     clear_guest_pv(dst, len))
> -#define __raw_copy_to_guest(dst, src, len)      \
> -    (is_hvm_vcpu(current) ?                     \
> -     copy_to_user_hvm((dst), (src), (len)) :    \
> -     __copy_to_guest_pv(dst, src, len))
> -#define __raw_copy_from_guest(dst, src, len)    \
> -    (is_hvm_vcpu(current) ?                     \
> -     copy_from_user_hvm((dst), (src), (len)) :  \
> -     __copy_from_guest_pv(dst, src, len))
> +static inline bool raw_use_hvm_access(const struct vcpu *v)
> +{
> +    return IS_ENABLED(CONFIG_HVM) && (!IS_ENABLED(CONFIG_PV) || 
> is_hvm_vcpu(v));
> +}

Without a full audit (likely tedious and error prone) this still is a
behavioral change for some (likely unintended) use against a system domain
(likely the idle one): With HVM=y PV=n we'd suddenly use the HVM accessor
there. IOW imo the "system domains are implicitly PV" aspect wants
retaining, even if only "just in case". It's okay not to invoke the PV
accessor (but return "len" instead), but it's not okay to invoke the HVM
one.

> +static inline unsigned int raw_copy_to_guest(void *dst, const void *src,
> +                                             unsigned int len)
> +{
> +    if ( raw_use_hvm_access(current) )
> +        return copy_to_user_hvm(dst, src, len);
> +    else if ( IS_ENABLED(CONFIG_PV) )
> +        return copy_to_guest_pv(dst, src, len);
> +    else
> +        return len;
> +}
> +
> +static inline unsigned int raw_copy_from_guest(void *dst, const void *src,
> +                                               unsigned int len)
> +{
> +    if ( raw_use_hvm_access(current) )
> +        return copy_from_user_hvm(dst, src, len);
> +    else if ( IS_ENABLED(CONFIG_PV) )
> +        return copy_from_guest_pv(dst, src, len);
> +    else
> +        return len;
> +}
> +
> +static inline unsigned int raw_clear_guest(void *dst, unsigned int len)
> +{
> +    if ( raw_use_hvm_access(current) )
> +        return clear_user_hvm(dst, len);
> +    else if ( IS_ENABLED(CONFIG_PV) )
> +        return clear_guest_pv(dst, len);
> +    else
> +        return len;
> +}
> +
> +static inline unsigned int __raw_copy_to_guest(void *dst, const void *src,
> +                                               unsigned int len)
> +{
> +    if ( raw_use_hvm_access(current) )
> +        return copy_to_user_hvm(dst, src, len);
> +    else if ( IS_ENABLED(CONFIG_PV) )
> +        return __copy_to_guest_pv(dst, src, len);
> +    else
> +        return len;
> +}
> +
> +static inline unsigned int __raw_copy_from_guest(void *dst, const void *src,
> +                                                 unsigned int len)
> +{
> +    if ( raw_use_hvm_access(current) )
> +        return copy_from_user_hvm(dst, src, len);
> +    else if ( IS_ENABLED(CONFIG_PV) )
> +        return __copy_from_guest_pv(dst, src, len);
> +    else
> +        return len;
> +}

I have to admit that I'm not quite happy about the redundancy here (leaving
aside the imo Misra-conflicting uses of "else"). It looks as if some macro-
ization could still help. Not sure what others think, though.

Jan