xen/arch/x86/Makefile | 2 +- xen/arch/x86/include/asm/guest_access.h | 78 ++++++++++++++++++------- 2 files changed, 59 insertions(+), 21 deletions(-)
From: Grygorii Strashko <grygorii_strashko@epam.com>
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))
This pattern works depending 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 static inline functions which account for
above PV/HVM possible configurations with main intention to optimize code
for (PV=n and HVM=y) case.
For the case (PV=n and HVM=n) return "len" value indicating a failure (no
guests should be possible in this case, which means no access to guest
memory should ever happen).
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: 2/9 grow/shrink: 2/90 up/down: 1678/-32560 (-30882)
Total: Before=1937092, After=1906210, chg -1.59%
[teddy.astie@vates.tech: Suggested to use static inline functions vs
macro combinations]
Suggested-by: Teddy Astie <teddy.astie@vates.tech>
Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
---
changes in v3:
- add raw_use_hvm_access() wrapper
changes in v2:
- use static inline functions instead of macro combinations
xen/arch/x86/Makefile | 2 +-
xen/arch/x86/include/asm/guest_access.h | 78 ++++++++++++++++++-------
2 files changed, 59 insertions(+), 21 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..eb1fb0aef76d 100644
--- 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));
+}
+
+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;
+}
/*
* Pre-validate a guest handle.
--
2.34.1On 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/.
> --- 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.
> +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.
Jan
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
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.
Jan
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()?
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.
Am I missing smth?
Or you meant smth. else?
--
Best regards,
-grygorii
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) )
return ..._hvm();
if ( !IS_ENABLED(CONFIG_PV) )
return len;
return ..._pv();
Jan
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
On 12.11.2025 18:43, Grygorii Strashko wrote:
> 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
Right, and intentionally so (as explained).
Jan
>> 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.
>
On 2025-11-07 13:17, Grygorii Strashko wrote: > From: Grygorii Strashko <grygorii_strashko@epam.com> > > 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)) > > This pattern works depending 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 static inline functions which account for > above PV/HVM possible configurations with main intention to optimize code > for (PV=n and HVM=y) case. > > For the case (PV=n and HVM=n) return "len" value indicating a failure (no > guests should be possible in this case, which means no access to guest > memory should ever happen). > > 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: 2/9 grow/shrink: 2/90 up/down: 1678/-32560 (-30882) > Total: Before=1937092, After=1906210, chg -1.59% > > [teddy.astie@vates.tech: Suggested to use static inline functions vs > macro combinations] > Suggested-by: Teddy Astie <teddy.astie@vates.tech> > Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> Thanks, Jason
© 2016 - 2025 Red Hat, Inc.