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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.10.2025 22:20, Grygorii Strashko wrote:
> --- a/xen/arch/x86/include/asm/guest_access.h
> +++ b/xen/arch/x86/include/asm/guest_access.h
> @@ -13,6 +13,7 @@
>  #include <asm/hvm/guest_access.h>
>  
>  /* Raw access functions: no type checking. */
> +#if defined(CONFIG_PV) && defined(CONFIG_HVM)
>  #define raw_copy_to_guest(dst, src, len)        \
>      (is_hvm_vcpu(current) ?                     \
>       copy_to_user_hvm((dst), (src), (len)) :    \
> @@ -34,6 +35,43 @@
>       copy_from_user_hvm((dst), (src), (len)) :  \
>       __copy_from_guest_pv(dst, src, len))
>  
> +#elif defined(CONFIG_HVM)
> +#define raw_copy_to_guest(dst, src, len)        \
> +     copy_to_user_hvm((dst), (src), (len))
> +#define raw_copy_from_guest(dst, src, len)      \
> +     copy_from_user_hvm((dst), (src), (len))
> +#define raw_clear_guest(dst,  len)              \
> +     clear_user_hvm((dst), (len))
> +#define __raw_copy_to_guest(dst, src, len)      \
> +     copy_to_user_hvm((dst), (src), (len))
> +#define __raw_copy_from_guest(dst, src, len)    \
> +     copy_from_user_hvm((dst), (src), (len))
> +
> +#elif defined(CONFIG_PV)
> +#define raw_copy_to_guest(dst, src, len)        \
> +     copy_to_guest_pv(dst, src, len)
> +#define raw_copy_from_guest(dst, src, len)      \
> +     copy_from_guest_pv(dst, src, len)
> +#define raw_clear_guest(dst,  len)              \
> +     clear_guest_pv(dst, len)
> +#define __raw_copy_to_guest(dst, src, len)      \
> +     __copy_to_guest_pv(dst, src, len)
> +#define __raw_copy_from_guest(dst, src, len)    \
> +     __copy_from_guest_pv(dst, src, len)
> +
> +#else
> +#define raw_copy_to_guest(dst, src, len)        \
> +        ((void)(dst), (void)(src), (void)(len), 1)
> +#define raw_copy_from_guest(dst, src, len)      \
> +        ((void)(dst), (void)(src), (void)(len), 1)
> +#define raw_clear_guest(dst, len)               \
> +        ((void)(dst), (void)(len), 1)
> +#define __raw_copy_to_guest(dst, src, len)      \
> +        ((void)(dst), (void)(src), (void)(len), 1)
> +#define __raw_copy_from_guest(dst, src, len)    \
> +        ((void)(dst), (void)(src), (void)(len), 1)
> +#endif

I have to admit that I don't really like the repetition.

Style-wise you want to be consistent with the adding of blank lines around the
preprocessor directives: Imo here there want to be ones on both sides of each
of the directives.

For the last block, I'd further prefer if "len" was returned. That's properly
representing that nothing was copied. And if these were all using a single
inline stub function, ...

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1985,8 +1985,9 @@ bool update_runstate_area(struct vcpu *v)
>  #endif
>          guest_handle--;
>          runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
> -        __raw_copy_to_guest(guest_handle,
> -                            (void *)(&runstate.state_entry_time + 1) - 1, 1);
> +        (void)__raw_copy_to_guest(guest_handle,
> +                                  (void *)(&runstate.state_entry_time + 1) - 
> 1,
> +                                  1);
>          smp_wmb();
>      }
>  
> @@ -2008,8 +2009,9 @@ bool update_runstate_area(struct vcpu *v)
>      {
>          runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
>          smp_wmb();
> -        __raw_copy_to_guest(guest_handle,
> -                            (void *)(&runstate.state_entry_time + 1) - 1, 1);
> +        (void)__raw_copy_to_guest(guest_handle,
> +                                  (void *)(&runstate.state_entry_time + 1) - 
> 1,
> +                                  1);
>      }
>  
>      update_guest_memory_policy(v, &policy);

... these changes would become unnecessary (I dislike such unnecessary - as per
the C spec - casts, even if I understand that for Misra we may need to gain 
quite
a few of them).

Jan