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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 12.11.25 15:07, Jan Beulich wrote:
> On 12.11.2025 12:27, Grygorii Strashko wrote:
> > On 12.11.25 08:38, Jan Beulich wrote:
> > > On 11.11.2025 18:52, Grygorii Strashko wrote:
> > > > On 10.11.25 09:11, Jan Beulich wrote:
> > > > > On 07.11.2025 19:17, Grygorii Strashko wrote:
> > > > > > --- 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.
> > > I wouldn't go as far as saying that. It can be arranged to avid the corner
> > > case I mentioned, I think.
> > do you mean adding "&& !is_system_domain(v->domain)" in raw_use_hvm_access()?
> No, we want to avoid adding any new any runtime checks.
>
> > Hm, I see that vcpu(s) are not even created for system domains in 
> > domain_create().
> > So seems !is_system_domain(v->domain) == true always here.
> "always" in what sense? It _should_ be always true, but in the unlikely event we
> have a path where it isn't (which we could be sure of only after a full audit),
> behavior there shouldn't change in the described problematic way.
>
> > Am I missing smth?
> > Or you meant smth. else?
> I was thinking of something along the lines of
>
>      if ( is_hvm_vcpu(current) )
this condition will not be constified any more for HVM=y and PV=n

>          return ..._hvm();
>
>      if ( !IS_ENABLED(CONFIG_PV) )
>          return len;
>
>      return ..._pv();
Possible benefit will be reduced from:
  add/remove: 2/9 grow/shrink: 2/90 up/down: 1678/-32560 (-30882)

to:
  add/remove: 3/8 grow/shrink: 3/89 up/down: 1018/-12087 (-11069)

Any way it is smth.

--
Best regards,
-grygorii