[PATCH v2 0/2] Add support for MSI injection on Arm

[PATCH v2 0/2] Add support for MSI injection on Arm

[Mykyta Poturai]

This series adds the base support for MSI injection on Arm. This is
needed to streamline virtio-pci interrupt triggering.

With this patches, MSIs can be triggered in guests by issuing the new
DM op, inject_msi2. This op is similar to inject_msi, but it allows
to specify the source id of the MSI.

We chose the approach of adding a new DM op instead of using the pad
field of inject_msi because we have no clear way of distinguishing
between set and unset pad fields. New implementations also adds flags
field to clearly specify if the SBDF is set.

Patches were tested on QEMU with QEMU virtio-pci backends, with 
virtio-pci patches and patches for ITS support for DomUs applied.

Branch with all relevant Xen patches:
https://github.com/Deedone/xen/tree/4.20-dev%2Bvirtio

Branch with all relevant QEMU patches:
https://github.com/Deedone/qemu/tree/virtio-msi2

Mykyta Poturai (2):
  arm: vgic: Add the ability to trigger MSIs from the Hypervisor
  xen/dm: arm: Introduce inject_msi2 DM op

 tools/include/xendevicemodel.h               | 14 ++++++++++++++
 tools/libs/devicemodel/core.c                | 20 ++++++++++++++++++++
 tools/libs/devicemodel/libxendevicemodel.map |  5 +++++
 xen/arch/arm/dm.c                            | 17 +++++++++++++++++
 xen/arch/arm/include/asm/vgic.h              | 11 +++++++++++
 xen/arch/arm/vgic-v3-its.c                   | 19 +++++++++++++++++++
 xen/arch/x86/hvm/dm.c                        | 18 ++++++++++++++++++
 xen/include/public/hvm/dm_op.h               | 18 ++++++++++++++++++
 8 files changed, 122 insertions(+)

-- 
2.34.1

[PATCH v2 1/2] arm: vgic: Add the ability to trigger MSIs from the Hypervisor

[Mykyta Poturai]

From: Mykyta Poturai <Mykyta_Poturai@epam.com>

Add the vgic_its_trigger_msi() function to the vgic interface. This
function allows to inject MSIs from the Hypervisor to the guest.
Which is useful for userspace PCI backend drivers.

Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
v1->v2:
* replace -1 with -ENOENT
* reduce guest memory access in vgic_its_trigger_msi
---
 xen/arch/arm/include/asm/vgic.h | 11 +++++++++++
 xen/arch/arm/vgic-v3-its.c      | 19 +++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
index e309dca1ad..3d8e3a8343 100644
--- a/xen/arch/arm/include/asm/vgic.h
+++ b/xen/arch/arm/include/asm/vgic.h
@@ -318,6 +318,17 @@ extern bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
 extern void vgic_check_inflight_irqs_pending(struct vcpu *v,
                                              unsigned int rank, uint32_t r);
 
+#ifdef CONFIG_HAS_ITS
+int vgic_its_trigger_msi(struct domain *d, paddr_t doorbell_address,
+                                u32 devid, u32 eventid);
+#else
+static inline int vgic_its_trigger_msi(struct domain *d, paddr_t doorbell_address,
+                                u32 devid, u32 eventid)
+{
+    return -EOPNOTSUPP;
+}
+#endif /* CONFIG_HAS_ITS */
+
 #endif /* !CONFIG_NEW_VGIC */
 
 /*** Common VGIC functions used by Xen arch code ****/
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index c65c1dbf52..be5bfe0d21 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -1484,6 +1484,25 @@ static int vgic_v3_its_init_virtual(struct domain *d, paddr_t guest_addr,
     return 0;
 }
 
