This patch adds the ability for the device emulator to inject MSI
interrupts into guests. This is done by adding a new DM op to the device
model library.

It is not possible to reuse already existing inject_msi DM op, because
it does not have a devid parameter, which is required for translation of
MSIs to interrupt numbers on ARM.

This approach was successfully tested on a virtio-pci setup with QEMU
backend emulating block devices with following changes from the mainline
Xen:

This branch was taken as a base for virtio-pci:
https://github.com/xen-troops/xen/tree/xen-4.18-xt0.2

With added new VGICv3 from here:
https://github.com/Deedone/xen/tree/new_vgic_v2
(although it should also work with the current VGIC)

And patches from this branch to allow for proper ITS emulation in guests:
https://github.com/stewdk/xen/commits/pcie-passthrough-arm-vpci-v11

The main purpose of this RFC is to get some feedback on the addition of
the new DM op. Is it the right approach or should we somehow modify the
existing inject_msi DM op to be compatible with ARM?

Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
 tools/include/xendevicemodel.h               |  4 ++++
 tools/libs/devicemodel/core.c                | 19 +++++++++++++++++++
 tools/libs/devicemodel/libxendevicemodel.map |  5 +++++
 xen/arch/arm/dm.c                            | 15 +++++++++++++++
 xen/arch/arm/include/asm/new_vgic.h          |  2 ++
 xen/arch/arm/vgic/vgic-its.c                 |  2 +-
 xen/include/public/hvm/dm_op.h               | 10 ++++++++++
 7 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h
index 797e0c6b29..e28710a6a5 100644
--- a/tools/include/xendevicemodel.h
+++ b/tools/include/xendevicemodel.h
@@ -236,6 +236,10 @@ int xendevicemodel_inject_msi(
     xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr,
     uint32_t msi_data);
 
