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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.