[v3] Disable domctl-op via CONFIG_MGMT_HYPERCALLS

[PATCH v3 27/28] xen/domctl: make HVM_PARAM_IDENT_PT conditional upon CONFIG_MGMT_HYPERCALLS

Posted by Penny Zheng 3 months, 4 weeks ago

Helper domctl_lock_{acquire,release} is domctl_lock, which HVM_PARAM_IDENT_PT
uses to ensure synchronization and hence being a toolstack-only operation.
So we shall make HVM_PARAM_IDENT_PT conditional upon CONFIG_MGMT_HYPERCALLS,
returning -EOPNOTSUPP when MGMT_HYPERCALLS=n.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v2 -> v3:
- new commit
---
 xen/arch/x86/hvm/hvm.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e77b0c03ed..e7d630af95 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4281,7 +4281,6 @@ static int hvm_allow_set_param(struct domain *d,
 static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
 {
     struct domain *curr_d = current->domain;
-    struct vcpu *v;
     int rc;
 
     rc = hvm_allow_set_param(d, index, value);
@@ -4307,6 +4306,10 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
             rc = -EINVAL;
         break;
     case HVM_PARAM_IDENT_PT:
+    {
+#ifdef CONFIG_MGMT_HYPERCALLS
+        struct vcpu *v;
+
         /*
          * Only actually required for VT-x lacking unrestricted_guest
          * capabilities.  Short circuit the pause if possible.
@@ -4334,7 +4337,11 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
         domain_unpause(d);
 
         domctl_lock_release();
+#else
+        rc = -EOPNOTSUPP;
+#endif /* CONFIG_MGMT_HYPERCALLS */
         break;
+    }
     case HVM_PARAM_DM_DOMAIN:
         /* The only value this should ever be set to is DOMID_SELF */
         if ( value != DOMID_SELF )
-- 
2.34.1

Re: [PATCH v3 27/28] xen/domctl: make HVM_PARAM_IDENT_PT conditional upon CONFIG_MGMT_HYPERCALLS

Posted by Jan Beulich 3 months, 1 week ago

On 13.10.2025 12:15, Penny Zheng wrote:
> Helper domctl_lock_{acquire,release} is domctl_lock, which HVM_PARAM_IDENT_PT
> uses to ensure synchronization and hence being a toolstack-only operation.
> So we shall make HVM_PARAM_IDENT_PT conditional upon CONFIG_MGMT_HYPERCALLS,
> returning -EOPNOTSUPP when MGMT_HYPERCALLS=n.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>

I fear this isn't quite what I suggested. The param get/set are XSM_TARGET, i.e.
can be used by DM as well. The particular one here shouldn't be used by a DM, but
that's a different question. Similarly in principle the PVH Dom0 building code
should be able to use this path; it doesn't right now in favor of some open-
coding.

What iirc I did suggest was that the serialization isn't needed when no domctl can
be used to otherwise alter (relevant) guest state.

Jan

RE: [PATCH v3 27/28] xen/domctl: make HVM_PARAM_IDENT_PT conditional upon CONFIG_MGMT_HYPERCALLS

Posted by Penny, Zheng 2 months, 3 weeks ago

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, October 30, 2025 9:35 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; oleksii.kurochko@gmail.com; Andrew
> Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
> xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v3 27/28] xen/domctl: make HVM_PARAM_IDENT_PT
> conditional upon CONFIG_MGMT_HYPERCALLS
>
> On 13.10.2025 12:15, Penny Zheng wrote:
> > Helper domctl_lock_{acquire,release} is domctl_lock, which
> > HVM_PARAM_IDENT_PT uses to ensure synchronization and hence being a
> toolstack-only operation.
> > So we shall make HVM_PARAM_IDENT_PT conditional upon
> > CONFIG_MGMT_HYPERCALLS, returning -EOPNOTSUPP when
> MGMT_HYPERCALLS=n.
> >
> > Suggested-by: Jan Beulich <jbeulich@suse.com>
>
> I fear this isn't quite what I suggested. The param get/set are XSM_TARGET, i.e.
> can be used by DM as well. The particular one here shouldn't be used by a DM, but
> that's a different question. Similarly in principle the PVH Dom0 building code should
> be able to use this path; it doesn't right now in favor of some open- coding.
>
> What iirc I did suggest was that the serialization isn't needed when no domctl can
> be used to otherwise alter (relevant) guest state.

Ah, true, serialization isn't needed when MGMT_HYPERCALLS=n, as no domctl-op could alter the guest state at the same time.
Then maybe adding IS_ENABLED() checking is enough:
```
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 5a50721bd0..4e1b3ee5f4 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4324,7 +4324,7 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
          * the domctl_lock.
          */
         rc = -ERESTART;
-        if ( !domctl_lock_acquire() )
+        if ( IS_ENABLED(CONFIG_MGMT_HYPERCALLS) && !domctl_lock_acquire() )
             break;

         rc = 0;
@@ -4334,7 +4334,8 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
             paging_update_cr3(v, false);
         domain_unpause(d);

-        domctl_lock_release();
+        if ( IS_ENABLED(CONFIG_MGMT_HYPERCALLS) )
+            domctl_lock_release();
         break;
     case HVM_PARAM_DM_DOMAIN:
         /* The only value this should ever be set to is DOMID_SELF */
```

>
> Jan

Re: [PATCH v3 27/28] xen/domctl: make HVM_PARAM_IDENT_PT conditional upon CONFIG_MGMT_HYPERCALLS

Posted by Jan Beulich 2 months, 3 weeks ago

On 18.11.2025 07:45, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, October 30, 2025 9:35 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; oleksii.kurochko@gmail.com; Andrew
>> Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
>> xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH v3 27/28] xen/domctl: make HVM_PARAM_IDENT_PT
>> conditional upon CONFIG_MGMT_HYPERCALLS
>>
>> On 13.10.2025 12:15, Penny Zheng wrote:
>>> Helper domctl_lock_{acquire,release} is domctl_lock, which
>>> HVM_PARAM_IDENT_PT uses to ensure synchronization and hence being a
>> toolstack-only operation.
>>> So we shall make HVM_PARAM_IDENT_PT conditional upon
>>> CONFIG_MGMT_HYPERCALLS, returning -EOPNOTSUPP when
>> MGMT_HYPERCALLS=n.
>>>
>>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>>
>> I fear this isn't quite what I suggested. The param get/set are XSM_TARGET, i.e.
>> can be used by DM as well. The particular one here shouldn't be used by a DM, but
>> that's a different question. Similarly in principle the PVH Dom0 building code should
>> be able to use this path; it doesn't right now in favor of some open- coding.
>>
>> What iirc I did suggest was that the serialization isn't needed when no domctl can
>> be used to otherwise alter (relevant) guest state.
> 
> Ah, true, serialization isn't needed when MGMT_HYPERCALLS=n, as no domctl-op could alter the guest state at the same time.
> Then maybe adding IS_ENABLED() checking is enough:

Yes, that or indeed introducing stubs. Which one is the lesser evil I'm having
a hard time determining. Hence I'd suggest that you go with the below, unless
someone else chimes in.

Jan

> ```
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 5a50721bd0..4e1b3ee5f4 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4324,7 +4324,7 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
>           * the domctl_lock.
>           */
>          rc = -ERESTART;
> -        if ( !domctl_lock_acquire() )
> +        if ( IS_ENABLED(CONFIG_MGMT_HYPERCALLS) && !domctl_lock_acquire() )
>              break;
> 
>          rc = 0;
> @@ -4334,7 +4334,8 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
>              paging_update_cr3(v, false);
>          domain_unpause(d);
> 
> -        domctl_lock_release();
> +        if ( IS_ENABLED(CONFIG_MGMT_HYPERCALLS) )
> +            domctl_lock_release();
>          break;
>      case HVM_PARAM_DM_DOMAIN:
>          /* The only value this should ever be set to is DOMID_SELF */
> ```
> 
>>
>> Jan