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
