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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

Hi Teddy, Jan,

On 06.11.25 17:57, Teddy Astie wrote:
> Le 31/10/2025 à 22:25, Grygorii Strashko a écrit :
> > 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))
> >
> > How this pattern is working depends 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 required functions explicitly for each
> > combination of CONFIG_PV/CONFIG_HVM with main intention to optimize code for
> > (PV=n and HVM=y) case.
> >
> > For the case (PV=n and HVM=n) empty stubs are created which return (1)
> > indicating failure. Hence, no guests should possible in this case -
> > which means no access to guest memory  should ever happen.
> > The two calls of __raw_copy_to_guest() in 
> > common/domain.c->update_runstate_area()
> > are fixed for this case by explicitly cast the return value to void
> > (MISRA C Rule 17.7).
> >
> > 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: 0/10 grow/shrink: 2/90 up/down: 163/-30932 (-30769)
> >     Total: Before=1937113, After=1906344, chg -1.59%
> >
> > Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> > ---
> >    xen/arch/x86/Makefile                   |  2 +-
> >    xen/arch/x86/include/asm/guest_access.h | 38 +++++++++++++++++++++++++
> >    xen/common/domain.c                     | 10 ++++---
> >    3 files changed, 45 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index 407571c510e1..27f131ffeb61 100644
> > --- 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
> >    obj-y += x86_emulate.o
> >    obj-$(CONFIG_TBOOT) += tboot.o
> >    obj-y += hpet.o
> > diff --git a/xen/arch/x86/include/asm/guest_access.h 
> > b/xen/arch/x86/include/asm/guest_access.h
> > index 69716c8b41bb..36aeb89524ab 100644
> > --- 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
> > +
> >    /*
> >     * Pre-validate a guest handle.
> >     * Allows use of faster __copy_* functions.
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 4f91316ad93e..c603edcc7d46 100644
> > --- 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);
> Alternatively, we can make all the raw_* functions `static inline` and
> have something like this which should have the same effect with much
> less redundancy.
>
> static inline
> unsigned int raw_copy_to_user_hvm(void *to, const void *from,
>                                     unsigned int len)
> {
>       if ( IS_ENABLED(CONFIG_HVM) &&
>            (!IS_ENABLED(CONFIG_PV) || is_hvm_vcpu(current) )
>          copy_to_user_hvm(to, from, len);
>       else if ( IS_ENABLED(CONFIG_PV) )
>          copy_to_guest_pv(to, from, len);
        else
           return len;
> }
Can try.

Jan, would it be acceptable?


--
Best regards,
-grygorii