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);
+
 /**
  * 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

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

On 24.01.24 22:25, Andrew Cooper wrote:
> 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);
>>   
>> + * @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.
> 

Hi Andrew,
I was thinking that the plan is to eventually deprecate the inject_msi 
call and have only the new one with different behaviors depending on 
source_id_valid flag.
Do you mean we are leaving both of them indefinitely and should treat 
source id as a valid one implicitly from the fact that the new call is 
being issued?

-- 
Mykyta

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
>

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

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.

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

On 15.01.24 11:35, Jan Beulich wrote:
> On 14.01.2024 11:01, Mykyta Poturai wrote:
>> --- 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).
> 
Hi Jan,

I'm planning on resuming this series in the near future and want to 
clarify the DM op interface.

Wouldn't it be better to keep things consistent and use 
XEN_DMOP_PCI_SBDF as it's done in xen_dm_op_ioreq_server_range? Also 
with this, the value can be easily casted to pci_sbdf_t later and split 
to segment and BDF if needed.

>> +    uint32_t flags;
>> +};
> 
> Just like in struct xen_dm_op_inject_msi padding wants making explicit,
> and then wants checking for being zero.
> 
> Jan

Will do.

-- 
Mykyta

On 18.03.2025 10:10, Mykyta Poturai wrote:
> On 15.01.24 11:35, Jan Beulich wrote:
>> On 14.01.2024 11:01, Mykyta Poturai wrote:
>>> --- 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).
> 
> I'm planning on resuming this series in the near future and want to 
> clarify the DM op interface.
> 
> Wouldn't it be better to keep things consistent and use 
> XEN_DMOP_PCI_SBDF as it's done in xen_dm_op_ioreq_server_range? Also 
> with this, the value can be easily casted to pci_sbdf_t later and split 
> to segment and BDF if needed.

The essence of my earlier comment is: Naming, contents, and comments need
to be in sync.

I question though that "casting to pci_sbdf_t" is technically possible.
Nor am I convinced that it would be desirable to do so if it was possible
(or if "casting" was intended to mean something else than what this is in
C). See my recent comments on some of Andrii's patches [1][2].

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01294.html
[2] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01301.html

On 18.03.25 12:11, Jan Beulich wrote:
> On 18.03.2025 10:10, Mykyta Poturai wrote:
>> On 15.01.24 11:35, Jan Beulich wrote:
>>> On 14.01.2024 11:01, Mykyta Poturai wrote:
>>>> --- 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).
>>
>> I'm planning on resuming this series in the near future and want to
>> clarify the DM op interface.
>>
>> Wouldn't it be better to keep things consistent and use
>> XEN_DMOP_PCI_SBDF as it's done in xen_dm_op_ioreq_server_range? Also
>> with this, the value can be easily casted to pci_sbdf_t later and split
>> to segment and BDF if needed.
>
> The essence of my earlier comment is: Naming, contents, and comments need
> to be in sync.
>
> I question though that "casting to pci_sbdf_t" is technically possible.
> Nor am I convinced that it would be desirable to do so if it was possible
> (or if "casting" was intended to mean something else than what this is in
> C). See my recent comments on some of Andrii's patches [1][2].
>
> Jan
>
> [1] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01294.html
> [2] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01301.html

Would something like this be okay then?

struct xen_dm_op_inject_msi2 {
     /* IN - MSI data (lower 32 bits) */
     uint32_t data;
     /* IN - ITS devid of the device triggering the interrupt */
     uint32_t source_id;
     uint32_t flags;
     uint32_t pad;
     /* IN - MSI address */
     uint64_aligned_t addr;
};

Added padding and explained source_id purpose better.
--
Mykyta

