Re: [Qemu-devel] [RFC PATCH v2 10/13] linux headers update for DDW

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 08/21/2014 05:44 AM, Alex Williamson wrote:
> On Wed, 2014-08-20 at 17:49 +1000, Alexey Kardashevskiy wrote:
>> On 08/19/2014 03:42 AM, Alex Williamson wrote:
>>> On Fri, 2014-08-15 at 20:12 +1000, Alexey Kardashevskiy wrote:
>>>> Since the changes are not in upstream yet, no tag or branch is specified here.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>>  linux-headers/linux/vfio.h | 37 ++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
>>>> index 26c218e..f0aa97d 100644
>>>> --- a/linux-headers/linux/vfio.h
>>>> +++ b/linux-headers/linux/vfio.h
>>>> @@ -448,13 +448,48 @@ struct vfio_iommu_type1_dma_unmap {
>>>>   */
>>>>  struct vfio_iommu_spapr_tce_info {
>>>>  	__u32 argsz;
>>>> -	__u32 flags;			/* reserved for future use */
>>>> +	__u32 flags;
>>>> +#define VFIO_IOMMU_SPAPR_TCE_FLAG_DDW	1 /* Support dynamic windows */
>>>>  	__u32 dma32_window_start;	/* 32 bit window start (bytes) */
>>>>  	__u32 dma32_window_size;	/* 32 bit window size (bytes) */
>>>>  };
>>>>  
>>>>  #define VFIO_IOMMU_SPAPR_TCE_GET_INFO	_IO(VFIO_TYPE, VFIO_BASE + 12)
>>>>  
>>>> +/*
>>>> + * Dynamic DMA windows
>>>> + */
>>>> +struct vfio_iommu_spapr_tce_query {
>>>> +	__u32 argsz;
>>>> +	/* out */
>>>> +	__u32 windows_available;
>>>> +	__u32 page_size_mask;
>>>> +};
>>>
>>> Why do we need a new ioctl for this vs extending tce_info to include it?
>>> That's sort of the point of including argsz and flags in the ioctl.
>>
>>
>> It is not actual now but I can imagine that these numbers may change
>> depending on multiple calls of create()/remove().
> 
> Why does that matter?

Well, I could try imagining some hardware which would be able to have 2
small windows or 1 big window and only after the userspace created one
windows the host kernel could say if that window was small enough and it
still can do big window, or something like.

On the other hand, since we do not support this anyway and I do not think
we ever will and if we will, no idea what form it will take, I'll remove it
for now.


>>>> +#define VFIO_IOMMU_SPAPR_TCE_QUERY	_IO(VFIO_TYPE, VFIO_BASE + 17)
>>>> +
>>>> +struct vfio_iommu_spapr_tce_create {
>>>> +	__u32 argsz;
>>>> +	/* in */
>>>> +	__u32 page_shift;
>>>> +	__u32 window_shift;
>>>> +	/* out */
>>>> +	__u64 start_addr;
>>>> +
>>>> +};
>>>> +#define VFIO_IOMMU_SPAPR_TCE_CREATE	_IO(VFIO_TYPE, VFIO_BASE + 18)
>>>> +
>>>> +struct vfio_iommu_spapr_tce_remove {
>>>> +	__u32 argsz;
>>>> +	/* in */
>>>> +	__u64 start_addr;
>>>> +};
>>>> +#define VFIO_IOMMU_SPAPR_TCE_REMOVE	_IO(VFIO_TYPE, VFIO_BASE + 19)
>>>> +
>>>> +struct vfio_iommu_spapr_tce_reset {
>>>> +	__u32 argsz;
>>>> +};
>>>> +#define VFIO_IOMMU_SPAPR_TCE_RESET	_IO(VFIO_TYPE, VFIO_BASE + 20)
>>>> +
>>>
>>> argsz by itself seems rather pointless if we don't have a flags field to
>>> augment the structure.  Thanks,
>>
>>
>> Add flags and check for zero  or  remove it? Cannot choose, please help :)
> 
> Do we really need to hash this out again?  Almost every vfio ioctl
> includes argsz and flags.  Please continue to do this unless you have a
> good reason otherwise.  Thanks,

Will do. Thanks!



-- 
Alexey