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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 14/04/2025 18:51, Mykyta Poturai wrote:
> From: Mykyta Poturai <Mykyta_Poturai@xxxxxxxx>
>
> 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@xxxxxxxx>
> ---
> 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