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

[Mykyta Poturai <Mykyta_Poturai@xxxxxxxx>]

On 18.03.25 16:26, Jan Beulich wrote:
> On 18.03.2025 14:31, Mykyta Poturai wrote:
>> 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.
>
> I fear the comment is far from making clear what layout source_id is to
> have, hence also leaving open whether a segment number is included there.
> Of course the issue here may be that I have no clue what "ITS devid"
> means or implies. What is clear is that "ITS devid" is meaningless on
> x86, for example.

ITS devid is implementation defined. Its size is also implementation
defined but up to 32 bits.

Quotes from Arm Base System Architecture[1]:
 > The system designer assigns a requester a unique StreamID to device
traffic input to the SMMU.
 > If a requester is a bridge from a different interconnect with an
originator ID, like a PCIe RequesterID, and devices on that interconnect
might need to send MSIs, the originator ID is used to generate a
DeviceID. The function to generate the DeviceID should be an identity or
a simple offset.

On the Xen's side it is used as an offset into the ITS translation
tables and is sourced from msi-map nodes in the device tree.

Practically for PCI the requester ID is usually the SBDF. Where the
segment is used to find the host bridge node that contains the msi-map
node with defined offsets but it is also included as part of an id.
That's why it was originally called SBDF in the comment. I don't know if
there is a better way to describe what it is concisely in the comments.

> I'm further puzzled by "(lower 32 bits)" - isn't the data written to
> trigger an MSI always a 32-bit quantity?
>
> Jan

Hmm, it actually is, I copied this line from xen_dm_op_inject_msi, not
sure why it is specified like that there. But I'll remove this part.

[1]: https://developer.arm.com/documentation/den0094/latest/

--
Mykyta