From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36702) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCQgf-0000h2-B1 for qemu-devel@nongnu.org; Tue, 07 Jul 2015 07:05:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCQgZ-0003MX-C5 for qemu-devel@nongnu.org; Tue, 07 Jul 2015 07:05:17 -0400 Received: from mail-pd0-f176.google.com ([209.85.192.176]:36048) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCQgZ-0003Lc-4g for qemu-devel@nongnu.org; Tue, 07 Jul 2015 07:05:11 -0400 Received: by pddu5 with SMTP id u5so36604322pdd.3 for ; Tue, 07 Jul 2015 04:05:10 -0700 (PDT) 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> From: Alexey Kardashevskiy Message-ID: <559BB25E.8000008@ozlabs.ru> Date: Tue, 7 Jul 2015 21:05:02 +1000 MIME-Version: 1.0 In-Reply-To: <20150707122125.0e58b09e@thh440s> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit 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: Thomas Huth Cc: Michael Roth , qemu-devel@nongnu.org, Gavin Shan , Alex Williamson , qemu-ppc@nongnu.org, David Gibson 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.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. >>>> @@ -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? > >>>> @@ -717,19 +791,36 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as) >>>> * when container fd is closed so we do not call it explicitly >>>> * in this file. >>>> */ >>>> - ret = ioctl(fd, VFIO_IOMMU_ENABLE); >>>> - if (ret) { >>>> - error_report("vfio: failed to enable container: %m"); >>>> - ret = -errno; >>>> - goto free_container_exit; >>>> + if (!v2) { >>>> + ret = ioctl(fd, VFIO_IOMMU_ENABLE); >>>> + if (ret) { >>>> + error_report("vfio: failed to enable container: %m"); >>>> + ret = -errno; >>>> + goto free_container_exit; >>>> + } >>>> } >>>> >>>> container->iommu_data.type1.listener = vfio_memory_listener; >>>> - container->iommu_data.release = vfio_listener_release; >>>> - >>>> memory_listener_register(&container->iommu_data.type1.listener, >>>> container->space->as); >>>> >>>> + if (!v2) { >>>> + container->iommu_data.release = vfio_listener_release; >>>> + } else { >>>> + container->iommu_data.release = vfio_spapr_listener_release_v2; >>>> + container->iommu_data.register_listener = >>>> + vfio_ram_memory_listener; >>>> + memory_listener_register(&container->iommu_data.register_listener, >>>> + &address_space_memory); >>>> + >>>> + if (container->iommu_data.ram_reg_error) { >>>> + error_report("vfio: RAM memory listener initialization failed for container"); >>> >>> Line > 80 columns? >> >> afaik user visible strings are an exception in QEMU and kernel. > > You're right for the kernel, but AFAIK QEMU (currently still) has a > hard limit at 80 columns. This is not an error, this is warning and in fact nobody is enforcing this (and this is a good thing) and for example VFIO already has longer lines. -- Alexey