[PATCH for-4.22? 0/9] domctl: XSA-492 and -491 follow-on

[PATCH for-4.22? 0/9] domctl: XSA-492 and -491 follow-on

[Jan Beulich]

A number of further possible improvements were identified when putting
together the patches for these XSAs; some therefore already have acks or
alike. Some of these may want considering to take for 4.22.

Many of the patches here are largely independent, but the last one
(following on to XSA-491, while all others are XSA-492 related) really
depends on the 2nd to last one. Or else bigger changes would be
necessary there.

1: sched: introduce specialization of "running only" vcpu_runstate_get()
2: domctl: move XEN_DOMCTL_irq_permission handling to x86 code
3: domctl: rename a label
4: domctl: error code adjustment for unpriv callers
5: domctl/XSM: avoid XSM_OTHER with xsm_domctl()
6: domctl: handle XEN_DOMCTL_getvcpuinfo without acquiring domctl lock
7: domctl: restrict permission check for XEN_DOMCTL_memory_mapping's remove form
8: x86/domctl: don't imply I/O port permissions from I/O port mapping
9: x86/HVM: more checking for XEN_DOMCTL_ioport_mapping

Jan

[PATCH for-4.22? 1/9] sched: introduce specialization of "running only" vcpu_runstate_get()

[Jan Beulich]

