Re: [PATCH v4 7/9] x86/dmop: Add XEN_DMOP_{bind,unbind}_pt_msi_irq DM ops

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.04.2026 15:54, Julian Vetter wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -574,6 +574,14 @@ long arch_do_domctl(
>          if ( !is_hvm_domain(d) )
>              break;
>  
> +        /*
> +         * PT_IRQ_TYPE_MSI is obsoleted by XEN_DMOP_bind_pt_msi_irq, which
> +         * passes raw MSI address/data so Xen can decode extended destination
> +         * ID bits. Device models must use the DM op path instead.
> +         */
> +        if ( bind->irq_type == PT_IRQ_TYPE_MSI )
> +            break;

Oh, here is where you have put the reject logic. With the other call to
pt_irq_create_bind() having been removed by patch 5, respective logic in
that function (as last modified also by patch 5) is now unreachable,
violating Misra rule 2.1.

Then again you cannot do this anyway, as it breaks older DMs. You want to
reject this only when XEN_DMOP_enable_ext_dest_id (subject to rename) was
called earlier. And you want to reject XEN_DMOP_enable_ext_dest_id when
XEN_DOMCTL_bind_pt_irq with PT_IRQ_TYPE_MSI was called earlier on. We
want to make sure that we get to see uses of only one kind of interface
(unless both interfaces can be made interoperate cleanly).

> @@ -607,6 +611,68 @@ int dm_op(const struct dmop_args *op_args)
>          break;
>      }
>  
> +    case XEN_DMOP_bind_pt_msi_irq:
> +    {
> +        const struct xen_dm_op_bind_pt_msi_irq *data =
> +            &op.u.bind_pt_msi_irq;
> +        int irq;
> +
> +        rc = -EINVAL;
> +        if ( data->pad || (data->flags & ~XEN_DMOP_MSI_FLAG_UNMASKED) )
> +            break;
> +
> +        irq = domain_pirq_to_irq(d, data->machine_irq);
> +
> +        rc = -EPERM;
> +        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
> +            break;
> +
> +        rc = -ESRCH;
> +        if ( is_iommu_enabled(d) )
> +        {
> +            read_lock(&d->pci_lock);
> +            rc = pt_irq_bind_msi(d, data->machine_irq, data->addr, 
> data->data,
> +                                 data->gtable,
> +                                 !!(data->flags & 
> XEN_DMOP_MSI_FLAG_UNMASKED));

As before, no need for !!.

> +            read_unlock(&d->pci_lock);
> +        }
> +        if ( rc < 0 )
> +            printk(XENLOG_G_ERR
> +                   "XEN_DMOP_bind_pt_msi_irq: pt_irq_bind_msi failed (%ld) 
> for %pd\n",

Imo this is too verbose. If anything needs logging here at all (which I
question), "%pd: pt_irq_bind_msi() failed: %ld\n" would likely do, without
becoming ambiguous. (Same below then, obviously.)

> +                   rc, d);
> +        break;
> +    }

Where did, btw, the XSM check go that the original code has? Daniel - I
don't think such can simply be dropped, despite there being xsm_dm_op()
on the path here?