+int xendevicemodel_arm_inject_msi(
+    xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t devid,
+    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..d15ffa46fb 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -448,6 +448,25 @@ int xendevicemodel_set_irq_level(
     return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
 }
 
+int xendevicemodel_arm_inject_msi(
+    xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t msi_data,
+    uint32_t devid)
+{
+    struct xen_dm_op op;
+    struct xen_dm_op_arm_inject_msi *data;
+
+    memset(&op, 0, sizeof(op));
+
+    op.op = XEN_DMOP_arm_inject_msi;
+    data = &op.u.arm_inject_msi;
+
+    data->addr = msi_addr;
+    data->devid = devid;
+    data->data = msi_data;
+
+    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..8dceba5056 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_arm_inject_msi;
+} VERS_1.4;
diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c
index 5569efa121..b42f11e569 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_arm_inject_msi]                   = sizeof(struct xen_dm_op_arm_inject_msi),
         [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_arm_inject_msi:
+    {
+        const struct xen_dm_op_arm_inject_msi *data =
+            &op.u.arm_inject_msi;
+
+        if ( d->arch.vgic.its == NULL )
+        {
+            rc = -EINVAL;
+            break;
+        }
+        vgic_its_trigger_msi(d, d->arch.vgic.its, data->devid, data->data);
+        break;
+
+    }
     case XEN_DMOP_nr_vcpus:
     {
         struct xen_dm_op_nr_vcpus *data = &op.u.nr_vcpus;
diff --git a/xen/arch/arm/include/asm/new_vgic.h b/xen/arch/arm/include/asm/new_vgic.h
index dfc434ab41..dedc294ce9 100644
--- a/xen/arch/arm/include/asm/new_vgic.h
+++ b/xen/arch/arm/include/asm/new_vgic.h
@@ -275,6 +275,8 @@ int vgic_its_add_device(struct domain *d, struct vgic_its_device *its_dev);
 void vgic_its_delete_device(struct domain *d, struct vgic_its_device *its_dev);
 struct vgic_its_device* vgic_its_get_device(struct domain *d, paddr_t vdoorbell,
                                          uint32_t vdevid);
+int vgic_its_trigger_msi(struct domain *d, struct vgic_its *its,
+                                u32 devid, u32 eventid);
 #else
 static inline void vgic_enable_lpis(struct vcpu *vcpu)
 {
diff --git a/xen/arch/arm/vgic/vgic-its.c b/xen/arch/arm/vgic/vgic-its.c
index fd5aaf4a70..706987d93a 100644
--- a/xen/arch/arm/vgic/vgic-its.c
+++ b/xen/arch/arm/vgic/vgic-its.c
@@ -608,7 +608,7 @@ int vgic_its_inject_cached_translation(struct domain *d, struct vgic_its *its, u
  * Returns 0 on success, a positive error value for any ITS mapping
  * related errors and negative error values for generic errors.
  */
-static int vgic_its_trigger_msi(struct domain *d, struct vgic_its *its,
+int vgic_its_trigger_msi(struct domain *d, struct vgic_its *its,
                                 u32 devid, u32 eventid)
 {
     struct vgic_irq *irq = NULL;
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index fa98551914..a7d72e4389 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -444,6 +444,15 @@ struct xen_dm_op_nr_vcpus {
 };
 typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t;
 
+#define XEN_DMOP_arm_inject_msi 21
+
+struct xen_dm_op_arm_inject_msi {
+    uint64_t addr;
+    uint32_t data;
+    uint32_t devid;
+};
+typedef struct xen_dm_op_arm_inject_msi xen_dm_op_arm_inject_msi_t;
+
 struct xen_dm_op {
     uint32_t op;
     uint32_t pad;
@@ -463,6 +472,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_arm_inject_msi_t arm_inject_msi;
         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 19/12/2023 1:48 pm, Mykyta Poturai wrote:
> This patch adds the ability for the device emulator to inject MSI
> interrupts into guests. This is done by adding a new DM op to the device
> model library.
>
> It is not possible to reuse already existing inject_msi DM op, because
> it does not have a devid parameter, which is required for translation of
> MSIs to interrupt numbers on ARM.

Ok, so the original hypercall is broken.

But the new hypercall isn't ARM specific. It's just better form of
inject_msi, and needed for all architectures.

So, name it DMOP_inject_msi2 and get rid of the ARM infix.

> This approach was successfully tested on a virtio-pci setup with QEMU
> backend emulating block devices with following changes from the mainline
> Xen:
>
> This branch was taken as a base for virtio-pci:
> https://github.com/xen-troops/xen/tree/xen-4.18-xt0.2
>
> With added new VGICv3 from here:
> https://github.com/Deedone/xen/tree/new_vgic_v2
> (although it should also work with the current VGIC)
>
> And patches from this branch to allow for proper ITS emulation in guests:
> https://github.com/stewdk/xen/commits/pcie-passthrough-arm-vpci-v11
>
> The main purpose of this RFC is to get some feedback on the addition of
> the new DM op. Is it the right approach or should we somehow modify the
> existing inject_msi DM op to be compatible with ARM?

The DM_OP ABI does allow you to extend the structure behind
DMOP_inject_msi, as long as 0 is meaningful.

However, the semantics of zero-extending are wrong in this case, because
it would mean that users of DMOP_inject_msi on an updated Xen would be
sending interrupts with an implicit source id of host bridge.

So you need a new DMOP_inject_msi2 that has better semantics.

> diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
> index fa98551914..a7d72e4389 100644
> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -444,6 +444,15 @@ struct xen_dm_op_nr_vcpus {
>  };
>  typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t;
>  
> +#define XEN_DMOP_arm_inject_msi 21
> +
> +struct xen_dm_op_arm_inject_msi {
> +    uint64_t addr;
> +    uint32_t data;
> +    uint32_t devid;

uint32_t source_id; /* PCI SBDF */

Technically the PCI spec calls it the Requester ID, but Source ID is the
more common name.

It is important not to use "devid" because that implies it's only a 3
bit number, and it isn't.

~Andrew

On 19.12.2023 15:19, Andrew Cooper wrote:
> On 19/12/2023 1:48 pm, Mykyta Poturai wrote:
>> This patch adds the ability for the device emulator to inject MSI
>> interrupts into guests. This is done by adding a new DM op to the device
>> model library.
>>
>> It is not possible to reuse already existing inject_msi DM op, because
>> it does not have a devid parameter, which is required for translation of
>> MSIs to interrupt numbers on ARM.
> 
> Ok, so the original hypercall is broken.
> 
> But the new hypercall isn't ARM specific. It's just better form of
> inject_msi, and needed for all architectures.
> 
> So, name it DMOP_inject_msi2 and get rid of the ARM infix.
> 
>> This approach was successfully tested on a virtio-pci setup with QEMU
>> backend emulating block devices with following changes from the mainline
>> Xen:
>>
>> This branch was taken as a base for virtio-pci:
>> https://github.com/xen-troops/xen/tree/xen-4.18-xt0.2
>>
>> With added new VGICv3 from here:
>> https://github.com/Deedone/xen/tree/new_vgic_v2
>> (although it should also work with the current VGIC)
>>
>> And patches from this branch to allow for proper ITS emulation in guests:
>> https://github.com/stewdk/xen/commits/pcie-passthrough-arm-vpci-v11
>>
>> The main purpose of this RFC is to get some feedback on the addition of
>> the new DM op. Is it the right approach or should we somehow modify the
>> existing inject_msi DM op to be compatible with ARM?
> 
> The DM_OP ABI does allow you to extend the structure behind
> DMOP_inject_msi, as long as 0 is meaningful.
> 
> However, the semantics of zero-extending are wrong in this case, because
> it would mean that users of DMOP_inject_msi on an updated Xen would be
> sending interrupts with an implicit source id of host bridge.
> 
> So you need a new DMOP_inject_msi2 that has better semantics.

As said in another reply, the existing structure has a 32-bit padding
field, which could be used here. In the handler it's properly being
checked to be zero right now; whether that would want to remain this
way, or whether we'd expect source ID to also be passed on x86 I don't
know (yet).

Jan

On 19/12/2023 2:25 pm, Jan Beulich wrote:
> On 19.12.2023 15:19, Andrew Cooper wrote:
>> On 19/12/2023 1:48 pm, Mykyta Poturai wrote:
>>> This patch adds the ability for the device emulator to inject MSI
>>> interrupts into guests. This is done by adding a new DM op to the device
>>> model library.
>>>
>>> It is not possible to reuse already existing inject_msi DM op, because
>>> it does not have a devid parameter, which is required for translation of
>>> MSIs to interrupt numbers on ARM.
>> Ok, so the original hypercall is broken.
>>
>> But the new hypercall isn't ARM specific. It's just better form of
>> inject_msi, and needed for all architectures.
>>
>> So, name it DMOP_inject_msi2 and get rid of the ARM infix.
>>
>>> This approach was successfully tested on a virtio-pci setup with QEMU
>>> backend emulating block devices with following changes from the mainline
>>> Xen:
>>>
>>> This branch was taken as a base for virtio-pci:
>>> https://github.com/xen-troops/xen/tree/xen-4.18-xt0.2
>>>
>>> With added new VGICv3 from here:
>>> https://github.com/Deedone/xen/tree/new_vgic_v2
>>> (although it should also work with the current VGIC)
>>>
>>> And patches from this branch to allow for proper ITS emulation in guests:
>>> https://github.com/stewdk/xen/commits/pcie-passthrough-arm-vpci-v11
>>>
>>> The main purpose of this RFC is to get some feedback on the addition of
>>> the new DM op. Is it the right approach or should we somehow modify the
>>> existing inject_msi DM op to be compatible with ARM?
>> The DM_OP ABI does allow you to extend the structure behind
>> DMOP_inject_msi, as long as 0 is meaningful.
>>
>> However, the semantics of zero-extending are wrong in this case, because
>> it would mean that users of DMOP_inject_msi on an updated Xen would be
>> sending interrupts with an implicit source id of host bridge.
>>
>> So you need a new DMOP_inject_msi2 that has better semantics.
> As said in another reply, the existing structure has a 32-bit padding
> field, which could be used here. In the handler it's properly being
> checked to be zero right now;

It's still not safe to reuse this zero for a source ID semantic behind
the back of older userspace.

>  whether that would want to remain this
> way, or whether we'd expect source ID to also be passed on x86 I don't
> know (yet).

We do need the source ID in x86, as soon as the guest has vIOMMU for any
reason.

It's a design error that it wasn't added originally, but I suppose you
can say the same of x86 platforms in general, having to retrofit an
OS-visible Source ID to HPETs/IO-APICs to make them compatible with IOMMUs.

~Andrew

On 19.12.2023 15:33, Andrew Cooper wrote:
> On 19/12/2023 2:25 pm, Jan Beulich wrote:
>> On 19.12.2023 15:19, Andrew Cooper wrote:
>>> On 19/12/2023 1:48 pm, Mykyta Poturai wrote:
>>>> This patch adds the ability for the device emulator to inject MSI
>>>> interrupts into guests. This is done by adding a new DM op to the device
>>>> model library.
>>>>
>>>> It is not possible to reuse already existing inject_msi DM op, because
>>>> it does not have a devid parameter, which is required for translation of
>>>> MSIs to interrupt numbers on ARM.
>>> Ok, so the original hypercall is broken.
>>>
>>> But the new hypercall isn't ARM specific. It's just better form of
>>> inject_msi, and needed for all architectures.
>>>
>>> So, name it DMOP_inject_msi2 and get rid of the ARM infix.
>>>
>>>> This approach was successfully tested on a virtio-pci setup with QEMU
>>>> backend emulating block devices with following changes from the mainline
>>>> Xen:
>>>>
>>>> This branch was taken as a base for virtio-pci:
>>>> https://github.com/xen-troops/xen/tree/xen-4.18-xt0.2
>>>>
>>>> With added new VGICv3 from here:
>>>> https://github.com/Deedone/xen/tree/new_vgic_v2
>>>> (although it should also work with the current VGIC)
>>>>
>>>> And patches from this branch to allow for proper ITS emulation in guests:
>>>> https://github.com/stewdk/xen/commits/pcie-passthrough-arm-vpci-v11
>>>>
>>>> The main purpose of this RFC is to get some feedback on the addition of
>>>> the new DM op. Is it the right approach or should we somehow modify the
>>>> existing inject_msi DM op to be compatible with ARM?
>>> The DM_OP ABI does allow you to extend the structure behind
>>> DMOP_inject_msi, as long as 0 is meaningful.
>>>
>>> However, the semantics of zero-extending are wrong in this case, because
>>> it would mean that users of DMOP_inject_msi on an updated Xen would be
>>> sending interrupts with an implicit source id of host bridge.
>>>
>>> So you need a new DMOP_inject_msi2 that has better semantics.
>> As said in another reply, the existing structure has a 32-bit padding
>> field, which could be used here. In the handler it's properly being
>> checked to be zero right now;
> 
> It's still not safe to reuse this zero for a source ID semantic behind
> the back of older userspace.

As long as we simply ignore that field's value, I don't see anything
wrong there (not very different from Arm ignoring the address, as the
intent looks to be). And ...

>>  whether that would want to remain this
>> way, or whether we'd expect source ID to also be passed on x86 I don't
>> know (yet).
> 
> We do need the source ID in x86, as soon as the guest has vIOMMU for any
> reason.

... I wonder whether I'll still be around when Xen actually gets there.

Jan

> It's a design error that it wasn't added originally, but I suppose you
> can say the same of x86 platforms in general, having to retrofit an
> OS-visible Source ID to HPETs/IO-APICs to make them compatible with IOMMUs.
> 
> ~Andrew
>

Following up with relevant QEMU patch link.

https://patchwork.kernel.org/project/qemu-devel/patch/c7a180a5874f036c246fc39f921eefafecbc8c76.1702994649.git.mykyta_poturai@epam.com/

Hi,

On 19/12/2023 14:18, Mykyta Poturai wrote:
> Following up with relevant QEMU patch link.
> 
> https://patchwork.kernel.org/project/qemu-devel/patch/c7a180a5874f036c246fc39f921eefafecbc8c76.1702994649.git.mykyta_poturai@epam.com/

I don't seem to have the patch in my inbox. I guess you didn't CC xen-devel?

Anyway, I will reply here. I think this is a mistake for QEMU to assume 
that Xen will expose a GICv3 ITS to the guest (we may decide to 
implement another MSI controller).

But QEMU should really not need to implement a full ITS. What it needs 
is a way to forward the MSI to Xen. That's it.

Stefano, do you have any suggestion how to do this in QEMU?

Cheers,

-- 
Julien Grall

On Tue, 19 Dec 2023, Julien Grall wrote:
> Hi,
> 
> On 19/12/2023 14:18, Mykyta Poturai wrote:
> > Following up with relevant QEMU patch link.
> > 
> > https://patchwork.kernel.org/project/qemu-devel/patch/c7a180a5874f036c246fc39f921eefafecbc8c76.1702994649.git.mykyta_poturai@epam.com/
> 
> I don't seem to have the patch in my inbox. I guess you didn't CC xen-devel?
> 
> Anyway, I will reply here. I think this is a mistake for QEMU to assume that
> Xen will expose a GICv3 ITS to the guest (we may decide to implement another
> MSI controller).
> 
> But QEMU should really not need to implement a full ITS. What it needs is a
> way to forward the MSI to Xen. That's it.

I fully agree with Julien


> Stefano, do you have any suggestion how to do this in QEMU?

Yes, we just need something like hw/i386/xen/xen_apic.c but for ARM

Hi,

On 20.12.23 02:42, Stefano Stabellini wrote:
> On Tue, 19 Dec 2023, Julien Grall wrote:
>>
>> But QEMU should really not need to implement a full ITS. What it needs is a
>> way to forward the MSI to Xen. That's it.
> 
> I fully agree with Julien
> 
> 
>> Stefano, do you have any suggestion how to do this in QEMU?
> 
> Yes, we just need something like hw/i386/xen/xen_apic.c but for ARM

It is exactly like xen_apic.c. All this implementation does is getting 
the MSI messages and forwarding them to Xen using the DM op from this patch.

Hi,

On 20/12/2023 09:13, Mykyta Poturai wrote:
> 
> Hi,
> 
> On 20.12.23 02:42, Stefano Stabellini wrote:
>> On Tue, 19 Dec 2023, Julien Grall wrote:
>>>
>>> But QEMU should really not need to implement a full ITS. What it needs is a
>>> way to forward the MSI to Xen. That's it.
>>
>> I fully agree with Julien
>>
>>
>>> Stefano, do you have any suggestion how to do this in QEMU?
>>
>> Yes, we just need something like hw/i386/xen/xen_apic.c but for ARM
> 
> It is exactly like xen_apic.c. All this implementation does is getting
> the MSI messages and forwarding them to Xen using the DM op from this patch.

If this implementation is only getting the MSI messages and forwarding 
them, then why is it built on top of the vGICv3 ITS structure?

Shouldn't instead be something that is agnostic to the interrupt 
controller (and even possibly arch agnostic)?

The point here is in Xen context, QEMU doesn't know which interrupt 
controller will be exposed to the guest. QEMU is just used for emulating 
devices and none of this should really be architecture specific. 
Instead, it should be a Xen specific way to inject MSI/IRQ.

Cheers,

-- 
Julien Grall

Hi,

On 19/12/2023 13:48, Mykyta Poturai wrote:
> This patch adds the ability for the device emulator to inject MSI
> interrupts into guests. This is done by adding a new DM op to the device
> model library.
> 
> It is not possible to reuse already existing inject_msi DM op, because
> it does not have a devid parameter, which is required for translation of
> MSIs to interrupt numbers on ARM.

 From the code below, it is not 100% clear what is the devid. It seems 
to be the device ID from the ITS PoV. However, the ID space is per ITS 
and I don't think it would be a good idea to design the DM OP interface 
with just one ITS in mind.

It is also not clear how QEMU would be able to know the device ID. So I 
think the operation should take an SBDF.

> 
> This approach was successfully tested on a virtio-pci setup with QEMU
> backend emulating block devices with following changes from the mainline
> Xen:
> 
> This branch was taken as a base for virtio-pci:
> https://github.com/xen-troops/xen/tree/xen-4.18-xt0.2
> 
> With added new VGICv3 from here:
> https://github.com/Deedone/xen/tree/new_vgic_v2
> (although it should also work with the current VGIC)
> 
> And patches from this branch to allow for proper ITS emulation in guests:
> https://github.com/stewdk/xen/commits/pcie-passthrough-arm-vpci-v11
> 
> The main purpose of this RFC is to get some feedback on the addition of
> the new DM op. Is it the right approach or should we somehow modify the
> existing inject_msi DM op to be compatible with ARM?

In general the DM op interface is stable. So modification needs to be 
done with care.

> 
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> ---
>   tools/include/xendevicemodel.h               |  4 ++++
>   tools/libs/devicemodel/core.c                | 19 +++++++++++++++++++
>   tools/libs/devicemodel/libxendevicemodel.map |  5 +++++
>   xen/arch/arm/dm.c                            | 15 +++++++++++++++
>   xen/arch/arm/include/asm/new_vgic.h          |  2 ++
>   xen/arch/arm/vgic/vgic-its.c                 |  2 +-
>   xen/include/public/hvm/dm_op.h               | 10 ++++++++++
>   7 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h
> index 797e0c6b29..e28710a6a5 100644
> --- a/tools/include/xendevicemodel.h
> +++ b/tools/include/xendevicemodel.h
> @@ -236,6 +236,10 @@ int xendevicemodel_inject_msi(
>       xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr,
>       uint32_t msi_data);
>   
> +int xendevicemodel_arm_inject_msi(
> +    xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t devid,

In your new proposal, msi_addr is not used. As Jan just suggested, can 
we use it? If not, can you explain why?

> +    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..d15ffa46fb 100644
> --- a/tools/libs/devicemodel/core.c
> +++ b/tools/libs/devicemodel/core.c
> @@ -448,6 +448,25 @@ int xendevicemodel_set_irq_level(
>       return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
>   }
>   
> +int xendevicemodel_arm_inject_msi(
> +    xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t msi_data,
> +    uint32_t devid)
> +{
> +    struct xen_dm_op op;
> +    struct xen_dm_op_arm_inject_msi *data;
> +
> +    memset(&op, 0, sizeof(op));
> +
> +    op.op = XEN_DMOP_arm_inject_msi;
> +    data = &op.u.arm_inject_msi;
> +
> +    data->addr = msi_addr;
> +    data->devid = devid;
> +    data->data = msi_data;
> +
> +    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..8dceba5056 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_arm_inject_msi;
> +} VERS_1.4;
> diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c
> index 5569efa121..b42f11e569 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_arm_inject_msi]                   = sizeof(struct xen_dm_op_arm_inject_msi),
>           [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_arm_inject_msi:
> +    {
> +        const struct xen_dm_op_arm_inject_msi *data =
> +            &op.u.arm_inject_msi;
> +
> +        if ( d->arch.vgic.its == NULL )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +        vgic_its_trigger_msi(d, d->arch.vgic.its, data->devid, data->data);

vgic_its_trigger_msi() returns a value. So surely we want to propagate 
(possibly after translating the value).

> +        break;
> +
> +    }
>       case XEN_DMOP_nr_vcpus:
>       {
>           struct xen_dm_op_nr_vcpus *data = &op.u.nr_vcpus;
> diff --git a/xen/arch/arm/include/asm/new_vgic.h b/xen/arch/arm/include/asm/new_vgic.h
> index dfc434ab41..dedc294ce9 100644
> --- a/xen/arch/arm/include/asm/new_vgic.h
> +++ b/xen/arch/arm/include/asm/new_vgic.h
> @@ -275,6 +275,8 @@ int vgic_its_add_device(struct domain *d, struct vgic_its_device *its_dev);
>   void vgic_its_delete_device(struct domain *d, struct vgic_its_device *its_dev);
>   struct vgic_its_device* vgic_its_get_device(struct domain *d, paddr_t vdoorbell,
>                                            uint32_t vdevid);
> +int vgic_its_trigger_msi(struct domain *d, struct vgic_its *its,
> +                                u32 devid, u32 eventid);
>   #else
>   static inline void vgic_enable_lpis(struct vcpu *vcpu)
>   {
> diff --git a/xen/arch/arm/vgic/vgic-its.c b/xen/arch/arm/vgic/vgic-its.c
> index fd5aaf4a70..706987d93a 100644
> --- a/xen/arch/arm/vgic/vgic-its.c
> +++ b/xen/arch/arm/vgic/vgic-its.c
> @@ -608,7 +608,7 @@ int vgic_its_inject_cached_translation(struct domain *d, struct vgic_its *its, u
>    * Returns 0 on success, a positive error value for any ITS mapping
>    * related errors and negative error values for generic errors.
>    */
> -static int vgic_its_trigger_msi(struct domain *d, struct vgic_its *its,
> +int vgic_its_trigger_msi(struct domain *d, struct vgic_its *its,
>                                   u32 devid, u32 eventid)
>   {
>       struct vgic_irq *irq = NULL;
> diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
> index fa98551914..a7d72e4389 100644
> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -444,6 +444,15 @@ struct xen_dm_op_nr_vcpus {
>   };
>   typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t;
>   
> +#define XEN_DMOP_arm_inject_msi 21
> +
> +struct xen_dm_op_arm_inject_msi {
> +    uint64_t addr;
> +    uint32_t data;
> +    uint32_t devid;
> +};
> +typedef struct xen_dm_op_arm_inject_msi xen_dm_op_arm_inject_msi_t;
> +
>   struct xen_dm_op {
>       uint32_t op;
>       uint32_t pad;
> @@ -463,6 +472,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_arm_inject_msi_t arm_inject_msi;
>           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;

Cheers,

-- 
Julien Grall

On 19.12.2023 14:48, Mykyta Poturai wrote:
> This patch adds the ability for the device emulator to inject MSI
> interrupts into guests. This is done by adding a new DM op to the device
> model library.
> 
> It is not possible to reuse already existing inject_msi DM op, because
> it does not have a devid parameter, which is required for translation of
> MSIs to interrupt numbers on ARM.

Yet then ...

> @@ -112,6 +113,20 @@ int dm_op(const struct dmop_args *op_args)
>          break;
>      }
>  
> +    case XEN_DMOP_arm_inject_msi:
> +    {
> +        const struct xen_dm_op_arm_inject_msi *data =
> +            &op.u.arm_inject_msi;
> +
> +        if ( d->arch.vgic.its == NULL )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +        vgic_its_trigger_msi(d, d->arch.vgic.its, data->devid, data->data);
> +        break;
> +
> +    }

... you're not using the addr field at all, which therefore could likely
hold the devid data (encoded to really represent some form of address,
or stored directly - much depends on what purpose the address serves on
Arm for MSI).

> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -444,6 +444,15 @@ struct xen_dm_op_nr_vcpus {
>  };
>  typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t;
>  
> +#define XEN_DMOP_arm_inject_msi 21
> +
> +struct xen_dm_op_arm_inject_msi {
> +    uint64_t addr;
> +    uint32_t data;
> +    uint32_t devid;
> +};

Additionally xen_dm_op_inject_msi has a padding field, which is likely
possible to use as well (perhaps by simply renaming it).

Also note how the addr field there is using uint64_aligned_t.

Jan

Hi Jan,

On 19/12/2023 13:57, Jan Beulich wrote:
> On 19.12.2023 14:48, Mykyta Poturai wrote:
>> This patch adds the ability for the device emulator to inject MSI
>> interrupts into guests. This is done by adding a new DM op to the device
>> model library.
>>
>> It is not possible to reuse already existing inject_msi DM op, because
>> it does not have a devid parameter, which is required for translation of
>> MSIs to interrupt numbers on ARM.
> 
> Yet then ...
> 
>> @@ -112,6 +113,20 @@ int dm_op(const struct dmop_args *op_args)
>>           break;
>>       }
>>   
>> +    case XEN_DMOP_arm_inject_msi:
>> +    {
>> +        const struct xen_dm_op_arm_inject_msi *data =
>> +            &op.u.arm_inject_msi;
>> +
>> +        if ( d->arch.vgic.its == NULL )
>> +        {
>> +            rc = -EINVAL;
>> +            break;
>> +        }
>> +        vgic_its_trigger_msi(d, d->arch.vgic.its, data->devid, data->data);
>> +        break;
>> +
>> +    }
> 
> ... you're not using the addr field at all, which therefore could likely
> hold the devid data (encoded to really represent some form of address,
> or stored directly - much depends on what purpose the address serves on
> Arm for MSI).

For real HW, the address would point to an ITS doorbell. All access will 
be tagged by the HW with the DeviceID. This is then used with the data 
(an event ID) to look up the associated interrupt.

I think for Xen on Arm, we want 'addr' to be the SBDF. This could then 
be used to find the associated vITS and device ID.

Ideally, this is an interface that will be agnostic to the MSI 
controller (someone need to check if the model I suggested above works 
with the GICv2m).

Cheers,

-- 
Julien Grall

On 19.12.2023 15:12, Julien Grall wrote:
> On 19/12/2023 13:57, Jan Beulich wrote:
>> On 19.12.2023 14:48, Mykyta Poturai wrote:
>>> This patch adds the ability for the device emulator to inject MSI
>>> interrupts into guests. This is done by adding a new DM op to the device
>>> model library.
>>>
>>> It is not possible to reuse already existing inject_msi DM op, because
>>> it does not have a devid parameter, which is required for translation of
>>> MSIs to interrupt numbers on ARM.
>>
>> Yet then ...
>>
>>> @@ -112,6 +113,20 @@ int dm_op(const struct dmop_args *op_args)
>>>           break;
>>>       }
>>>   
>>> +    case XEN_DMOP_arm_inject_msi:
>>> +    {
>>> +        const struct xen_dm_op_arm_inject_msi *data =
>>> +            &op.u.arm_inject_msi;
>>> +
>>> +        if ( d->arch.vgic.its == NULL )
>>> +        {
>>> +            rc = -EINVAL;
>>> +            break;
>>> +        }
>>> +        vgic_its_trigger_msi(d, d->arch.vgic.its, data->devid, data->data);
>>> +        break;
>>> +
>>> +    }
>>
>> ... you're not using the addr field at all, which therefore could likely
>> hold the devid data (encoded to really represent some form of address,
>> or stored directly - much depends on what purpose the address serves on
>> Arm for MSI).
> 
> For real HW, the address would point to an ITS doorbell. All access will 
> be tagged by the HW with the DeviceID. This is then used with the data 
> (an event ID) to look up the associated interrupt.

So no properties of the destination are encoded in the address (besides
it being an ITS specific address of course)?

> I think for Xen on Arm, we want 'addr' to be the SBDF. This could then 
> be used to find the associated vITS and device ID.

FTAOD, the vSBDF you mean then?

Jan

Hi Jan,

On 19/12/2023 14:17, Jan Beulich wrote:
> On 19.12.2023 15:12, Julien Grall wrote:
>> On 19/12/2023 13:57, Jan Beulich wrote:
>>> On 19.12.2023 14:48, Mykyta Poturai wrote:
>>>> This patch adds the ability for the device emulator to inject MSI
>>>> interrupts into guests. This is done by adding a new DM op to the device
>>>> model library.
>>>>
>>>> It is not possible to reuse already existing inject_msi DM op, because
>>>> it does not have a devid parameter, which is required for translation of
>>>> MSIs to interrupt numbers on ARM.
>>>
>>> Yet then ...
>>>
>>>> @@ -112,6 +113,20 @@ int dm_op(const struct dmop_args *op_args)
>>>>            break;
>>>>        }
>>>>    
>>>> +    case XEN_DMOP_arm_inject_msi:
>>>> +    {
>>>> +        const struct xen_dm_op_arm_inject_msi *data =
>>>> +            &op.u.arm_inject_msi;
>>>> +
>>>> +        if ( d->arch.vgic.its == NULL )
>>>> +        {
>>>> +            rc = -EINVAL;
>>>> +            break;
>>>> +        }
>>>> +        vgic_its_trigger_msi(d, d->arch.vgic.its, data->devid, data->data);
>>>> +        break;
>>>> +
>>>> +    }
>>>
>>> ... you're not using the addr field at all, which therefore could likely
>>> hold the devid data (encoded to really represent some form of address,
>>> or stored directly - much depends on what purpose the address serves on
>>> Arm for MSI).
>>
>> For real HW, the address would point to an ITS doorbell. All access will
>> be tagged by the HW with the DeviceID. This is then used with the data
>> (an event ID) to look up the associated interrupt.
> 
> So no properties of the destination are encoded in the address (besides
> it being an ITS specific address of course)?

 From my understanding, for the ITS yes. This could be different for 
other MSI controller.

> 
>> I think for Xen on Arm, we want 'addr' to be the SBDF. This could then
>> be used to find the associated vITS and device ID.
> 
> FTAOD, the vSBDF you mean then?

Indeed.

Cheers,

-- 
Julien Grall