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/08/2015 02:30 PM, David Gibson wrote:
> On Tue, Jul 07, 2015 at 09:05:02PM +1000, Alexey Kardashevskiy wrote:
>> 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.
>
> __u64 is just an alias typedef used by the kernel in uapi headers for
> 64-bit integers.  You should use uint64_t here.


Out of curiosity - I do not mind but still fail to see the point. 
reg::vaddr has a very specific type - __u64 - why should I cast to 
something which I am not going to pass to the kernel and which will be 
casted to __u64 anyway?

Is there some guideline about these uapi types? I'll read and shut up :)


-- 
Alexey