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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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



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