On 18.03.2025 14:31, Mykyta Poturai wrote:
> On 18.03.25 12:11, Jan Beulich wrote:
>> On 18.03.2025 10:10, Mykyta Poturai wrote:
>>> On 15.01.24 11:35, Jan Beulich wrote:
>>>> On 14.01.2024 11:01, Mykyta Poturai wrote:
>>>>> --- 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).
>>>
>>> I'm planning on resuming this series in the near future and want to
>>> clarify the DM op interface.
>>>
>>> Wouldn't it be better to keep things consistent and use
>>> XEN_DMOP_PCI_SBDF as it's done in xen_dm_op_ioreq_server_range? Also
>>> with this, the value can be easily casted to pci_sbdf_t later and split
>>> to segment and BDF if needed.
>>
>> The essence of my earlier comment is: Naming, contents, and comments need
>> to be in sync.
>>
>> I question though that "casting to pci_sbdf_t" is technically possible.
>> Nor am I convinced that it would be desirable to do so if it was possible
>> (or if "casting" was intended to mean something else than what this is in
>> C). See my recent comments on some of Andrii's patches [1][2].
>>
>> Jan
>>
>> [1] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01294.html
>> [2] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01301.html
> 
> Would something like this be okay then?
> 
> struct xen_dm_op_inject_msi2 {
>      /* IN - MSI data (lower 32 bits) */
>      uint32_t data;
>      /* IN - ITS devid of the device triggering the interrupt */
>      uint32_t source_id;
>      uint32_t flags;
>      uint32_t pad;
>      /* IN - MSI address */
>      uint64_aligned_t addr;
> };
> 
> Added padding and explained source_id purpose better.

I fear the comment is far from making clear what layout source_id is to
have, hence also leaving open whether a segment number is included there.
Of course the issue here may be that I have no clue what "ITS devid"
means or implies. What is clear is that "ITS devid" is meaningless on
x86, for example.

I'm further puzzled by "(lower 32 bits)" - isn't the data written to
trigger an MSI always a 32-bit quantity?

Jan

On 18.03.25 16:26, Jan Beulich wrote:
> On 18.03.2025 14:31, Mykyta Poturai wrote:
>> On 18.03.25 12:11, Jan Beulich wrote:
>>> On 18.03.2025 10:10, Mykyta Poturai wrote:
>>>> On 15.01.24 11:35, Jan Beulich wrote:
>>>>> On 14.01.2024 11:01, Mykyta Poturai wrote:
>>>>>> --- 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).
>>>>
>>>> I'm planning on resuming this series in the near future and want to
>>>> clarify the DM op interface.
>>>>
>>>> Wouldn't it be better to keep things consistent and use
>>>> XEN_DMOP_PCI_SBDF as it's done in xen_dm_op_ioreq_server_range? Also
>>>> with this, the value can be easily casted to pci_sbdf_t later and split
>>>> to segment and BDF if needed.
>>>
>>> The essence of my earlier comment is: Naming, contents, and comments need
>>> to be in sync.
>>>
>>> I question though that "casting to pci_sbdf_t" is technically possible.
>>> Nor am I convinced that it would be desirable to do so if it was possible
>>> (or if "casting" was intended to mean something else than what this is in
>>> C). See my recent comments on some of Andrii's patches [1][2].
>>>
>>> Jan
>>>
>>> [1] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01294.html
>>> [2] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01301.html
>>
>> Would something like this be okay then?
>>
>> struct xen_dm_op_inject_msi2 {
>>       /* IN - MSI data (lower 32 bits) */
>>       uint32_t data;
>>       /* IN - ITS devid of the device triggering the interrupt */
>>       uint32_t source_id;
>>       uint32_t flags;
>>       uint32_t pad;
>>       /* IN - MSI address */
>>       uint64_aligned_t addr;
>> };
>>
>> Added padding and explained source_id purpose better.
>
> I fear the comment is far from making clear what layout source_id is to
> have, hence also leaving open whether a segment number is included there.
> Of course the issue here may be that I have no clue what "ITS devid"
> means or implies. What is clear is that "ITS devid" is meaningless on
> x86, for example.

ITS devid is implementation defined. Its size is also implementation
defined but up to 32 bits.

Quotes from Arm Base System Architecture[1]:
 > The system designer assigns a requester a unique StreamID to device
traffic input to the SMMU.
 > If a requester is a bridge from a different interconnect with an