About half the callers of vcpu_runstate_get() are solely after the
"running" time of a vCPU. Introduce a specialization with a smaller
read critical section and thus reduced risk of a need for retries.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The function name was chosen such that grep-ing for "vcpu_runstate_get"
would still turn up all uses. If that was deemed largely irrelevant, a
better name might be e.g. vcpu_get_running_time().

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -56,7 +56,6 @@ void getdomaininfo(struct domain *d, str
     struct vcpu *v;
     u64 cpu_time = 0;
     int flags = XEN_DOMINF_blocked;
-    struct vcpu_runstate_info runstate;
 
     memset(info, 0, sizeof(*info));
 
@@ -69,8 +68,7 @@ void getdomaininfo(struct domain *d, str
      */
     for_each_vcpu ( d, v )
     {
-        vcpu_runstate_get(v, &runstate);
-        cpu_time += runstate.time[RUNSTATE_running];
+        cpu_time += vcpu_runstate_get_running(v);
         info->max_vcpu_id = v->vcpu_id;
         if ( !(v->pause_flags & VPF_down) )
         {
@@ -829,8 +827,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
     case XEN_DOMCTL_getvcpuinfo:
     {
-        struct vcpu   *v;
-        struct vcpu_runstate_info runstate;
+        const struct vcpu *v;
 
         ret = -EINVAL;
         if ( op->u.getvcpuinfo.vcpu >= d->max_vcpus )
@@ -840,12 +837,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         if ( (v = d->vcpu[op->u.getvcpuinfo.vcpu]) == NULL )
             break;
 
-        vcpu_runstate_get(v, &runstate);
-
         op->u.getvcpuinfo.online   = !(v->pause_flags & VPF_down);
         op->u.getvcpuinfo.blocked  = !!(v->pause_flags & VPF_blocked);
         op->u.getvcpuinfo.running  = v->is_running;
-        op->u.getvcpuinfo.cpu_time = runstate.time[RUNSTATE_running];
+        op->u.getvcpuinfo.cpu_time = vcpu_runstate_get_running(v);
         op->u.getvcpuinfo.cpu      = v->processor;
         ret = 0;
         copyback = 1;
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -325,15 +325,35 @@ void vcpu_runstate_get(const struct vcpu
     }
 }
 
-uint64_t get_cpu_idle_time(unsigned int cpu)
+uint64_t vcpu_runstate_get_running(const struct vcpu *v)
 {
-    struct vcpu_runstate_info state = { 0 };
-    const struct vcpu *v = idle_vcpu[cpu];
+    struct seqcount seq = SEQCNT_ZERO();
+    const struct seqcount *s = v == current ? &seq : &v->runstate_seq;
+    unsigned int count;
+    uint64_t running;
+    s_time_t delta;
+
+    do {
+        count = read_seqcount_begin(s);
+
+        running = v->runstate.time[RUNSTATE_running];
+        delta = v->runstate.state == RUNSTATE_running
+                ? NOW() - v->runstate.state_entry_time
+                : 0;
+    } while ( read_seqcount_retry(s, count) );
+
+    if ( delta > 0 )
+        running += delta;
 
+    return running;
+}
+
+uint64_t get_cpu_idle_time(unsigned int cpu)
+{
     if ( cpu_online(cpu) && get_sched_res(cpu) )
-        vcpu_runstate_get(v, &state);
+        return vcpu_runstate_get_running(idle_vcpu[cpu]);
 
-    return state.time[RUNSTATE_running];
+    return 0;
 }
 
 /*
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1121,6 +1121,7 @@ int vcpu_affinity_domctl(struct domain *
 
 void vcpu_runstate_get(const struct vcpu *v,
                        struct vcpu_runstate_info *runstate);
+uint64_t vcpu_runstate_get_running(const struct vcpu *v);
 uint64_t get_cpu_idle_time(unsigned int cpu);
 void sched_guest_idle(void (*idle) (void), unsigned int cpu);
 void scheduler_enable(void);

Re: [PATCH for-4.22? 1/9] sched: introduce specialization of "running only" vcpu_runstate_get()

[Roger Pau Monné]

On Mon, Jun 15, 2026 at 04:12:11PM +0200, Jan Beulich wrote:
> About half the callers of vcpu_runstate_get() are solely after the
> "running" time of a vCPU. Introduce a specialization with a smaller
> read critical section and thus reduced risk of a need for retries.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> The function name was chosen such that grep-ing for "vcpu_runstate_get"
> would still turn up all uses. If that was deemed largely irrelevant, a
> better name might be e.g. vcpu_get_running_time().

FWIW, I think the fact that you can find call sites by using
vcpu_runstate_get is likely to become irrelevant in the long run.

> 
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -56,7 +56,6 @@ void getdomaininfo(struct domain *d, str
>      struct vcpu *v;
>      u64 cpu_time = 0;
>      int flags = XEN_DOMINF_blocked;
> -    struct vcpu_runstate_info runstate;
>  
>      memset(info, 0, sizeof(*info));
>  
> @@ -69,8 +68,7 @@ void getdomaininfo(struct domain *d, str
>       */
>      for_each_vcpu ( d, v )
>      {
> -        vcpu_runstate_get(v, &runstate);
> -        cpu_time += runstate.time[RUNSTATE_running];
> +        cpu_time += vcpu_runstate_get_running(v);
>          info->max_vcpu_id = v->vcpu_id;
>          if ( !(v->pause_flags & VPF_down) )
>          {
> @@ -829,8 +827,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>  
>      case XEN_DOMCTL_getvcpuinfo:
>      {
> -        struct vcpu   *v;
> -        struct vcpu_runstate_info runstate;
> +        const struct vcpu *v;
>  
>          ret = -EINVAL;
>          if ( op->u.getvcpuinfo.vcpu >= d->max_vcpus )
> @@ -840,12 +837,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>          if ( (v = d->vcpu[op->u.getvcpuinfo.vcpu]) == NULL )
>              break;
>  
> -        vcpu_runstate_get(v, &runstate);
> -
>          op->u.getvcpuinfo.online   = !(v->pause_flags & VPF_down);
>          op->u.getvcpuinfo.blocked  = !!(v->pause_flags & VPF_blocked);
>          op->u.getvcpuinfo.running  = v->is_running;
> -        op->u.getvcpuinfo.cpu_time = runstate.time[RUNSTATE_running];
> +        op->u.getvcpuinfo.cpu_time = vcpu_runstate_get_running(v);
>          op->u.getvcpuinfo.cpu      = v->processor;
>          ret = 0;
>          copyback = 1;
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -325,15 +325,35 @@ void vcpu_runstate_get(const struct vcpu
>      }
>  }
>  
> -uint64_t get_cpu_idle_time(unsigned int cpu)
> +uint64_t vcpu_runstate_get_running(const struct vcpu *v)
>  {
> -    struct vcpu_runstate_info state = { 0 };
> -    const struct vcpu *v = idle_vcpu[cpu];
> +    struct seqcount seq = SEQCNT_ZERO();
> +    const struct seqcount *s = v == current ? &seq : &v->runstate_seq;

Does it make a difference to use a local fake sequence counter or the
real one if the vCPU is the one currently running in this pCPU?  If
it's the running vCPU then it won't get the counters updated, and
hence using the real or a fake counter won't matter, as it will never
be updated while carrying out the read.  IOW: the usage of a local
sequence counter for that specific case just adds more logic without a
real benefit?

Thanks, Roger.

Re: [PATCH for-4.22? 1/9] sched: introduce specialization of "running only" vcpu_runstate_get()

[Jan Beulich]

On 16.06.2026 10:41, Roger Pau Monné wrote:
> On Mon, Jun 15, 2026 at 04:12:11PM +0200, Jan Beulich wrote:
>> About half the callers of vcpu_runstate_get() are solely after the
>> "running" time of a vCPU. Introduce a specialization with a smaller
>> read critical section and thus reduced risk of a need for retries.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -325,15 +325,35 @@ void vcpu_runstate_get(const struct vcpu
>>      }
>>  }
>>  
>> -uint64_t get_cpu_idle_time(unsigned int cpu)
>> +uint64_t vcpu_runstate_get_running(const struct vcpu *v)
>>  {
>> -    struct vcpu_runstate_info state = { 0 };
>> -    const struct vcpu *v = idle_vcpu[cpu];
>> +    struct seqcount seq = SEQCNT_ZERO();
>> +    const struct seqcount *s = v == current ? &seq : &v->runstate_seq;
> 
> Does it make a difference to use a local fake sequence counter or the
> real one if the vCPU is the one currently running in this pCPU?  If
> it's the running vCPU then it won't get the counters updated, and
> hence using the real or a fake counter won't matter, as it will never
> be updated while carrying out the read.  IOW: the usage of a local
> sequence counter for that specific case just adds more logic without a
> real benefit?

While the counter wouldn't be updated, adjacent items in struct vcpu may
be touched from remote CPUs. By using a stack variable we avoid possible
cache line bouncing in that case.

In any event - what is done here follows what vcpu_runstate_get() does,
i.e. this is meant to strictly be a special case thereof. If we wanted
to omit this extra logic, it should also (either first, or subsequently
in both places) be dropped from there.

Jan

Re: [PATCH for-4.22? 1/9] sched: introduce specialization of "running only" vcpu_runstate_get()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 365 bytes --]

On 15.06.26 16:12, Jan Beulich wrote:
> About half the callers of vcpu_runstate_get() are solely after the
> "running" time of a vCPU. Introduce a specialization with a smaller
> read critical section and thus reduced risk of a need for retries.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH for-4.22? 2/9] domctl: move XEN_DOMCTL_irq_permission handling to x86 code

[Jan Beulich]

HAS_PIRQ is selected by x86 only, and that's expected to remain that way.
Avoid the #ifdef needed by moving the logic to arch_do_domctl(). Leverage
"currd" being available as a local variable there while doing so.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -258,6 +258,36 @@ long arch_do_domctl(
         break;
     }
 
+    case XEN_DOMCTL_irq_permission:
+    {
+        unsigned int pirq = domctl->u.irq_permission.pirq, irq;
+        bool allow = domctl->u.irq_permission.allow_access;
+
+        ret = -EINVAL;
+        if ( pirq >= currd->nr_pirqs )
+            break;
+
+        irq = domain_pirq_to_irq(currd, pirq);
+
+        ret = -EPERM;
+        if ( irq )
+            ret = xsm_irq_permission(XSM_PRIV, d, irq, allow);
+        if ( ret )
+            break;
+
+        iocaps_double_lock(d, true);
+
+        if ( !irq_access_permitted(currd, irq) )
+            ret = -EPERM;
+        else if ( allow )
+            ret = irq_permit_access(d, irq);
+        else
+            ret = irq_deny_access(d, irq);
+
+        iocaps_double_unlock(d, true);
+        break;
+    }
+
     case XEN_DOMCTL_gsi_permission:
     {
         int irq;
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -471,38 +471,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         goto domctl_out_unlock_domonly;
     }
 
-#ifdef CONFIG_HAS_PIRQ
-    case XEN_DOMCTL_irq_permission:
-    {
-        unsigned int pirq = op->u.irq_permission.pirq, irq;
-        bool allow = op->u.irq_permission.allow_access;
-
-        ret = -EINVAL;
-        if ( pirq >= current->domain->nr_pirqs )
-            goto domctl_out_unlock_domonly;
-
-        irq = domain_pirq_to_irq(current->domain, pirq);
-
-        ret = -EPERM;
-        if ( irq )
-            ret = xsm_irq_permission(XSM_PRIV, d, irq, allow);
-        if ( ret )
-            goto domctl_out_unlock_domonly;
-
-        iocaps_double_lock(d, true);
-
-        if ( !irq_access_permitted(current->domain, irq) )
-            ret = -EPERM;
-        else if ( allow )
-            ret = irq_permit_access(d, irq);
-        else
-            ret = irq_deny_access(d, irq);
-
-        iocaps_double_unlock(d, true);
-        goto domctl_out_unlock_domonly;
-    }
-#endif
-
     case XEN_DOMCTL_set_target:
     {
         struct domain *e = get_domain_by_id(op->u.set_target.target);
@@ -563,6 +531,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
     case XEN_DOMCTL_ioport_permission:
     case XEN_DOMCTL_ioport_mapping:
+    case XEN_DOMCTL_irq_permission:
     case XEN_DOMCTL_gsi_permission:
     case XEN_DOMCTL_bind_pt_irq:
     case XEN_DOMCTL_unbind_pt_irq:

Re: [PATCH for-4.22? 2/9] domctl: move XEN_DOMCTL_irq_permission handling to x86 code

[Oleksii Kurochko]

On 6/15/26 4:12 PM, Jan Beulich wrote:
> HAS_PIRQ is selected by x86 only, and that's expected to remain that way.
> Avoid the #ifdef needed by moving the logic to arch_do_domctl(). Leverage
> "currd" being available as a local variable there while doing so.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 

Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

[PATCH for-4.22? 3/9] domctl: rename a label

[Jan Beulich]

There's no real domain unlocking here, it's merely RCU which is being
"unlocked".

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -373,7 +373,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
             copyback = true;
         }
 
-        goto domctl_out_unlock_domonly;
+        goto domctl_out_unlock_rcuonly;
 
     case XEN_DOMCTL_get_domain_state:
         ret = xsm_get_domain_state(XSM_XS_PRIV, d);
@@ -381,7 +381,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
             ret = get_domain_state(&op->u.get_domain_state, d, &op->domain);
         if ( !ret )
             copyback = true;
-        goto domctl_out_unlock_domonly;
+        goto domctl_out_unlock_rcuonly;
 
     case XEN_DOMCTL_iomem_permission:
     {
@@ -391,11 +391,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
         ret = -EINVAL;
         if ( (mfn + nr_mfns - 1) < mfn ) /* Wrap? */
-            goto domctl_out_unlock_domonly;
+            goto domctl_out_unlock_rcuonly;
 
         ret = xsm_iomem_permission(XSM_PRIV, d, mfn, mfn + nr_mfns - 1, allow);
         if ( ret )
-            goto domctl_out_unlock_domonly;
+            goto domctl_out_unlock_rcuonly;
 
         iocaps_double_lock(d, true);
 
@@ -408,7 +408,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
             ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
 
         iocaps_double_unlock(d, true);
-        goto domctl_out_unlock_domonly;
+        goto domctl_out_unlock_rcuonly;
     }
 
     case XEN_DOMCTL_memory_mapping:
@@ -423,17 +423,17 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         if ( mfn_end < mfn || /* Wrap? */
              ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
              (gfn + nr_mfns - 1) < gfn ) /* Wrap? */
-            goto domctl_out_unlock_domonly;
+            goto domctl_out_unlock_rcuonly;
 
         ret = xsm_iomem_mapping(XSM_DM_PRIV, d, mfn, mfn_end, add);
         if ( ret || !paging_mode_translate(d) )
-            goto domctl_out_unlock_domonly;
+            goto domctl_out_unlock_rcuonly;
 
 #ifndef CONFIG_X86 /* XXX ARM!? */
         ret = -E2BIG;
         /* Must break hypercall up as this could take a while. */
         if ( nr_mfns > 64 )
-            goto domctl_out_unlock_domonly;
+            goto domctl_out_unlock_rcuonly;
 #endif
 
         iocaps_double_lock(d, false);
@@ -468,7 +468,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         }
 
         iocaps_double_unlock(d, false);
-        goto domctl_out_unlock_domonly;
+        goto domctl_out_unlock_rcuonly;
     }
 
     case XEN_DOMCTL_set_target:
@@ -477,7 +477,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
         ret = -ESRCH;
         if ( !e )
-            goto domctl_out_unlock_domonly;
+            goto domctl_out_unlock_rcuonly;
 
         if ( d == e )
             ret = -EINVAL;
@@ -492,7 +492,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
         if ( ret )
             put_domain(e);
-        goto domctl_out_unlock_domonly;
+        goto domctl_out_unlock_rcuonly;
     }
 
     case XEN_DOMCTL_vm_event_op:
@@ -502,12 +502,12 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
             ret = vm_event_domctl(d, &op->u.vm_event_op);
             if ( !ret )
                 copyback = true;
-            goto domctl_out_unlock_domonly;
+            goto domctl_out_unlock_rcuonly;
         }
         if ( !d )
         {
             ret = -ESRCH;
-            goto domctl_out_unlock_domonly;
+            goto domctl_out_unlock_rcuonly;
         }
         /* Other sub-ops handled further down. */
         break;
@@ -517,17 +517,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
              op->u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_PEEK )
         {
             ret = xsm_domctl(XSM_OTHER, d, op);
-            if ( ret )
-                goto domctl_out_unlock_domonly;
-
-            ret = arch_do_domctl(op, d, u_domctl);
-            goto domctl_out_unlock_domonly;
+            if ( !ret )
+                ret = arch_do_domctl(op, d, u_domctl);
+            goto domctl_out_unlock_rcuonly;
         }
         break;
 
     case XEN_DOMCTL_get_device_group:
         ret = iommu_do_domctl(op, d, u_domctl);
