RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> Sent: Friday, March 27, 2020 12:45 AM
> 
> Hi Christoph,
> 
> Thanks for the review. Please see my comments inline.
> 
> On Thu, 26 Mar 2020 02:23:16 -0700
> Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> 
> > On Wed, Mar 25, 2020 at 04:17:05PM -0700, Jacob Pan wrote:
> > > Having a single UAPI version to govern the user-kernel data
> > > structures makes compatibility check straightforward. On the
> > > contrary, supporting combinations of multiple versions of the data
> > > can be a nightmare to maintain.
> > >
> > > This patch defines a unified UAPI version to be used for
> > > compatibility checks between user and kernel.
> >
> > This is bullshit.  Version numbers don't scale and we've avoided them
> > everywhere.  You need need flags specifying specific behavior.
> >
> We have flags or the equivalent in each UAPI structures. The flags
> are used for checking validity of extensions as well. And you are right
> we can use them for checking specific behavior.
> 
> So we have two options here:
> 1. Have a overall version for a quick compatibility check while
> starting a VM. If not compatible, we will stop guest SVA entirely.
> 
> 2. Let each API calls check its own capabilities/flags at runtime. It
> may fail later on and lead to more complex error handling.
> For example, guest starts with SVA support, allocate a PASID, bind
> guest PASID works great. Then when IO page fault happens, it try to do
> page request service and found out certain flags are not compatible.
> This case is more complex to handle.

If those API calls are inter-dependent for composing a feature (e.g. SVA),
shouldn't we need a way to check them together before exposing the 
feature to the guest, e.g. through a iommu_get_uapi_capabilities interface?

> 
> I am guessing your proposal is #2. right?
> 
> Overall, we don;t expect much change to the UAPI other than adding some
> vendor specific part of the union. Is the scalability concern based on
> frequent changes?
> 
> > > +#define IOMMU_UAPI_VERSION	1
> > > +static inline int iommu_get_uapi_version(void)
> > > +{
> > > +	return IOMMU_UAPI_VERSION;
> > > +}
> >
> > Also inline functions like this in UAPI headers that actually get
> > included by userspace programs simply don't work.
> 
> I will remove that, user can just use IOMMU_UAPI_VERSION directly.