originator ID, like a PCIe RequesterID, and devices on that interconnect
might need to send MSIs, the originator ID is used to generate a
DeviceID. The function to generate the DeviceID should be an identity or
a simple offset.

On the Xen's side it is used as an offset into the ITS translation
tables and is sourced from msi-map nodes in the device tree.

Practically for PCI the requester ID is usually the SBDF. Where the
segment is used to find the host bridge node that contains the msi-map
node with defined offsets but it is also included as part of an id.
That's why it was originally called SBDF in the comment. I don't know if
there is a better way to describe what it is concisely in the comments.

> I'm further puzzled by "(lower 32 bits)" - isn't the data written to
> trigger an MSI always a 32-bit quantity?
>
> Jan

Hmm, it actually is, I copied this line from xen_dm_op_inject_msi, not
sure why it is specified like that there. But I'll remove this part.

[1]: https://developer.arm.com/documentation/den0094/latest/

--
Mykyta

On 19.03.2025 13:05, Mykyta Poturai wrote:
> On 18.03.25 16:26, Jan Beulich wrote:
>> On 18.03.2025 14:31, Mykyta Poturai wrote:
>>> On 18.03.25 12:11, Jan Beulich wrote:
>>>> On 18.03.2025 10:10, Mykyta Poturai wrote:
>>>>> On 15.01.24 11:35, Jan Beulich wrote:
>>>>>> On 14.01.2024 11:01, Mykyta Poturai wrote:
>>>>>>> --- 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).
>>>>>
>>>>> I'm planning on resuming this series in the near future and want to
>>>>> clarify the DM op interface.
>>>>>
>>>>> Wouldn't it be better to keep things consistent and use
>>>>> XEN_DMOP_PCI_SBDF as it's done in xen_dm_op_ioreq_server_range? Also
>>>>> with this, the value can be easily casted to pci_sbdf_t later and split
>>>>> to segment and BDF if needed.
>>>>
>>>> The essence of my earlier comment is: Naming, contents, and comments need
>>>> to be in sync.
>>>>
>>>> I question though that "casting to pci_sbdf_t" is technically possible.
>>>> Nor am I convinced that it would be desirable to do so if it was possible
>>>> (or if "casting" was intended to mean something else than what this is in
>>>> C). See my recent comments on some of Andrii's patches [1][2].
>>>>
>>>> Jan
>>>>
>>>> [1] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01294.html
>>>> [2] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01301.html
>>>
>>> Would something like this be okay then?
>>>
>>> struct xen_dm_op_inject_msi2 {
>>>       /* IN - MSI data (lower 32 bits) */
>>>       uint32_t data;
>>>       /* IN - ITS devid of the device triggering the interrupt */
>>>       uint32_t source_id;
>>>       uint32_t flags;
>>>       uint32_t pad;
>>>       /* IN - MSI address */
>>>       uint64_aligned_t addr;
>>> };
>>>
>>> Added padding and explained source_id purpose better.
>>
>> I fear the comment is far from making clear what layout source_id is to
>> have, hence also leaving open whether a segment number is included there.
>> Of course the issue here may be that I have no clue what "ITS devid"
>> means or implies. What is clear is that "ITS devid" is meaningless on
>> x86, for example.
> 
> ITS devid is implementation defined. Its size is also implementation
> defined but up to 32 bits.
> 
> Quotes from Arm Base System Architecture[1]:
>  > The system designer assigns a requester a unique StreamID to device
> traffic input to the SMMU.
>  > If a requester is a bridge from a different interconnect with an
> originator ID, like a PCIe RequesterID, and devices on that interconnect
> might need to send MSIs, the originator ID is used to generate a
> DeviceID. The function to generate the DeviceID should be an identity or
> a simple offset.
> 
> On the Xen's side it is used as an offset into the ITS translation
> tables and is sourced from msi-map nodes in the device tree.
> 
> Practically for PCI the requester ID is usually the SBDF. Where the
> segment is used to find the host bridge node that contains the msi-map
> node with defined offsets but it is also included as part of an id.
> That's why it was originally called SBDF in the comment. I don't know if
> there is a better way to describe what it is concisely in the comments.

