diff mbox series

[2/2] xen/dm: arm: Introduce inject_msi2 DM op

Message ID 6c551b03796fbf091b22fcde96d894cd5308ff91.1705066642.git.mykyta_poturai@epam.com (mailing list archive)
State New
Headers show
Series Add support for MSI injection on Arm | expand

Commit Message

Mykyta Poturai Jan. 14, 2024, 10:01 a.m. UTC
Add the second version of inject_msi DM op, which allows to specify
the source_id of an MSI interrupt. This is needed for correct MSI
injection on ARM.

It would not be safe to include the source_id in the original inject_msi
in the pad field, because we have no way to know if it is set or not.

Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
 tools/include/xendevicemodel.h               | 14 +++++++++++++
 tools/libs/devicemodel/core.c                | 22 ++++++++++++++++++++
 tools/libs/devicemodel/libxendevicemodel.map |  5 +++++
 xen/arch/arm/dm.c                            | 15 +++++++++++++
 xen/arch/x86/hvm/dm.c                        | 13 ++++++++++++
 xen/include/public/hvm/dm_op.h               | 12 +++++++++++
 6 files changed, 81 insertions(+)

Comments

Jan Beulich Jan. 15, 2024, 9:35 a.m. UTC | #1
On 14.01.2024 11:01, Mykyta Poturai wrote:
> Add the second version of inject_msi DM op, which allows to specify
> the source_id of an MSI interrupt. This is needed for correct MSI
> injection on ARM.
> 
> It would not be safe to include the source_id in the original inject_msi
> in the pad field, because we have no way to know if it is set or not.

At least on x86 I think 0 could serve as a "not valid" indicator, as that's
always a host bridge, which would never be handed through to guests. Can't
speak for Arm, as I don't know whether 00:00.0 always being a host bridge
(or unpopulated) is a universal requirement of the spec. Therefore this may
be insufficient justification for adding a new sub-op.

> --- a/xen/arch/arm/dm.c
> +++ b/xen/arch/arm/dm.c
> @@ -27,6 +27,7 @@ int dm_op(const struct dmop_args *op_args)
>          [XEN_DMOP_set_ioreq_server_state]           = sizeof(struct xen_dm_op_set_ioreq_server_state),
>          [XEN_DMOP_destroy_ioreq_server]             = sizeof(struct xen_dm_op_destroy_ioreq_server),
>          [XEN_DMOP_set_irq_level]                    = sizeof(struct xen_dm_op_set_irq_level),
> +        [XEN_DMOP_inject_msi2]                      = sizeof(struct xen_dm_op_inject_msi2),
>          [XEN_DMOP_nr_vcpus]                         = sizeof(struct xen_dm_op_nr_vcpus),
>      };
>  
> @@ -112,6 +113,20 @@ int dm_op(const struct dmop_args *op_args)
>          break;
>      }
>  
> +    case XEN_DMOP_inject_msi2:
> +    {
> +        const struct xen_dm_op_inject_msi2 *data =
> +            &op.u.inject_msi2;
> +
> +        if ( !(data->flags & XEN_DMOP_MSI_SOURCE_ID_VALID) )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }

While I can see the reason for this check here, ...

