Re: [PATCH v5 09/11] x86/dmop: Add XEN_DMOP_{,un}bind_pt_msi_irq

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Sep 03, 2026 at 04:14:07PM +0200, Julian Vetter wrote:
> Add two device-model ops that bind / unbind a passthrough interrupt to a
> guest MSI from the raw MSI message (address + data) the guest
> programmed. Xen decodes the message itself via pt_irq_bind_msi(), so the
> emulator needs no knowledge of the MSI layout and, in particular,
> extended (15-bit) destination IDs are handled without emulator changes.
> 
> The MSI-X table base is passed as a guest-physical address. Fields are
> named msg_addr / msg_data / pirq, and the flag is
> XEN_DMOP_MSI_BIND_UNMASKED. The unbind op only requires a valid pirq
> mapping, not current IRQ permission, so an emulator can remove a binding
> even after the device has been deassigned.
> 
> With this in place, PT_IRQ_TYPE_MSI is removed from
> XEN_DOMCTL_{,un}bind_pt_irq (returns -EINVAL) and from
> pt_irq_create_bind(). libxc's xc_domain_{update,unbind}_msi_irq() and
> vPCI already funnel through pt_irq_bind_msi(). This is an incompatible
> change for device models still using the domctl sub-case (noted in
> CHANGELOG.md and public/domctl.h).

"Incompatible" you say? Is there any patch for QEMU that I missed?