-        goto domctl_out_unlock_domonly;
+        goto domctl_out_unlock_rcuonly;
 
     case XEN_DOMCTL_ioport_permission:
     case XEN_DOMCTL_ioport_mapping:
@@ -537,7 +535,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
     case XEN_DOMCTL_unbind_pt_irq:
     case XEN_DOMCTL_getpageframeinfo3:
         ret = arch_do_domctl(op, d, u_domctl);
-        goto domctl_out_unlock_domonly;
+        goto domctl_out_unlock_rcuonly;
 
     default:
         /* Everything else handled further down. */
@@ -546,7 +544,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
     ret = xsm_domctl(XSM_OTHER, d, op);
     if ( ret )
-        goto domctl_out_unlock_domonly;
+        goto domctl_out_unlock_rcuonly;
 
     if ( !domctl_lock_acquire() )
     {
@@ -941,7 +939,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
     domctl_lock_release();
 
- domctl_out_unlock_domonly:
+ domctl_out_unlock_rcuonly:
     if ( d && !is_system_domain(d) )
         rcu_unlock_domain(d);
 

Re: [PATCH for-4.22? 3/9] domctl: rename a label

[Oleksii Kurochko]

On 6/15/26 4:13 PM, Jan Beulich wrote:
> There's no real domain unlocking here, it's merely RCU which is being
> "unlocked".
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Release-Acked-by: Oleskii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

Re: [PATCH for-4.22? 3/9] domctl: rename a label

[Roger Pau Monné]

On Mon, Jun 15, 2026 at 04:13:01PM +0200, Jan Beulich wrote:
> There's no real domain unlocking here, it's merely RCU which is being
> "unlocked".
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[PATCH for-4.22? 4/9] domctl: error code adjustment for unpriv callers

[Jan Beulich]

Unprivileged callers better wouldn't be in the position of figuring out
domain existence from error codes. Adjust the respective path sitting
ahead of XSM checks to produce -EPERM in such cases, just like the
subsequent XSM check would yield.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
---
With more lockless cases likely to appear down the road, we may want to
centralize determining which error code to use, latching the result into
a local variable.

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -356,7 +356,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
     default:
         d = rcu_lock_domain_by_id(op->domain);
         if ( !d )
-            return -ESRCH;
+            return is_control_domain(current->domain) ? -ESRCH : -EPERM;
         break;
     }
 

Re: [PATCH for-4.22? 4/9] domctl: error code adjustment for unpriv callers

[Oleksii Kurochko]

On 6/15/26 4:13 PM, Jan Beulich wrote:
> Unprivileged callers better wouldn't be in the position of figuring out
> domain existence from error codes. Adjust the respective path sitting
> ahead of XSM checks to produce -EPERM in such cases, just like the
> subsequent XSM check would yield.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

[PATCH for-4.22? 5/9] domctl/XSM: avoid XSM_OTHER with xsm_domctl()

[Jan Beulich]