If this is to be usable for other architectures too, it may need to be
a union to put there. With appropriate comments for each member.

Jan

On 19.03.25 14:37, Jan Beulich wrote:

Hello Jan, all.


> On 19.03.2025 13:05, Mykyta Poturai wrote:
>> On 18.03.25 16:26, Jan Beulich wrote:
>>> On 18.03.2025 14:31, Mykyta Poturai wrote:
>>>> On 18.03.25 12:11, Jan Beulich wrote:
>>>>> On 18.03.2025 10:10, Mykyta Poturai wrote:
>>>>>> On 15.01.24 11:35, Jan Beulich wrote:
>>>>>>> On 14.01.2024 11:01, Mykyta Poturai wrote:
>>>>>>>> --- 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).
>>>>>>
>>>>>> I'm planning on resuming this series in the near future and want to
>>>>>> clarify the DM op interface.
>>>>>>
>>>>>> Wouldn't it be better to keep things consistent and use
>>>>>> XEN_DMOP_PCI_SBDF as it's done in xen_dm_op_ioreq_server_range? Also
>>>>>> with this, the value can be easily casted to pci_sbdf_t later and split
>>>>>> to segment and BDF if needed.
>>>>>
>>>>> The essence of my earlier comment is: Naming, contents, and comments need
>>>>> to be in sync.
>>>>>
>>>>> I question though that "casting to pci_sbdf_t" is technically possible.
>>>>> Nor am I convinced that it would be desirable to do so if it was possible
>>>>> (or if "casting" was intended to mean something else than what this is in
>>>>> C). See my recent comments on some of Andrii's patches [1][2].
>>>>>
>>>>> Jan
>>>>>
>>>>> [1] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01294.html
>>>>> [2] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01301.html
>>>>
>>>> Would something like this be okay then?
>>>>
>>>> struct xen_dm_op_inject_msi2 {
>>>>        /* IN - MSI data (lower 32 bits) */
>>>>        uint32_t data;
>>>>        /* IN - ITS devid of the device triggering the interrupt */
>>>>        uint32_t source_id;
>>>>        uint32_t flags;
>>>>        uint32_t pad;
>>>>        /* IN - MSI address */
>>>>        uint64_aligned_t addr;
>>>> };
>>>>
>>>> Added padding and explained source_id purpose better.
>>>
>>> I fear the comment is far from making clear what layout source_id is to
>>> have, hence also leaving open whether a segment number is included there.
>>> Of course the issue here may be that I have no clue what "ITS devid"
>>> means or implies. What is clear is that "ITS devid" is meaningless on
>>> x86, for example.
>>
>> ITS devid is implementation defined. Its size is also implementation
>> defined but up to 32 bits.
>>
>> Quotes from Arm Base System Architecture[1]:
>>   > The system designer assigns a requester a unique StreamID to device
>> traffic input to the SMMU.
>>   > If a requester is a bridge from a different interconnect with an
>> originator ID, like a PCIe RequesterID, and devices on that interconnect
>> might need to send MSIs, the originator ID is used to generate a
>> DeviceID. The function to generate the DeviceID should be an identity or
>> a simple offset.
>>
>> On the Xen's side it is used as an offset into the ITS translation
>> tables and is sourced from msi-map nodes in the device tree.
>>
>> Practically for PCI the requester ID is usually the SBDF. Where the
>> segment is used to find the host bridge node that contains the msi-map
>> node with defined offsets but it is also included as part of an id.
>> That's why it was originally called SBDF in the comment. I don't know if
>> there is a better way to describe what it is concisely in the comments.
> 
> If this is to be usable for other architectures too, it may need to be
> a union to put there. With appropriate comments for each member.


If I got correctly what is wrote in current thread (and in RFC version 
where it was an attempt to create just Arm64's counterpart of 
XEN_DMOP_inject_msi), my understanding (maybe not precise/correct, since 
I am not quite familiar with x86 details) that we would need something 
like that:


