Re: [PATCH RFC] x86/PV: avoid HVM-copying alternatives in PV-only code
Andrew Cooper <[email protected]>
| Newsgroups | org.xenproject.lists.xen-devel |
|---|---|
| Message-ID | <[email protected]> |
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: diff --git a/xen/arch/x86/include/asm/guest_access.h b/xen/arch/x86/include/asm/guest_access.h index 69716c8b41bb..49920e181832 100644 --- 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, then extended GUEST_ACCESS_HVM to: diff --git a/xen/arch/x86/include/asm/guest_access.h b/xen/arch/x86/include/asm/guest_access.h index 49920e181832..eba47e46de88 100644 --- 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. 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. 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. ~Andrew