Make explicit at the call sites what (default) permission is required.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -331,7 +331,7 @@ long arch_do_domctl(
         /* Games to allow this code block to handle a compat guest. */
         void __user *guest_handle = domctl->u.getpageframeinfo3.array.p;
 
-        ret = xsm_domctl(XSM_OTHER, d, domctl);
+        ret = xsm_domctl(XSM_PRIV, d, domctl);
         if ( ret )
             break;
 
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -743,7 +743,7 @@ long do_paging_domctl_cont(
     if ( d == NULL )
         return -ESRCH;
 
-    ret = xsm_domctl(XSM_OTHER, d, &op);
+    ret = xsm_domctl(XSM_PRIV, d, &op);
     if ( !ret )
     {
         bool lock = !(op.u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_CLEAN ||
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -516,7 +516,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         if ( op->u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_CLEAN ||
              op->u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_PEEK )
         {
-            ret = xsm_domctl(XSM_OTHER, d, op);
+            ret = xsm_domctl(XSM_PRIV, d, op);
             if ( !ret )
                 ret = arch_do_domctl(op, d, u_domctl);
             goto domctl_out_unlock_rcuonly;
@@ -542,7 +542,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         break;
     }
 
-    ret = xsm_domctl(XSM_OTHER, d, op);
+    ret = xsm_domctl(XSM_PRIV, d, op);
     if ( ret )
         goto domctl_out_unlock_rcuonly;
 
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -157,7 +157,7 @@ static XSM_INLINE int cf_check xsm_set_t
 static XSM_INLINE int cf_check xsm_domctl(
     XSM_DEFAULT_ARG struct domain *d, struct xen_domctl *op)
 {
-    XSM_ASSERT_ACTION(XSM_OTHER);
+    XSM_ASSERT_ACTION(XSM_PRIV);
     switch ( op->cmd )
     {
     case XEN_DOMCTL_bind_pt_irq:
@@ -176,7 +176,7 @@ static XSM_INLINE int cf_check xsm_domct
         return -EILSEQ;
 
     default:
-        return xsm_default_action(XSM_PRIV, current->domain, d);
+        return xsm_default_action(action, current->domain, d);
     }
 }
 

Re: [PATCH for-4.22? 5/9] domctl/XSM: avoid XSM_OTHER with xsm_domctl()

[Daniel P. Smith]

On 6/15/26 10:13 AM, Jan Beulich wrote:
> Make explicit at the call sites what (default) permission is required.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -331,7 +331,7 @@ long arch_do_domctl(
>           /* Games to allow this code block to handle a compat guest. */
>           void __user *guest_handle = domctl->u.getpageframeinfo3.array.p;
>   
> -        ret = xsm_domctl(XSM_OTHER, d, domctl);
> +        ret = xsm_domctl(XSM_PRIV, d, domctl);
>           if ( ret )
>               break;
>   
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -743,7 +743,7 @@ long do_paging_domctl_cont(
>       if ( d == NULL )
>           return -ESRCH;
>   
> -    ret = xsm_domctl(XSM_OTHER, d, &op);
> +    ret = xsm_domctl(XSM_PRIV, d, &op);
>       if ( !ret )
>       {
>           bool lock = !(op.u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_CLEAN ||
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -516,7 +516,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>           if ( op->u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_CLEAN ||
>                op->u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_PEEK )
>           {
> -            ret = xsm_domctl(XSM_OTHER, d, op);
> +            ret = xsm_domctl(XSM_PRIV, d, op);
>               if ( !ret )
>                   ret = arch_do_domctl(op, d, u_domctl);
>               goto domctl_out_unlock_rcuonly;
> @@ -542,7 +542,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>           break;
>       }
>   
> -    ret = xsm_domctl(XSM_OTHER, d, op);
> +    ret = xsm_domctl(XSM_PRIV, d, op);
>       if ( ret )
>           goto domctl_out_unlock_rcuonly;
>   
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -157,7 +157,7 @@ static XSM_INLINE int cf_check xsm_set_t
>   static XSM_INLINE int cf_check xsm_domctl(
>       XSM_DEFAULT_ARG struct domain *d, struct xen_domctl *op)
>   {
> -    XSM_ASSERT_ACTION(XSM_OTHER);
> +    XSM_ASSERT_ACTION(XSM_PRIV);
>       switch ( op->cmd )
>       {
>       case XEN_DOMCTL_bind_pt_irq:
> @@ -176,7 +176,7 @@ static XSM_INLINE int cf_check xsm_domct
>           return -EILSEQ;
>   
>       default:
> -        return xsm_default_action(XSM_PRIV, current->domain, d);
> +        return xsm_default_action(action, current->domain, d);
>       }
>   }
>   
> 

Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>

[PATCH for-4.22? 6/9] domctl: handle XEN_DOMCTL_getvcpuinfo without acquiring domctl lock

[Jan Beulich]

Like for XEN_DOMCTL_getdomaininfo there's no need to hold the domctl
lock for XEN_DOMCTL_getvcpuinfo. While moving the code also switch to
using domain_vcpu().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tentatively-acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -546,6 +546,33 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
     if ( ret )
         goto domctl_out_unlock_rcuonly;
 
+    switch ( op->cmd )
+    {
+    case XEN_DOMCTL_getvcpuinfo:
+    {
+        const struct vcpu *v;
+
+        if ( (v = domain_vcpu(d, op->u.getvcpuinfo.vcpu)) == NULL )
+        {
+            ret = -ENOENT;
+            goto domctl_out_unlock_rcuonly;
+        }
+
+        op->u.getvcpuinfo.online   = !(v->pause_flags & VPF_down);
+        op->u.getvcpuinfo.blocked  = !!(v->pause_flags & VPF_blocked);
+        op->u.getvcpuinfo.running  = v->is_running;
+        op->u.getvcpuinfo.cpu_time = vcpu_runstate_get_running(v);
+        op->u.getvcpuinfo.cpu      = v->processor;
+
+        copyback = true;
+        goto domctl_out_unlock_rcuonly;
+    }
+
+    default:
+        /* Everything else handled further up or further down. */
+        break;
+    }
+
     if ( !domctl_lock_acquire() )
     {
         if ( d && d != dom_io )
@@ -792,28 +819,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         break;
     }
 
-    case XEN_DOMCTL_getvcpuinfo:
-    {
-        const struct vcpu *v;
-
-        ret = -EINVAL;
-        if ( op->u.getvcpuinfo.vcpu >= d->max_vcpus )
-            break;
-
-        ret = -ESRCH;
-        if ( (v = d->vcpu[op->u.getvcpuinfo.vcpu]) == NULL )
-            break;
-
-        op->u.getvcpuinfo.online   = !(v->pause_flags & VPF_down);
-        op->u.getvcpuinfo.blocked  = !!(v->pause_flags & VPF_blocked);
-        op->u.getvcpuinfo.running  = v->is_running;
-        op->u.getvcpuinfo.cpu_time = vcpu_runstate_get_running(v);
-        op->u.getvcpuinfo.cpu      = v->processor;
-        ret = 0;
-        copyback = 1;
-        break;
-    }
-
     case XEN_DOMCTL_max_mem:
     {
         uint64_t new_max = op->u.max_mem.max_memkb >> (PAGE_SHIFT - 10);

Re: [PATCH for-4.22? 6/9] domctl: handle XEN_DOMCTL_getvcpuinfo without acquiring domctl lock

[Roger Pau Monné]

On Mon, Jun 15, 2026 at 04:15:12PM +0200, Jan Beulich wrote:
> Like for XEN_DOMCTL_getdomaininfo there's no need to hold the domctl
> lock for XEN_DOMCTL_getvcpuinfo. While moving the code also switch to
> using domain_vcpu().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tentatively-acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Some suggestions below.

> 
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -546,6 +546,33 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>      if ( ret )
>          goto domctl_out_unlock_rcuonly;
>  
> +    switch ( op->cmd )
> +    {
> +    case XEN_DOMCTL_getvcpuinfo:
> +    {
> +        const struct vcpu *v;
> +
> +        if ( (v = domain_vcpu(d, op->u.getvcpuinfo.vcpu)) == NULL )

Since you are moving the code anyway, I would rather do:

        const struct vcpu *v = domain_vcpu(d, op->u.getvcpuinfo.vcpu);

        if ( !v )
            ...

> +        {
> +            ret = -ENOENT;
> +            goto domctl_out_unlock_rcuonly;
> +        }
> +
> +        op->u.getvcpuinfo.online   = !(v->pause_flags & VPF_down);
> +        op->u.getvcpuinfo.blocked  = !!(v->pause_flags & VPF_blocked);
> +        op->u.getvcpuinfo.running  = v->is_running;
> +        op->u.getvcpuinfo.cpu_time = vcpu_runstate_get_running(v);
> +        op->u.getvcpuinfo.cpu      = v->processor;
> +
> +        copyback = true;
> +        goto domctl_out_unlock_rcuonly;
> +    }
> +
> +    default:
> +        /* Everything else handled further up or further down. */
> +        break;
> +    }

As you are introducing this here, we might want to also move
XEN_DOMCTL_shadow_op handling into this new switch block: with the RCU
taken and after the xsm_domctl() call.

Thanks, Roger.

Re: [PATCH for-4.22? 6/9] domctl: handle XEN_DOMCTL_getvcpuinfo without acquiring domctl lock

[Jan Beulich]

On 16.06.2026 10:56, Roger Pau Monné wrote:
> On Mon, Jun 15, 2026 at 04:15:12PM +0200, Jan Beulich wrote:
>> Like for XEN_DOMCTL_getdomaininfo there's no need to hold the domctl
>> lock for XEN_DOMCTL_getvcpuinfo. While moving the code also switch to
>> using domain_vcpu().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Tentatively-acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -546,6 +546,33 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>>      if ( ret )
>>          goto domctl_out_unlock_rcuonly;
>>  
>> +    switch ( op->cmd )
>> +    {
>> +    case XEN_DOMCTL_getvcpuinfo:
>> +    {
>> +        const struct vcpu *v;
>> +
>> +        if ( (v = domain_vcpu(d, op->u.getvcpuinfo.vcpu)) == NULL )
> 
> Since you are moving the code anyway, I would rather do:
> 
>         const struct vcpu *v = domain_vcpu(d, op->u.getvcpuinfo.vcpu);
> 
>         if ( !v )
>             ...

Ah yes, I agree.

>> +        {
>> +            ret = -ENOENT;
>> +            goto domctl_out_unlock_rcuonly;
>> +        }
>> +
>> +        op->u.getvcpuinfo.online   = !(v->pause_flags & VPF_down);
>> +        op->u.getvcpuinfo.blocked  = !!(v->pause_flags & VPF_blocked);
>> +        op->u.getvcpuinfo.running  = v->is_running;
>> +        op->u.getvcpuinfo.cpu_time = vcpu_runstate_get_running(v);
>> +        op->u.getvcpuinfo.cpu      = v->processor;
>> +
>> +        copyback = true;
>> +        goto domctl_out_unlock_rcuonly;
>> +    }
>> +
>> +    default:
>> +        /* Everything else handled further up or further down. */
>> +        break;
>> +    }
> 
> As you are introducing this here, we might want to also move
> XEN_DOMCTL_shadow_op handling into this new switch block: with the RCU
> taken and after the xsm_domctl() call.

Indeed, as indicated yesterday in [1]. Meanwhile I have a patch ready to
include in a possible v2 of this series.

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2026-06/msg00861.html

[PATCH for-4.22? 7/9] domctl: restrict permission check for XEN_DOMCTL_memory_mapping's remove form

[Jan Beulich]

Like is already done for I/O ports on x86 and for IRQ unbinding, check
only the requesting domain's permissions (for it to not interfere with
MMIO backed by another stubdom DM), but not the target domain's: Removal
should be okay even (perhaps: especially) when permissions were already
revoked.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -436,11 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
             goto domctl_out_unlock_rcuonly;
 #endif
 
+        /*
+         * NB: The double lock isn't really needed when !add, but is used anyway
+         * to keep things simple.
+         */
         iocaps_double_lock(d, false);
 
         ret = -EPERM;
         if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
-             !iomem_access_permitted(d, mfn, mfn_end) )
+             (add && !iomem_access_permitted(d, mfn, mfn_end)) )
             /* Nothing. */;
         else if ( add )
         {

Re: [PATCH for-4.22? 7/9] domctl: restrict permission check for XEN_DOMCTL_memory_mapping's remove form

[Roger Pau Monné]

On Mon, Jun 15, 2026 at 04:15:36PM +0200, Jan Beulich wrote:
> Like is already done for I/O ports on x86 and for IRQ unbinding, check
> only the requesting domain's permissions (for it to not interfere with
> MMIO backed by another stubdom DM), but not the target domain's: Removal
> should be okay even (perhaps: especially) when permissions were already
> revoked.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -436,11 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>              goto domctl_out_unlock_rcuonly;
>  #endif
>  
> +        /*
> +         * NB: The double lock isn't really needed when !add, but is used anyway
> +         * to keep things simple.
> +         */
>          iocaps_double_lock(d, false);
>  
>          ret = -EPERM;
>          if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
> -             !iomem_access_permitted(d, mfn, mfn_end) )
> +             (add && !iomem_access_permitted(d, mfn, mfn_end)) )

You seem to be doing the opposite of what the commit message states
here, and checking for permissions on the target domain, not
permissions of the requesting domain?

XEN_DOMCTL_ioport_mapping does check against current->domain, and not
against d.

FWIW, we could also remove one branch here by doing:

ret = -EPERM
if ( add && iomem_access_permitted(current->domain, mfn, mfn_end) )
{
    /* add logic. */
}
else if ( !add )
{
    /* remove logic. */
}

Thanks, Roger.

Re: [PATCH for-4.22? 7/9] domctl: restrict permission check for XEN_DOMCTL_memory_mapping's remove form

[Jan Beulich]

On 16.06.2026 11:08, Roger Pau Monné wrote:
> On Mon, Jun 15, 2026 at 04:15:36PM +0200, Jan Beulich wrote:
>> Like is already done for I/O ports on x86 and for IRQ unbinding, check
>> only the requesting domain's permissions (for it to not interfere with
>> MMIO backed by another stubdom DM), but not the target domain's: Removal
>> should be okay even (perhaps: especially) when permissions were already
>> revoked.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -436,11 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>>              goto domctl_out_unlock_rcuonly;
>>  #endif
>>  
>> +        /*
>> +         * NB: The double lock isn't really needed when !add, but is used anyway
>> +         * to keep things simple.
>> +         */
>>          iocaps_double_lock(d, false);
>>  
>>          ret = -EPERM;
>>          if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
>> -             !iomem_access_permitted(d, mfn, mfn_end) )
>> +             (add && !iomem_access_permitted(d, mfn, mfn_end)) )
> 
> You seem to be doing the opposite of what the commit message states
> here, and checking for permissions on the target domain, not
> permissions of the requesting domain?

I'm always checking permissions of the requesting domain, while the
target's are now checked only for "add". That's what the description
also says.

What's wrong with the description is ...

> XEN_DOMCTL_ioport_mapping does check against current->domain, and not
> against d.

... that it suggests this to be the behavior at the point of this patch,
when it really is moved to that only in patch 8. The patches used to be
ordered differently earlier on. I guess I should change the wording to
be closer to what's used in "x86/domctl: don't imply I/O port permissions
from I/O port mapping".

> FWIW, we could also remove one branch here by doing:
> 
> ret = -EPERM
> if ( add && iomem_access_permitted(current->domain, mfn, mfn_end) )
> {
>     /* add logic. */
> }
> else if ( !add )
> {
>     /* remove logic. */
> }

Indeed I was wondering whether something like this would be worthwhile,
but I opted for the variant with less overall churn.

Jan

Re: [PATCH for-4.22? 7/9] domctl: restrict permission check for XEN_DOMCTL_memory_mapping's remove form

[Roger Pau Monné]

On Tue, Jun 16, 2026 at 11:51:54AM +0200, Jan Beulich wrote:
> On 16.06.2026 11:08, Roger Pau Monné wrote:
> > On Mon, Jun 15, 2026 at 04:15:36PM +0200, Jan Beulich wrote:
> >> Like is already done for I/O ports on x86 and for IRQ unbinding, check
> >> only the requesting domain's permissions (for it to not interfere with
> >> MMIO backed by another stubdom DM), but not the target domain's: Removal
> >> should be okay even (perhaps: especially) when permissions were already
> >> revoked.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/xen/common/domctl.c
> >> +++ b/xen/common/domctl.c
> >> @@ -436,11 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
> >>              goto domctl_out_unlock_rcuonly;
> >>  #endif
> >>  
> >> +        /*
> >> +         * NB: The double lock isn't really needed when !add, but is used anyway
> >> +         * to keep things simple.
> >> +         */
> >>          iocaps_double_lock(d, false);
> >>  
> >>          ret = -EPERM;
> >>          if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
> >> -             !iomem_access_permitted(d, mfn, mfn_end) )
> >> +             (add && !iomem_access_permitted(d, mfn, mfn_end)) )
> > 
> > You seem to be doing the opposite of what the commit message states
> > here, and checking for permissions on the target domain, not
> > permissions of the requesting domain?
> 
> I'm always checking permissions of the requesting domain, while the
> target's are now checked only for "add". That's what the description
> also says.
> 
> What's wrong with the description is ...
> 
> > XEN_DOMCTL_ioport_mapping does check against current->domain, and not
> > against d.
> 
> ... that it suggests this to be the behavior at the point of this patch,
> when it really is moved to that only in patch 8. The patches used to be
> ordered differently earlier on. I guess I should change the wording to
> be closer to what's used in "x86/domctl: don't imply I/O port permissions
> from I/O port mapping".