+int vgic_its_trigger_msi(struct domain *d, paddr_t doorbell_address,
+                                u32 devid, u32 eventid)
+{
+    struct pending_irq *pend;
+    unsigned int vcpu_id;
+
+    pend = gicv3_its_get_event_pending_irq(d,doorbell_address, devid, eventid);
+    if ( !pend )
+        return -ENOENT;
+    
+    vcpu_id = ACCESS_ONCE(pend->lpi_vcpu_id);
+    if ( vcpu_id >= d->max_vcpus )
+          return -ENOENT;
+
+    vgic_inject_irq(d, d->vcpu[vcpu_id], pend->irq, true);
+
+    return 0;
+}
+
 unsigned int vgic_v3_its_count(const struct domain *d)
 {
     struct host_its *hw_its;
-- 
2.34.1

Re: [PATCH v2 1/2] arm: vgic: Add the ability to trigger MSIs from the Hypervisor

[Julien Grall]

Hi,

On 14/04/2025 18:51, Mykyta Poturai wrote:
> From: Mykyta Poturai <Mykyta_Poturai@epam.com>
> 
> Add the vgic_its_trigger_msi() function to the vgic interface. This
> function allows to inject MSIs from the Hypervisor to the guest.
> Which is useful for userspace PCI backend drivers.
> 
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> ---
> v1->v2:
> * replace -1 with -ENOENT
> * reduce guest memory access in vgic_its_trigger_msi
> ---
>   xen/arch/arm/include/asm/vgic.h | 11 +++++++++++
>   xen/arch/arm/vgic-v3-its.c      | 19 +++++++++++++++++++
>   2 files changed, 30 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
> index e309dca1ad..3d8e3a8343 100644
> --- a/xen/arch/arm/include/asm/vgic.h
> +++ b/xen/arch/arm/include/asm/vgic.h
> @@ -318,6 +318,17 @@ extern bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
>   extern void vgic_check_inflight_irqs_pending(struct vcpu *v,
>                                                unsigned int rank, uint32_t r);
>   
> +#ifdef CONFIG_HAS_ITS
> +int vgic_its_trigger_msi(struct domain *d, paddr_t doorbell_address,
> +                                u32 devid, u32 eventid);
> +#else
> +static inline int vgic_its_trigger_msi(struct domain *d, paddr_t doorbell_address,
> +                                u32 devid, u32 eventid)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif /* CONFIG_HAS_ITS */
> +
>   #endif /* !CONFIG_NEW_VGIC */
>   
>   /*** Common VGIC functions used by Xen arch code ****/
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index c65c1dbf52..be5bfe0d21 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -1484,6 +1484,25 @@ static int vgic_v3_its_init_virtual(struct domain *d, paddr_t guest_addr,
>       return 0;
>   }
>   
> +int vgic_its_trigger_msi(struct domain *d, paddr_t doorbell_address,
> +                                u32 devid, u32 eventid)

Looking at how this is used in the next patch, you are assuming that 
devid == sbdf. However, given there is no support for guest vITS yet, I 
don't think we took a decision on whether the device ID match the SBDF.

This goes back to what I wrote in the cover letter. It seems that there 
are more work needed to be merged before this series.

Also coding style: we use uint32_t in newer code.

> +{
> +    struct pending_irq *pend;
> +    unsigned int vcpu_id;
> +
> +    pend = gicv3_its_get_event_pending_irq(d,doorbell_address, devid, eventid);

A couple of questions:

  1. What prevents pending_irq to be freed behind our back?
  2. Going back to my point about the missing guest ITS support, it is 
unclear how virtual device will work and whether we want QEMU to inject 
interrupts which belongs to a physical device.

coding style: Missing space before the comma.

> +    if ( !pend )
> +        return -ENOENT;
> +
> +    vcpu_id = ACCESS_ONCE(pend->lpi_vcpu_id);
> +    if ( vcpu_id >= d->max_vcpus )
> +          return -ENOENT;
> +
> +    vgic_inject_irq(d, d->vcpu[vcpu_id], pend->irq, true);
> +
> +    return 0;
> +}
> +
>   unsigned int vgic_v3_its_count(const struct domain *d)
>   {
>       struct host_its *hw_its;

Cheers,

-- 
Julien Grall

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

[Mykyta Poturai]

From: Mykyta Poturai <Mykyta_Poturai@epam.com>

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>
---
v1->v2:
* fix warning for ignored source_id
* rework xen_dm_op_inject_msi2 struct
* rework xendevicemodel_inject_msi2 params
---
 tools/include/xendevicemodel.h               | 14 ++++++++++++++
 tools/libs/devicemodel/core.c                | 20 ++++++++++++++++++++
 tools/libs/devicemodel/libxendevicemodel.map |  5 +++++
 xen/arch/arm/dm.c                            | 17 +++++++++++++++++
 xen/arch/x86/hvm/dm.c                        | 18 ++++++++++++++++++
 xen/include/public/hvm/dm_op.h               | 18 ++++++++++++++++++
 6 files changed, 92 insertions(+)

diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h
index 227e7fd810..d0847dfdc8 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
+ * @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 addr, uint32_t source_id,
+    uint32_t data);
+
 /**
  * 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..92db92e89b 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -448,6 +448,26 @@ 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 addr, uint32_t source_id,
+    uint32_t data)
+{
+    uint16_t segment = source_id >> 16;
+    uint16_t bdf = source_id & 0xffff;
+    struct xen_dm_op op = {
+        .op = XEN_DMOP_inject_msi2,
+        .u.inject_msi2 = {
+            .addr = addr,
+            .data = data,
+            .segment = segment,
+            .source_id = bdf,
+            .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 773a0a2592..a1340b45a3 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,22 @@ 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;
+        uint32_t pci_sbdf = data->segment << 16 | data->source_id;
+
+        if ( !(data->flags & XEN_DMOP_MSI_SOURCE_ID_VALID) || data->pad || 
+             data->flags & ~XEN_DMOP_MSI_SOURCE_ID_VALID )
+        {
+            rc = -EINVAL;
+            break;
+        }
+
+        rc = vgic_its_trigger_msi(d, data->addr, pci_sbdf, 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..da0a00844b 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,23 @@ 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->pad || data->flags & ~XEN_DMOP_MSI_SOURCE_ID_VALID )
+        {
+            rc = -EINVAL;
+            break;
+        }
+
+        if ( data->flags & XEN_DMOP_MSI_SOURCE_ID_VALID )
+            gprintk(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 2bf0fdc1ae..4141af4300 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -444,6 +444,23 @@ 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
+
+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;
+};
+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 +480,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

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

[Jan Beulich]

On 14.04.2025 11:51, Mykyta Poturai wrote:
> From: Mykyta Poturai <Mykyta_Poturai@epam.com>
> 
> 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.

If this is about Arm, why does x86 also gain (incomplete) handling?

> @@ -539,6 +540,23 @@ 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->pad || data->flags & ~XEN_DMOP_MSI_SOURCE_ID_VALID )

Nit: If the x86 code is to stay in the first place, parentheses please
around the & expression. Even if not, similar issues appear to exist
in the Arm code.

> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +
> +        if ( data->flags & XEN_DMOP_MSI_SOURCE_ID_VALID )
> +            gprintk(XENLOG_WARNING "XEN_DMOP_inject_msi2: source_id is ignored\n");

Does this compile? It looks to me as if there was a missing comma.

> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -444,6 +444,23 @@ 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
> +
> +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)

What purpose does this flag serve? IOW what's the value of using this
sub-op when one doesn't want to specify a source ID?

Jan

Re: [PATCH v2 0/2] Add support for MSI injection on Arm

[Julien Grall]

Hi Mykyta,

On 14/04/2025 18:51, Mykyta Poturai wrote:
> This series adds the base support for MSI injection on Arm. This is
> needed to streamline virtio-pci interrupt triggering.
> 
> With this patches, MSIs can be triggered in guests by issuing the new
> DM op, inject_msi2. This op is similar to inject_msi, but it allows
> to specify the source id of the MSI.
> 
> We chose the approach of adding a new DM op instead of using the pad
> field of inject_msi because we have no clear way of distinguishing
> between set and unset pad fields. New implementations also adds flags
> field to clearly specify if the SBDF is set.
> 
> Patches were tested on QEMU with

[...]

> patches for ITS support for DomUs applied.

This means this series is unusable without external patches. Given this 
is adding a new DM operations, I think it would be more sensible to have 
the vITS support merged first. Then we can look at merging this series.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 0/2] Add support for MSI injection on Arm

[Stefano Stabellini]

On Mon, 14 Apr 2025, Julien Grall wrote:
> Hi Mykyta,
> 
> On 14/04/2025 18:51, Mykyta Poturai wrote:
> > This series adds the base support for MSI injection on Arm. This is
> > needed to streamline virtio-pci interrupt triggering.
> > 
> > With this patches, MSIs can be triggered in guests by issuing the new
> > DM op, inject_msi2. This op is similar to inject_msi, but it allows
> > to specify the source id of the MSI.
> > 
> > We chose the approach of adding a new DM op instead of using the pad
> > field of inject_msi because we have no clear way of distinguishing
> > between set and unset pad fields. New implementations also adds flags
> > field to clearly specify if the SBDF is set.
> > 
> > Patches were tested on QEMU with
> 
> [...]
> 
> > patches for ITS support for DomUs applied.
> 
> This means this series is unusable without external patches. Given this is
> adding a new DM operations, I think it would be more sensible to have the vITS
> support merged first. Then we can look at merging this series.

Hi Mykyta, just checking explicitly with you whether this series
requires vgic-v3-its.c for DomUs?

If yes, how far are you from sending the relevant patches to xen-devel?
How many are they?

Re: [PATCH v2 0/2] Add support for MSI injection on Arm

[Mykyta Poturai]

On 16.04.25 01:40, Stefano Stabellini wrote:
> On Mon, 14 Apr 2025, Julien Grall wrote:
>> Hi Mykyta,
>>
>> On 14/04/2025 18:51, Mykyta Poturai wrote:
>>> This series adds the base support for MSI injection on Arm. This is
>>> needed to streamline virtio-pci interrupt triggering.
>>>
>>> With this patches, MSIs can be triggered in guests by issuing the new
>>> DM op, inject_msi2. This op is similar to inject_msi, but it allows
>>> to specify the source id of the MSI.
>>>
>>> We chose the approach of adding a new DM op instead of using the pad
>>> field of inject_msi because we have no clear way of distinguishing
>>> between set and unset pad fields. New implementations also adds flags
>>> field to clearly specify if the SBDF is set.
>>>
>>> Patches were tested on QEMU with
>>
>> [...]
>>
>>> patches for ITS support for DomUs applied.
>>
>> This means this series is unusable without external patches. Given this is
>> adding a new DM operations, I think it would be more sensible to have the vITS
>> support merged first. Then we can look at merging this series.
> 
> Hi Mykyta, just checking explicitly with you whether this series
> requires vgic-v3-its.c for DomUs?
> 
> If yes, how far are you from sending the relevant patches to xen-devel?
> How many are they?

Hi Stefano,
Yes, I am planning to send them together with VPCI MSI support after 
SMMU patches are merged.

Although the DomU vits itself is only two patches.
1. 
https://github.com/Deedone/xen/commit/c7fe54e776e4205fbb993e47a6b826f5154fad76
2. 
https://github.com/Deedone/xen/commit/b6b4ffadb368f540a0ae8ef6b4572f0cfd2eccaa

--
Mykyta

Re: [PATCH v2 0/2] Add support for MSI injection on Arm

[Julien Grall]

(+ Rahul)

On 16/04/2025 17:37, Mykyta Poturai wrote:
> On 16.04.25 01:40, Stefano Stabellini wrote:
>> On Mon, 14 Apr 2025, Julien Grall wrote:
>>> Hi Mykyta,
>>>
>>> On 14/04/2025 18:51, Mykyta Poturai wrote:
>>>> This series adds the base support for MSI injection on Arm. This is
>>>> needed to streamline virtio-pci interrupt triggering.
>>>>
>>>> With this patches, MSIs can be triggered in guests by issuing the new
>>>> DM op, inject_msi2. This op is similar to inject_msi, but it allows
>>>> to specify the source id of the MSI.
>>>>
>>>> We chose the approach of adding a new DM op instead of using the pad
>>>> field of inject_msi because we have no clear way of distinguishing
>>>> between set and unset pad fields. New implementations also adds flags
>>>> field to clearly specify if the SBDF is set.
>>>>
>>>> Patches were tested on QEMU with
>>>
>>> [...]
>>>
>>>> patches for ITS support for DomUs applied.
>>>
>>> This means this series is unusable without external patches. Given this is
>>> adding a new DM operations, I think it would be more sensible to have the vITS
>>> support merged first. Then we can look at merging this series.
>>
>> Hi Mykyta, just checking explicitly with you whether this series
>> requires vgic-v3-its.c for DomUs?
>>
>> If yes, how far are you from sending the relevant patches to xen-devel?
>> How many are they?
> 
> Hi Stefano,
> Yes, I am planning to send them together with VPCI MSI support after
> SMMU patches are merged.
> 
> Although the DomU vits itself is only two patches.

I am assuming this is work in progress patches rather than the one you 
plan to send, correct?

Asking, because currently there are a few 
ASSERT(is_hardware_domain(its->d)) to indicate where changes are likely 
for the guests. You seem to remove them without explaining why or any 
associated code.

While I will not ask to have a security support guest vITS from the 
start. I would like some confidence that we are going in the right 
direction. IOW, I would like to see a design document that would explain 
how we can achieve it. Some of the part to consider:
   * Command queue
   * LPIs
   * 1:1 pITS <-> vITS vs one vITS to many pITS
   * The page-tables are shared between the device and CPU. Are we ok to 
keep the doorbell writable by the CPU?

There was some discussion in the past about it (I have added Rahul 
because IIRC he was driving the discussion). So most likely, we would 
need the design to be respinned and agreed first.

Lastly, I see you seem to go down the route of exposing one vITS only. 
But I don't think your patch is sufficient to support multiple pITS (the 
guest doorbell will be mapped to a different host doorbell depending on 
the guest).

Cheers,

-- 
Julien Grall

Re: [PATCH v2 0/2] Add support for MSI injection on Arm

[Stefano Stabellini]

On Wed, 16 Apr 2025, Julien Grall wrote:
> (+ Rahul)
> 
> On 16/04/2025 17:37, Mykyta Poturai wrote:
> > On 16.04.25 01:40, Stefano Stabellini wrote:
> > > On Mon, 14 Apr 2025, Julien Grall wrote:
> > > > Hi Mykyta,
> > > > 
> > > > On 14/04/2025 18:51, Mykyta Poturai wrote:
> > > > > This series adds the base support for MSI injection on Arm. This is
> > > > > needed to streamline virtio-pci interrupt triggering.
> > > > > 
> > > > > With this patches, MSIs can be triggered in guests by issuing the new
> > > > > DM op, inject_msi2. This op is similar to inject_msi, but it allows
> > > > > to specify the source id of the MSI.
> > > > > 
> > > > > We chose the approach of adding a new DM op instead of using the pad
> > > > > field of inject_msi because we have no clear way of distinguishing
> > > > > between set and unset pad fields. New implementations also adds flags
> > > > > field to clearly specify if the SBDF is set.
> > > > > 
> > > > > Patches were tested on QEMU with
> > > > 
> > > > [...]
> > > > 
> > > > > patches for ITS support for DomUs applied.
> > > > 
> > > > This means this series is unusable without external patches. Given this
> > > > is
> > > > adding a new DM operations, I think it would be more sensible to have
> > > > the vITS
> > > > support merged first. Then we can look at merging this series.
> > > 
> > > Hi Mykyta, just checking explicitly with you whether this series
> > > requires vgic-v3-its.c for DomUs?
> > > 
> > > If yes, how far are you from sending the relevant patches to xen-devel?
> > > How many are they?
> > 
> > Hi Stefano,
> > Yes, I am planning to send them together with VPCI MSI support after
> > SMMU patches are merged.
> > 
> > Although the DomU vits itself is only two patches.
> 
> I am assuming this is work in progress patches rather than the one you plan to
> send, correct?
> 
> Asking, because currently there are a few ASSERT(is_hardware_domain(its->d))
> to indicate where changes are likely for the guests. You seem to remove them
> without explaining why or any associated code.
> 
> While I will not ask to have a security support guest vITS from the start. I
> would like some confidence that we are going in the right direction. IOW, I
> would like to see a design document that would explain how we can achieve it.
> Some of the part to consider:
>   * Command queue
>   * LPIs
>   * 1:1 pITS <-> vITS vs one vITS to many pITS
>   * The page-tables are shared between the device and CPU. Are we ok to keep
> the doorbell writable by the CPU?
> 
> There was some discussion in the past about it (I have added Rahul because
> IIRC he was driving the discussion). So most likely, we would need the design
> to be respinned and agreed first.
> 
> Lastly, I see you seem to go down the route of exposing one vITS only. But I
> don't think your patch is sufficient to support multiple pITS (the guest
> doorbell will be mapped to a different host doorbell depending on the guest).

To add to what Julien wrote, one thing is enabling a component with some
limitations, such as lacking a given feature. That might be OK.

A different thing is lack of isolation between VMs, or introducing a new
vehicle for interference, e.g. one VM using vITS causing troubles to
another VM using vITS.

Lack of isolation and interference are not OK. We need to be very
careful and be very aware if there any constraints or limitations we are
hitting against (e.g. size of the command queue) where one VM could
cause a denial of service to another VM because it is exhausting a given
physical resource.