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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-11-06 17:26, Grygorii Strashko wrote:
> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
>
> Xen uses below pattern for raw_x_guest() functions:
>
> 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))
>
> This pattern works depending on CONFIG_PV/CONFIG_HVM as:
> - PV=y and HVM=y
>    Proper guest access function is selected depending on domain type.
> - PV=y and HVM=n
>    Only PV domains are possible. is_hvm_domain/vcpu() will constify to "false"
>    and compiler will optimize code and skip HVM specific part.
> - PV=n and HVM=y
>    Only HVM domains are possible. is_hvm_domain/vcpu() will not be constified.
>    No PV specific code will be optimized by compiler.
> - PV=n and HVM=n
>    No guests should possible. The code will still follow PV path.
>
> Rework raw_x_guest() code to use static inline functions which account for
> above PV/HVM possible configurations with main intention to optimize code
> for (PV=n and HVM=y) case.
>
> For the case (PV=n and HVM=n) return "len" value indicating a failure (no
> guests should be possible in this case, which means no access to guest
> memory should ever happen).
>
> Finally build arch/x86/usercopy.c only for PV=y.
>
> The measured (bloat-o-meter) improvement for (PV=n and HVM=y) case is:
>    add/remove: 2/9 grow/shrink: 2/90 up/down: 1678/-32560 (-30882)
>    Total: Before=1937092, After=1906210, chg -1.59%
>
> Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> [teddy.astie@xxxxxxxxxx: Suggested to use static inline functions vs macro 
> combinations]
> Suggested-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
I think Teddy's goes before your SoB.

> ---
> diff --git a/xen/arch/x86/include/asm/guest_access.h 
> b/xen/arch/x86/include/asm/guest_access.h
> index 69716c8b41bb..576eac9722e6 100644
> --- 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 unsigned int raw_copy_to_guest(void *to, const void *src,
Maybe s/to/dst/ to keep this consistent with the rest?

> +                                             unsigned int len)
> +{
> +    if ( IS_ENABLED(CONFIG_HVM) &&
> +         (!IS_ENABLED(CONFIG_PV) || is_hvm_vcpu(current)) )
Since this is repeated, maybe put into a helper like 
use_hvm_access(current)?
Thanks,
Jason