Yeah, I've noticed after looking at the next patch.

> 
> > FWIW, we could also remove one branch here by doing:
> > 
> > ret = -EPERM
> > if ( add && iomem_access_permitted(current->domain, mfn, mfn_end) )
> > {
> >     /* add logic. */
> > }
> > else if ( !add )
> > {
> >     /* remove logic. */
> > }
> 
> Indeed I was wondering whether something like this would be worthwhile,
> but I opted for the variant with less overall churn.

Since you have to adjust the commit message, I wouldn't mind if you
also want to adjust the logic to remove the extra branch.

[PATCH for-4.22? 8/9] x86/domctl: don't imply I/O port permissions from I/O port mapping

[Jan Beulich]

Rather than granting permissions when mapping (an operation that DM-s are
allowed to carry out, while they can't invoke ioport-permission), check
whether permissions actually were granted when adding a mapping. This then
also allows relaxing the necessary locking.

Fixes: 192c4dabc344 ("domctl and p2m changes for PCI passthru")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
libxl has libxl__grant_vga_iomem_permission(), but I can't spot any I/O
port equivalent (nor a revoke counterpart, btw). Everywhere else MMIO and
I/O ports look to be treated equally.

Qemu uses both xc_domain_{iomem_permission,memory_mapping}() in
igd_write_opregion(), but only xc_domain_{memory,ioport}_mapping() in
xen_pt_region_update() and xen_pt_{,un}register_vga_regions(). Is the IGD
region special in any way? Clearly this can't work from a stubdom.

--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -714,9 +714,14 @@ long arch_do_domctl(
             break;
 
         hvm = &d->arch.hvm;
-        iocaps_double_lock(d, true);
+        /*
+         * NB: The double lock isn't really needed when !add, but is used anyway
+         * to keep things simple.
+         */
+        iocaps_double_lock(d, false);
 
-        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) )
+        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) ||
+             (add && !ioports_access_permitted(d, fmp, fmp + np - 1)) )
             ret = -EPERM;
         else if ( add )
         {
@@ -747,15 +752,6 @@ long arch_do_domctl(
                 list_add_tail(&g2m_ioport->list, &hvm->g2m_ioport_list);
             }
             write_unlock(&hvm->g2m_ioport_lock);
-            if ( !ret )
-                ret = ioports_permit_access(d, fmp, fmp + np - 1);
-            if ( ret && !found && g2m_ioport )
-            {
-                write_lock(&hvm->g2m_ioport_lock);
-                list_del(&g2m_ioport->list);
-                write_unlock(&hvm->g2m_ioport_lock);
-                xfree(g2m_ioport);
-            }
         }
         else
         {
@@ -772,15 +768,9 @@ long arch_do_domctl(
                     break;
                 }
             write_unlock(&hvm->g2m_ioport_lock);
-
-            ret = ioports_deny_access(d, fmp, fmp + np - 1);
-            if ( ret && is_hardware_domain(currd) )
-                printk(XENLOG_ERR
-                       "ioport_map: error %ld denying dom%d access to [%x,%x]\n",
-                       ret, d->domain_id, fmp, fmp + np - 1);
         }
 
