From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39901) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCiZN-0005yj-HA for qemu-devel@nongnu.org; Wed, 08 Jul 2015 02:10:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCiZL-0006aR-Fv for qemu-devel@nongnu.org; Wed, 08 Jul 2015 02:10:57 -0400 Date: Wed, 8 Jul 2015 14:30:29 +1000 From: David Gibson Message-ID: <20150708043029.GL17857@voom.redhat.com> References: <1436148670-6592-1-git-send-email-aik@ozlabs.ru> <1436148670-6592-14-git-send-email-aik@ozlabs.ru> <20150707092311.728e2cd7@thh440s> <559BA465.50009@ozlabs.ru> <20150707122125.0e58b09e@thh440s> <559BB25E.8000008@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="DN8g+DOX2TxGxleI" Content-Disposition: inline In-Reply-To: <559BB25E.8000008@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH qemu v10 13/14] vfio: spapr: Add SPAPR IOMMU v2 support (DMA memory preregistering) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Thomas Huth , Michael Roth , qemu-devel@nongnu.org, Gavin Shan , Alex Williamson , qemu-ppc@nongnu.org --DN8g+DOX2TxGxleI Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 wrote: > > > >>On 07/07/2015 05:23 PM, Thomas Huth wrote: > >>>On Mon, 6 Jul 2015 12:11:09 +1000 > >>>Alexey Kardashevskiy 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.listen= er); > >>>> } > >>>> > >>>>+static void vfio_ram_do_region(VFIOContainer *container, > >>>>+ MemoryRegionSection *section, unsigned= long req) > >>>>+{ > >>>>+ int ret; > >>>>+ struct vfio_iommu_spapr_register_memory reg =3D { .argsz =3D siz= eof(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 =3D (__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. >=20 > 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 *gr= oup, AddressSpace *as) > >>>> > >>>> container->iommu_data.type1.initialized =3D 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_IOM= MU)) { > >>>>+ bool v2 =3D !!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE= _v2_IOMMU); > >>> > >>>That "!!" sounds somewhat wrong here. I think you either want to check > >>>for "ioctl() =3D=3D 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 do= es > >>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. >=20 >=20 > 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). Unl= ess > the original author tells me to convert all the existing occurences of "!= !" > to "!=3D0" (or something like this) before I post new ones. >=20 > 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 !=3D (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. --=20 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 --DN8g+DOX2TxGxleI Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVnKdlAAoJEGw4ysog2bOS2KIP/iHkbl3k/az1Ry1v79W1JV9j r/7uX1X+GQ/42uuI2VZsArjDzCB4hAaDe+e1cIdGzOjqFMK6LGfOoIO8Fr5M+9w/ AJJ9kUM1gI3m+/jCBXSlUVDhzXofZTpY7Pe+yWJPttIkd6SXqpFsaSjyTtFgvDOQ KGr9eiQl9CON2aRXS1FvMDZzhLvKiNxokNEnhY3A00k/EgLayLcjauelPwfMfGLJ /HQa/FqQUVHxOnmTQnVnDy1b0F7AyJyipQNqwGwCT0ItRMXe0nzgjiNpI5ggqLJe 8SOplnaGlEiwKqptyQjZSj4awIqqBB95J2+cpPFMRW6zV1hytxW529dYkZ1PpbE5 w+qAFFRv4wPiusot0lx+l7GU0pvVhFyqKaY7fMt7cTweFTBccaZ5QuqyPeP2n3t4 7J3QHjW7bGcx5QxTRFpF4ZJ5x+w0ZT1PfmHujCgmUhm1VkA07XH783DH9JMzz5Wl sOi2GShIcfBPHptRY3UfCTduTVNKiS5V9dVRRQzlQqQ8/YCq4BKxw/Xn8BBb8Gix mdDWKhPmLLWTmGwSeDIyvDGfS+dtz0iQ4qBGwm6GJzDsT9eUA4eVRGbtSfclGlMF 1EJQ+jHTBSWAlydYMuUa9aHHh61Ybyvz7LY5iCVpykPAq4V2jrQmG4AeKV06NJen kK5KNqZNyDhUH7iqtFPQ =Zf4D -----END PGP SIGNATURE----- --DN8g+DOX2TxGxleI--