| Message ID | b53c7853-b53a-37a0-d3bb-81093b19f305@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | (mainly) IOMMU hook adjustments | expand |
On 01.12.21 10:42, Jan Beulich wrote: > The array of IDs is an output. > > Fixes: 79647c5bc9c6 ("libxc: convert domctl interfaces over to hypercall buffers") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
On 01/12/2021 09:42, Jan Beulich wrote: > The array of IDs is an output. > > Fixes: 79647c5bc9c6 ("libxc: convert domctl interfaces over to hypercall buffers") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Clearly the function, including its Python wrapper, cannot have been > used by anything for many years. I wonder whether that isn't good enough > a reason to sanitize the layout of the array elements: Right now they > have BDF in bits 8...23, when conventionally this would be bits 0...15. There is a lot of WTF with this hypercall. It's obviously an attempt to do the thing that Linux calls IOMMU groups now, except that the correct way to do this would be for the group ID to be the unit of assignment/deassignment. (We need to do this anyway for other reasons.) The last user was deleted with Xend (2013), which suggests that it was broken for 3 years due to the incorrect bounce direction (2010). Furthermore, it will arbitrarily fail if targetting domains without an IOMMU configured, but everything else seems to be invariant of the passed domain. This should clearly be sysctl, not a domctl. I suggest ripping all of this infrastructure out. It's clearly unused (and broken in Xen too - see patch 1), and not something which should be used in this form in the future. ~Andrew
On 01.12.2021 16:11, Andrew Cooper wrote: > On 01/12/2021 09:42, Jan Beulich wrote: >> The array of IDs is an output. >> >> Fixes: 79647c5bc9c6 ("libxc: convert domctl interfaces over to hypercall buffers") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> Clearly the function, including its Python wrapper, cannot have been >> used by anything for many years. I wonder whether that isn't good enough >> a reason to sanitize the layout of the array elements: Right now they >> have BDF in bits 8...23, when conventionally this would be bits 0...15. > > There is a lot of WTF with this hypercall. It's obviously an attempt to > do the thing that Linux calls IOMMU groups now, except that the correct > way to do this would be for the group ID to be the unit of > assignment/deassignment. (We need to do this anyway for other reasons.) > > The last user was deleted with Xend (2013), which suggests that it was > broken for 3 years due to the incorrect bounce direction (2010). > > Furthermore, it will arbitrarily fail if targetting domains without an > IOMMU configured, but everything else seems to be invariant of the > passed domain. This should clearly be sysctl, not a domctl. > > > I suggest ripping all of this infrastructure out. It's clearly unused > (and broken in Xen too - see patch 1), I've not seen you point out any breakage in reply to patch 1, unless you mean VT-d's returning of -1 (which you didn't point out as broken, but which I can see would lead to misbehavior). > and not something which should be used in this form in the future. I didn't think the concept here was wrong. What's missing is the tool stack actually making use of this plus a way to do assignment in groups. Iirc the latter was something Paul had started work on before leaving Citrix? (That's leaving aside the bug you mention plus potential further ones.) Paul, Ian, Wei - what are your thoughts towards Andrew's proposal? Jan
--- a/tools/libs/ctrl/xc_domain.c +++ b/tools/libs/ctrl/xc_domain.c @@ -1546,7 +1546,8 @@ int xc_get_device_group( { int rc; DECLARE_DOMCTL; - DECLARE_HYPERCALL_BOUNCE(sdev_array, max_sdevs * sizeof(*sdev_array), XC_HYPERCALL_BUFFER_BOUNCE_IN); + DECLARE_HYPERCALL_BOUNCE(sdev_array, max_sdevs * sizeof(*sdev_array), + XC_HYPERCALL_BUFFER_BOUNCE_OUT); if ( xc_hypercall_bounce_pre(xch, sdev_array) ) {
The array of IDs is an output. Fixes: 79647c5bc9c6 ("libxc: convert domctl interfaces over to hypercall buffers") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Clearly the function, including its Python wrapper, cannot have been used by anything for many years. I wonder whether that isn't good enough a reason to sanitize the layout of the array elements: Right now they have BDF in bits 8...23, when conventionally this would be bits 0...15.