-        iocaps_double_unlock(d, true);
+        iocaps_double_unlock(d, false);
         break;
     }
 

Re: [PATCH for-4.22? 8/9] x86/domctl: don't imply I/O port permissions from I/O port mapping

[Roger Pau Monné]

On Mon, Jun 15, 2026 at 04:16:11PM +0200, Jan Beulich wrote:
> Rather than granting permissions when mapping (an operation that DM-s are
> allowed to carry out, while they can't invoke ioport-permission), check
> whether permissions actually were granted when adding a mapping. This then
> also allows relaxing the necessary locking.
> 
> Fixes: 192c4dabc344 ("domctl and p2m changes for PCI passthru")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> libxl has libxl__grant_vga_iomem_permission(), but I can't spot any I/O
> port equivalent (nor a revoke counterpart, btw). Everywhere else MMIO and
> I/O ports look to be treated equally.
> 
> Qemu uses both xc_domain_{iomem_permission,memory_mapping}() in
> igd_write_opregion(), but only xc_domain_{memory,ioport}_mapping() in
> xen_pt_region_update() and xen_pt_{,un}register_vga_regions(). Is the IGD
> region special in any way? Clearly this can't work from a stubdom.
> 
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -714,9 +714,14 @@ long arch_do_domctl(
>              break;
>  
>          hvm = &d->arch.hvm;
> -        iocaps_double_lock(d, true);
> +        /*
> +         * NB: The double lock isn't really needed when !add, but is used anyway
> +         * to keep things simple.
> +         */
> +        iocaps_double_lock(d, false);
>  
> -        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) )
> +        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) ||
> +             (add && !ioports_access_permitted(d, fmp, fmp + np - 1)) )
>              ret = -EPERM;
>          else if ( add )
>          {
> @@ -747,15 +752,6 @@ long arch_do_domctl(
>                  list_add_tail(&g2m_ioport->list, &hvm->g2m_ioport_list);
>              }
>              write_unlock(&hvm->g2m_ioport_lock);
> -            if ( !ret )
> -                ret = ioports_permit_access(d, fmp, fmp + np - 1);
> -            if ( ret && !found && g2m_ioport )
> -            {
> -                write_lock(&hvm->g2m_ioport_lock);
> -                list_del(&g2m_ioport->list);
> -                write_unlock(&hvm->g2m_ioport_lock);
> -                xfree(g2m_ioport);
> -            }
>          }
>          else
>          {
> @@ -772,15 +768,9 @@ long arch_do_domctl(
>                      break;
>                  }
>              write_unlock(&hvm->g2m_ioport_lock);
> -
> -            ret = ioports_deny_access(d, fmp, fmp + np - 1);
> -            if ( ret && is_hardware_domain(currd) )
> -                printk(XENLOG_ERR
> -                       "ioport_map: error %ld denying dom%d access to [%x,%x]\n",
> -                       ret, d->domain_id, fmp, fmp + np - 1);
>          }
>  
> -        iocaps_double_unlock(d, true);
> +        iocaps_double_unlock(d, false);

