On Tue, Jul 09, 2024 at 03:26:31PM +0200, 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.
FWIW, it seems this forced identity mapping was introduced to overcome a regression in xend as a result of an XSA: 934a5253d932 fix XSA-46 regression with xend/xm Not sure however if other tools have since then come to rely on this behavior. > 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). I'm fine with the naming. Thanks, Roger.
