| Message ID | 20240708114124.407797-6-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
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
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
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.
diff --git a/tools/libs/ctrl/xc_physdev.c b/tools/libs/ctrl/xc_physdev.c index 460a8e779ce8..e9fcd755fa62 100644 --- 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)); diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c index 9feb12ae2b16..f8c9db7115ee 100644 --- 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);