On 17/05/17 11:27, Liu, Yi L wrote:
> On Fri, May 12, 2017 at 03:58:51PM -0600, Alex Williamson wrote:
>> On Wed, 26 Apr 2017 18:12:02 +0800
>> "Liu, Yi L" <yi.l....@intel.com> wrote:
>>>
>>> +/* IOCTL for Shared Virtual Memory Bind */
>>> +struct vfio_device_svm {
>>> + __u32 argsz;
>>> +#define VFIO_SVM_BIND_PASIDTBL (1 << 0) /* Bind PASID Table */
>>> +#define VFIO_SVM_BIND_PASID (1 << 1) /* Bind PASID from userspace
>>> driver */
>>> +#define VFIO_SVM_BIND_PGTABLE (1 << 2) /* Bind guest mmu page table */
>>> + __u32 flags;
>>> + __u32 length;
>>> + __u8 data[];
>>
>> In the case of VFIO_SVM_BIND_PASIDTBL this is clearly struct
>> pasid_table_info? So at a minimum this is a union including struct
>> pasid_table_info. Furthermore how does a user learn what the opaque
>> data in struct pasid_table_info is without looking at the code? A user
>> API needs to be clear and documented, not opaque and variable. We
>> should also have references to the hardware spec for an Intel or ARM
>> PASID table in uapi. flags should be defined as they're used, let's
>> not reserve them with the expectation of future use.
>>
>
> Agree. would add description accordingly. For the flags, I would remove
> the last two as I wouldn't use. I think Jean would add them in his/her
> patchset. Anyhow, one of us need to do merge on the flags.
Yes, I can add the VFIO_SVM_BIND_PASID (or rather _TASK) flag as (1 << 1)
in my series if it helps the merge. The PGTABLE flag is for another series
which I don't plan to send out anytime soon, since there already is enough
pending work on this.
Thanks,
Jean
