Re: [XEN PATCH v12 5/7] tools/libxc: Allow gsi be mapped into a free pirq

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/7/9 21:26, Jan Beulich wrote:
> On 08.07.2024 13:41, Jiqian Chen wrote:
>> Hypercall PHYSDEVOP_map_pirq support to map a gsi into a specific
>> pirq or a free pirq, it depends on the parameter pirq(>0 or <0).
>> But in current xc_physdev_map_pirq, it set *pirq=index when
>> parameter pirq is <0, it causes to force all cases to be mapped
>> to a specific pirq. That has some problems, one is caller can't
>> get a free pirq value, another is that once the pecific pirq was
>> already mapped to other gsi, then it will fail.
>>
>> So, change xc_physdev_map_pirq to allow to pass negative parameter
>> in and then get a free pirq.
>>
>> There are four caller of xc_physdev_map_pirq in original codes, so
>> clarify the affect below(just need to clarify the pirq<0 case):
>>
>> First, pci_add_dm_done->xc_physdev_map_pirq, it pass irq to pirq
>> parameter, if pirq<0 means irq<0, then it will fail at check
>> "index < 0" in allocate_and_map_gsi_pirq and get EINVAL, logic is
>> the same as original code.
> 
> There we have
> 
>     int pirq = XEN_PT_UNASSIGNED_PIRQ;
> 
> (with XEN_PT_UNASSIGNED_PIRQ being -1) and then
> 
>     rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
> 
> Therefore ...
> 
>> --- a/tools/libs/ctrl/xc_physdev.c
>> +++ b/tools/libs/ctrl/xc_physdev.c
>> @@ -50,7 +50,7 @@ int xc_physdev_map_pirq(xc_interface *xch,
>>      map.domid = domid;
>>      map.type = MAP_PIRQ_TYPE_GSI;
>>      map.index = index;
>> -    map.pirq = *pirq < 0 ? index : *pirq;
>> +    map.pirq = *pirq;
>>  
>>      rc = do_physdev_op(xch, PHYSDEVOP_map_pirq, &map, sizeof(map));
> 
> ... this very much looks like a change in behavior to me: *pirq is
> negative, and hence index would have been put in map.pirq instead. While
> with your change we'd then pass -1, i.e. requesting to obtain a new
> pIRQ.
> 
> I also consider it questionable to go by in-tree users. I think proof of
> no functional change needs to also consider possible out-of-tree users,
> not the least seeing the Python binding below (even if right there you
> indeed attempt to retain prior behavior). The one aspect in your favor
> is that libxc isn't considered to have a stable ABI.
> 
> Overall I see little room to avoid introducing a new function with this
> improved behavior (maybe xc_physdev_map_pirq_gsi()). Ideally existing
> callers would then be switched, to eventually allow removing the old
> function (thus cleanly and noticeably breaking any out-of-tree users
> that there may be, indicating to their developers that they need to
> adjust their code).
Make sense, adding a new function xc_physdev_map_pirq_gsi is much better, and 
it has the least impact.
Thank you very much!
I will change to add xc_physdev_map_pirq_gsi in next version.

> 
>> --- a/tools/python/xen/lowlevel/xc/xc.c
>> +++ b/tools/python/xen/lowlevel/xc/xc.c
>> @@ -774,6 +774,8 @@ static PyObject *pyxc_physdev_map_pirq(PyObject *self,
>>      if ( !PyArg_ParseTupleAndKeywords(args, kwds, "iii", kwd_list,
>>                                        &dom, &index, &pirq) )
>>          return NULL;
>> +    if ( pirq < 0 )
>> +        pirq = index;
>>      ret = xc_physdev_map_pirq(xc->xc_handle, dom, index, &pirq);
>>      if ( ret != 0 )
>>            return pyxc_error_to_exception(xc->xc_handle);
> 
> I question this change, yet without Cc-ing the maintainer (now added)
> you're not very likely to get a comment (let alone an ack) on this.
> 
> Jan

-- 
Best regards,
Jiqian Chen.