Re: [Qemu-devel] [PATCH for-2.11] vfio: Fix vfio-kvm group registration

[Alexey Kardashevskiy]

On 06/12/17 12:30, Alex Williamson wrote:
> On Wed, 6 Dec 2017 12:02:01 +1100
> Alexey Kardashevskiy <address@hidden> wrote:
> 
>> On 06/12/17 08:09, Alex Williamson wrote:
>>> Commit 8c37faa475f3 ("vfio-pci, ppc64/spapr: Reorder group-to-container
>>> attaching") moved registration of groups with the vfio-kvm device from
>>> vfio_get_group() to vfio_connect_container(), but it missed the case
>>> where a group is attached to an existing container and takes an early
>>> exit.  Perhaps this is a less common case on ppc64/spapr, but on x86
>>> (without viommu) all groups are connected to the same container and
>>> thus only the first group gets registered with the vfio-kvm device.
>>> This becomes a problem if we then hot-unplug the devices associated
>>> with that first group and we end up with KVM being misinformed about
>>> any vfio connections that might remain.  Fix by including the call to
>>> vfio_kvm_device_add_group() in this early exit path.
>>>
>>> Fixes: 8c37faa475f3 ("vfio-pci, ppc64/spapr: Reorder group-to-container 
>>> attaching")
>>> Cc: address@hidden # qemu-2.10+
>>> Signed-off-by: Alex Williamson <address@hidden>
>>> ---
>>>
>>> This bug also existed in QEMU 2.10, but I think the fix is sufficiently
>>> obvious (famous last words) to propose for 2.11 at this late date.  If
>>> the first group is hot unplugged then KVM may revert to code emulation
>>> that assumes no non-coherent DMA is present on some systems.  Also for
>>> KVMGT, if the vGPU is not the first device registered, then the
>>> notifier to enable linkages to KVM would not be called.  Please review.  
>>
>> For what it is worth
>>
>> Reviewed-by: Alexey Kardashevskiy <address@hidden>
> 
> Thanks!
> 
>> Sorry for the breakage...
>>
>> One question - how was this discovered? I'd love to set up a test
>> environment on my old thinkpad x230 if possible.
> 
> Assign two devices from separate iommu groups, hot unplug the first
> device, followed by the second device.  The second unplug will trigger:
> 
> qemu-kvm: Failed to remove group ## from KVM VFIO device: No such file or 
> directory
> 
> Laptops don't have many devices and we're not good about keeping up
> with ACS quirks on laptop chipsets, so it might be difficult to find
> the prerequisite setup there.  Thanks,

Tried the laptop, these worked:

03:00.0 Network controller: Intel Corporation Centrino Advanced-N 6205
[Taylor Peak] (rev 34)
00:1a.0 USB controller: Intel Corporation 7 Series/C216 Chipset Family USB
Enhanced Host Controller #2 (rev 04)


However VGA did not.

$ lspci -nns 00:02.0
00:02.0 VGA compatible controller [0300]: Intel Corporation 3rd Gen Core
processor Graphics Controller [8086:0166] (rev 09)

I run like this:

pbuild/qemu-localhost-x86_64/x86_64-softmmu/qemu-system-x86_64 \
-enable-kvm -m 2G \
-netdev "tap,id=TAP0,helper=/home/aik/qemu-bridge-helper --br=br0" \
-device "virtio-net-pci,id=vnet0,mac=C0:41:49:4b:00:32,netdev=TAP0" \
virtimg/fc27-32GB.qcow2 -nodefaults \
-chardev stdio,id=STDIO0,signal=off,mux=on \
-device isa-serial,id=isa-serial0,chardev=STDIO0 \
-mon id=MON0,chardev=STDIO0,mode=readline -nographic -vga none \
-snapshot \
-device "vfio-pci,id=vfio0000_00_02_0,host=0000:00:02.0"

and it crashes pretty soon, I suppose, as @pc does not change:

(qemu) info cpus
* CPU #0: pc=0x00000000000c5afa thread_id=4024
(qemu) info cpus
* CPU #0: pc=0x00000000000c5afa thread_id=4024

and it does not seem to reach seabios or it does and seabios is
initializing VGA - hard to tell, without any VGA - seabios prints messages
to the console and shows grub. Is there any trick to try? Not big deal if
none, just curious. Thanks.




> 
> Alex
> 
>>>  hw/vfio/common.c |    1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 7b2924c0ef19..7007878e345e 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -968,6 +968,7 @@ static int vfio_connect_container(VFIOGroup *group, 
>>> AddressSpace *as,
>>>          if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
>>>              group->container = container;
>>>              QLIST_INSERT_HEAD(&container->group_list, group, 
>>> container_next);
>>> +            vfio_kvm_device_add_group(group);
>>>              return 0;
>>>          }
>>>      }
>>>   
>>
>>
> 


-- 
Alexey