On Tue, 7 Jul 2015 20:05:25 +1000 Alexey Kardashevskiy <a...@ozlabs.ru> wrote:
> On 07/07/2015 05:23 PM, Thomas Huth wrote: > > On Mon, 6 Jul 2015 12:11:09 +1000 > > Alexey Kardashevskiy <a...@ozlabs.ru> 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. > >> @@ -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. > >> @@ -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. Thomas