Re: [PATCH RFC] x86/PV: avoid HVM-copying alternatives in PV-only code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.07.2026 22:07, Andrew Cooper wrote:
> On 30/06/2026 2:38 pm, Jan Beulich wrote:
>> --- a/xen/arch/x86/include/asm/guest_access.h
>> +++ b/xen/arch/x86/include/asm/guest_access.h
>> @@ -14,11 +14,15 @@
>>  
>>  /* Raw access functions: no type checking. */
>>  #define raw_copy_to_guest(dst, src, len)        \
>> -    (is_hvm_vcpu(current) ?                     \
>> +    ((IS_ENABLED(HVM_ONLY_SOURCE) ||            \
>> +      (!IS_ENABLED(PV_ONLY_SOURCE) &&           \
>> +       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) ?                     \
>> +    ((IS_ENABLED(HVM_ONLY_SOURCE) ||            \
>> +      (!IS_ENABLED(PV_ONLY_SOURCE) &&           \
>> +       is_hvm_vcpu(current))) ?                 \
>>       copy_from_user_hvm((dst), (src), (len)) :  \
>>       copy_from_guest_pv(dst, src, len))
>>  #define raw_clear_guest(dst,  len)              \
>> @@ -26,11 +30,15 @@
>>       clear_user_hvm((dst), (len)) :             \
>>       clear_guest_pv(dst, len))
>>  #define __raw_copy_to_guest(dst, src, len)      \
>> -    (is_hvm_vcpu(current) ?                     \
>> +    ((IS_ENABLED(HVM_ONLY_SOURCE) ||            \
>> +      (!IS_ENABLED(PV_ONLY_SOURCE) &&           \
>> +       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) ?                     \
>> +    ((IS_ENABLED(HVM_ONLY_SOURCE) ||            \
>> +      (!IS_ENABLED(PV_ONLY_SOURCE) &&           \
>> +       is_hvm_vcpu(current))) ?                 \
>>       copy_from_user_hvm((dst), (src), (len)) :  \
>>       __copy_from_guest_pv(dst, src, len))
> 
> Looking back at my own attempt to do this, which was from August 2024, I
> started by cleaning up this code block as such:
> 
> --- a/xen/arch/x86/include/asm/guest_access.h
> +++ b/xen/arch/x86/include/asm/guest_access.h
> @@ -12,27 +12,33 @@
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/guest_access.h>
>  
> +#define GUEST_ACCESS_HVM is_hvm_vcpu(current)
> +
>  /* 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))
> +    (GUEST_ACCESS_HVM                           \
> +     ? 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))
> +    (GUEST_ACCESS_HVM                           \
> +     ? 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))
> +    (GUEST_ACCESS_HVM                           \
> +     ? 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))
> +    (GUEST_ACCESS_HVM                           \
> +     ?   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))
> +    (GUEST_ACCESS_HVM                           \
> +     ?   copy_from_user_hvm(dst, src, len)      \
> +     : __copy_from_guest_pv(dst, src, len))
>  
>  /*
>   * Pre-validate a guest handle.
> 
> 
> 
> so they're all consistent and legible,

Sure, why not. There's one Misra concern though: In my patch I'm
specifically avoiding (naming aside)

#define GUEST_ACCESS_HVM is_hvm_vcpu(current)

in the header, as that then requires #undef elsewhere to override. Misra
wants us to avoid #undef as much as possible. In x86/mm.c this then would
also require the original form to be re-instated. (Yes, I'm not getting
away without #undef either, but it's truly removal of the definition, not
removal to then re-instate the macro with a different expansion.)

Perhaps something like

#define IS_HVM_ACCESS                    \
    (IS_ENABLED(GUEST_ACCESS_HVM) ||     \
     (!IS_ENABLED(GUEST_ACCESS_PV) &&    \
      is_hvm_vcpu(current)))             \

to reduce redundancy in the macros while keeping the property of my patch?

> then extended GUEST_ACCESS_HVM to:
> 
> --- a/xen/arch/x86/include/asm/guest_access.h
> +++ b/xen/arch/x86/include/asm/guest_access.h
> @@ -12,7 +12,12 @@
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/guest_access.h>
>  
> -#define GUEST_ACCESS_HVM is_hvm_vcpu(current)
> +#define GUEST_ACCESS_HVM                        \
> +    (IS_ENABLED(GA_ALWAYS_PV)                   \
> +     ? 0                                        \
> +     : (IS_ENABLED(GA_ALWAYS_HVM)               \
> +        ? 1                                     \
> +        : is_hvm_vcpu(current)))                \
>  
>  /* Raw access functions: no type checking. */
>  #define raw_copy_to_guest(dst, src, len)        \
> 
> 
> 
> to allow ways to force it one way or another.

Sure, perhaps simplified to

#define GUEST_ACCESS_HVM                        \
    (IS_ENABLED(GA_ALWAYS_PV)                   \
     ? false                                    \
     : (IS_ENABLED(GA_ALWAYS_HVM) ||            \
        is_hvm_vcpu(current)))                  \

or something yet closer to what I have in my patch. The conditional
operators aren't overly useful here.

Yet one of the RFC questions remains with this: Is there anywhere we'd
actually be able to use GA_ALWAYS_HVM?

> For forcing it one way or another, while ...
> 
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -3407,6 +3407,9 @@ int new_guest_cr3(mfn_t mfn)
>>  #endif
>>  
>>  #ifdef CONFIG_PV
>> +
>> +#define PV_ONLY_SOURCE 1
>> +
> 
> ... mm.c is certainly complicated, others like ...
> 
>> --- a/xen/arch/x86/pv/callback.c
>> +++ b/xen/arch/x86/pv/callback.c
>> @@ -5,6 +5,8 @@
>>   * hypercall handles and helper functions for guest callback
>>   */
>>  
>> +#define PV_ONLY_SOURCE 1
>> +
>>  #include <xen/event.h>
>>  #include <xen/hypercall.h>
>>  #include <xen/guest_access.h>
>> --- a/xen/arch/x86/pv/descriptor-tables.c
>> +++ b/xen/arch/x86/pv/descriptor-tables.c
>> @@ -8,6 +8,8 @@
>>   * Copyright (c) 2004 Christian Limpach
>>   */
>>  
>> +#define PV_ONLY_SOURCE 1
>> +
>>  #include <xen/guest_access.h>
>>  #include <xen/hypercall.h>
>>  
>> --- a/xen/arch/x86/pv/shim.c
>> +++ b/xen/arch/x86/pv/shim.c
>> @@ -6,6 +6,9 @@
>>   *
>>   * Copyright (c) 2017 Citrix Systems Ltd.
>>   */
>> +
>> +#define PV_ONLY_SOURCE 1
>> +
>>  #include <xen/event.h>
>>  #include <xen/guest_access.h>
>>  #include <xen/hypercall.h>
> 
> all of these could have a general CFLAGS += in arch/x86/pv/Makefile
> rather than having to state it in every file.

A general CFLAGS += is (at least latently / conceptually) wrong in pv/,
as code there may come into play when a PVH Dom0 controls a PV DomU. Much
like a fair bit of care is needed in x86/mm.c (where we may want to try
to move some more code to x86/pv/mm.c).

I did consider this on a per-object basis and meant to put in a remark to
this effect, but then forgot. The issue I see with using this approach is
the disconnect between setting of the control (Makefile) and it being
correct to use for the particular source file (in particular when making
changes down the road). Having the #define near the top of the files has
(imo) at least slightly better chances of being noticed when opening the
file for editing.

> Naming wise, while PV_ONLY_SOURCE works in pv/, it's more dubious in
> arch/x86/mm.c.  GUEST_ACCESS_* at least identifies the property that's
> being decided upon.

I'm happy to see your naming used; I wasn't overly happy with my naming
choice anyway.

Jan