> @@ -539,6 +540,18 @@ int dm_op(const struct dmop_args *op_args)
>          break;
>      }
>  
> +    case XEN_DMOP_inject_msi2:
> +    {
> +        const struct xen_dm_op_inject_msi2 *data =
> +            &op.u.inject_msi2;
> +
> +        if ( !(data->flags & XEN_DMOP_MSI_SOURCE_ID_VALID) )
> +            printk(XENLOG_WARNING "XEN_DMOP_inject_msi2: source_id is ignored\n");

... I don't understand what's going on here: If the flag isn't set, it's
obvious that the field is to be ignored. Is the conditional inverted?

Also in both cases you will need to check that all other flags fields
are clear, or else we won't be able to give them meaning down the road.

Finally such a message, if really warranted, wants to use gprintk().

> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -444,6 +444,17 @@ struct xen_dm_op_nr_vcpus {
>  };
>  typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t;
>  
> +#define XEN_DMOP_inject_msi2 21
> +#define XEN_DMOP_MSI_SOURCE_ID_VALID (1u << 0)
> +
> +struct xen_dm_op_inject_msi2 {
> +    uint64_aligned_t addr;
> +    uint32_t data;
> +    uint32_t source_id; /* PCI SBDF */

Since the comment says SBDF (not BDF), how are multiple segments handled
here? At least on x86 (VT-d and V-i) source IDs are only 16 bits iirc,
and are local to the respective segment. It would feel wrong to use a
32-bit quantity there; IOW I'd rather see this as being two 16-bit fields
(segment and source_id).

> +    uint32_t flags;
> +};

Just like in struct xen_dm_op_inject_msi padding wants making explicit,
and then wants checking for being zero.

Jan
Stefano Stabellini Jan. 24, 2024, 1:07 a.m. UTC | #2
On Sun, 14 Jan 2024, Mykyta Poturai wrote:
> Add the second version of inject_msi DM op, which allows to specify
> the source_id of an MSI interrupt. This is needed for correct MSI
> injection on ARM.
> 
> It would not be safe to include the source_id in the original inject_msi
> in the pad field, because we have no way to know if it is set or not.
> 
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> ---
>  tools/include/xendevicemodel.h               | 14 +++++++++++++
>  tools/libs/devicemodel/core.c                | 22 ++++++++++++++++++++
>  tools/libs/devicemodel/libxendevicemodel.map |  5 +++++
>  xen/arch/arm/dm.c                            | 15 +++++++++++++
>  xen/arch/x86/hvm/dm.c                        | 13 ++++++++++++
>  xen/include/public/hvm/dm_op.h               | 12 +++++++++++
>  6 files changed, 81 insertions(+)
> 
> diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h
> index 797e0c6b29..4833e55bce 100644
> --- a/tools/include/xendevicemodel.h
> +++ b/tools/include/xendevicemodel.h
> @@ -236,6 +236,20 @@ int xendevicemodel_inject_msi(
>      xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr,
>      uint32_t msi_data);
>  
> +/**
> + * This function injects an MSI into a guest.
> + *
> + * @parm dmod a handle to an open devicemodel interface.
> + * @parm domid the domain id to be serviced
> + * @parm msi_addr the MSI address (0xfeexxxxx)
> + * @parm source_id the PCI SBDF of the source device
> + * @parm msi_data the MSI data
> + * @return 0 on success, -1 on failure.
> +*/
> +int xendevicemodel_inject_msi2(
> +    xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t source_id,
> +    uint32_t msi_data, unsigned int source_id_valid);


What is "source_id_valid"? It is not described in the comment. Also, it
should be a fixed size int. I agree with Jan that we could reuse
xendevicemodel_inject_msi by assuing that PCI BDF zero is invalid.


>  /**
>   * This function enables tracking of changes in the VRAM area.
>   *
> diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> index 8e619eeb0a..17ad00c5d9 100644
> --- a/tools/libs/devicemodel/core.c
> +++ b/tools/libs/devicemodel/core.c
> @@ -448,6 +448,28 @@ int xendevicemodel_set_irq_level(
>      return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
>  }
>  
> +int xendevicemodel_inject_msi2(
> +    xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t source_id,
> +    uint32_t msi_data, unsigned int source_id_valid)
> +{
> +    struct xen_dm_op op;
> +    struct xen_dm_op_inject_msi2 *data;
> +
> +    memset(&op, 0, sizeof(op));
> +
> +    op.op = XEN_DMOP_inject_msi2;
> +    data = &op.u.inject_msi2;
> +
> +    data->addr = msi_addr;
> +    data->data = msi_data;
> +    if ( source_id_valid ) {
> +        data->source_id = source_id;
> +        data->flags = XEN_DMOP_MSI_SOURCE_ID_VALID;
> +    }
> +
> +    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
> +}
> +
>  int xendevicemodel_set_pci_link_route(
>      xendevicemodel_handle *dmod, domid_t domid, uint8_t link, uint8_t irq)
>  {
> diff --git a/tools/libs/devicemodel/libxendevicemodel.map b/tools/libs/devicemodel/libxendevicemodel.map
> index f7f9e3d932..aa05768642 100644
> --- a/tools/libs/devicemodel/libxendevicemodel.map
> +++ b/tools/libs/devicemodel/libxendevicemodel.map
> @@ -44,3 +44,8 @@ VERS_1.4 {
>  		xendevicemodel_set_irq_level;
>  		xendevicemodel_nr_vcpus;
>  } VERS_1.3;
> +
> +VERS_1.5 {
> +	global:
> +		xendevicemodel_inject_msi2;
> +} VERS_1.4;
> diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c
> index 5569efa121..c45e196561 100644
> --- a/xen/arch/arm/dm.c
> +++ b/xen/arch/arm/dm.c
> @@ -27,6 +27,7 @@ int dm_op(const struct dmop_args *op_args)
>          [XEN_DMOP_set_ioreq_server_state]           = sizeof(struct xen_dm_op_set_ioreq_server_state),
>          [XEN_DMOP_destroy_ioreq_server]             = sizeof(struct xen_dm_op_destroy_ioreq_server),
>          [XEN_DMOP_set_irq_level]                    = sizeof(struct xen_dm_op_set_irq_level),
> +        [XEN_DMOP_inject_msi2]                      = sizeof(struct xen_dm_op_inject_msi2),
>          [XEN_DMOP_nr_vcpus]                         = sizeof(struct xen_dm_op_nr_vcpus),
>      };
>  
> @@ -112,6 +113,20 @@ int dm_op(const struct dmop_args *op_args)
>          break;
>      }
>  
> +    case XEN_DMOP_inject_msi2:
> +    {
> +        const struct xen_dm_op_inject_msi2 *data =
> +            &op.u.inject_msi2;
> +
> +        if ( !(data->flags & XEN_DMOP_MSI_SOURCE_ID_VALID) )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +        rc = vgic_its_trigger_msi(d, data->addr, data->source_id, data->data);
> +        break;
> +
> +    }
>      case XEN_DMOP_nr_vcpus:
>      {
>          struct xen_dm_op_nr_vcpus *data = &op.u.nr_vcpus;
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index 462691f91d..a4a0e3dff9 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -344,6 +344,7 @@ int dm_op(const struct dmop_args *op_args)
>          [XEN_DMOP_set_mem_type]                     = sizeof(struct xen_dm_op_set_mem_type),
>          [XEN_DMOP_inject_event]                     = sizeof(struct xen_dm_op_inject_event),
>          [XEN_DMOP_inject_msi]                       = sizeof(struct xen_dm_op_inject_msi),
> +        [XEN_DMOP_inject_msi2]                      = sizeof(struct xen_dm_op_inject_msi2),
>          [XEN_DMOP_map_mem_type_to_ioreq_server]     = sizeof(struct xen_dm_op_map_mem_type_to_ioreq_server),
>          [XEN_DMOP_remote_shutdown]                  = sizeof(struct xen_dm_op_remote_shutdown),
>          [XEN_DMOP_relocate_memory]                  = sizeof(struct xen_dm_op_relocate_memory),
> @@ -539,6 +540,18 @@ int dm_op(const struct dmop_args *op_args)
>          break;
>      }
>  
> +    case XEN_DMOP_inject_msi2:
> +    {
> +        const struct xen_dm_op_inject_msi2 *data =
> +            &op.u.inject_msi2;
> +
> +        if ( !(data->flags & XEN_DMOP_MSI_SOURCE_ID_VALID) )
> +            printk(XENLOG_WARNING "XEN_DMOP_inject_msi2: source_id is ignored\n");
> +
> +        rc = hvm_inject_msi(d, data->addr, data->data);
> +        break;
> +    }
> +
>      case XEN_DMOP_remote_shutdown:
>      {
>          const struct xen_dm_op_remote_shutdown *data =
> diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
> index fa98551914..da2ce4a7f7 100644
> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -444,6 +444,17 @@ struct xen_dm_op_nr_vcpus {
>  };
>  typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t;
>  
> +#define XEN_DMOP_inject_msi2 21
> +#define XEN_DMOP_MSI_SOURCE_ID_VALID (1u << 0)
> +
> +struct xen_dm_op_inject_msi2 {
> +    uint64_aligned_t addr;
> +    uint32_t data;
> +    uint32_t source_id; /* PCI SBDF */
> +    uint32_t flags;
> +};
> +typedef struct xen_dm_op_inject_msi2 xen_dm_op_inject_msi2_t;
> +
>  struct xen_dm_op {
>      uint32_t op;
>      uint32_t pad;
> @@ -463,6 +474,7 @@ struct xen_dm_op {
>          xen_dm_op_set_mem_type_t set_mem_type;
>          xen_dm_op_inject_event_t inject_event;
>          xen_dm_op_inject_msi_t inject_msi;
> +        xen_dm_op_inject_msi2_t inject_msi2;
>          xen_dm_op_map_mem_type_to_ioreq_server_t map_mem_type_to_ioreq_server;
>          xen_dm_op_remote_shutdown_t remote_shutdown;
>          xen_dm_op_relocate_memory_t relocate_memory;
> -- 
> 2.34.1
>
Andrew Cooper Jan. 24, 2024, 8:25 p.m. UTC | #3
On 14/01/2024 10:01 am, Mykyta Poturai wrote:
> diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h
> index 797e0c6b29..4833e55bce 100644
> --- a/tools/include/xendevicemodel.h
> +++ b/tools/include/xendevicemodel.h
> @@ -236,6 +236,20 @@ int xendevicemodel_inject_msi(
>      xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr,
>      uint32_t msi_data);
>  
> +/**
> + * This function injects an MSI into a guest.
> + *
> + * @parm dmod a handle to an open devicemodel interface.
> + * @parm domid the domain id to be serviced
> + * @parm msi_addr the MSI address (0xfeexxxxx)

This 0xfeexxxxx is an x86-ism which I doubt is correct for ARM.  I'd
suggest dropping it from the comment.

> + * @parm source_id the PCI SBDF of the source device
> + * @parm msi_data the MSI data
> + * @return 0 on success, -1 on failure.
> +*/
> +int xendevicemodel_inject_msi2(
> +    xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t source_id,
> +    uint32_t msi_data, unsigned int source_id_valid);

The Source ID is always valid when making this call.  It is only within
the hypervisor itself that we may need to worry about the source ID
being invalid.

This means you don't have the flags field, and as a consequence, there's
no padding to worry about.

Also, the msi_ prefix to address and data are redundant.  Either drop
them, or put a prefix on source_id too, because we shouldn't be
inconsistent here.

> +
>  /**
>   * This function enables tracking of changes in the VRAM area.
>   *
> diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> index 8e619eeb0a..17ad00c5d9 100644
> --- a/tools/libs/devicemodel/core.c
> +++ b/tools/libs/devicemodel/core.c
> @@ -448,6 +448,28 @@ int xendevicemodel_set_irq_level(
>      return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
>  }
>  
> +int xendevicemodel_inject_msi2(
> +    xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t source_id,
> +    uint32_t msi_data, unsigned int source_id_valid)
> +{
> +    struct xen_dm_op op;
> +    struct xen_dm_op_inject_msi2 *data;
> +
> +    memset(&op, 0, sizeof(op));
> +
> +    op.op = XEN_DMOP_inject_msi2;
> +    data = &op.u.inject_msi2;
> +
> +    data->addr = msi_addr;
> +    data->data = msi_data;
> +    if ( source_id_valid ) {
> +        data->source_id = source_id;
> +        data->flags = XEN_DMOP_MSI_SOURCE_ID_VALID;
> +    }
> +
> +    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));

{
    struct xen_dm_op op = {
        .op = XEN_DMOP_inject_msi2,
        .u.inject_msi2 = {
            .addr = msi_addr,
            .data = msi_data,
            .source_id = source_id,
        },
    };

    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
}


> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index 462691f91d..a4a0e3dff9 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -539,6 +540,18 @@ int dm_op(const struct dmop_args *op_args)
>          break;
>      }
>  
> +    case XEN_DMOP_inject_msi2:
> +    {
> +        const struct xen_dm_op_inject_msi2 *data =
> +            &op.u.inject_msi2;
> +
> +        if ( !(data->flags & XEN_DMOP_MSI_SOURCE_ID_VALID) )
> +            printk(XENLOG_WARNING "XEN_DMOP_inject_msi2: source_id is ignored\n");
> +
> +        rc = hvm_inject_msi(d, data->addr, data->data);

You need a prep patch adding a source id parameter into hvm_inject_msi().

The XEN_DMOP_inject_msi case can probably pass in 0 in the short term,
and it can probably be discarded internally.

As I said before, the source id doesn't matter until we start trying to
expose vIOMMUs to guests, at which point I suspect the easiest option
will simply to be to reject XEN_DMOP_inject_msi against a domain with a
vIOMMU and force Qemu/whatever in dom0 to use XEN_DMOP_inject_msi2.

~Andrew
Andrew Cooper Jan. 24, 2024, 8:50 p.m. UTC | #4
On 24/01/2024 1:07 am, Stefano Stabellini wrote:
>> I agree with Jan that we could reuse
>> xendevicemodel_inject_msi by assuing that PCI BDF zero is invalid.

I've already explained why that will break future x86.  (See the other
thread off this patch for specifics).

When we add vIOMMUs to guests, we *must* have correct source ids.  0 is
ambiguous, so not safe to reuse.


Allocating a new subop now is the only way to not shoot in the foot later.

~Andrew
Stefano Stabellini Jan. 25, 2024, 12:01 a.m. UTC | #5
On Wed, 24 Jan 2024, Andrew Cooper wrote:
> On 24/01/2024 1:07 am, Stefano Stabellini wrote:
> >> I agree with Jan that we could reuse
> >> xendevicemodel_inject_msi by assuing that PCI BDF zero is invalid.
> 
> I've already explained why that will break future x86.  (See the other
> thread off this patch for specifics).
> 
> When we add vIOMMUs to guests, we *must* have correct source ids.  0 is
> ambiguous, so not safe to reuse.
> 
> 
> Allocating a new subop now is the only way to not shoot in the foot later.

OK, what you wrote makes sense to me.
diff mbox series

Patch

diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h
index 797e0c6b29..4833e55bce 100644
--- a/tools/include/xendevicemodel.h
+++ b/tools/include/xendevicemodel.h
@@ -236,6 +236,20 @@  int xendevicemodel_inject_msi(
     xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr,
     uint32_t msi_data);
 
+/**
+ * This function injects an MSI into a guest.
+ *
+ * @parm dmod a handle to an open devicemodel interface.
+ * @parm domid the domain id to be serviced
+ * @parm msi_addr the MSI address (0xfeexxxxx)
+ * @parm source_id the PCI SBDF of the source device
+ * @parm msi_data the MSI data
+ * @return 0 on success, -1 on failure.
+*/
+int xendevicemodel_inject_msi2(
+    xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t source_id,
+    uint32_t msi_data, unsigned int source_id_valid);
+
 /**
  * This function enables tracking of changes in the VRAM area.
  *
diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index 8e619eeb0a..17ad00c5d9 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -448,6 +448,28 @@  int xendevicemodel_set_irq_level(
     return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
 }
 
+int xendevicemodel_inject_msi2(
+    xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t source_id,
+    uint32_t msi_data, unsigned int source_id_valid)
+{
+    struct xen_dm_op op;
+    struct xen_dm_op_inject_msi2 *data;
+
+    memset(&op, 0, sizeof(op));
+
+    op.op = XEN_DMOP_inject_msi2;
+    data = &op.u.inject_msi2;
+
+    data->addr = msi_addr;
+    data->data = msi_data;
+    if ( source_id_valid ) {
+        data->source_id = source_id;
+        data->flags = XEN_DMOP_MSI_SOURCE_ID_VALID;
+    }
+
+    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
+}
+
 int xendevicemodel_set_pci_link_route(
     xendevicemodel_handle *dmod, domid_t domid, uint8_t link, uint8_t irq)
 {
diff --git a/tools/libs/devicemodel/libxendevicemodel.map b/tools/libs/devicemodel/libxendevicemodel.map
index f7f9e3d932..aa05768642 100644
--- a/tools/libs/devicemodel/libxendevicemodel.map
+++ b/tools/libs/devicemodel/libxendevicemodel.map
@@ -44,3 +44,8 @@  VERS_1.4 {
 		xendevicemodel_set_irq_level;
 		xendevicemodel_nr_vcpus;
 } VERS_1.3;
+
+VERS_1.5 {
+	global:
+		xendevicemodel_inject_msi2;
+} VERS_1.4;
diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c
index 5569efa121..c45e196561 100644
--- a/xen/arch/arm/dm.c
+++ b/xen/arch/arm/dm.c
@@ -27,6 +27,7 @@  int dm_op(const struct dmop_args *op_args)
         [XEN_DMOP_set_ioreq_server_state]           = sizeof(struct xen_dm_op_set_ioreq_server_state),
         [XEN_DMOP_destroy_ioreq_server]             = sizeof(struct xen_dm_op_destroy_ioreq_server),
         [XEN_DMOP_set_irq_level]                    = sizeof(struct xen_dm_op_set_irq_level),
+        [XEN_DMOP_inject_msi2]                      = sizeof(struct xen_dm_op_inject_msi2),
         [XEN_DMOP_nr_vcpus]                         = sizeof(struct xen_dm_op_nr_vcpus),
     };
 
@@ -112,6 +113,20 @@  int dm_op(const struct dmop_args *op_args)
         break;
     }
 
+    case XEN_DMOP_inject_msi2:
+    {
+        const struct xen_dm_op_inject_msi2 *data =
+            &op.u.inject_msi2;
+
+        if ( !(data->flags & XEN_DMOP_MSI_SOURCE_ID_VALID) )
+        {
+            rc = -EINVAL;
+            break;
+        }
+        rc = vgic_its_trigger_msi(d, data->addr, data->source_id, data->data);
+        break;
+
+    }
     case XEN_DMOP_nr_vcpus:
     {
         struct xen_dm_op_nr_vcpus *data = &op.u.nr_vcpus;
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 462691f91d..a4a0e3dff9 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -344,6 +344,7 @@  int dm_op(const struct dmop_args *op_args)
         [XEN_DMOP_set_mem_type]                     = sizeof(struct xen_dm_op_set_mem_type),
         [XEN_DMOP_inject_event]                     = sizeof(struct xen_dm_op_inject_event),
         [XEN_DMOP_inject_msi]                       = sizeof(struct xen_dm_op_inject_msi),
+        [XEN_DMOP_inject_msi2]                      = sizeof(struct xen_dm_op_inject_msi2),
         [XEN_DMOP_map_mem_type_to_ioreq_server]     = sizeof(struct xen_dm_op_map_mem_type_to_ioreq_server),
         [XEN_DMOP_remote_shutdown]                  = sizeof(struct xen_dm_op_remote_shutdown),
         [XEN_DMOP_relocate_memory]                  = sizeof(struct xen_dm_op_relocate_memory),
@@ -539,6 +540,18 @@  int dm_op(const struct dmop_args *op_args)
         break;
     }
 
+    case XEN_DMOP_inject_msi2:
+    {
+        const struct xen_dm_op_inject_msi2 *data =
+            &op.u.inject_msi2;
+
+        if ( !(data->flags & XEN_DMOP_MSI_SOURCE_ID_VALID) )
+            printk(XENLOG_WARNING "XEN_DMOP_inject_msi2: source_id is ignored\n");
+
+        rc = hvm_inject_msi(d, data->addr, data->data);
+        break;
+    }
+
     case XEN_DMOP_remote_shutdown:
     {
         const struct xen_dm_op_remote_shutdown *data =
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index fa98551914..da2ce4a7f7 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -444,6 +444,17 @@  struct xen_dm_op_nr_vcpus {
 };
 typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t;
 
+#define XEN_DMOP_inject_msi2 21
+#define XEN_DMOP_MSI_SOURCE_ID_VALID (1u << 0)
+
+struct xen_dm_op_inject_msi2 {
+    uint64_aligned_t addr;
+    uint32_t data;
+    uint32_t source_id; /* PCI SBDF */
+    uint32_t flags;
+};
+typedef struct xen_dm_op_inject_msi2 xen_dm_op_inject_msi2_t;
+
 struct xen_dm_op {
     uint32_t op;
     uint32_t pad;
@@ -463,6 +474,7 @@  struct xen_dm_op {
         xen_dm_op_set_mem_type_t set_mem_type;
         xen_dm_op_inject_event_t inject_event;
         xen_dm_op_inject_msi_t inject_msi;
+        xen_dm_op_inject_msi2_t inject_msi2;
         xen_dm_op_map_mem_type_to_ioreq_server_t map_mem_type_to_ioreq_server;
         xen_dm_op_remote_shutdown_t remote_shutdown;
         xen_dm_op_relocate_memory_t relocate_memory;