I think the new behavior is more sane, however the problematic aspect
of this change is the removal case IMO: we cannot be sure whether
existing callers rely on XEN_DOMCTL_ioport_mapping also removing the
permissions, and hence Xen no longer removing the permissions might
lead to leaks.

This is a risk we might be willing to take, but it must be stated in
the commit message.  And likely in a CHANGELOG entry so that external
consumers are aware of this change and can adjust as necessary.

Thanks, Roger.

Re: [PATCH for-4.22? 8/9] x86/domctl: don't imply I/O port permissions from I/O port mapping

[Jan Beulich]

On 16.06.2026 11:21, Roger Pau Monné wrote:
> On Mon, Jun 15, 2026 at 04:16:11PM +0200, Jan Beulich wrote:
>> Rather than granting permissions when mapping (an operation that DM-s are
>> allowed to carry out, while they can't invoke ioport-permission), check
>> whether permissions actually were granted when adding a mapping. This then
>> also allows relaxing the necessary locking.
>>
>> Fixes: 192c4dabc344 ("domctl and p2m changes for PCI passthru")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> libxl has libxl__grant_vga_iomem_permission(), but I can't spot any I/O
>> port equivalent (nor a revoke counterpart, btw). Everywhere else MMIO and
>> I/O ports look to be treated equally.
>>
>> Qemu uses both xc_domain_{iomem_permission,memory_mapping}() in
>> igd_write_opregion(), but only xc_domain_{memory,ioport}_mapping() in
>> xen_pt_region_update() and xen_pt_{,un}register_vga_regions(). Is the IGD
>> region special in any way? Clearly this can't work from a stubdom.

Both of these remarks are relevant to your response below. I realize I should
have Cc-ed Anthony, for him to comment on them.

>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -714,9 +714,14 @@ long arch_do_domctl(
>>              break;
>>  
>>          hvm = &d->arch.hvm;
>> -        iocaps_double_lock(d, true);
>> +        /*
>> +         * NB: The double lock isn't really needed when !add, but is used anyway
>> +         * to keep things simple.
>> +         */
>> +        iocaps_double_lock(d, false);
>>  
>> -        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) )
>> +        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) ||
>> +             (add && !ioports_access_permitted(d, fmp, fmp + np - 1)) )
>>              ret = -EPERM;
>>          else if ( add )
>>          {
>> @@ -747,15 +752,6 @@ long arch_do_domctl(
>>                  list_add_tail(&g2m_ioport->list, &hvm->g2m_ioport_list);
>>              }
>>              write_unlock(&hvm->g2m_ioport_lock);
>> -            if ( !ret )
>> -                ret = ioports_permit_access(d, fmp, fmp + np - 1);
>> -            if ( ret && !found && g2m_ioport )
>> -            {
>> -                write_lock(&hvm->g2m_ioport_lock);
>> -                list_del(&g2m_ioport->list);
>> -                write_unlock(&hvm->g2m_ioport_lock);
>> -                xfree(g2m_ioport);
>> -            }
>>          }
>>          else
>>          {
>> @@ -772,15 +768,9 @@ long arch_do_domctl(
>>                      break;
>>                  }
>>              write_unlock(&hvm->g2m_ioport_lock);
>> -
>> -            ret = ioports_deny_access(d, fmp, fmp + np - 1);
>> -            if ( ret && is_hardware_domain(currd) )
>> -                printk(XENLOG_ERR
>> -                       "ioport_map: error %ld denying dom%d access to [%x,%x]\n",
>> -                       ret, d->domain_id, fmp, fmp + np - 1);
>>          }
>>  
>> -        iocaps_double_unlock(d, true);
>> +        iocaps_double_unlock(d, false);
> 
> I think the new behavior is more sane, however the problematic aspect
> of this change is the removal case IMO: we cannot be sure whether
> existing callers rely on XEN_DOMCTL_ioport_mapping also removing the
> permissions, and hence Xen no longer removing the permissions might
> lead to leaks.
> 
> This is a risk we might be willing to take, but it must be stated in
> the commit message.

I've added

"While no longer granting permissions upon mapping is "only" at risk of
 breaking guests, no longer revoking permissions upon unmapping strictly
 requires callers to additionally invoke XEN_DOMCTL_ioport_permission. Or
 else a security issue would arise. In-tree code already does so."

>  And likely in a CHANGELOG entry so that external
> consumers are aware of this change and can adjust as necessary.

Will do.

Jan

Re: [PATCH for-4.22? 8/9] x86/domctl: don't imply I/O port permissions from I/O port mapping

[Roger Pau Monné]

