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é <roger.pau@xxxxxxxxxx>]

On Thu, Jun 18, 2026 at 09:15:36AM +0200, Jan Beulich wrote:
> On 17.06.2026 17:18, Roger Pau Monné wrote:
> > 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.
> 
> I share the concern, yet the Fixes: tags suggest this (and the subsequent
> change) wants backporting. Which means we'll "gain" the behavioral change
> in minor releases then anyway. IOW - I'm of two minds here.

IMO I would refrain from backporting this.  Albeit the final behavior
is what we think should be the correct one, and how the hypercall
should have behaved in the first place, it's none the less an ABI
change, and we tend to avoid ABI changes to stable releases.

> > On Wed, Jun 17, 2026 at 11:30:04AM +0200, Jan Beulich wrote:
> >> ---
> >> 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?
> 
> 
> Hence the remark, and the desire to get input from Anthony. Aiui there
> either already is an issue there as of 0561e1f01e87 ("xen/common: do not
> implicitly permit access to mapped I/O memory"), from over 10 years ago.
> Or there is none here either.
> 
> >> --- 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 ..."
> 
> Sure, and then also ...
> 
> >> +     port range in question.  XEN_DOMCTL_ioport_permission now needs 
> >> invoking
> >> +     up front.
> 
> ... "up front / afterwards."

Yes, indeed.

> >> --- 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;
> 
> For the comment below, note that there hasn't been any log message here.
> Hence ...
> 
> >> @@ -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?
> 
> ... I'm pretty uncertain towards emitting one here. Similarly
> XEN_DOMCTL_memory_mapping doesn't emit a log message if either of the
> two iomem_access_permitted() fail.

I didn't have in mind log parity with other similar hypercalls in
mind, I was mostly concerned about providing an easy way for existing
callers that rely on XEN_DOMCTL_ioport_mapping granting permissions to
identify the error.

I'm not going to insist on adding the error, but I think it could be
helpful despite the non-parity with XEN_DOMCTL_memory_mapping (or we
could also add one there later on).

Thanks, Roger.