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

[Alex Williamson <alex.williamson@redhat.com>]

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?

> >> +#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,

Alex