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

[PATCH v2 for-4.22? 0/7] 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.

v2, besides addressing review comments, has one new patch and includes
one patch which was previously submitted independently. See individual
patches for details on changes.

1: sched: introduce specialization of "running only" vcpu_runstate_get()
2: domctl: handle XEN_DOMCTL_getvcpuinfo without acquiring domctl lock
3: domctl: move early special casing of XEN_DOMCTL_shadow_op
4: domctl: restrict permission check for XEN_DOMCTL_memory_mapping's remove form
5: domctl: correct return value of XEN_DOMCTL_[gs]etvcpuaffinity
6: x86/domctl: don't imply I/O port permissions from I/O port mapping
7: x86/HVM: more checking for XEN_DOMCTL_ioport_mapping

Jan

[PATCH v2 for-4.22? 1/7] 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>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Juergen Gross <jgross@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) )
         {
@@ -796,8 +794,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 )
@@ -807,12 +804,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);

[PATCH v2 for-4.22? 2/7] 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>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
v2: Use initializer for local var.

--- 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 = 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;
+    }
+
     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);

[PATCH v2 for-4.22? 3/7] domctl: move early special casing of XEN_DOMCTL_shadow_op

[Jan Beulich]

This wants xsm_domctl() invoked, but the domctl lock not taken. Move the
handling to the respective switch(), thus eliminating the need for a
separate xsm_domctl() invocation.

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

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -512,17 +512,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         /* Other sub-ops handled further down. */
         break;
 
-    case XEN_DOMCTL_shadow_op:
-        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_PRIV, d, op);
-            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_rcuonly;
@@ -568,6 +557,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         goto domctl_out_unlock_rcuonly;
     }
 
+    case XEN_DOMCTL_shadow_op:
+        if ( op->u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_CLEAN ||
+             op->u.shadow_op.op == XEN_DOMCTL_SHADOW_OP_PEEK )
+        {
+            ret = arch_do_domctl(op, d, u_domctl);
+            goto domctl_out_unlock_rcuonly;
+        }
+        /* Other sub-ops handled by arch_do_domctl() further down. */
+        break;
+
     default:
         /* Everything else handled further up or further down. */
         break;

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

[Jan Beulich]

While the granting of permissions when mapping was already removed from
this operation, check whether permissions actually were granted when
adding a mapping; the check of the requester having permission remains
unaltered.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Avoid double evaluation of "add". Re-do description, to avoid
    mentioning behavior introduced only by a subsequent patch.

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -436,25 +436,16 @@ 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) )
+        if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
             /* Nothing. */;
-        else if ( add )
-        {
-            printk(XENLOG_G_DEBUG
-                   "memory_map:add: %pd gfn=%lx mfn=%lx nr=%lx\n",
-                   d, gfn, mfn, nr_mfns);
-
-            ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn));
-            if ( ret < 0 )
-                printk(XENLOG_G_WARNING
-                       "memory_map:fail: %pd gfn=%lx mfn=%lx nr=%lx ret:%ld\n",
-                       d, gfn, mfn, nr_mfns, ret);
-        }
-        else
+        else if ( !add )
         {
             printk(XENLOG_G_DEBUG
                    "memory_map:remove: %pd gfn=%lx mfn=%lx nr=%lx\n",
@@ -466,6 +457,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
                        "memory_map: error %ld removing %pd access to [%lx,%lx]\n",
                        ret, d, mfn, mfn_end);
         }
+        else if ( iomem_access_permitted(d, mfn, mfn_end) )
+        {
+            printk(XENLOG_G_DEBUG
+                   "memory_map:add: %pd gfn=%lx mfn=%lx nr=%lx\n",
+                   d, gfn, mfn, nr_mfns);
+
+            ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn));
+            if ( ret < 0 )
+                printk(XENLOG_G_WARNING
+                       "memory_map:fail: %pd gfn=%lx mfn=%lx nr=%lx ret:%ld\n",
+                       d, gfn, mfn, nr_mfns, ret);
+        }
 
         iocaps_double_unlock(d, false);
         goto domctl_out_unlock_rcuonly;

[PATCH v2 for-4.22? 5/7] domctl: correct return value of XEN_DOMCTL_[gs]etvcpuaffinity

[Jan Beulich]

cpumask_to_xenctl_bitmap() may return errors. Clearing the error indicator
of an earlier such call by a (successful) later call is misleading the
caller. For "set", keep setting soft affinity if the hard affinity copy-
back fails; only accumulate respective errors.

