vfio: Fix handling VFIO_IOMMU_GET_INFO results

Kernel headers define VFIO_IOMMU_INFO_PGSIZES flag, however it has actually been never used, probably by mistake which now became a part of the ABI. The kernel always sets info.flags to 0:

http://lxr.free-electrons.com/source/drivers/vfio/vfio_iommu_type1.c?v=3.7#L675
http://lxr.free-electrons.com/source/drivers/vfio/vfio_iommu_type1.c#L974

Signed-off-by Pavel Fedin <[email protected]>

---
 hw/vfio/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

hw/vfio/common.c +1 -1

index 6797208..afc10c7 100644
@@ -704,7 +704,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
         info.argsz = sizeof(info);
         ret = ioctl(fd, VFIO_IOMMU_GET_INFO, &info);
         /* Ignore errors */
-        if ((ret == 0) && (info.flags & VFIO_IOMMU_INFO_PGSIZES)) {
+        if (ret == 0) {
             container->iova_pgsizes = info.iova_pgsizes;
         }
     } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) {

Quoted 3 lines

Kernel headers define VFIO_IOMMU_INFO_PGSIZES flag, however it has actually been never used, probably by mistake which now became a part of the ABI. The kernel always sets info.flags to 0:

I don't see how this implies that it becomes part of the ABI. In fact, as the defacto userspace driver for vfio, QEMU honoring the flag and not using the value the kernel provides implies the ABI is still valid. We should fix the kernel instead.

Hello!

Quoted 4 lines

Quoted 3 lines

Kernel headers define VFIO_IOMMU_INFO_PGSIZES flag, however it has actually been never used, probably by mistake which now became a part of the ABI. The kernel always sets info.flags to 0:

Quoted 4 lines

I don't see how this implies that it becomes part of the ABI. In fact, as the defacto userspace driver for vfio, QEMU honoring the flag and not using the value the kernel provides implies the ABI is still valid. We should fix the kernel instead.

Well... I intentionally put two links to LXR. From the very
beginning, this ioctl returned valid page sizes. And it never set this flag. We simply cannot have a kernel which does not report page sizes.

If we fix qemu, it will automatically start working with all
available kernels which are there in the wild. If we fix kernel, older versions will still not work, however they can.
That's why i think that we should adapt qemu to what already exists.
But, well, you are The Boss, so you can just say "i don't care". So, just let me now if you strongly disagree with this.

Quoted 6 lines

If we fix qemu, it will automatically start working with all
available kernels which are there in the wild. If we fix kernel, older versions will still not work, however they can.
That's why i think that we should adapt qemu to what already exists.
But, well, you are The Boss, so you can just say "i don't care". So, just let me now if you strongly disagree with this.

I do care, in fact I care enough about the ABI that I'm suggesting what I think is the correct fix rather than taking the quick and dirty solution. It's an unfortunate bug, but it's not worth changing the ABI and removing the kernel's ability to indicate whether the pgsize bitmap field is valid IMO.

Hello!

Quoted 7 lines

Quoted 6 lines

If we fix qemu, it will automatically start working with all
available kernels which are there in the wild. If we fix kernel, older versions will still not work, however they can.
That's why i think that we should adapt qemu to what already exists.
But, well, you are The Boss, so you can just say "i don't care". So, just let me now if you strongly disagree with this.

Quoted 5 lines

I do care, in fact I care enough about the ABI that I'm suggesting what I think is the correct fix rather than taking the quick and dirty solution. It's an unfortunate bug, but it's not worth changing the ABI and removing the kernel's ability to indicate whether the pgsize bitmap field is valid IMO.

Ok, i see your point...
But what about fix, which would work both with future kernels, which
do provide this flag, as well as would be compatible with already existing kernels, which set flags == 0?
We could check for ((info.flags == 0) || (info.flags &
VFIO_IOMMU_INFO_PGSIZES)). This would conform to both behaviors:
a) All current kernels set flags = 0 and report page sizes.
b) Some future kernels could have set some flags, but not reported
page sizes and not set VFIO_IOMMI_PGSIZES

What would you say about this? Yes, this would be a "workaround"
instead of "fix".