[Qemu-devel] [PATCH] vfio: Fix handling VFIO_IOMMU_GET_INFO results

[Qemu-devel] [PATCH] vfio: Fix handling VFIO_IOMMU_GET_INFO results

[Pavel Fedin]

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 <p.fedin@samsung.com>
---
 hw/vfio/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 6797208..afc10c7 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -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)) {
-- 
1.9.5.msysgit.0

Re: [Qemu-devel] [PATCH] vfio: Fix handling VFIO_IOMMU_GET_INFO results

[Alex Williamson]

On Thu, 2015-11-12 at 10:16 +0300, Pavel Fedin wrote:
> 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.  Thanks,

Alex

> 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 <p.fedin@samsung.com>
> ---
>  hw/vfio/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 6797208..afc10c7 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -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)) {

Re: [Qemu-devel] [PATCH] vfio: Fix handling VFIO_IOMMU_GET_INFO results

[Pavel Fedin]

 Hello!

> > 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.

 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.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH] vfio: Fix handling VFIO_IOMMU_GET_INFO results

[Alex Williamson]

On Thu, 2015-11-12 at 17:35 +0300, Pavel Fedin wrote:
>  Hello!
> 
> > > 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.
> 
>  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.

Isn't that the state we've been in since the very beginning?  QEMU gets
along ok assuming a minimum page size.  If that assumption were to be
invalid, DMA map would fail.  The fact that this bug has been there
since the very beginning and that QEMU has still been honoring the flag
suggests to me that it's not sufficiently critical to change the ABI.

>  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.  Thanks,

Alex

Re: [Qemu-devel] [PATCH] vfio: Fix handling VFIO_IOMMU_GET_INFO results

[Pavel Fedin]

 Hello!

> >  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.

 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".

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH] vfio: Fix handling VFIO_IOMMU_GET_INFO results

[Alex Williamson]

On Fri, 2015-11-13 at 12:33 +0300, Pavel Fedin wrote:
>  Hello!
> 
> > >  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.
> 
>  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".

You haven't presented a compelling reason to do this.  What is the case
that you're thinking of that assuming PAGE_SIZE doesn't work or at least
provide some degree of backwards compatibility?  QEMU has been doing
this all along.  Thanks,

Alex