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

[Mykyta Poturai <Mykyta_Poturai@xxxxxxxx>]

On 18.03.25 12:11, Jan Beulich wrote:
> On 18.03.2025 10:10, Mykyta Poturai wrote:
>> On 15.01.24 11:35, Jan Beulich wrote:
>>> On 14.01.2024 11:01, Mykyta Poturai wrote:
>>>> --- 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).
>>
>> I'm planning on resuming this series in the near future and want to
>> clarify the DM op interface.
>>
>> Wouldn't it be better to keep things consistent and use
>> XEN_DMOP_PCI_SBDF as it's done in xen_dm_op_ioreq_server_range? Also
>> with this, the value can be easily casted to pci_sbdf_t later and split
>> to segment and BDF if needed.
>
> The essence of my earlier comment is: Naming, contents, and comments need
> to be in sync.
>
> I question though that "casting to pci_sbdf_t" is technically possible.
> Nor am I convinced that it would be desirable to do so if it was possible
> (or if "casting" was intended to mean something else than what this is in
> C). See my recent comments on some of Andrii's patches [1][2].
>
> Jan
>
> [1] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01294.html
> [2] https://lists.xen.org/archives/html/xen-devel/2025-03/msg01301.html

Would something like this be okay then?

struct xen_dm_op_inject_msi2 {
     /* IN - MSI data (lower 32 bits) */
     uint32_t data;
     /* IN - ITS devid of the device triggering the interrupt */
     uint32_t source_id;
     uint32_t flags;
     uint32_t pad;
     /* IN - MSI address */
     uint64_aligned_t addr;
};

Added padding and explained source_id purpose better.
--
Mykyta