From: Grygorii Strashko <grygorii_strashko@epam.com>
Introduce vcpu_is_hcall_compat() helper and use it instead of direct access
to struct vcpu->hcall_compat field in preparation for making HVM COMPAT
code optional.
Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
---
xen/arch/x86/hvm/hvm.c | 9 +++++----
xen/arch/x86/hvm/hypercall.c | 6 +++---
xen/arch/x86/hypercall.c | 6 +-----
xen/common/kernel.c | 2 +-
xen/include/xen/sched.h | 9 +++++++++
5 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0c60faa39d7b..2e47a71714fd 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3500,7 +3500,7 @@ unsigned int copy_to_user_hvm(void *to, const void *from, unsigned int len)
{
int rc;
- if ( current->hcall_compat && is_compat_arg_xlat_range(to, len) )
+ if ( vcpu_is_hcall_compat(current) && is_compat_arg_xlat_range(to, len) )
{
memcpy(to, from, len);
return 0;
@@ -3514,7 +3514,7 @@ unsigned int clear_user_hvm(void *to, unsigned int len)
{
int rc;
- if ( current->hcall_compat && is_compat_arg_xlat_range(to, len) )
+ if ( vcpu_is_hcall_compat(current) && is_compat_arg_xlat_range(to, len) )
{
memset(to, 0x00, len);
return 0;
@@ -3529,7 +3529,7 @@ unsigned int copy_from_user_hvm(void *to, const void *from, unsigned int len)
{
int rc;
- if ( current->hcall_compat && is_compat_arg_xlat_range(from, len) )
+ if ( vcpu_is_hcall_compat(current) && is_compat_arg_xlat_range(from, len) )
{
memcpy(to, from, len);
return 0;
@@ -5214,7 +5214,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
break;
case HVMOP_altp2m:
- rc = current->hcall_compat ? compat_altp2m_op(arg) : do_altp2m_op(arg);
+ rc = vcpu_is_hcall_compat(current) ? compat_altp2m_op(arg)
+ : do_altp2m_op(arg);
break;
default:
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index b254b3e2f7d6..549e25445e67 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -29,7 +29,7 @@ long hvm_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
return -ENOSYS;
}
- if ( !current->hcall_compat )
+ if ( !vcpu_is_hcall_compat(current) )
rc = do_memory_op(cmd, arg);
else
rc = compat_memory_op(cmd, arg);
@@ -57,7 +57,7 @@ long hvm_grant_table_op(
return -ENOSYS;
}
- if ( !current->hcall_compat )
+ if ( !vcpu_is_hcall_compat(current) )
return do_grant_table_op(cmd, uop, count);
else
return compat_grant_table_op(cmd, uop, count);
@@ -96,7 +96,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
return -ENOSYS;
}
- if ( !curr->hcall_compat )
+ if ( !vcpu_is_hcall_compat(curr) )
return do_physdev_op(cmd, arg);
else
return compat_physdev_op(cmd, arg);
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index dc0a90ca0915..3a1a363d8648 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -53,11 +53,7 @@ unsigned long hypercall_create_continuation(
regs->rax = op;
-#ifdef CONFIG_COMPAT
- if ( !curr->hcall_compat )
-#else
- if ( true )
-#endif
+ if ( !vcpu_is_hcall_compat(curr) )
{
for ( i = 0; *p != '\0'; i++ )
{
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index e6979352e100..99be0fbb9d90 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -615,7 +615,7 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
const struct vcpu *curr = current;
#ifdef CONFIG_COMPAT
- if ( curr->hcall_compat )
+ if ( vcpu_is_hcall_compat(curr) )
{
compat_platform_parameters_t params = {
.virt_start = is_pv_vcpu(curr)
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 02bdc256ce37..e3ce427f0bd9 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -311,6 +311,15 @@ struct vcpu
#endif
};
+static inline bool vcpu_is_hcall_compat(const struct vcpu *v)
+{
+#ifdef CONFIG_COMPAT
+ return v->hcall_compat;
+#else
+ return false;
+#endif /* CONFIG_COMPAT */
+}
+
struct sched_unit {
struct domain *domain;
struct vcpu *vcpu_list;
--
2.34.1On 11.11.2025 18:54, Grygorii Strashko wrote:
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -311,6 +311,15 @@ struct vcpu
> #endif
> };
>
> +static inline bool vcpu_is_hcall_compat(const struct vcpu *v)
Does the vcpu_ prefix really buy us much here? The per-vCPU aspect is already
conveyed by the function parameter, I'd say.
Actually - is a parameter in fact needed? It's always current afaics. (Then,
if the parameter was to stay for some reason, it would want naming "curr".)
> +{
> +#ifdef CONFIG_COMPAT
> + return v->hcall_compat;
As long as you don't remove the struct field, ...
> +#else
> + return false;
> +#endif /* CONFIG_COMPAT */
... why not
return IS_ENABLED(CONFIG_COMPAT) && v->hcall_compat;
?
Jan
On 13.11.25 14:22, Jan Beulich wrote:
> On 11.11.2025 18:54, Grygorii Strashko wrote:
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -311,6 +311,15 @@ struct vcpu
>> #endif
>> };
>>
>> +static inline bool vcpu_is_hcall_compat(const struct vcpu *v)
>
> Does the vcpu_ prefix really buy us much here? The per-vCPU aspect is already
> conveyed by the function parameter, I'd say.
Such annotation makes it clear that operation is performed on struct vcpu object,
which improves readability and code analyze (might help also if somebody will ever
try split sched.h).
For example:
is_hcall_compat(curr) in the code doesn't say too much - need parse code
(or have modern editor which can parse and highlight parameter type for us,
still need some mouse manipulations:))
vcpu_is_hcall_compat(curr) - is kinda absolutely clear from the first look.
>
> Actually - is a parameter in fact needed? It's always current afaics. (Then,
> if the parameter was to stay for some reason, it would want naming "curr".)
"curr" it be
>
>> +{
>> +#ifdef CONFIG_COMPAT
>> + return v->hcall_compat;
>
> As long as you don't remove the struct field, ...
It is already under
#ifdef CONFIG_COMPAT
/* A hypercall is using the compat ABI? */
bool hcall_compat;
/* Physical runstate area registered via compat ABI? */
bool runstate_guest_area_compat;
#endif
which is the main reason for vcpu_is_hcall_compat() introduction.
>
>> +#else
>> + return false;
>> +#endif /* CONFIG_COMPAT */
>
> ... why not
>
> return IS_ENABLED(CONFIG_COMPAT) && v->hcall_compat;
--
Best regards,
-grygorii
On 13.11.2025 14:07, Grygorii Strashko wrote: > On 13.11.25 14:22, Jan Beulich wrote: >> On 11.11.2025 18:54, Grygorii Strashko wrote: >>> --- a/xen/include/xen/sched.h >>> +++ b/xen/include/xen/sched.h >>> @@ -311,6 +311,15 @@ struct vcpu >>> #endif >>> }; >>> >>> +static inline bool vcpu_is_hcall_compat(const struct vcpu *v) >> >> Does the vcpu_ prefix really buy us much here? The per-vCPU aspect is already >> conveyed by the function parameter, I'd say. > > Such annotation makes it clear that operation is performed on struct vcpu object, > which improves readability and code analyze (might help also if somebody will ever > try split sched.h). > > For example: > > is_hcall_compat(curr) in the code doesn't say too much - need parse code > (or have modern editor which can parse and highlight parameter type for us, > still need some mouse manipulations:)) That's one of the reasons we aim at using consistently named variables. "curr" should be pretty clear to everyone as being "current" latched into a local variable. Plus of course for every function invocation call site context is relevant anyway, when it comes to arguments passed to the function. > vcpu_is_hcall_compat(curr) - is kinda absolutely clear from the first look. > >> >> Actually - is a parameter in fact needed? It's always current afaics. (Then, >> if the parameter was to stay for some reason, it would want naming "curr".) > > "curr" it be My preference would still be is_hcall_compat(void). Jan
On 2025-11-11 12:54, Grygorii Strashko wrote: > From: Grygorii Strashko <grygorii_strashko@epam.com> > > Introduce vcpu_is_hcall_compat() helper and use it instead of direct access > to struct vcpu->hcall_compat field in preparation for making HVM COMPAT > code optional. > > Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> Thanks, Jason
© 2016 - 2025 Red Hat, Inc.