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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 10.11.25 09:11, Jan Beulich wrote:
> 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/.
I've tried it :( But at that time it has failed for me because of macro magic 
which uses
"# include __FILE__".

Now I see that additional Makefile line:

# Allows usercopy.c to include itself
$(obj)/usercopy.o: CFLAGS-y += -iquote .

need to be moved also.

> > --- 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.
This patch is subset of "constify is_hvm_domain() for PV=n case" attempts.

It was made under assumption that:
"System domains do not have Guests running, so can't initiate hypecalls and
 can not be users of copy_to/from_user() routines. There are no Guest and no user 
memory".
[IDLE, COW, IO, XEN]

If above assumption is correct - this patch was assumed safe.

if not - it all make no sense, probably.
 

> > +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.


Just an observation note:
 From my recent experience I see that macro-magic makes Coverage report (gcov)
to provide not exactly correct results in terms of code lines coverage at least 
:(
For example, for usercopy.o: 7 functions reported, but lines coverage is 
accounted
only till '# undef GUARD".


--
Best regards,
-grygorii