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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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