> +    case XEN_DMOP_unbind_pt_msi_irq:
> +    {
> +        const struct xen_dm_op_unbind_pt_msi_irq *data =
> +            &op.u.unbind_pt_msi_irq;
> +        struct xen_domctl_bind_pt_irq bind = {
> +            .machine_irq = data->machine_irq,
> +            .irq_type = PT_IRQ_TYPE_MSI,
> +        };
> +        int irq;
> +
> +        irq = domain_pirq_to_irq(d, bind.machine_irq);
> +
> +        rc = -EPERM;
> +        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
> +            break;

As we're making a new interface, we need to consider getting rid of bogus
aspects of the old one. Along the lines of what 6df6f24251db ("domctl:
restrict permission check for XEN_DOMCTL_memory_mapping's remove form")
says, and as then also mirrored by 6e42fa383c70 ("x86/domctl: don't imply
I/O port permissions from I/O port mapping"), a permission check on unmap
(here: unbind) for current->domain may be excessive: Even if permission
was already removed, the DM should still be able to unbind the guest's
IRQ.

> +        rc = -ESRCH;
> +        if ( is_iommu_enabled(d) )
> +        {
> +            read_lock(&d->pci_lock);
> +            rc = pt_irq_destroy_bind(d, &bind);
> +            read_unlock(&d->pci_lock);

Here and above - please pay attention to impending locking changes at the
original site, as per (much) earlier discussion. (As said there, I don't
think a lock needs taking here - or above - at all.)

> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -444,6 +444,41 @@ struct xen_dm_op_nr_vcpus {
>  };
>  typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t;
>  
> +#define XEN_DMOP_bind_pt_msi_irq   21
> +#define XEN_DMOP_unbind_pt_msi_irq 22
> +
> +struct xen_dm_op_bind_pt_msi_irq {
> +    /* IN - physical IRQ (pirq) */
> +    uint32_t machine_irq;

Please can comment and field identifier match up with one another? We don't
want to carry over such an inconsistency from the old interface.

> +    /* IN - MSI data word (bits [7:0] are the guest vector) */

The part in parentheses is x86-centric, which we'd better avoid in the public
headers.

> +    uint32_t data;
> +    /* IN - flags */
> +    uint32_t flags;
> +#define XEN_DMOP_MSI_FLAG_UNMASKED (1u << 0)

s/FLAG/BIND/ perhaps?

> +    uint32_t pad;
> +    /* IN - MSI address (includes extended destination ID in bits [11:5]) */

Please again omit the x86-centric part.

> +    uint64_aligned_t addr;
> +    /* IN - MSI-X table base GFN, 0 for plain MSI */
> +    uint64_aligned_t gtable;

This is a GADDR, not a GFN, isn't it?

With this, the earlier field being named just "addr" also ends up potentially
ambiguous. Perhaps msg_addr (and then also msg_data)?

More generally: Why does the DM need to be bothered about IRQ numbers in the
first place? To identify a particular MSI, what you need are device coordinates
and an index. Once passed in like this, the need for passing in "gtable" for
MSI-X should then also disappear. That said, re-working accordingly may incur
significant effort. That needs weighing against the downsides of introducing
another partly screwed interface.

> +};
> +
> +typedef struct xen_dm_op_bind_pt_msi_irq xen_dm_op_bind_pt_msi_irq_t;

Please omit the intermediate blank line, just like ...

> +struct xen_dm_op_unbind_pt_msi_irq {
> +    /* IN - physical IRQ (pirq) */
> +    uint32_t machine_irq;
> +};
> +typedef struct xen_dm_op_unbind_pt_msi_irq xen_dm_op_unbind_pt_msi_irq_t;

... you do here. That said - are these typedefs needed anywhere in the
first place?

> +/*
> + * XEN_DMOP_enable_ext_dest_id: Signal to Xen that this device model will use
> + * XEN_DMOP_bind_pt_msi_irq for all passthrough MSI bindings, passing raw MSI
> + * address/data fields. Once called, Xen will advertise
> + * XEN_HVM_CPUID_EXT_DEST_ID to the guest. Must be called before the guest
> + * starts.
> + */
> +#define XEN_DMOP_enable_ext_dest_id 23

I don't understand this. With XEN_DOMCTL_bind_pt_irq's PT_IRQ_TYPE_MSI case
cut off, DMs have no alternative besides using XEN_DMOP_bind_pt_msi_irq. If
that cut-off was viable, I think this comment would want re-wording almost
from scratch. As the cut-off needs dropping / constraining, some less severe
edit may do. The requirement to call this before the guest starts isn't
enough imo: It also needs to be called ahead of any binding, as the behavior
of the binding logic will need to be dependent upon whether this call was
issued.

The identifier XEN_DMOP_enable_ext_dest_id isn't suitable, though, as this
is about the choice of interface the DM is going to use. The newer interface
offering extended-ID support is merely a wanted side effect.

And then it's pretty odd that you add this #define here, but there's no
handling of the new sub-op. Was this perhaps meant to go in the next patch?

Jan