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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 19.03.25 14:37, Jan Beulich wrote:

Hello Jan, all.


> 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.

If I got correctly what is wrote in current thread (and in RFC version 
where it was an attempt to create just Arm64's counterpart of 
XEN_DMOP_inject_msi), my understanding (maybe not precise/correct, since 
I am not quite familiar with x86 details) that we would need something 
like that:

/*
 * XEN_DMOP_inject_msi2: An enhanced version of the sub-ob to inject an MSI
 *                       for an emulated device, which allows specifying
 *                       the ID of the device triggering the MSI 
(source ID).
 *
 * The source ID is specified by a pair of <segment> and <source_id>.
 * If <flags> does not contain XEN_DMOP_MSI_SOURCE_ID_VALID then source ID
 * is invalid and should be ignored.
 */
#define XEN_DMOP_inject_msi 21

struct xen_dm_op_inject_msi2 {
     /* IN - MSI data */
     uint32_t data;
     /* IN - next two fields form an ID of the device triggering the MSI */
     uint16_t segment; /* The segment number */
     uint16_t source_id; /* The source ID that is local to segment (PCI 
BDF) */
     /* IN - types of source ID */
     uint32_t flags;
#define XEN_DMOP_MSI_SOURCE_ID_VALID (1u << 0)
     uint32_t pad;
     /* IN - MSI address */
     uint64_aligned_t addr;
};


This is arch agnostic sub-op without the (obvious) specifics of the 
underlying MSI controller. The sub-ob, I hope, will be suitable on both 
Arm64 soon (segment + source_id provide vSBDF, then it will possible to 
locate vITS and devid) and on x86 in future (for the vIOMMU needs).
Would you be ok with that in general? Please share you opinion.


> Jan