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

[Alexey Kardashevskiy]

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.



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