[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/2] xen/dm: arm: Introduce inject_msi2 DM op
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |