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.
>
