[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 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. I'm further puzzled by "(lower 32 bits)" - isn't the data written to trigger an MSI always a 32-bit quantity? Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |