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.
