diff mbox series

[4/4] libxc: correct bounce direction in xc_get_device_group()

Message ID b53c7853-b53a-37a0-d3bb-81093b19f305@suse.com (mailing list archive)
State Superseded
Headers show
Series (mainly) IOMMU hook adjustments | expand

Commit Message

Jan Beulich Dec. 1, 2021, 9:42 a.m. UTC
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.

Comments

Jürgen Groß Dec. 1, 2021, 12:11 p.m. UTC | #1
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
Andrew Cooper Dec. 1, 2021, 3:11 p.m. UTC | #2
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
Jan Beulich Dec. 2, 2021, 7:30 a.m. UTC | #3
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
diff mbox series

Patch

--- 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) )
     {