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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.03.2026 13:31, Julian Vetter wrote:
> @@ -607,6 +631,83 @@ 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;
> +        struct xen_domctl_bind_pt_irq bind = {
> +            .machine_irq = data->machine_irq,
> +            .irq_type = PT_IRQ_TYPE_MSI,
> +        };
> +        int irq;
> +
> +        rc = -EINVAL;
> +        if ( data->pad0 || data->pad1 )
> +            break;
> +
> +        if ( data->flags & ~XEN_DMOP_MSI_FLAG_UNMASKED )
> +            break;
> +
> +        irq = domain_pirq_to_irq(d, bind.machine_irq);
> +
> +        rc = -EPERM;
> +        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
> +            break;
> +
> +        bind.u.msi.gvec = data->gvec;
> +        bind.u.msi.gflags =
> +            msi_addr_to_gflags(data->addr, data->data,
> +                               !(data->flags & XEN_DMOP_MSI_FLAG_UNMASKED));
> +        bind.u.msi.gtable = data->gtable;
> +
> +        rc = -ESRCH;
> +        if ( is_iommu_enabled(d) )
> +        {
> +            pcidevs_lock();
> +            rc = pt_irq_create_bind(d, &bind);
> +            pcidevs_unlock();

I understand the same locking is used at the other call site, but it's as
questionable there as it is here. We should try hard to avoid use of this
global lock when something lighter-weight would do.

> +        }
> +        if ( rc < 0 )
> +            printk(XENLOG_G_ERR
> +                   "pt_irq_create_bind failed (%ld) for dom%d\n",
> +                   rc, d->domain_id);

%pd please in new code. Also this message needs to be distinguishable
from that in arch_do_domctl().

> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -444,6 +444,28 @@ 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;
> +    /* IN - guest vector */
> +    uint8_t  gvec;
> +    uint8_t  pad0;
> +    uint16_t pad1;
> +    /* IN - MSI data (vector, delivery, trigger) */
> +    uint32_t data;
> +    /* IN - flags */
> +    uint32_t flags;
> +#define XEN_DMOP_MSI_FLAG_UNMASKED (1u << 0)
> +    /* IN - MSI address (0xfeexxxxx, includes ext dest) */

The address given is x86-specific, while the header is arch-generic. (If
this was to be an x86-only interface, ...

> +    uint64_aligned_t addr;

... this also wouldn't need to be a 64-bit address, for example.)

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

Commentary throughout also leaves unclear which of the fields are actually
meaningful for XEN_DMOP_unbind_pt_msi_irq. In a new interface, the unused
fields would want checking to be zero. Yet better may be to have unbind
have its own, much smaller structure.

> +typedef struct xen_dm_op_bind_pt_msi_irq xen_dm_op_bind_pt_msi_irq_t;

Is this typedef actually needed anywhere? Clearly ...

> @@ -468,6 +490,7 @@ struct xen_dm_op {
>          xen_dm_op_relocate_memory_t relocate_memory;
>          xen_dm_op_pin_memory_cacheattr_t pin_memory_cacheattr;
>          xen_dm_op_nr_vcpus_t nr_vcpus;
> +        xen_dm_op_bind_pt_msi_irq_t bind_pt_msi_irq;

... the plain struct can be used e.g. here. Imo we should stop cluttering
the namespace with typedef-s which aren't actually needed. (A case where
they are needed is when a guest handle needs defining for the type.)

> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -107,6 +107,7 @@
>  ?    dm_op_map_mem_type_to_ioreq_server hvm/dm_op.h
>  ?    dm_op_modified_memory           hvm/dm_op.h
>  ?    dm_op_nr_vcpus                  hvm/dm_op.h
> +?    dm_op_bind_pt_msi_irq           hvm/dm_op.h
>  ?    dm_op_pin_memory_cacheattr      hvm/dm_op.h
>  ?    dm_op_relocate_memory           hvm/dm_op.h
>  ?    dm_op_remote_shutdown           hvm/dm_op.h

I'm curious: How did you determine the insertion point?

Jan