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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.01.2024 11:01, Mykyta Poturai wrote:
> 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.

At least on x86 I think 0 could serve as a "not valid" indicator, as that's
always a host bridge, which would never be handed through to guests. Can't
speak for Arm, as I don't know whether 00:00.0 always being a host bridge
(or unpopulated) is a universal requirement of the spec. Therefore this may
be insufficient justification for adding a new sub-op.

> --- 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,20 @@ 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) )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }

While I can see the reason for this check here, ...

> @@ -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");

... I don't understand what's going on here: If the flag isn't set, it's
obvious that the field is to be ignored. Is the conditional inverted?

Also in both cases you will need to check that all other flags fields
are clear, or else we won't be able to give them meaning down the road.

Finally such a message, if really warranted, wants to use gprintk().

> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -444,6 +444,17 @@ 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
> +#define XEN_DMOP_MSI_SOURCE_ID_VALID (1u << 0)
> +
> +struct xen_dm_op_inject_msi2 {
> +    uint64_aligned_t addr;
> +    uint32_t data;
> +    uint32_t source_id; /* PCI SBDF */

Since the comment says SBDF (not BDF), how are multiple segments handled
here? At least on x86 (VT-d and V-i) source IDs are only 16 bits iirc,
and are local to the respective segment. It would feel wrong to use a
32-bit quantity there; IOW I'd rather see this as being two 16-bit fields
(segment and source_id).

> +    uint32_t flags;
> +};

Just like in struct xen_dm_op_inject_msi padding wants making explicit,
and then wants checking for being zero.

Jan