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

[Thomas Huth]

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.

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