On Tue, Jun 16, 2026 at 11:36:53AM +0200, Jan Beulich wrote:
> On 16.06.2026 11:21, Roger Pau Monné wrote:
> > On Mon, Jun 15, 2026 at 04:16:11PM +0200, Jan Beulich wrote:
> >> Rather than granting permissions when mapping (an operation that DM-s are
> >> allowed to carry out, while they can't invoke ioport-permission), check
> >> whether permissions actually were granted when adding a mapping. This then
> >> also allows relaxing the necessary locking.
> >>
> >> Fixes: 192c4dabc344 ("domctl and p2m changes for PCI passthru")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> libxl has libxl__grant_vga_iomem_permission(), but I can't spot any I/O
> >> port equivalent (nor a revoke counterpart, btw). Everywhere else MMIO and
> >> I/O ports look to be treated equally.
> >>
> >> Qemu uses both xc_domain_{iomem_permission,memory_mapping}() in
> >> igd_write_opregion(), but only xc_domain_{memory,ioport}_mapping() in
> >> xen_pt_region_update() and xen_pt_{,un}register_vga_regions(). Is the IGD
> >> region special in any way? Clearly this can't work from a stubdom.
> 
> Both of these remarks are relevant to your response below. I realize I should
> have Cc-ed Anthony, for him to comment on them.

Partially yes, but those are only for the callers we know.  My comment
was thinking about possible out-of-tree users.

> >> --- a/xen/arch/x86/domctl.c
> >> +++ b/xen/arch/x86/domctl.c
> >> @@ -714,9 +714,14 @@ long arch_do_domctl(
> >>              break;
> >>  
> >>          hvm = &d->arch.hvm;
> >> -        iocaps_double_lock(d, true);
> >> +        /*
> >> +         * NB: The double lock isn't really needed when !add, but is used anyway
> >> +         * to keep things simple.
> >> +         */
> >> +        iocaps_double_lock(d, false);
> >>  
> >> -        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) )
> >> +        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) ||
> >> +             (add && !ioports_access_permitted(d, fmp, fmp + np - 1)) )
> >>              ret = -EPERM;
> >>          else if ( add )
> >>          {
> >> @@ -747,15 +752,6 @@ long arch_do_domctl(
> >>                  list_add_tail(&g2m_ioport->list, &hvm->g2m_ioport_list);
> >>              }
> >>              write_unlock(&hvm->g2m_ioport_lock);
> >> -            if ( !ret )
> >> -                ret = ioports_permit_access(d, fmp, fmp + np - 1);
> >> -            if ( ret && !found && g2m_ioport )
> >> -            {
> >> -                write_lock(&hvm->g2m_ioport_lock);
> >> -                list_del(&g2m_ioport->list);
> >> -                write_unlock(&hvm->g2m_ioport_lock);
> >> -                xfree(g2m_ioport);
> >> -            }
> >>          }
> >>          else
> >>          {
> >> @@ -772,15 +768,9 @@ long arch_do_domctl(
> >>                      break;
> >>                  }
> >>              write_unlock(&hvm->g2m_ioport_lock);
> >> -
> >> -            ret = ioports_deny_access(d, fmp, fmp + np - 1);
> >> -            if ( ret && is_hardware_domain(currd) )
> >> -                printk(XENLOG_ERR
> >> -                       "ioport_map: error %ld denying dom%d access to [%x,%x]\n",
> >> -                       ret, d->domain_id, fmp, fmp + np - 1);
> >>          }
> >>  
> >> -        iocaps_double_unlock(d, true);
> >> +        iocaps_double_unlock(d, false);
> > 
> > I think the new behavior is more sane, however the problematic aspect
> > of this change is the removal case IMO: we cannot be sure whether
> > existing callers rely on XEN_DOMCTL_ioport_mapping also removing the
> > permissions, and hence Xen no longer removing the permissions might
> > lead to leaks.
> > 
> > This is a risk we might be willing to take, but it must be stated in
> > the commit message.
> 
> I've added
> 
> "While no longer granting permissions upon mapping is "only" at risk of
>  breaking guests, no longer revoking permissions upon unmapping strictly
>  requires callers to additionally invoke XEN_DOMCTL_ioport_permission. Or
>  else a security issue would arise. In-tree code already does so."
> 
> >  And likely in a CHANGELOG entry so that external
> > consumers are aware of this change and can adjust as necessary.
> 
> Will do.

Thanks, Roger.

[PATCH for-4.22? 9/9] x86/HVM: more checking for XEN_DOMCTL_ioport_mapping

[Jan Beulich]

When adding ranges, only alter existing ones when there is an exact match.
Don't accept ranges overlapping existing ones.

When removing ranges, only remove a range if there's an exact match.
Return an error when the range isn't found, and also don't call
ioports_deny_access() in that case.

Fixes: 192c4dabc344 ("domctl and p2m changes for PCI passthru")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Should "exact match" perhaps also include the guest port number? I'm
uncertain here as that kind of conflicts with "add" being treated as
"change" when the host port (and now count) match.

--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -731,14 +731,21 @@ long arch_do_domctl(
 
             write_lock(&hvm->g2m_ioport_lock);
             list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
-                if (g2m_ioport->mport == fmp )
+            {
+                if ( g2m_ioport->mport == fmp && g2m_ioport->np == np )
                 {
                     g2m_ioport->gport = fgp;
-                    g2m_ioport->np = np;
                     found = 1;
                     break;
                 }
-            if ( !found )
+                if ( fmp + np >= g2m_ioport->mport &&
+                     g2m_ioport->mport + g2m_ioport->np >= fmp )
+                {
+                    ret = -EBUSY;
+                    break;
+                }
+            }
+            if ( !found && !ret )
             {
                 g2m_ioport = xmalloc(struct g2m_ioport);
                 if ( !g2m_ioport )
@@ -759,12 +766,14 @@ long arch_do_domctl(
                    "ioport_map:remove: dom%d gport=%x mport=%x nr=%x\n",
                    d->domain_id, fgp, fmp, np);
 
+            ret = -ENOENT;
             write_lock(&hvm->g2m_ioport_lock);
             list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
-                if ( g2m_ioport->mport == fmp )
+                if ( g2m_ioport->mport == fmp && g2m_ioport->np == np )
                 {
                     list_del(&g2m_ioport->list);
                     xfree(g2m_ioport);
+                    ret = 0;
                     break;
                 }
             write_unlock(&hvm->g2m_ioport_lock);

Re: [PATCH for-4.22? 9/9] x86/HVM: more checking for XEN_DOMCTL_ioport_mapping

[Roger Pau Monné]

On Mon, Jun 15, 2026 at 04:16:41PM +0200, Jan Beulich wrote:
> When adding ranges, only alter existing ones when there is an exact match.
> Don't accept ranges overlapping existing ones.
> 
> When removing ranges, only remove a range if there's an exact match.
> Return an error when the range isn't found, and also don't call
> ioports_deny_access() in that case.

Isn't the ioports_deny_access() part stale now?  As you remove the
permission adjustments in patch 8/9.

> Fixes: 192c4dabc344 ("domctl and p2m changes for PCI passthru")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> Should "exact match" perhaps also include the guest port number? I'm
> uncertain here as that kind of conflicts with "add" being treated as
> "change" when the host port (and now count) match.

I think we want to keep the existing behavior and allow using an add
operation to change the guest port.

Thanks, Roger.

Re: [PATCH for-4.22? 9/9] x86/HVM: more checking for XEN_DOMCTL_ioport_mapping

[Jan Beulich]

On 16.06.2026 12:01, Roger Pau Monné wrote:
> On Mon, Jun 15, 2026 at 04:16:41PM +0200, Jan Beulich wrote:
>> When adding ranges, only alter existing ones when there is an exact match.
>> Don't accept ranges overlapping existing ones.
>>
>> When removing ranges, only remove a range if there's an exact match.
>> Return an error when the range isn't found, and also don't call
>> ioports_deny_access() in that case.
> 
> Isn't the ioports_deny_access() part stale now?  As you remove the
> permission adjustments in patch 8/9.

Oh, indeed. Dropped that last half sentence.

>> Fixes: 192c4dabc344 ("domctl and p2m changes for PCI passthru")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> Should "exact match" perhaps also include the guest port number? I'm
>> uncertain here as that kind of conflicts with "add" being treated as
>> "change" when the host port (and now count) match.
> 
> I think we want to keep the existing behavior and allow using an add
> operation to change the guest port.

Okay.

Jan