Re: [Qemu-devel] [RFC PATCH 7/8] VFIO: Add new IOCTL for IOMMU TLB invalidate propagation

[Alex Williamson]

On Mon, 17 Jul 2017 10:58:41 +0000
"Liu, Yi L" <address@hidden> wrote:

> Hi Alex,
> 
> Pls refer to the response inline.
> 
> > -----Original Message-----
> > From: address@hidden [mailto:address@hidden On Behalf
> > Of Alex Williamson
> > Sent: Saturday, July 15, 2017 2:16 AM
> > To: Liu, Yi L <address@hidden>
> > Cc: Jean-Philippe Brucker <address@hidden>; Tian, Kevin
> > <address@hidden>; Liu, Yi L <address@hidden>; Lan, Tianyu
> > <address@hidden>; Raj, Ashok <address@hidden>; address@hidden;
> > address@hidden; Will Deacon <address@hidden>; address@hidden;
> > address@hidden; address@hidden; Pan, Jacob jun
> > <address@hidden>; Joerg Roedel <address@hidden>
> > Subject: Re: [Qemu-devel] [RFC PATCH 7/8] VFIO: Add new IOCTL for IOMMU TLB
> > invalidate propagation
> > 
> > On Fri, 14 Jul 2017 08:58:02 +0000
> > "Liu, Yi L" <address@hidden> wrote:
> >   
> > > Hi Alex,
> > >
> > > Against to the opaque open, I'd like to propose the following
> > > definition based on the existing comments. Pls note that I've merged
> > > the pasid table binding and iommu tlb invalidation into a single IOCTL
> > > and make different flags to indicate the iommu operations. Per Kevin's
> > > comments, there may be iommu invalidation for guest IOVA tlb, so I
> > > renamed the IOCTL and data structure to be non-svm specific. Pls
> > > kindly have a review, so that we can make the opaque open closed and
> > > move forward. Surely, comments and ideas are welcomed. And for the
> > > scope and flags definition in struct iommu_tlb_invalidate, it's also 
> > > welcomed to  
> > give your ideas on it.  
> > >
> > > 1. Add a VFIO IOCTL for iommu operations from user-space
> > >
> > > #define VFIO_IOMMU_OP_IOCTL _IO(VFIO_TYPE, VFIO_BASE + 24)
> > >
> > > Corresponding data structure:
> > > struct vfio_iommu_operation_info {
> > >   __u32   argsz;
> > > #define VFIO_IOMMU_BIND_PASIDTBL  (1 << 0) /* Bind PASID Table */
> > > #define VFIO_IOMMU_BIND_PASID     (1 << 1) /* Bind PASID from userspace  
> > driver*/  
> > > #define VFIO_IOMMU_BIND_PGTABLE   (1 << 2) /* Bind guest mmu page table */
> > > #define VFIO_IOMMU_INVAL_IOTLB    (1 << 3) /* Invalidate iommu tlb */
> > >   __u32   flag;
> > >   __u32   length; // length of the data[] part in byte
> > >   __u8    data[]; // stores the data for iommu op indicated by flag field
> > > };  
> > 
> > If we're doing a generic "Ops" ioctl, then we should have an "op" field 
> > which is
> > defined by an enum.  It doesn't make sense to use flags for this, for 
> > example can we
> > set multiple flag bits?  If not then it's not a good use for a bit field.  
> > I'm also not sure I
> > understand the value of the "length" field, can't it always be calculated 
> > from argsz?  
> 
> Agreed, enum would be better. "length" field could be calculated from argsz. 
> I used
> it just to avoid offset calculations. May remove it.
>  
> > > For iommu tlb invalidation from userspace, the "__u8 data[]" stores
> > > data which would be parsed by the "struct iommu_tlb_invalidate"
> > > defined below.
> > >
> > > 2. Definitions in include/uapi/linux/iommu.h(newly added header file)
> > >
> > > /* IOMMU model definition for iommu operations from userspace */ enum
> > > iommu_model {
> > >   INTLE_IOMMU,
> > >   ARM_SMMU,
> > >   AMD_IOMMU,
> > >   SPAPR_IOMMU,
> > >   S390_IOMMU,
> > > };
> > >
> > > struct iommu_tlb_invalidate {
> > >   __u32   scope;
> > > /* pasid-selective invalidation described by @pasid */
> > > #define IOMMU_INVALIDATE_PASID    (1 << 0)
> > > /* address-selevtive invalidation described by (@vaddr, @size) */
> > > #define IOMMU_INVALIDATE_VADDR    (1 << 1)  
> > 
> > Again, is a bit field appropriate here, can a user set both bits?  
> 
> yes, user may set both bits. It would be invalidate address range
> which is tagged with a PASID value.
> 
> >   
> > >   __u32   flags;
> > > /*  targets non-pasid mappings, @pasid is not valid */
> > > #define IOMMU_INVALIDATE_NO_PASID (1 << 0)
> > > /* indicating that the pIOMMU doesn't need to invalidate
> > >   all intermediate tables cached as part of the PTE for
> > >   vaddr, only the last-level entry (pte). This is a hint. */
> > > #define IOMMU_INVALIDATE_VADDR_LEAF       (1 << 1)  
> > 
> > Are we venturing into vendor specific attributes here?  
> 
> These two attributes are still in discussion. Jean and me synced
> several rounds. But lack of comments from other vendors.
> 
> Personally, I think both should be generic.
> IOMMU_INVALIDATE_NO_PASID is to indicate no PASID used
> for the invalidation. IOMMU_INVALIDATE_VADDR_LEAF indicates
> only invalidate leaf mappings. 
> I would see if other vendor is object on it. If yes, I'm fine to move
> it to vendor specific part.
>  
> >   
> > >   __u32   pasid;
> > >   __u64   vaddr;
> > >   __u64   size;
> > >   enum iommu_model model;  
> > 
> > How does a user learn which model(s) are supported by the interface?
> > How do they learn which ops are supported?  Perhaps a good use for one of 
> > those
> > flag bits in the outer data structure is "probe".  
> 
> My initial plan to user fills it, if the underlying HW doesn't support the
> model, it refuses to service it. User should get a failure and stop to use
> it. But your suggestion to have a probe or kinds of query makes sense.
> How about we add one more operation for such purpose? Besides the
> supported model query, I'd like to add more. E.g the HW IOMMU capabilities.

We also have VFIO_IOMMU_GET_INFO where the structure can be extended
for missing capabilities.  Depending on the capability you want to
describe, this might be a better, existing interface for it.
 
> > >   /*
> > >    Vendor may have different HW version and thus the
> > >    data part of this structure differs, use sub_version
> > >    to indicate such difference.
> > >    */
> > >   __u322 sub_version;  
> > 
> > Not sure I see the value of this vs creating an INTEL_IOMMUv2 entry in the 
> > model
> > enum.  
> 
> Both are fine to me. Just see the opinions from other guys.
> 
> > >   __u64 length; // length of the data[] part in byte  
> > 
> > Questionably useful vs calculating from argsz again , but it certainly 
> > doesn't need to
> > be a qword :-o  
> 
> Thx for the remind. 32bits would be enough. It is surely to get it from 
> argsz. However,
> I would like to leave it here. Reason is:
> argsz is in vfio layer, the "length" here is actually used in vendor-specific 
> iommu driver
> layer. So would require vfio to pass argsz or the size of " struct 
> iommu_tlb_invalidate"
> to vendor-specific iommu driver layer by means of parameter or so. 
> Personally, I prefer
> to pass it in the structure. If it's better to pass it as a parameter, I 
> would do it.

Ok, then the layer that does the copy_from_user will need to validate
that length is fully contained within the copied data structure, we
can't let the user trick the kernel into using kernel memory for this.

> > >   __u8    data[];
> > > };
> > >
> > > For Intel, the data structue is:
> > > struct intel_iommu_invalidate_data {
> > >   __u64 low;
> > >   __u64 high;
> > > }  
> > 
> > high/low what?  This is a pretty weak uapi definition.  Thanks,  
> 
> For this part, for Intel platform, we plan to pass a 128 bit data for the 
> invalidation.
> The structure varies from invalidation type to type. Here is my thought on 
> it. Define
> an 128 bits union. List the invalidation data details for each invalidation 
> type. What's
> your opinion on it? So far, we have 7 types for invalidation. The prq 
> response is not
> included.

I want this interface to be fully defined, but at the same time I don't
necessarily want to create useless data structures.  I believe the
intention here is to pass these directly through to a QI entry, where
we must match a hardware definition.  I'm tempted to suggest
referencing the hardware specification, but see below...

A concern for this model is that hardware may trust the iommu driver
not to create QI entries that don't set reserved bits or set invalid
field data.  If it does those kinds of things, it's a kernel driver
bug.  Once exposed to the user, we cannot guarantee that.  Does Intel
have confidence that a user cannot maliciously interfere with other
contexts or the general operation of the invalidation queue if a user is
effectively given direct access?  Will the invalidation data be
sanitized by the iommu driver?
 
> union intel_iommu_invalidate_data {
>       struct {
>               __u64 low;
>               __u64 high;
>       } invalidate_data;
> 
>       struct {
>               __u64 type: 4;
>               __u64 gran: 2;
>               __u64 rsv1: 10;
>               __u64 did: 16;
>               __u64 sid: 16;
>               __u64 func_mask: 2;
>               __u64 rsv2: 14;
>               __64 rsv3: 64;
>       } context_cache_inv;
>       ....

Here's part of the issue with not fully defining these, we have did,
sid, and func_mask.  I think we're claiming that the benefit of passing
through the hardware data structure is performance, but the user needs
to replace these IDs to match the physical device rather than the
virtual device, perhaps even entirely recreating it because there's not
necessarily a 1:1 mapping of things like func_mask between virtual and
physical hardware topologies (assuming I'm interpreting these fields
correctly).  Doesn't the kernel also need to validate any such field to
prevent the user spoofing entries for other devices?  Is there any
actual performance benefit remaining vs defining a generic interface
after multiple levels have manipulated, recreated, and sanitized
these structures?  We can't evaluate these sorts of risks if we don't
define what we're passing through.  Thanks,

Alex