/*
  * XEN_DMOP_inject_msi2: An enhanced version of the sub-ob to inject an MSI
  *                       for an emulated device, which allows specifying
  *                       the ID of the device triggering the MSI 
(source ID).
  *
  * The source ID is specified by a pair of <segment> and <source_id>.
  * If <flags> does not contain XEN_DMOP_MSI_SOURCE_ID_VALID then source ID
  * is invalid and should be ignored.
  */
#define XEN_DMOP_inject_msi 21

struct xen_dm_op_inject_msi2 {
      /* IN - MSI data */
      uint32_t data;
      /* IN - next two fields form an ID of the device triggering the MSI */
      uint16_t segment; /* The segment number */
      uint16_t source_id; /* The source ID that is local to segment (PCI 
BDF) */
      /* IN - types of source ID */
      uint32_t flags;
#define XEN_DMOP_MSI_SOURCE_ID_VALID (1u << 0)
      uint32_t pad;
      /* IN - MSI address */
      uint64_aligned_t addr;
};


This is arch agnostic sub-op without the (obvious) specifics of the 
underlying MSI controller. The sub-ob, I hope, will be suitable on both 
Arm64 soon (segment + source_id provide vSBDF, then it will possible to 
locate vITS and devid) and on x86 in future (for the vIOMMU needs).

Would you be ok with that in general? Please share you opinion.


> 
> Jan
>

On 19.03.2025 21:42, Oleksandr Tyshchenko wrote:
> On 19.03.25 14:37, Jan Beulich wrote:
>> On 19.03.2025 13:05, Mykyta Poturai wrote:
>>> On 18.03.25 16:26, Jan Beulich wrote:
>>>> On 18.03.2025 14:31, Mykyta Poturai wrote:
>>>>> On 18.03.25 12:11, Jan Beulich wrote:
>>>>>> On 18.03.2025 10:10, Mykyta Poturai wrote:
>>>>>>> On 15.01.24 11:35, Jan Beulich wrote:
>>>>>>>> On 14.01.2024 11:01, Mykyta Poturai wrote:
>>>>>>>>> --- 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).
>>>>>>>
>>>>>>> I'm planning on resuming this series in the near future and want to
>>>>>>> clarify the DM op interface.
>>>>>>>
>>>>>>> Wouldn't it be better to keep things consistent and use
>>>>>>> XEN_DMOP_PCI_SBDF as it's done in xen_dm_op_ioreq_server_range? Also
>>>>>>> with this, the value can be easily casted to pci_sbdf_t later and split
>>>>>>> to segment and BDF if needed.
>>>>>>
>>>>>> The essence of my earlier comment is: Naming, contents, and comments need
>>>>>> to be in sync.
>>>>>>
>>>>>> I question though that "casting to pci_sbdf_t" is technically possible.
>>>>>> Nor am I convinced that it would be desirable to do so if it was possible
>>>>>> (or if "casting" was intended to mean something else than what this is in
>>>>>> C). See my recent comments on some of Andrii's patches [1][2].
>>>>>>
>>>>>> Jan
>>>>>>
>>>>>> [1] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01294.html
>>>>>> [2] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01301.html
>>>>>
>>>>> Would something like this be okay then?
>>>>>
>>>>> struct xen_dm_op_inject_msi2 {
>>>>>        /* IN - MSI data (lower 32 bits) */
>>>>>        uint32_t data;
>>>>>        /* IN - ITS devid of the device triggering the interrupt */
>>>>>        uint32_t source_id;
>>>>>        uint32_t flags;
>>>>>        uint32_t pad;
>>>>>        /* IN - MSI address */
>>>>>        uint64_aligned_t addr;
>>>>> };
>>>>>
>>>>> Added padding and explained source_id purpose better.
>>>>
>>>> I fear the comment is far from making clear what layout source_id is to
>>>> have, hence also leaving open whether a segment number is included there.
>>>> Of course the issue here may be that I have no clue what "ITS devid"
>>>> means or implies. What is clear is that "ITS devid" is meaningless on
>>>> x86, for example.
>>>
>>> ITS devid is implementation defined. Its size is also implementation
>>> defined but up to 32 bits.
>>>
>>> Quotes from Arm Base System Architecture[1]:
>>>   > The system designer assigns a requester a unique StreamID to device
>>> traffic input to the SMMU.
>>>   > If a requester is a bridge from a different interconnect with an
>>> originator ID, like a PCIe RequesterID, and devices on that interconnect
>>> might need to send MSIs, the originator ID is used to generate a
>>> DeviceID. The function to generate the DeviceID should be an identity or
>>> a simple offset.
>>>
>>> On the Xen's side it is used as an offset into the ITS translation
>>> tables and is sourced from msi-map nodes in the device tree.
>>>
>>> Practically for PCI the requester ID is usually the SBDF. Where the
>>> segment is used to find the host bridge node that contains the msi-map
>>> node with defined offsets but it is also included as part of an id.
>>> That's why it was originally called SBDF in the comment. I don't know if
>>> there is a better way to describe what it is concisely in the comments.
>>
>> If this is to be usable for other architectures too, it may need to be
>> a union to put there. With appropriate comments for each member.
> 
> 
> If I got correctly what is wrote in current thread (and in RFC version 
> where it was an attempt to create just Arm64's counterpart of 
> XEN_DMOP_inject_msi), my understanding (maybe not precise/correct, since 
> I am not quite familiar with x86 details) that we would need something 
> like that:
> 
> 
> /*
>   * XEN_DMOP_inject_msi2: An enhanced version of the sub-ob to inject an MSI
>   *                       for an emulated device, which allows specifying
>   *                       the ID of the device triggering the MSI 
> (source ID).
>   *
>   * The source ID is specified by a pair of <segment> and <source_id>.
>   * If <flags> does not contain XEN_DMOP_MSI_SOURCE_ID_VALID then source ID
>   * is invalid and should be ignored.
>   */
> #define XEN_DMOP_inject_msi 21
> 
> struct xen_dm_op_inject_msi2 {
>       /* IN - MSI data */
>       uint32_t data;
>       /* IN - next two fields form an ID of the device triggering the MSI */
>       uint16_t segment; /* The segment number */
>       uint16_t source_id; /* The source ID that is local to segment (PCI 
> BDF) */
>       /* IN - types of source ID */
>       uint32_t flags;
> #define XEN_DMOP_MSI_SOURCE_ID_VALID (1u << 0)
>       uint32_t pad;
>       /* IN - MSI address */
>       uint64_aligned_t addr;
> };
> 
> 
> This is arch agnostic sub-op without the (obvious) specifics of the 
> underlying MSI controller. The sub-ob, I hope, will be suitable on both 
> Arm64 soon (segment + source_id provide vSBDF, then it will possible to 
> locate vITS and devid) and on x86 in future (for the vIOMMU needs).
> 
> Would you be ok with that in general? Please share you opinion.