While fiddling with return values, also drop a redundant clearing of
"ret". This eliminates a Misra C:2012 rule 2.2 ("There shall be no dead
code") violation.

Fixes: 6e4ecc6d5884 ("sched: DOMCTL_*vcpuaffinity works with hard and soft affinity")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
v2: Consistently return first error. Integrate into series.

--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -1708,7 +1708,7 @@ int vcpu_affinity_domctl(struct domain *
 {
     struct vcpu *v;
     const struct sched_unit *unit;
-    int ret = 0;
+    int ret = 0, hret = 0;
 
     if ( vcpuaff->vcpu >= d->max_vcpus )
         return -EINVAL;
@@ -1746,19 +1746,17 @@ int vcpu_affinity_domctl(struct domain *
         if ( vcpuaff->flags & XEN_VCPUAFFINITY_FORCE )
             vcpu_temporary_affinity(v, NR_CPUS, VCPU_AFFINITY_OVERRIDE);
 
-        ret = 0;
-
         /*
          * We both set a new affinity and report back to the caller what
          * the scheduler will be effectively using.
          */
         if ( vcpuaff->flags & XEN_VCPUAFFINITY_HARD )
         {
-            ret = xenctl_bitmap_to_bitmap(cpumask_bits(new_affinity),
-                                          &vcpuaff->cpumap_hard, nr_cpu_ids);
-            if ( !ret )
-                ret = vcpu_set_hard_affinity(v, new_affinity);
-            if ( ret )
+            hret = xenctl_bitmap_to_bitmap(cpumask_bits(new_affinity),
+                                           &vcpuaff->cpumap_hard, nr_cpu_ids);
+            if ( !hret )
+                hret = vcpu_set_hard_affinity(v, new_affinity);
+            if ( hret )
                 goto setvcpuaffinity_out;
 
             /*
@@ -1766,7 +1764,7 @@ int vcpu_affinity_domctl(struct domain *
              * cpupool's online mask and the new hard affinity.
              */
             cpumask_and(new_affinity, online, unit->cpu_hard_affinity);
-            ret = cpumask_to_xenctl_bitmap(&vcpuaff->cpumap_hard, new_affinity);
+            hret = cpumask_to_xenctl_bitmap(&vcpuaff->cpumap_hard, new_affinity);
         }
         if ( vcpuaff->flags & XEN_VCPUAFFINITY_SOFT )
         {
@@ -1804,14 +1802,14 @@ int vcpu_affinity_domctl(struct domain *
     else
     {
         if ( vcpuaff->flags & XEN_VCPUAFFINITY_HARD )
-            ret = cpumask_to_xenctl_bitmap(&vcpuaff->cpumap_hard,
-                                           unit->cpu_hard_affinity);
+            hret = cpumask_to_xenctl_bitmap(&vcpuaff->cpumap_hard,
+                                            unit->cpu_hard_affinity);
         if ( vcpuaff->flags & XEN_VCPUAFFINITY_SOFT )
             ret = cpumask_to_xenctl_bitmap(&vcpuaff->cpumap_soft,
                                            unit->cpu_soft_affinity);
     }
 
-    return ret;
+    return hret ?: ret;
 }
 
 bool alloc_affinity_masks(struct affinity_masks *affinity)

[PATCH v2 for-4.22? 6/7] 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.

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.

While there switch to using %pd in the two log messages.

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.
---
v2: Avoid double evaluation of "add". Add ChangeLog entry.

--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,9 @@ The format is based on [Keep a Changelog
  - On x86:
    - Enable pf-fixup option by default for PVH dom0.
    - The libxenguest bzImage loader now uses the system liblz4 library.
+   - XEN_DOMCTL_ioport_mapping no longer implicitly grants permissions for the
+     port range in question.  XEN_DOMCTL_ioport_permission now needs invoking
+     up front.
 
 ### Added
  - Support for per-domain Xenstore quota in C xenstored (includes
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -714,15 +714,35 @@ 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) )
             ret = -EPERM;
-        else if ( add )
+        else if ( !add )
         {
             printk(XENLOG_G_INFO
-                   "ioport_map:add: dom%d gport=%x mport=%x nr=%x\n",
-                   d->domain_id, fgp, fmp, np);
+                   "ioport_map:remove: %pd gport=%x mport=%x nr=%x\n",
+                   d, fgp, fmp, np);
+
+            write_lock(&hvm->g2m_ioport_lock);
+            list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
+                if ( g2m_ioport->mport == fmp )
+                {
+                    list_del(&g2m_ioport->list);
+                    xfree(g2m_ioport);
+                    break;
+                }
+            write_unlock(&hvm->g2m_ioport_lock);
+        }
+        else if ( ioports_access_permitted(d, fmp, fmp + np - 1) )
+        {
+            printk(XENLOG_G_INFO
+                   "ioport_map:add: %pd gport=%x mport=%x nr=%x\n",
+                   d, fgp, fmp, np);
 
             write_lock(&hvm->g2m_ioport_lock);
             list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
@@ -747,40 +767,11 @@ 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
-        {
-            printk(XENLOG_G_INFO
-                   "ioport_map:remove: dom%d gport=%x mport=%x nr=%x\n",
-                   d->domain_id, fgp, fmp, np);
-
-            write_lock(&hvm->g2m_ioport_lock);
-            list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
-                if ( g2m_ioport->mport == fmp )
-                {
-                    list_del(&g2m_ioport->list);
-                    xfree(g2m_ioport);
-                    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);
-        }
+            ret = -EPERM;
 
-        iocaps_double_unlock(d, true);
+        iocaps_double_unlock(d, false);
         break;
     }
 

[PATCH v2 for-4.22? 7/7] 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.

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>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
v2: Re-base over change to earlier patch.

--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -728,12 +728,14 @@ long arch_do_domctl(
                    "ioport_map:remove: %pd gport=%x mport=%x nr=%x\n",
                    d, 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);
@@ -746,14 +748,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 )

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

[Oleksii Kurochko]

On 6/17/26 11:26 AM, 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>
> Reviewed-by: Juergen Gross<jgross@suse.com>
> ---

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

Thanks.

~ Oleksii

Re: [PATCH v2 for-4.22? 5/7] domctl: correct return value of XEN_DOMCTL_[gs]etvcpuaffinity

[Jürgen Groß]

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

On 17.06.26 11:29, Jan Beulich wrote:
> cpumask_to_xenctl_bitmap() may return errors. Clearing the error indicator
> of an earlier such call by a (successful) later call is misleading the
> caller. For "set", keep setting soft affinity if the hard affinity copy-
> back fails; only accumulate respective errors.
> 
> While fiddling with return values, also drop a redundant clearing of
> "ret". This eliminates a Misra C:2012 rule 2.2 ("There shall be no dead
> code") violation.
> 
> Fixes: 6e4ecc6d5884 ("sched: DOMCTL_*vcpuaffinity works with hard and soft affinity")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.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 --]

Re: [PATCH v2 for-4.22? 3/7] domctl: move early special casing of XEN_DOMCTL_shadow_op

[Roger Pau Monné]

On Wed, Jun 17, 2026 at 11:27:07AM +0200, Jan Beulich wrote:
> This wants xsm_domctl() invoked, but the domctl lock not taken. Move the
> handling to the respective switch(), thus eliminating the need for a
> separate xsm_domctl() invocation.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Thanks, Roger.

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

[Roger Pau Monné]

On Wed, Jun 17, 2026 at 11:27:42AM +0200, Jan Beulich wrote:
> While the granting of permissions when mapping was already removed from
> this operation, check whether permissions actually were granted when
> adding a mapping; the check of the requester having permission remains
> unaltered.

I would possibly reword this as:

"Be less strict with permissions checks when removing a mapping and
only request the caller domain to have access to the region.  Keep the
same permission checks for addition operations."

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

Reviewed-by: Roger Pau Monné <roger.pau@citirx.com>

Thanks, Roger.

Re: [PATCH v2 for-4.22? 5/7] domctl: correct return value of XEN_DOMCTL_[gs]etvcpuaffinity

[Roger Pau Monné]

On Wed, Jun 17, 2026 at 11:29:09AM +0200, Jan Beulich wrote:
> cpumask_to_xenctl_bitmap() may return errors. Clearing the error indicator
> of an earlier such call by a (successful) later call is misleading the
> caller. For "set", keep setting soft affinity if the hard affinity copy-
> back fails; only accumulate respective errors.
> 
> While fiddling with return values, also drop a redundant clearing of
> "ret". This eliminates a Misra C:2012 rule 2.2 ("There shall be no dead
> code") violation.
> 
> Fixes: 6e4ecc6d5884 ("sched: DOMCTL_*vcpuaffinity works with hard and soft affinity")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

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

Thanks, Roger.

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

[Jan Beulich]

On 17.06.2026 13:37, Roger Pau Monné wrote:
> On Wed, Jun 17, 2026 at 11:27:42AM +0200, Jan Beulich wrote:
>> While the granting of permissions when mapping was already removed from
>> this operation, check whether permissions actually were granted when
>> adding a mapping; the check of the requester having permission remains
>> unaltered.
> 
> I would possibly reword this as:
> 
> "Be less strict with permissions checks when removing a mapping and
> only request the caller domain to have access to the region.  Keep the
> same permission checks for addition operations."

Sure.

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

Thanks. I've taken the liberty of correcting the typo in the domain name.

Jan

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

[Oleksii Kurochko]

On 6/17/26 11:30 AM, 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.
> 
> 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.
> 
> While there switch to using %pd in the two log messages.
> 
> Fixes: 192c4dabc344 ("domctl and p2m changes for PCI passthru")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.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.
> ---
> v2: Avoid double evaluation of "add". Add ChangeLog entry.
> 
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -14,6 +14,9 @@ The format is based on [Keep a Changelog
>    - On x86:
>      - Enable pf-fixup option by default for PVH dom0.
>      - The libxenguest bzImage loader now uses the system liblz4 library.
> +   - XEN_DOMCTL_ioport_mapping no longer implicitly grants permissions for the
> +     port range in question.  XEN_DOMCTL_ioport_permission now needs invoking
> +     up front.
>   

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

Thanks.

~ Oleksii

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

[Oleksii Kurochko]

On 6/17/26 11:27 AM, Jan Beulich wrote:
> While the granting of permissions when mapping was already removed from
> this operation, check whether permissions actually were granted when
> adding a mapping; the check of the requester having permission remains
> unaltered.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

~ Oleksii

Re: [PATCH v2 for-4.22? 3/7] domctl: move early special casing of XEN_DOMCTL_shadow_op

[Oleksii Kurochko]

On 6/17/26 11:27 AM, Jan Beulich wrote:
> This wants xsm_domctl() invoked, but the domctl lock not taken. Move the
> handling to the respective switch(), thus eliminating the need for a
> separate xsm_domctl() invocation.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---

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

thanks.

~ Oleksii

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

[Roger Pau Monné]

Overall I would defer this change to the start of the 4.23 development
window, and commit it then.  It's IMO a bit risky to change the
interface behavior so late in the development process.

On Wed, Jun 17, 2026 at 11:30:04AM +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.
> 
> 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.
> 
> While there switch to using %pd in the two log messages.
> 
> 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.

Hm, I'm unsure that code will work correctly after the change here, as
xen_pt_register_vga_regions() doesn't grant access to the IO/memory
regions to the remote domain ahead of assigning them?

> ---
> v2: Avoid double evaluation of "add". Add ChangeLog entry.
> 
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -14,6 +14,9 @@ The format is based on [Keep a Changelog
>   - On x86:
>     - Enable pf-fixup option by default for PVH dom0.
>     - The libxenguest bzImage loader now uses the system liblz4 library.
> +   - XEN_DOMCTL_ioport_mapping no longer implicitly grants permissions for the

I would explicitly mention access revocation also, FTAOD:

"XEN_DOMCTL_ioport_mapping no longer implicitly grants or revokes
permissions ..."

> +     port range in question.  XEN_DOMCTL_ioport_permission now needs invoking
> +     up front.
>  
>  ### Added
>   - Support for per-domain Xenstore quota in C xenstored (includes
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -714,15 +714,35 @@ 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) )
>              ret = -EPERM;
> -        else if ( add )
> +        else if ( !add )
>          {
>              printk(XENLOG_G_INFO
> -                   "ioport_map:add: dom%d gport=%x mport=%x nr=%x\n",
> -                   d->domain_id, fgp, fmp, np);
> +                   "ioport_map:remove: %pd gport=%x mport=%x nr=%x\n",
> +                   d, fgp, fmp, np);
> +
> +            write_lock(&hvm->g2m_ioport_lock);
> +            list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
> +                if ( g2m_ioport->mport == fmp )
> +                {
> +                    list_del(&g2m_ioport->list);
> +                    xfree(g2m_ioport);
> +                    break;
> +                }
> +            write_unlock(&hvm->g2m_ioport_lock);
> +        }
> +        else if ( ioports_access_permitted(d, fmp, fmp + np - 1) )
> +        {
> +            printk(XENLOG_G_INFO
> +                   "ioport_map:add: %pd gport=%x mport=%x nr=%x\n",
> +                   d, fgp, fmp, np);
>  
>              write_lock(&hvm->g2m_ioport_lock);
>              list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
> @@ -747,40 +767,11 @@ 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
> -        {
> -            printk(XENLOG_G_INFO
> -                   "ioport_map:remove: dom%d gport=%x mport=%x nr=%x\n",
> -                   d->domain_id, fgp, fmp, np);
> -
> -            write_lock(&hvm->g2m_ioport_lock);
> -            list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
> -                if ( g2m_ioport->mport == fmp )
> -                {
> -                    list_del(&g2m_ioport->list);
> -                    xfree(g2m_ioport);
> -                    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);
> -        }
> +            ret = -EPERM;

Should we add a dprintk here at least, to make it easy to identify
what has gone wrong from just looking at the dmesg?

Thanks, Roger.