Re: [Qemu-ppc] [Qemu-devel] [PATCH qemu v10 13/14] vfio: spapr: Add SPAPR IOMMU v2 support (DMA memory preregistering)

[David Gibson]

On Tue, Jul 07, 2015 at 09:05:02PM +1000, Alexey Kardashevskiy wrote:
> On 07/07/2015 08:21 PM, Thomas Huth wrote:
> >On Tue, 7 Jul 2015 20:05:25 +1000
> >Alexey Kardashevskiy <address@hidden> wrote:
> >
> >>On 07/07/2015 05:23 PM, Thomas Huth wrote:
> >>>On Mon,  6 Jul 2015 12:11:09 +1000
> >>>Alexey Kardashevskiy <address@hidden> wrote:
> >...
> >>>>diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >>>>index 8eacfd7..0c7ba8c 100644
> >>>>--- a/hw/vfio/common.c
> >>>>+++ b/hw/vfio/common.c
> >>>>@@ -488,6 +488,76 @@ static void vfio_listener_release(VFIOContainer 
> >>>>*container)
> >>>>       memory_listener_unregister(&container->iommu_data.type1.listener);
> >>>>   }
> >>>>
> >>>>+static void vfio_ram_do_region(VFIOContainer *container,
> >>>>+                              MemoryRegionSection *section, unsigned 
> >>>>long req)
> >>>>+{
> >>>>+    int ret;
> >>>>+    struct vfio_iommu_spapr_register_memory reg = { .argsz = sizeof(reg) 
> >>>>};
> >>>>+
> >>>>+    if (!memory_region_is_ram(section->mr) ||
> >>>>+        memory_region_is_skip_dump(section->mr)) {
> >>>>+        return;
> >>>>+    }
> >>>>+
> >>>>+    if (unlikely((section->offset_within_region & (getpagesize() - 1)))) 
> >>>>{
> >>>>+        error_report("%s received unaligned region", __func__);
> >>>>+        return;
> >>>>+    }
> >>>>+
> >>>>+    reg.vaddr = (__u64) memory_region_get_ram_ptr(section->mr) +
> >>>
> >>>We're in usespace here ... I think it would be better to use uint64_t
> >>>instead of the kernel-type __u64.
> >>
> >>We are calling a kernel here - @reg is a kernel-defined struct.
> >
> >If you grep for __u64 in the QEMU sources, you'll see that hardly
> >anybody is using this type - even if calling ioctls. So for
> >consistency, I'd really suggest to use uint64_t here.
> 
> I am not using it, I am packing data to a struct. So does vfio_dma_map()
> already.

__u64 is just an alias typedef used by the kernel in uapi headers for
64-bit integers.  You should use uint64_t here.

> >>>>@@ -698,14 +768,18 @@ static int vfio_connect_container(VFIOGroup *group, 
> >>>>AddressSpace *as)
> >>>>
> >>>>           container->iommu_data.type1.initialized = true;
> >>>>
> >>>>-    } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) {
> >>>>+    } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU) ||
> >>>>+               ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU)) 
> >>>>{
> >>>>+        bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION, 
> >>>>VFIO_SPAPR_TCE_v2_IOMMU);
> >>>
> >>>That "!!" sounds somewhat wrong here. I think you either want to check
> >>>for "ioctl() == 1" (because only in this case you can be sure that v2
> >>>is supported), or you can simply omit the "!!" because you're 100% sure
> >>>that the ioctl only returns 0 or 1 (and never a negative error code).
> >>
> >>
> >>The host kernel does not return an error on these ioctls, it returns 0 or
> >>1. And "!!" is shorter than "(bool)". VFIO_CHECK_EXTENSION for Type1 does
> >>exactly the same already.
> >
> >Simply using nothing instead is even shorter than using "!!". The
> >compiler is smart enough to convert from 0 and 1 to bool.
> >"!!" is IMHO quite ugly and should only be used when it is really
> >necessary.
> 
> 
> imho it is not but either way I'd rather follow the existing style,
> especially if I do literally the same thing (checking IOMMU version). Unless
> the original author tells me to convert all the existing occurences of "!!"
> to "!=0" (or something like this) before I post new ones.
> 
> Alex, should I get rid of "!!"s in the patch?

I think !! is the lesser evil here.  The trouble is that in C "bool"
is not a first-class datatype, but just a typedef for some integer
type.  Which means that, confusingly, (bool)2 != (bool)1.  So using
the !! trick to force a value to be either 0 or 1 when assigning it to
a bool variable is probably a good idea.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpmOu7Lr6AvL.pgp
Description: PGP signature