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

[Jan Beulich <jbeulich@xxxxxxxx>]

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