Yes, this looks plausible. In the context of things like VMD (using
software established segment numbers wider than 16 bits) I wonder
though whether we wouldn't better make segment and source ID 32-bit
fields from the beginning. Out-of-range values would of course need
rejecting then.

Jan

On 20.03.25 09:30, Jan Beulich wrote:

Hello Jan, Mykyta, all


> On 19.03.2025 21:42, Oleksandr Tyshchenko wrote:
>> On 19.03.25 14:37, Jan Beulich wrote:
>>> On 19.03.2025 13:05, Mykyta Poturai wrote:
>>>> On 18.03.25 16:26, Jan Beulich wrote:
>>>>> On 18.03.2025 14:31, Mykyta Poturai wrote:
>>>>>> On 18.03.25 12:11, Jan Beulich wrote:
>>>>>>> On 18.03.2025 10:10, Mykyta Poturai wrote:
>>>>>>>> On 15.01.24 11:35, Jan Beulich wrote:
>>>>>>>>> On 14.01.2024 11:01, Mykyta Poturai wrote:
>>>>>>>>>> --- 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).
>>>>>>>>
>>>>>>>> I'm planning on resuming this series in the near future and want to
>>>>>>>> clarify the DM op interface.
>>>>>>>>
>>>>>>>> Wouldn't it be better to keep things consistent and use
>>>>>>>> XEN_DMOP_PCI_SBDF as it's done in xen_dm_op_ioreq_server_range? Also
>>>>>>>> with this, the value can be easily casted to pci_sbdf_t later and split
>>>>>>>> to segment and BDF if needed.
>>>>>>>
>>>>>>> The essence of my earlier comment is: Naming, contents, and comments need
>>>>>>> to be in sync.
>>>>>>>
>>>>>>> I question though that "casting to pci_sbdf_t" is technically possible.
>>>>>>> Nor am I convinced that it would be desirable to do so if it was possible
>>>>>>> (or if "casting" was intended to mean something else than what this is in
>>>>>>> C). See my recent comments on some of Andrii's patches [1][2].
>>>>>>>
>>>>>>> Jan
>>>>>>>
>>>>>>> [1] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01294.html
>>>>>>> [2] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01301.html
>>>>>>
>>>>>> Would something like this be okay then?
>>>>>>
>>>>>> struct xen_dm_op_inject_msi2 {
>>>>>>         /* IN - MSI data (lower 32 bits) */
>>>>>>         uint32_t data;
>>>>>>         /* IN - ITS devid of the device triggering the interrupt */
>>>>>>         uint32_t source_id;
>>>>>>         uint32_t flags;
>>>>>>         uint32_t pad;
>>>>>>         /* IN - MSI address */
>>>>>>         uint64_aligned_t addr;
>>>>>> };
>>>>>>
>>>>>> Added padding and explained source_id purpose better.
>>>>>
>>>>> I fear the comment is far from making clear what layout source_id is to
>>>>> have, hence also leaving open whether a segment number is included there.
>>>>> Of course the issue here may be that I have no clue what "ITS devid"
>>>>> means or implies. What is clear is that "ITS devid" is meaningless on
>>>>> x86, for example.
>>>>
>>>> ITS devid is implementation defined. Its size is also implementation
>>>> defined but up to 32 bits.
>>>>
>>>> Quotes from Arm Base System Architecture[1]:
>>>>    > The system designer assigns a requester a unique StreamID to device
>>>> traffic input to the SMMU.
>>>>    > If a requester is a bridge from a different interconnect with an
>>>> originator ID, like a PCIe RequesterID, and devices on that interconnect
>>>> might need to send MSIs, the originator ID is used to generate a
>>>> DeviceID. The function to generate the DeviceID should be an identity or
>>>> a simple offset.
>>>>
>>>> On the Xen's side it is used as an offset into the ITS translation
>>>> tables and is sourced from msi-map nodes in the device tree.
>>>>
>>>> Practically for PCI the requester ID is usually the SBDF. Where the
>>>> segment is used to find the host bridge node that contains the msi-map
>>>> node with defined offsets but it is also included as part of an id.
>>>> That's why it was originally called SBDF in the comment. I don't know if
>>>> there is a better way to describe what it is concisely in the comments.
>>>
>>> If this is to be usable for other architectures too, it may need to be
>>> a union to put there. With appropriate comments for each member.
>>
>>
>> If I got correctly what is wrote in current thread (and in RFC version
>> where it was an attempt to create just Arm64's counterpart of
>> XEN_DMOP_inject_msi), my understanding (maybe not precise/correct, since
>> I am not quite familiar with x86 details) that we would need something
>> like that:
>>
>>
>> /*
>>    * XEN_DMOP_inject_msi2: An enhanced version of the sub-ob to inject an MSI
>>    *                       for an emulated device, which allows specifying
>>    *                       the ID of the device triggering the MSI
>> (source ID).
>>    *
>>    * The source ID is specified by a pair of <segment> and <source_id>.
>>    * If <flags> does not contain XEN_DMOP_MSI_SOURCE_ID_VALID then source ID
>>    * is invalid and should be ignored.
>>    */
>> #define XEN_DMOP_inject_msi 21
>>
>> struct xen_dm_op_inject_msi2 {
>>        /* IN - MSI data */
>>        uint32_t data;
>>        /* IN - next two fields form an ID of the device triggering the MSI */
>>        uint16_t segment; /* The segment number */
>>        uint16_t source_id; /* The source ID that is local to segment (PCI
>> BDF) */
>>        /* IN - types of source ID */
>>        uint32_t flags;
>> #define XEN_DMOP_MSI_SOURCE_ID_VALID (1u << 0)
>>        uint32_t pad;
>>        /* IN - MSI address */
>>        uint64_aligned_t addr;
>> };
>>
>>
>> This is arch agnostic sub-op without the (obvious) specifics of the
>> underlying MSI controller. The sub-ob, I hope, will be suitable on both
>> Arm64 soon (segment + source_id provide vSBDF, then it will possible to
>> locate vITS and devid) and on x86 in future (for the vIOMMU needs).
>>
>> Would you be ok with that in general? Please share you opinion.
> 
> Yes, this looks plausible.

