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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Mykyta Poturai <Mykyta_Poturai@epam.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: "Wei Liu" <wl@xen.org>,
	"Anthony PERARD" <anthony.perard@citrix.com>,
	"Juergen Gross" <jgross@suse.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Jan Beulich" <jbeulich@suse.com>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Bertrand Marquis" <bertrand.marquis@arm.com>,
	"Michal Orzel" <michal.orzel@amd.com>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH 2/2] xen/dm: arm: Introduce inject_msi2 DM op
Date: Wed, 24 Jan 2024 20:25:56 +0000	[thread overview]
Message-ID: <a289ae5e-0831-4f7b-8f04-df71af23ae0b@citrix.com> (raw)
In-Reply-To: <6c551b03796fbf091b22fcde96d894cd5308ff91.1705066642.git.mykyta_poturai@epam.com>

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