[v1] tools/libxc: fix xc_physdev_map_pirq_msi() with PCI segments != 0

[PATCH for-4.21] tools/libxc: fix xc_physdev_map_pirq_msi() with PCI segments != 0

Posted by Roger Pau Monne 1 week, 6 days ago

Otherwise it's not possible for device models to map IRQs of devices on
segments different than 0.  Keep the same function prototype and pass the
segment in the high 16bits of the bus parameter, like it's done for the
hypercall itself.

Fixes: 7620c0cf9a4d ("PCI multi-seg: add new physdevop-s")
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I think it's 4.21 material, as otherwise it's not possible to passthrough
PCI devices on segments != 0.
---
 tools/libs/ctrl/xc_physdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libs/ctrl/xc_physdev.c b/tools/libs/ctrl/xc_physdev.c
index 25e686d7b389..1307d6836d72 100644
--- a/tools/libs/ctrl/xc_physdev.c
+++ b/tools/libs/ctrl/xc_physdev.c
@@ -79,7 +79,7 @@ int xc_physdev_map_pirq_msi(xc_interface *xch,
     }
     memset(&map, 0, sizeof(struct physdev_map_pirq));
     map.domid = domid;
-    map.type = MAP_PIRQ_TYPE_MSI;
+    map.type = MAP_PIRQ_TYPE_MSI_SEG;
     map.index = index;
     map.pirq = *pirq;
     map.bus = bus;
-- 
2.51.0

Re: [PATCH for-4.21] tools/libxc: fix xc_physdev_map_pirq_msi() with PCI segments != 0

Posted by Jan Beulich 1 week, 3 days ago

On 17.10.2025 16:14, Roger Pau Monne wrote:
> Otherwise it's not possible for device models to map IRQs of devices on
> segments different than 0.  Keep the same function prototype and pass the
> segment in the high 16bits of the bus parameter, like it's done for the
> hypercall itself.

While easiest, that's an odd interface, though. Should, at the very least the
function parameter then be named e.g. "segbus", along the lines of "devfn"?

> Fixes: 7620c0cf9a4d ("PCI multi-seg: add new physdevop-s")

This commit wasn't about tool stack uses of the interfaces at all.

Jan

Re: [PATCH for-4.21] tools/libxc: fix xc_physdev_map_pirq_msi() with PCI segments != 0

Posted by Roger Pau Monné 1 week, 3 days ago

On Mon, Oct 20, 2025 at 09:46:42AM +0200, Jan Beulich wrote:
> On 17.10.2025 16:14, Roger Pau Monne wrote:
> > Otherwise it's not possible for device models to map IRQs of devices on
> > segments different than 0.  Keep the same function prototype and pass the
> > segment in the high 16bits of the bus parameter, like it's done for the
> > hypercall itself.
> 
> While easiest, that's an odd interface, though. Should, at the very least the
> function parameter then be named e.g. "segbus", along the lines of "devfn"?

I certainly don't mind using segbus instead of plain bus, will adjust
now.

> > Fixes: 7620c0cf9a4d ("PCI multi-seg: add new physdevop-s")
> 
> This commit wasn't about tool stack uses of the interfaces at all.

But there should have been a tools side change somewhere to make use
of that interface, at the point that support for multi-segment was
added to Xen?  Otherwise the support feels like half done.

Would you prefer me to use the "Amends:" tag?  Or no tag at all.

Thanks, Roger.

Re: [PATCH for-4.21] tools/libxc: fix xc_physdev_map_pirq_msi() with PCI segments != 0

Posted by Jan Beulich 1 week, 3 days ago

On 20.10.2025 11:07, Roger Pau Monné wrote:
> On Mon, Oct 20, 2025 at 09:46:42AM +0200, Jan Beulich wrote:
>> On 17.10.2025 16:14, Roger Pau Monne wrote:
>>> Otherwise it's not possible for device models to map IRQs of devices on
>>> segments different than 0.  Keep the same function prototype and pass the
>>> segment in the high 16bits of the bus parameter, like it's done for the
>>> hypercall itself.
>>
>> While easiest, that's an odd interface, though. Should, at the very least the
>> function parameter then be named e.g. "segbus", along the lines of "devfn"?
> 
> I certainly don't mind using segbus instead of plain bus, will adjust
> now.
> 
>>> Fixes: 7620c0cf9a4d ("PCI multi-seg: add new physdevop-s")
>>
>> This commit wasn't about tool stack uses of the interfaces at all.
> 
> But there should have been a tools side change somewhere to make use
> of that interface, at the point that support for multi-segment was
> added to Xen?  Otherwise the support feels like half done.

Perhaps. I was after kernel functionality only at that point, and I
would really have liked toolstack folks to have picked up any work
needed there.

> Would you prefer me to use the "Amends:" tag?  Or no tag at all.

Both would be fine with me.

Jan

Re: [PATCH for-4.21] tools/libxc: fix xc_physdev_map_pirq_msi() with PCI segments != 0

Posted by Frediano Ziglio 1 week, 5 days ago