Jan, thanks


  In the context of things like VMD (using
> software established segment numbers wider than 16 bits) I wonder
> though whether we wouldn't better make segment and source ID 32-bit
> fields from the beginning.

Keeping in mind that dm_op hypercall is stable and segment can be >= 
0x10000, probably yes, makes sense to use 32-bit from the very beginning.


  Out-of-range values would of course need
> rejecting then.

yes, sure

***

Mykyta, are you ok with the proposed adjustments to the sub-op structure?


> 
> Jan

On 20.03.25 15:07, Oleksandr Tyshchenko wrote:
>>> If I got correctly what is wrote in current thread (and in RFC version
>>> where it was an attempt to create just Arm64's counterpart of
>>> XEN_DMOP_inject_msi), my understanding (maybe not precise/correct, since
>>> I am not quite familiar with x86 details) that we would need something
>>> like that:
>>>
>>>
>>> /*
>>>    * XEN_DMOP_inject_msi2: An enhanced version of the sub-ob to 
>>> inject an MSI
>>>    *                       for an emulated device, which allows 
>>> specifying
>>>    *                       the ID of the device triggering the MSI
>>> (source ID).
>>>    *
>>>    * The source ID is specified by a pair of <segment> and <source_id>.
>>>    * If <flags> does not contain XEN_DMOP_MSI_SOURCE_ID_VALID then 
>>> source ID
>>>    * is invalid and should be ignored.
>>>    */
>>> #define XEN_DMOP_inject_msi 21
>>>
>>> struct xen_dm_op_inject_msi2 {
>>>        /* IN - MSI data */
>>>        uint32_t data;
>>>        /* IN - next two fields form an ID of the device triggering 
>>> the MSI */
>>>        uint16_t segment; /* The segment number */
>>>        uint16_t source_id; /* The source ID that is local to segment 
>>> (PCI
>>> BDF) */
>>>        /* IN - types of source ID */
>>>        uint32_t flags;
>>> #define XEN_DMOP_MSI_SOURCE_ID_VALID (1u << 0)
>>>        uint32_t pad;
>>>        /* IN - MSI address */
>>>        uint64_aligned_t addr;
>>> };
>>>
>>>
>>> This is arch agnostic sub-op without the (obvious) specifics of the
>>> underlying MSI controller. The sub-ob, I hope, will be suitable on both
>>> Arm64 soon (segment + source_id provide vSBDF, then it will possible to
>>> locate vITS and devid) and on x86 in future (for the vIOMMU needs).
>>>
>>> Would you be ok with that in general? Please share you opinion.
>>
>> Yes, this looks plausible.
> 
> Jan, thanks
> 
> 
>   In the context of things like VMD (using
>> software established segment numbers wider than 16 bits) I wonder
>> though whether we wouldn't better make segment and source ID 32-bit
>> fields from the beginning.
> 
> Keeping in mind that dm_op hypercall is stable and segment can be >= 
> 0x10000, probably yes, makes sense to use 32-bit from the very beginning.
> 
> 
>   Out-of-range values would of course need
>> rejecting then.
> 
> yes, sure
> 
> ***
> 
> Mykyta, are you ok with the proposed adjustments to the sub-op structure?
> 
>>
>> Jan

Yes, seems fine for me. I will use it in the next version.

-- 
Mykyta