> libxendevicemodel gains xendevicemodel_{,un}bind_pt_msi_irq() (map
> version VERS_1.5).
> 
> Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
> ---
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index aa1a777dd4..76f5d06c91 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -34,6 +34,9 @@ The format is based on [Keep a 
> Changelog](https://keepachangelog.com/en/1.0.0/)
>   - On x86:
>     - Enable pf-fixup option by default for PVH dom0.
>     - The libxenguest bzImage loader now uses the system liblz4 library.
> +   - XEN_DOMCTL_{,un}bind_pt_irq no longer accept PT_IRQ_TYPE_MSI, device
> +     models must bind passthrough MSIs via the new
> +     XEN_DMOP_{,un}bind_pt_msi_irq, which carry the raw MSI message.
>  
>  ### Added
>   - Support for per-domain Xenstore quota in C xenstored (includes
> @@ -47,6 +50,9 @@ The format is based on [Keep a 
> Changelog](https://keepachangelog.com/en/1.0.0/)
>     - Support for CPIO microcode in discrete multiboot modules.
>     - Introduce get-core-temp command to xenpm to query CPU temperatures on
>       Intel platforms.
> +   - XEN_DMOP_{,un}bind_pt_msi_irq for binding passthrough MSIs from the raw
> +     guest MSI message, enabling extended (15-bit) destination IDs for HVM
> +     guests whose device models opt in.

Also:
  - Introduce xendevicemodel_{,un}bind_pt_msi_irq() stable ABI as
    replacement of the unstable xc_domain_{update,unbind}_msi_irq().
?

>   - On Arm:
>     - Support for guest suspend and resume to/from RAM via vPSCI.
> diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h
> index 227e7fd810..698d719119 100644
> --- a/tools/include/xendevicemodel.h
> +++ b/tools/include/xendevicemodel.h
> @@ -375,6 +375,36 @@ int xendevicemodel_nr_vcpus(
>   */
>  int xendevicemodel_restrict(xendevicemodel_handle *dmod, domid_t domid);
>  
> +/**
> + * This function binds a passthrough interrupt to a guest MSI, described by 
> the
> + * raw MSI message (address and data) the guest programmed. Xen decodes the
> + * message itself, so the caller does not need to interpret it.
> + *
> + * @parm dmod a handle to an open devicemodel interface.
> + * @parm domid the domain id to be serviced.
> + * @parm pirq the pass-through IRQ (pirq).
> + * @parm msg_addr the MSI message address, as programmed by the guest.
> + * @parm msg_data the MSI message data, as programmed by the guest.
> + * @parm gtable the MSI-X table base guest-physical address, or 0 for plain 
> MSI.
> + * @parm unmasked if non-zero, leave the IRQ unmasked after binding.
> + * @return 0 on success, -1 on failure.
> + */
> +int xendevicemodel_bind_pt_msi_irq(
> +    xendevicemodel_handle *dmod, domid_t domid, uint32_t pirq,
> +    uint64_t msg_addr, uint32_t msg_data, uint64_t gtable, int unmasked);

"gtable" I fell like it could have a better name. "guest table" feels a
bit generic and not really describing what the parameter is about. So is
a name like "msix_table" or "msix_base" or maybe "table_addr" since we
are in the context of msi or something a bit more descriptive? I'm not
completely sure what the MSI-X table is, so there's maybe another name
that would better described what the value is. If you still think
"gtable" is good enough, so be it.

Next, "unmasked", this one sound like it should be a `bool`, not an `int`.

> +
> +/**
> + * This function unbinds a passthrough interrupt previously bound with
> + * xendevicemodel_bind_pt_msi_irq.
> + *
> + * @parm dmod a handle to an open devicemodel interface.
> + * @parm domid the domain id to be serviced.
> + * @parm pirq the pass-through IRQ (pirq).
> + * @return 0 on success, -1 on failure.
> + */
> +int xendevicemodel_unbind_pt_msi_irq(
> +    xendevicemodel_handle *dmod, domid_t domid, uint32_t pirq);

One last think on the header, could you move both prototype to just
after "xendevicemodel_inject_msi()" ? It feels like that would be a
slightly better placement within the header.

>  #endif /* XENDEVICEMODEL_H */
>  
>  /*
> diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> index 8e619eeb0a..274a8eb28b 100644
> --- a/tools/libs/devicemodel/core.c
> +++ b/tools/libs/devicemodel/core.c
> @@ -645,6 +645,44 @@ int xendevicemodel_nr_vcpus(
>      return 0;
>  }
>  
> +int xendevicemodel_bind_pt_msi_irq(
> +    xendevicemodel_handle *dmod, domid_t domid, uint32_t pirq,
> +    uint64_t msg_addr, uint32_t msg_data, uint64_t gtable, int unmasked)
> +{
> +    struct xen_dm_op op;
> +    struct xen_dm_op_bind_pt_msi_irq *data;
> +
> +    memset(&op, 0, sizeof(op));
> +
> +    op.op = XEN_DMOP_bind_pt_msi_irq;

This memset and first assignment can be replaced by:

    struct xen_dm_op op = {
      .op = XEN_DMOP_bind_pt_msi_irq,
    };

;-)

> +    data = &op.u.bind_pt_msi_irq;
> +
> +    data->pirq = pirq;
> +    data->msg_data = msg_data;
> +    data->msg_addr = msg_addr;
> +    data->gtable = gtable;
> +    if ( unmasked )
> +        data->flags |= XEN_DMOP_MSI_BIND_UNMASKED;

That "data" variable feel a bit useless, and badly name (arguments or
parameters would have been a more descriptive). I know that how the rest
of the file is styled, but I'd like to propose something a bit cleaner
and initialise the struct with all the values:

    struct xen_dm_op op = {
      .op = XEN_DMOP_bind_pt_msi_irq,
      .u.bind_pt_msi_irq = {
        .pirq = pirq,
        .msg_data = msg_data,
        .msg_addr = msg_addr,
        .gtable = gtable,
        .flags = unmasked ? XEN_DMOP_MSI_BIND_UNMASKED : 0,
      }
    };

There's already quite a few use of this way of initialising the
parameters of an hypercall in the different libraries.

> +
> +    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
> +}
> +
> +int xendevicemodel_unbind_pt_msi_irq(
> +    xendevicemodel_handle *dmod, domid_t domid, uint32_t pirq)
> +{
> +    struct xen_dm_op op;
> +    struct xen_dm_op_unbind_pt_msi_irq *data;
> +
> +    memset(&op, 0, sizeof(op));
> +
> +    op.op = XEN_DMOP_unbind_pt_msi_irq;
> +    data = &op.u.unbind_pt_msi_irq;
> +
> +    data->pirq = pirq;
> +
> +    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
> +}

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech