[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Mykyta Poturai <Mykyta_Poturai@xxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 19 Mar 2025 13:37:39 +0100
  • Autocrypt: addr=jbeulich@xxxxxxxx; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL
  • Cc: Juergen Gross <jgross@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Michal Orzel <michal.orzel@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>
  • Delivery-date: Wed, 19 Mar 2025 12:37:54 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 19.03.2025 13:05, Mykyta Poturai wrote:
> 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.

If this is to be usable for other architectures too, it may need to be
a union to put there. With appropriate comments for each member.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.