On Fri, Oct 17, 2025 at 3:26 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> Otherwise it's not possible for device models to map IRQs of devices on
> segments different than 0.  Keep the same function prototype and pass the
> segment in the high 16bits of the bus parameter, like it's done for the
> hypercall itself.
>
> Fixes: 7620c0cf9a4d ("PCI multi-seg: add new physdevop-s")
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> I think it's 4.21 material, as otherwise it's not possible to passthrough
> PCI devices on segments != 0.
> ---
>  tools/libs/ctrl/xc_physdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libs/ctrl/xc_physdev.c b/tools/libs/ctrl/xc_physdev.c
> index 25e686d7b389..1307d6836d72 100644
> --- a/tools/libs/ctrl/xc_physdev.c
> +++ b/tools/libs/ctrl/xc_physdev.c
> @@ -79,7 +79,7 @@ int xc_physdev_map_pirq_msi(xc_interface *xch,
>      }
>      memset(&map, 0, sizeof(struct physdev_map_pirq));
>      map.domid = domid;
> -    map.type = MAP_PIRQ_TYPE_MSI;
> +    map.type = MAP_PIRQ_TYPE_MSI_SEG;
>      map.index = index;
>      map.pirq = *pirq;
>      map.bus = bus;

Reviewed-by: Frediano Ziglio <frediano.ziglio@citrx.com>

This was tested on a real machine.

About MAP_PIRQ_TYPE_MSI and MAP_PIRQ_TYPE_MSI_SEG, do we need to keep
ABI compatibility or we should just remove MAP_PIRQ_TYPE_MSI_SEG and
make MAP_PIRQ_TYPE_MSI consider the segment ?

Frediano

Re: [PATCH for-4.21] tools/libxc: fix xc_physdev_map_pirq_msi() with PCI segments != 0

Posted by Jan Beulich 1 week, 3 days ago

On 18.10.2025 06:46, Frediano Ziglio wrote:
> On Fri, Oct 17, 2025 at 3:26 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>>
>> Otherwise it's not possible for device models to map IRQs of devices on
>> segments different than 0.  Keep the same function prototype and pass the
>> segment in the high 16bits of the bus parameter, like it's done for the
>> hypercall itself.
>>
>> Fixes: 7620c0cf9a4d ("PCI multi-seg: add new physdevop-s")
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> I think it's 4.21 material, as otherwise it's not possible to passthrough
>> PCI devices on segments != 0.
>> ---
>>  tools/libs/ctrl/xc_physdev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/libs/ctrl/xc_physdev.c b/tools/libs/ctrl/xc_physdev.c
>> index 25e686d7b389..1307d6836d72 100644
>> --- a/tools/libs/ctrl/xc_physdev.c
>> +++ b/tools/libs/ctrl/xc_physdev.c
>> @@ -79,7 +79,7 @@ int xc_physdev_map_pirq_msi(xc_interface *xch,
>>      }
>>      memset(&map, 0, sizeof(struct physdev_map_pirq));
>>      map.domid = domid;
>> -    map.type = MAP_PIRQ_TYPE_MSI;
>> +    map.type = MAP_PIRQ_TYPE_MSI_SEG;
>>      map.index = index;
>>      map.pirq = *pirq;
>>      map.bus = bus;
> 
> Reviewed-by: Frediano Ziglio <frediano.ziglio@citrx.com>
> 
> This was tested on a real machine.
> 
> About MAP_PIRQ_TYPE_MSI and MAP_PIRQ_TYPE_MSI_SEG, do we need to keep
> ABI compatibility or we should just remove MAP_PIRQ_TYPE_MSI_SEG and
> make MAP_PIRQ_TYPE_MSI consider the segment ?

Assuming you talk about the hypervisor interface, could you explain how
you see breaking ABI compatibility to not break anything? These aren't
tools-only interfaces, after all.

Jan

Re: [PATCH for-4.21] tools/libxc: fix xc_physdev_map_pirq_msi() with PCI segments != 0

Posted by Oleksii Kurochko 1 week, 5 days ago

On 10/17/25 4:14 PM, Roger Pau Monne wrote:
> Otherwise it's not possible for device models to map IRQs of devices on
> segments different than 0.  Keep the same function prototype and pass the
> segment in the high 16bits of the bus parameter, like it's done for the
> hypercall itself.
>
> Fixes: 7620c0cf9a4d ("PCI multi-seg: add new physdevop-s")
> Signed-off-by: Roger Pau Monné<roger.pau@citrix.com>
> ---
> I think it's 4.21 material, as otherwise it's not possible to passthrough
> PCI devices on segments != 0.

Make sense to me:
   Release-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

> ---
>   tools/libs/ctrl/xc_physdev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libs/ctrl/xc_physdev.c b/tools/libs/ctrl/xc_physdev.c
> index 25e686d7b389..1307d6836d72 100644
> --- a/tools/libs/ctrl/xc_physdev.c
> +++ b/tools/libs/ctrl/xc_physdev.c
> @@ -79,7 +79,7 @@ int xc_physdev_map_pirq_msi(xc_interface *xch,
>       }
>       memset(&map, 0, sizeof(struct physdev_map_pirq));
>       map.domid = domid;
> -    map.type = MAP_PIRQ_TYPE_MSI;
> +    map.type = MAP_PIRQ_TYPE_MSI_SEG;
>       map.index = index;
>       map.pirq = *pirq;
>       map.bus = bus;