RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
Hi Alex,
> From: Alex Williamson
> Sent: Saturday, April 4, 2020 1:50 AM
> Subject: Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
>
> On Fri, 3 Apr 2020 13:12:50 +
> "Liu, Yi L" wrote:
>
> > Hi Alex,
> >
> > > From: Alex Williamson
> > > Sent: Friday, April 3, 2020 1:50 AM
> > > Subject: Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> > >
> > > On Sun, 22 Mar 2020 05:31:58 -0700
> > > "Liu, Yi L" wrote:
> > >
> > > > From: Liu Yi L
> > > >
[...]
> > > > static long vfio_iommu_type1_ioctl(void *iommu_data,
> > > >unsigned int cmd, unsigned long arg)
> > > > {
> > > > @@ -2276,6 +2333,53 @@ static long vfio_iommu_type1_ioctl(void
> > > *iommu_data,
> > > >
> > > > return copy_to_user((void __user *)arg, , minsz) ?
> > > > -EFAULT : 0;
> > > > +
> > > > + } else if (cmd == VFIO_IOMMU_PASID_REQUEST) {
> > > > + struct vfio_iommu_type1_pasid_request req;
> > > > + unsigned long offset;
> > > > +
> > > > + minsz = offsetofend(struct
> > > > vfio_iommu_type1_pasid_request,
> > > > + flags);
> > > > +
> > > > + if (copy_from_user(, (void __user *)arg, minsz))
> > > > + return -EFAULT;
> > > > +
> > > > + if (req.argsz < minsz ||
> > > > + !vfio_iommu_type1_pasid_req_valid(req.flags))
> > > > + return -EINVAL;
> > > > +
> > > > + if (copy_from_user((void *) + minsz,
> > > > + (void __user *)arg + minsz,
> > > > + sizeof(req) - minsz))
> > > > + return -EFAULT;
> > >
> > > Huh? Why do we have argsz if we're going to assume this is here?
> >
> > do you mean replacing sizeof(req) with argsz? if yes, I can do that.
>
> No, I mean the user tells us how much data they've provided via argsz.
> We create minsz the the end of flags and verify argsz includes flags.
> Then we proceed to ignore argsz to see if the user has provided the
> remainder of the structure.
I think I should avoid using sizeof(req) as it may be variable
new flag is added. I think better to make a data[] field in struct
vfio_iommu_type1_pasid_request and copy data[] per flag. I'll
make this change in new version.
> > > > +
> > > > + switch (req.flags & VFIO_PASID_REQUEST_MASK) {
> > > > + case VFIO_IOMMU_PASID_ALLOC:
> > > > + {
> > > > + int ret = 0, result;
> > > > +
> > > > + result = vfio_iommu_type1_pasid_alloc(iommu,
> > > > +
> > > > req.alloc_pasid.min,
> > > > +
> > > > req.alloc_pasid.max);
> > > > + if (result > 0) {
> > > > + offset = offsetof(
> > > > + struct
> > > > vfio_iommu_type1_pasid_request,
> > > > + alloc_pasid.result);
> > > > + ret = copy_to_user(
> > > > + (void __user *) (arg +
> > > > offset),
> > > > + , sizeof(result));
> > >
> > > Again assuming argsz supports this.
> >
> > same as above.
> >
> > >
> > > > + } else {
> > > > + pr_debug("%s: PASID alloc failed\n",
> > > > __func__);
> > >
> > > rate limit?
> >
> > not quite get. could you give more hints?
>
> A user can spam the host logs simply by allocating their quota of
> PASIDs and then trying to allocate more, or by specifying min/max such
> that they cannot allocate the requested PASID. If this logging is
> necessary for debugging, it should be ratelimited to avoid a DoS on the
> host.
got it. thanks for the coaching. will use pr_debug_ratelimited().
Regards,
Yi Liu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
On Tue, 7 Apr 2020 04:42:02 + "Tian, Kevin" wrote: > > From: Alex Williamson > > Sent: Friday, April 3, 2020 11:14 PM > > > > On Fri, 3 Apr 2020 05:58:55 + > > "Tian, Kevin" wrote: > > > > > > From: Alex Williamson > > > > Sent: Friday, April 3, 2020 1:50 AM > > > > > > > > On Sun, 22 Mar 2020 05:31:58 -0700 > > > > "Liu, Yi L" wrote: > > > > > > > > > From: Liu Yi L > > > > > > > > > > For a long time, devices have only one DMA address space from > > platform > > > > > IOMMU's point of view. This is true for both bare metal and directed- > > > > > access in virtualization environment. Reason is the source ID of DMA > > > > > in > > > > > PCIe are BDF (bus/dev/fnc ID), which results in only device > > > > > granularity > > > > > DMA isolation. However, this is changing with the latest advancement > > > > > in > > > > > I/O technology area. More and more platform vendors are utilizing the > > > > > > > > > PCIe > > > > > PASID TLP prefix in DMA requests, thus to give devices with multiple > > DMA > > > > > address spaces as identified by their individual PASIDs. For example, > > > > > Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able > > > > > to > > > > > let device access multiple process virtual address space by binding > > > > > the > > > > > virtual address space with a PASID. Wherein the PASID is allocated in > > > > > software and programmed to device per device specific manner. > > Devices > > > > > which support PASID capability are called PASID-capable devices. If > > > > > such > > > > > devices are passed through to VMs, guest software are also able to > > > > > bind > > > > > guest process virtual address space on such devices. Therefore, the > > guest > > > > > software could reuse the bare metal software programming model, > > which > > > > > means guest software will also allocate PASID and program it to device > > > > > directly. This is a dangerous situation since it has potential PASID > > > > > conflicts and unauthorized address space access. It would be safer to > > > > > let host intercept in the guest software's PASID allocation. Thus > > > > > PASID > > > > > are managed system-wide. > > > > > > > > Providing an allocation interface only allows for collaborative usage > > > > of PASIDs though. Do we have any ability to enforce PASID usage or can > > > > a user spoof other PASIDs on the same BDF? > > > > > > An user can access only PASIDs allocated to itself, i.e. the specific > > > IOASID > > > set tied to its mm_struct. > > > > A user is only _supposed_ to access PASIDs allocated to itself. AIUI > > the mm_struct is used for managing the pool of IOASIDs from which the > > user may allocate that PASID. We also state that programming the PASID > > into the device is device specific. Therefore, are we simply trusting > > the user to use a PASID that's been allocated to them when they program > > the device? If a user can program an arbitrary PASID into the device, > > then what prevents them from attempting to access data from another > > user via the device? I think I've asked this question before, so if > > there's a previous explanation or spec section I need to review, please > > point me to it. Thanks, > > > > There are two scenarios: > > (1) for PF/VF, the iommu driver maintains an individual PASID table per > PDF. Although the PASID namespace is global, the per-BDF PASID table > contains only valid entries for those PASIDs which are allocated to the > mm_struct. The user is free to program arbitrary PASID into the assigned > device, but using invalid PASIDs simply hit iommu fault. > > (2) for mdev, multiple mdev instances share the same PASID table of > the parent BDF. However, PASID programming is a privileged operation > in multiplexing usage, thus must be mediated by mdev device driver. > The mediation logic will guarantee that only allocated PASIDs are > forwarded to the device. Thanks, I was confused about multiple tenants sharing a BDF when PASID programming to the device is device specific, and therefore not something we can virtualize. However, the solution is device specific virtualization via mdev. Thus, any time we're sharing a BDF between tenants, we must virtualize the PASID programming and therefore it must be an mdev device currently. If a tenant is the exclusive user of the BDF, then no virtualization of the PASID programming is required. I think it's clear now (again). Thanks, Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Alex Williamson
> Sent: Saturday, April 4, 2020 1:50 AM
[...]
> > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > > index 9e843a1..298ac80 100644
> > > > --- a/include/uapi/linux/vfio.h
> > > > +++ b/include/uapi/linux/vfio.h
> > > > @@ -794,6 +794,47 @@ struct vfio_iommu_type1_dma_unmap {
> > > > #define VFIO_IOMMU_ENABLE _IO(VFIO_TYPE, VFIO_BASE + 15)
> > > > #define VFIO_IOMMU_DISABLE _IO(VFIO_TYPE, VFIO_BASE + 16)
> > > >
> > > > +/*
> > > > + * PASID (Process Address Space ID) is a PCIe concept which
> > > > + * has been extended to support DMA isolation in fine-grain.
> > > > + * With device assigned to user space (e.g. VMs), PASID alloc
> > > > + * and free need to be system wide. This structure defines
> > > > + * the info for pasid alloc/free between user space and kernel
> > > > + * space.
> > > > + *
> > > > + * @flag=VFIO_IOMMU_PASID_ALLOC, refer to the @alloc_pasid
> > > > + * @flag=VFIO_IOMMU_PASID_FREE, refer to @free_pasid
> > > > + */
> > > > +struct vfio_iommu_type1_pasid_request {
> > > > + __u32 argsz;
> > > > +#define VFIO_IOMMU_PASID_ALLOC (1 << 0)
> > > > +#define VFIO_IOMMU_PASID_FREE (1 << 1)
> > > > + __u32 flags;
> > > > + union {
> > > > + struct {
> > > > + __u32 min;
> > > > + __u32 max;
> > > > + __u32 result;
> > > > + } alloc_pasid;
> > > > + __u32 free_pasid;
> > > > + };
> > >
> > > We seem to be using __u8 data[] lately where the struct at data is
> > > defined by the flags. should we do that here?
> >
> > yeah, I can do that. BTW. Do you want to let the structure in the
> > lately patch share the same structure with this one? As I can foresee,
> > the two structures would look like similar as both of them include
> > argsz, flags and data[] fields. The difference is the definition of
> > flags. what about your opinion?
> >
> > struct vfio_iommu_type1_pasid_request {
> > __u32 argsz;
> > #define VFIO_IOMMU_PASID_ALLOC (1 << 0)
> > #define VFIO_IOMMU_PASID_FREE (1 << 1)
> > __u32 flags;
> > __u8data[];
> > };
> >
> > struct vfio_iommu_type1_bind {
> > __u32 argsz;
> > __u32 flags;
> > #define VFIO_IOMMU_BIND_GUEST_PGTBL (1 << 0)
> > #define VFIO_IOMMU_UNBIND_GUEST_PGTBL (1 << 1)
> > __u8data[];
> > };
>
>
> Yes, I was even wondering the same for the cache invalidate ioctl, or
> whether this is going too far for a general purpose "everything related
> to PASIDs" ioctl. We need to factor usability into the equation too.
> I'd be interested in opinions from others here too. Clearly I don't
> like single use, throw-away ioctls, but I can find myself on either
> side of the argument that allocation, binding, and invalidating are all
> within the domain of PASIDs and could fall within a single ioctl or
> they each represent different facets of managing PASIDs and should have
> separate ioctls. Thanks,
>
Looking at uapi/linux/iommu.h:
* Invalidations by %IOMMU_INV_GRANU_DOMAIN don't take any argument other than
* @version and @cache.
Although intel-iommu handles only PASID-related invalidation now, I
suppose other vendors (or future usages?) may allow non-pasid
based invalidation too based on above comment.
Thanks
Kevin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Alex Williamson > Sent: Friday, April 3, 2020 11:14 PM > > On Fri, 3 Apr 2020 05:58:55 + > "Tian, Kevin" wrote: > > > > From: Alex Williamson > > > Sent: Friday, April 3, 2020 1:50 AM > > > > > > On Sun, 22 Mar 2020 05:31:58 -0700 > > > "Liu, Yi L" wrote: > > > > > > > From: Liu Yi L > > > > > > > > For a long time, devices have only one DMA address space from > platform > > > > IOMMU's point of view. This is true for both bare metal and directed- > > > > access in virtualization environment. Reason is the source ID of DMA in > > > > PCIe are BDF (bus/dev/fnc ID), which results in only device granularity > > > > DMA isolation. However, this is changing with the latest advancement in > > > > I/O technology area. More and more platform vendors are utilizing the > > > PCIe > > > > PASID TLP prefix in DMA requests, thus to give devices with multiple > DMA > > > > address spaces as identified by their individual PASIDs. For example, > > > > Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able to > > > > let device access multiple process virtual address space by binding the > > > > virtual address space with a PASID. Wherein the PASID is allocated in > > > > software and programmed to device per device specific manner. > Devices > > > > which support PASID capability are called PASID-capable devices. If such > > > > devices are passed through to VMs, guest software are also able to bind > > > > guest process virtual address space on such devices. Therefore, the > guest > > > > software could reuse the bare metal software programming model, > which > > > > means guest software will also allocate PASID and program it to device > > > > directly. This is a dangerous situation since it has potential PASID > > > > conflicts and unauthorized address space access. It would be safer to > > > > let host intercept in the guest software's PASID allocation. Thus PASID > > > > are managed system-wide. > > > > > > Providing an allocation interface only allows for collaborative usage > > > of PASIDs though. Do we have any ability to enforce PASID usage or can > > > a user spoof other PASIDs on the same BDF? > > > > An user can access only PASIDs allocated to itself, i.e. the specific IOASID > > set tied to its mm_struct. > > A user is only _supposed_ to access PASIDs allocated to itself. AIUI > the mm_struct is used for managing the pool of IOASIDs from which the > user may allocate that PASID. We also state that programming the PASID > into the device is device specific. Therefore, are we simply trusting > the user to use a PASID that's been allocated to them when they program > the device? If a user can program an arbitrary PASID into the device, > then what prevents them from attempting to access data from another > user via the device? I think I've asked this question before, so if > there's a previous explanation or spec section I need to review, please > point me to it. Thanks, > There are two scenarios: (1) for PF/VF, the iommu driver maintains an individual PASID table per PDF. Although the PASID namespace is global, the per-BDF PASID table contains only valid entries for those PASIDs which are allocated to the mm_struct. The user is free to program arbitrary PASID into the assigned device, but using invalid PASIDs simply hit iommu fault. (2) for mdev, multiple mdev instances share the same PASID table of the parent BDF. However, PASID programming is a privileged operation in multiplexing usage, thus must be mediated by mdev device driver. The mediation logic will guarantee that only allocated PASIDs are forwarded to the device. Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
On Fri, 3 Apr 2020 13:12:50 + "Liu, Yi L" wrote: > Hi Alex, > > > From: Alex Williamson > > Sent: Friday, April 3, 2020 1:50 AM > > Subject: Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free) > > > > On Sun, 22 Mar 2020 05:31:58 -0700 > > "Liu, Yi L" wrote: > > > > > From: Liu Yi L > > > > > > For a long time, devices have only one DMA address space from platform > > > IOMMU's point of view. This is true for both bare metal and directed- > > > access in virtualization environment. Reason is the source ID of DMA in > > > PCIe are BDF (bus/dev/fnc ID), which results in only device granularity > > > DMA isolation. However, this is changing with the latest advancement in > > > I/O technology area. More and more platform vendors are utilizing the PCIe > > > PASID TLP prefix in DMA requests, thus to give devices with multiple DMA > > > address spaces as identified by their individual PASIDs. For example, > > > Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able to > > > let device access multiple process virtual address space by binding the > > > virtual address space with a PASID. Wherein the PASID is allocated in > > > software and programmed to device per device specific manner. Devices > > > which support PASID capability are called PASID-capable devices. If such > > > devices are passed through to VMs, guest software are also able to bind > > > guest process virtual address space on such devices. Therefore, the guest > > > software could reuse the bare metal software programming model, which > > > means guest software will also allocate PASID and program it to device > > > directly. This is a dangerous situation since it has potential PASID > > > conflicts and unauthorized address space access. It would be safer to > > > let host intercept in the guest software's PASID allocation. Thus PASID > > > are managed system-wide. > > > > Providing an allocation interface only allows for collaborative usage > > of PASIDs though. Do we have any ability to enforce PASID usage or can > > a user spoof other PASIDs on the same BDF? > > > > > This patch adds VFIO_IOMMU_PASID_REQUEST ioctl which aims to passdown > > > PASID allocation/free request from the virtual IOMMU. Additionally, such > > > requests are intended to be invoked by QEMU or other applications which > > > are running in userspace, it is necessary to have a mechanism to prevent > > > single application from abusing available PASIDs in system. With such > > > consideration, this patch tracks the VFIO PASID allocation per-VM. There > > > was a discussion to make quota to be per assigned devices. e.g. if a VM > > > has many assigned devices, then it should have more quota. However, it > > > is not sure how many PASIDs an assigned devices will use. e.g. it is > > > possible that a VM with multiples assigned devices but requests less > > > PASIDs. Therefore per-VM quota would be better. > > > > > > This patch uses struct mm pointer as a per-VM token. We also considered > > > using task structure pointer and vfio_iommu structure pointer. However, > > > task structure is per-thread, which means it cannot achieve per-VM PASID > > > alloc tracking purpose. While for vfio_iommu structure, it is visible > > > only within vfio. Therefore, structure mm pointer is selected. This patch > > > adds a structure vfio_mm. A vfio_mm is created when the first vfio > > > container is opened by a VM. On the reverse order, vfio_mm is free when > > > the last vfio container is released. Each VM is assigned with a PASID > > > quota, so that it is not able to request PASID beyond its quota. This > > > patch adds a default quota of 1000. This quota could be tuned by > > > administrator. Making PASID quota tunable will be added in another patch > > > in this series. > > > > > > Previous discussions: > > > https://patchwork.kernel.org/patch/11209429/ > > > > > > Cc: Kevin Tian > > > CC: Jacob Pan > > > Cc: Alex Williamson > > > Cc: Eric Auger > > > Cc: Jean-Philippe Brucker > > > Signed-off-by: Liu Yi L > > > Signed-off-by: Yi Sun > > > Signed-off-by: Jacob Pan > > > --- > > > drivers/vfio/vfio.c | 130 > > > > > > drivers/vfio/vfio_iommu_type1.c | 104 >
Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
On Fri, 3 Apr 2020 05:58:55 + "Tian, Kevin" wrote: > > From: Alex Williamson > > Sent: Friday, April 3, 2020 1:50 AM > > > > On Sun, 22 Mar 2020 05:31:58 -0700 > > "Liu, Yi L" wrote: > > > > > From: Liu Yi L > > > > > > For a long time, devices have only one DMA address space from platform > > > IOMMU's point of view. This is true for both bare metal and directed- > > > access in virtualization environment. Reason is the source ID of DMA in > > > PCIe are BDF (bus/dev/fnc ID), which results in only device granularity > > > DMA isolation. However, this is changing with the latest advancement in > > > I/O technology area. More and more platform vendors are utilizing the > > PCIe > > > PASID TLP prefix in DMA requests, thus to give devices with multiple DMA > > > address spaces as identified by their individual PASIDs. For example, > > > Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able to > > > let device access multiple process virtual address space by binding the > > > virtual address space with a PASID. Wherein the PASID is allocated in > > > software and programmed to device per device specific manner. Devices > > > which support PASID capability are called PASID-capable devices. If such > > > devices are passed through to VMs, guest software are also able to bind > > > guest process virtual address space on such devices. Therefore, the guest > > > software could reuse the bare metal software programming model, which > > > means guest software will also allocate PASID and program it to device > > > directly. This is a dangerous situation since it has potential PASID > > > conflicts and unauthorized address space access. It would be safer to > > > let host intercept in the guest software's PASID allocation. Thus PASID > > > are managed system-wide. > > > > Providing an allocation interface only allows for collaborative usage > > of PASIDs though. Do we have any ability to enforce PASID usage or can > > a user spoof other PASIDs on the same BDF? > > An user can access only PASIDs allocated to itself, i.e. the specific IOASID > set tied to its mm_struct. A user is only _supposed_ to access PASIDs allocated to itself. AIUI the mm_struct is used for managing the pool of IOASIDs from which the user may allocate that PASID. We also state that programming the PASID into the device is device specific. Therefore, are we simply trusting the user to use a PASID that's been allocated to them when they program the device? If a user can program an arbitrary PASID into the device, then what prevents them from attempting to access data from another user via the device? I think I've asked this question before, so if there's a previous explanation or spec section I need to review, please point me to it. Thanks, Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
Hi Alex, > From: Alex Williamson > Sent: Friday, April 3, 2020 1:50 AM > Subject: Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free) > > On Sun, 22 Mar 2020 05:31:58 -0700 > "Liu, Yi L" wrote: > > > From: Liu Yi L > > > > For a long time, devices have only one DMA address space from platform > > IOMMU's point of view. This is true for both bare metal and directed- > > access in virtualization environment. Reason is the source ID of DMA in > > PCIe are BDF (bus/dev/fnc ID), which results in only device granularity > > DMA isolation. However, this is changing with the latest advancement in > > I/O technology area. More and more platform vendors are utilizing the PCIe > > PASID TLP prefix in DMA requests, thus to give devices with multiple DMA > > address spaces as identified by their individual PASIDs. For example, > > Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able to > > let device access multiple process virtual address space by binding the > > virtual address space with a PASID. Wherein the PASID is allocated in > > software and programmed to device per device specific manner. Devices > > which support PASID capability are called PASID-capable devices. If such > > devices are passed through to VMs, guest software are also able to bind > > guest process virtual address space on such devices. Therefore, the guest > > software could reuse the bare metal software programming model, which > > means guest software will also allocate PASID and program it to device > > directly. This is a dangerous situation since it has potential PASID > > conflicts and unauthorized address space access. It would be safer to > > let host intercept in the guest software's PASID allocation. Thus PASID > > are managed system-wide. > > Providing an allocation interface only allows for collaborative usage > of PASIDs though. Do we have any ability to enforce PASID usage or can > a user spoof other PASIDs on the same BDF? > > > This patch adds VFIO_IOMMU_PASID_REQUEST ioctl which aims to passdown > > PASID allocation/free request from the virtual IOMMU. Additionally, such > > requests are intended to be invoked by QEMU or other applications which > > are running in userspace, it is necessary to have a mechanism to prevent > > single application from abusing available PASIDs in system. With such > > consideration, this patch tracks the VFIO PASID allocation per-VM. There > > was a discussion to make quota to be per assigned devices. e.g. if a VM > > has many assigned devices, then it should have more quota. However, it > > is not sure how many PASIDs an assigned devices will use. e.g. it is > > possible that a VM with multiples assigned devices but requests less > > PASIDs. Therefore per-VM quota would be better. > > > > This patch uses struct mm pointer as a per-VM token. We also considered > > using task structure pointer and vfio_iommu structure pointer. However, > > task structure is per-thread, which means it cannot achieve per-VM PASID > > alloc tracking purpose. While for vfio_iommu structure, it is visible > > only within vfio. Therefore, structure mm pointer is selected. This patch > > adds a structure vfio_mm. A vfio_mm is created when the first vfio > > container is opened by a VM. On the reverse order, vfio_mm is free when > > the last vfio container is released. Each VM is assigned with a PASID > > quota, so that it is not able to request PASID beyond its quota. This > > patch adds a default quota of 1000. This quota could be tuned by > > administrator. Making PASID quota tunable will be added in another patch > > in this series. > > > > Previous discussions: > > https://patchwork.kernel.org/patch/11209429/ > > > > Cc: Kevin Tian > > CC: Jacob Pan > > Cc: Alex Williamson > > Cc: Eric Auger > > Cc: Jean-Philippe Brucker > > Signed-off-by: Liu Yi L > > Signed-off-by: Yi Sun > > Signed-off-by: Jacob Pan > > --- > > drivers/vfio/vfio.c | 130 > > > > drivers/vfio/vfio_iommu_type1.c | 104 > > include/linux/vfio.h| 20 +++ > > include/uapi/linux/vfio.h | 41 + > > 4 files changed, 295 insertions(+) > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > index c848262..d13b483 100644 > > --- a/drivers/vfio/vfio.c > > +++ b/drivers/vfio/vfio.c > > @@ -32,6 +32,7 @@ > > #include > > #include > > #include > > +#include > > > > #
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Jean-Philippe Brucker
> Sent: Friday, April 3, 2020 8:40 PM
> Subject: Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
>
> On Fri, Apr 03, 2020 at 11:56:09AM +, Liu, Yi L wrote:
> > > > /**
> > > > + * VFIO_MM objects - create, release, get, put, search
> > > > + * Caller of the function should have held vfio.vfio_mm_lock.
> > > > + */
> > > > +static struct vfio_mm *vfio_create_mm(struct mm_struct *mm) {
> > > > + struct vfio_mm *vmm;
> > > > + struct vfio_mm_token *token;
> > > > + int ret = 0;
> > > > +
> > > > + vmm = kzalloc(sizeof(*vmm), GFP_KERNEL);
> > > > + if (!vmm)
> > > > + return ERR_PTR(-ENOMEM);
> > > > +
> > > > + /* Per mm IOASID set used for quota control and group
> > > > operations */
> > > > + ret = ioasid_alloc_set((struct ioasid_set *) mm,
> > >
> > > Hmm, either we need to change the token of ioasid_alloc_set() to
> > > "void *", or pass an actual ioasid_set struct, but this cast doesn't
> > > look good :)
> > >
> > > As I commented on the IOASID series, I think we could embed a struct
> > > ioasid_set into vfio_mm, pass that struct to all other ioasid_*
> > > functions, and get rid of ioasid_sid.
> >
> > I think change to "void *" is better as we needs the token to ensure
> > all threads within a single VM share the same ioasid_set.
>
> Don't they share the same vfio_mm?
that's right. then both works well for me.
Regards,
Yi Liu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
On Fri, Apr 03, 2020 at 11:56:09AM +, Liu, Yi L wrote:
> > > /**
> > > + * VFIO_MM objects - create, release, get, put, search
> > > + * Caller of the function should have held vfio.vfio_mm_lock.
> > > + */
> > > +static struct vfio_mm *vfio_create_mm(struct mm_struct *mm)
> > > +{
> > > + struct vfio_mm *vmm;
> > > + struct vfio_mm_token *token;
> > > + int ret = 0;
> > > +
> > > + vmm = kzalloc(sizeof(*vmm), GFP_KERNEL);
> > > + if (!vmm)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + /* Per mm IOASID set used for quota control and group operations */
> > > + ret = ioasid_alloc_set((struct ioasid_set *) mm,
> >
> > Hmm, either we need to change the token of ioasid_alloc_set() to "void *",
> > or pass an actual ioasid_set struct, but this cast doesn't look good :)
> >
> > As I commented on the IOASID series, I think we could embed a struct
> > ioasid_set into vfio_mm, pass that struct to all other ioasid_* functions,
> > and get rid of ioasid_sid.
>
> I think change to "void *" is better as we needs the token to ensure all
> threads within a single VM share the same ioasid_set.
Don't they share the same vfio_mm?
Thanks,
Jean
>
> > > +VFIO_DEFAULT_PASID_QUOTA, >ioasid_sid);
> > > + if (ret) {
> > > + kfree(vmm);
> > > + return ERR_PTR(ret);
> > > + }
> > > +
> > > + kref_init(>kref);
> > > + token = >token;
> > > + token->val = mm;
> >
> > Why the intermediate token struct? Could we just store the mm_struct
> > pointer within vfio_mm?
>
> Hmm, here we only want to use the pointer as a token, instead of using
> the structure behind the pointer. If store the mm_struct directly, may
> leave a space to further use its content, this is not good.
>
> Regards,
> Yi Liu
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
Hi Jean, > From: Jean-Philippe Brucker > Sent: Thursday, April 2, 2020 9:53 PM > To: Liu, Yi L > Subject: Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free) > > Hi Yi, > > On Sun, Mar 22, 2020 at 05:31:58AM -0700, Liu, Yi L wrote: > > From: Liu Yi L > > > > For a long time, devices have only one DMA address space from platform > > IOMMU's point of view. This is true for both bare metal and directed- > > access in virtualization environment. Reason is the source ID of DMA in > > PCIe are BDF (bus/dev/fnc ID), which results in only device granularity > > DMA isolation. However, this is changing with the latest advancement in > > I/O technology area. More and more platform vendors are utilizing the PCIe > > PASID TLP prefix in DMA requests, thus to give devices with multiple DMA > > address spaces as identified by their individual PASIDs. For example, > > Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able to > > let device access multiple process virtual address space by binding the > > virtual address space with a PASID. Wherein the PASID is allocated in > > software and programmed to device per device specific manner. Devices > > which support PASID capability are called PASID-capable devices. If such > > devices are passed through to VMs, guest software are also able to bind > > guest process virtual address space on such devices. Therefore, the guest > > software could reuse the bare metal software programming model, which > > means guest software will also allocate PASID and program it to device > > directly. This is a dangerous situation since it has potential PASID > > conflicts and unauthorized address space access. > > It's worth noting that this applies to Intel VT-d with scalable mode, not > IOMMUs that use one PASID space per VM Oh yes. will add it. > > > It would be safer to > > let host intercept in the guest software's PASID allocation. Thus PASID > > are managed system-wide. > > > > This patch adds VFIO_IOMMU_PASID_REQUEST ioctl which aims to passdown > > PASID allocation/free request from the virtual IOMMU. Additionally, such > > requests are intended to be invoked by QEMU or other applications which > > are running in userspace, it is necessary to have a mechanism to prevent > > single application from abusing available PASIDs in system. With such > > consideration, this patch tracks the VFIO PASID allocation per-VM. There > > was a discussion to make quota to be per assigned devices. e.g. if a VM > > has many assigned devices, then it should have more quota. However, it > > is not sure how many PASIDs an assigned devices will use. e.g. it is > > possible that a VM with multiples assigned devices but requests less > > PASIDs. Therefore per-VM quota would be better. > > > > This patch uses struct mm pointer as a per-VM token. We also considered > > using task structure pointer and vfio_iommu structure pointer. However, > > task structure is per-thread, which means it cannot achieve per-VM PASID > > alloc tracking purpose. While for vfio_iommu structure, it is visible > > only within vfio. Therefore, structure mm pointer is selected. This patch > > adds a structure vfio_mm. A vfio_mm is created when the first vfio > > container is opened by a VM. On the reverse order, vfio_mm is free when > > the last vfio container is released. Each VM is assigned with a PASID > > quota, so that it is not able to request PASID beyond its quota. This > > patch adds a default quota of 1000. This quota could be tuned by > > administrator. Making PASID quota tunable will be added in another patch > > in this series. > > > > Previous discussions: > > https://patchwork.kernel.org/patch/11209429/ > > > > Cc: Kevin Tian > > CC: Jacob Pan > > Cc: Alex Williamson > > Cc: Eric Auger > > Cc: Jean-Philippe Brucker > > Signed-off-by: Liu Yi L > > Signed-off-by: Yi Sun > > Signed-off-by: Jacob Pan > > --- > > drivers/vfio/vfio.c | 130 > > > > drivers/vfio/vfio_iommu_type1.c | 104 > > include/linux/vfio.h| 20 +++ > > include/uapi/linux/vfio.h | 41 + > > 4 files changed, 295 insertions(+) > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > index c848262..d13b483 100644 > > --- a/drivers/vfio/vfio.c > > +++ b/drivers/vfio/vfio.c > > @@ -32,6 +32,7 @@ > > #include > > #include > > #include > > +#include > > > > #defin
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Alex Williamson
> Sent: Friday, April 3, 2020 1:50 AM
>
> On Sun, 22 Mar 2020 05:31:58 -0700
> "Liu, Yi L" wrote:
>
> > From: Liu Yi L
> >
> > For a long time, devices have only one DMA address space from platform
> > IOMMU's point of view. This is true for both bare metal and directed-
> > access in virtualization environment. Reason is the source ID of DMA in
> > PCIe are BDF (bus/dev/fnc ID), which results in only device granularity
> > DMA isolation. However, this is changing with the latest advancement in
> > I/O technology area. More and more platform vendors are utilizing the
> PCIe
> > PASID TLP prefix in DMA requests, thus to give devices with multiple DMA
> > address spaces as identified by their individual PASIDs. For example,
> > Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able to
> > let device access multiple process virtual address space by binding the
> > virtual address space with a PASID. Wherein the PASID is allocated in
> > software and programmed to device per device specific manner. Devices
> > which support PASID capability are called PASID-capable devices. If such
> > devices are passed through to VMs, guest software are also able to bind
> > guest process virtual address space on such devices. Therefore, the guest
> > software could reuse the bare metal software programming model, which
> > means guest software will also allocate PASID and program it to device
> > directly. This is a dangerous situation since it has potential PASID
> > conflicts and unauthorized address space access. It would be safer to
> > let host intercept in the guest software's PASID allocation. Thus PASID
> > are managed system-wide.
>
> Providing an allocation interface only allows for collaborative usage
> of PASIDs though. Do we have any ability to enforce PASID usage or can
> a user spoof other PASIDs on the same BDF?
An user can access only PASIDs allocated to itself, i.e. the specific IOASID
set tied to its mm_struct.
Thanks
Kevin
>
> > This patch adds VFIO_IOMMU_PASID_REQUEST ioctl which aims to
> passdown
> > PASID allocation/free request from the virtual IOMMU. Additionally, such
> > requests are intended to be invoked by QEMU or other applications which
> > are running in userspace, it is necessary to have a mechanism to prevent
> > single application from abusing available PASIDs in system. With such
> > consideration, this patch tracks the VFIO PASID allocation per-VM. There
> > was a discussion to make quota to be per assigned devices. e.g. if a VM
> > has many assigned devices, then it should have more quota. However, it
> > is not sure how many PASIDs an assigned devices will use. e.g. it is
> > possible that a VM with multiples assigned devices but requests less
> > PASIDs. Therefore per-VM quota would be better.
> >
> > This patch uses struct mm pointer as a per-VM token. We also considered
> > using task structure pointer and vfio_iommu structure pointer. However,
> > task structure is per-thread, which means it cannot achieve per-VM PASID
> > alloc tracking purpose. While for vfio_iommu structure, it is visible
> > only within vfio. Therefore, structure mm pointer is selected. This patch
> > adds a structure vfio_mm. A vfio_mm is created when the first vfio
> > container is opened by a VM. On the reverse order, vfio_mm is free when
> > the last vfio container is released. Each VM is assigned with a PASID
> > quota, so that it is not able to request PASID beyond its quota. This
> > patch adds a default quota of 1000. This quota could be tuned by
> > administrator. Making PASID quota tunable will be added in another patch
> > in this series.
> >
> > Previous discussions:
> > https://patchwork.kernel.org/patch/11209429/
> >
> > Cc: Kevin Tian
> > CC: Jacob Pan
> > Cc: Alex Williamson
> > Cc: Eric Auger
> > Cc: Jean-Philippe Brucker
> > Signed-off-by: Liu Yi L
> > Signed-off-by: Yi Sun
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/vfio/vfio.c | 130
>
> > drivers/vfio/vfio_iommu_type1.c | 104
>
> > include/linux/vfio.h| 20 +++
> > include/uapi/linux/vfio.h | 41 +
> > 4 files changed, 295 insertions(+)
> >
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index c848262..d13b483 100644
> > --- a/drivers/vfio/vfio.c
> > +++ b/drivers/vfio/vfio.c
> > @@ -32,6 +32,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #define DRIVER_VERSION "0.3"
> > #define DRIVER_AUTHOR "Alex Williamson
> "
> > @@ -46,6 +47,8 @@ static struct vfio {
> > struct mutexgroup_lock;
> > struct cdev group_cdev;
> > dev_t group_devt;
> > + struct list_headvfio_mm_list;
> > + struct mutexvfio_mm_lock;
> > wait_queue_head_t release_q;
> > } vfio;
> >
> > @@ -2129,6
Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
On Sun, 22 Mar 2020 05:31:58 -0700
"Liu, Yi L" wrote:
> From: Liu Yi L
>
> For a long time, devices have only one DMA address space from platform
> IOMMU's point of view. This is true for both bare metal and directed-
> access in virtualization environment. Reason is the source ID of DMA in
> PCIe are BDF (bus/dev/fnc ID), which results in only device granularity
> DMA isolation. However, this is changing with the latest advancement in
> I/O technology area. More and more platform vendors are utilizing the PCIe
> PASID TLP prefix in DMA requests, thus to give devices with multiple DMA
> address spaces as identified by their individual PASIDs. For example,
> Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able to
> let device access multiple process virtual address space by binding the
> virtual address space with a PASID. Wherein the PASID is allocated in
> software and programmed to device per device specific manner. Devices
> which support PASID capability are called PASID-capable devices. If such
> devices are passed through to VMs, guest software are also able to bind
> guest process virtual address space on such devices. Therefore, the guest
> software could reuse the bare metal software programming model, which
> means guest software will also allocate PASID and program it to device
> directly. This is a dangerous situation since it has potential PASID
> conflicts and unauthorized address space access. It would be safer to
> let host intercept in the guest software's PASID allocation. Thus PASID
> are managed system-wide.
Providing an allocation interface only allows for collaborative usage
of PASIDs though. Do we have any ability to enforce PASID usage or can
a user spoof other PASIDs on the same BDF?
> This patch adds VFIO_IOMMU_PASID_REQUEST ioctl which aims to passdown
> PASID allocation/free request from the virtual IOMMU. Additionally, such
> requests are intended to be invoked by QEMU or other applications which
> are running in userspace, it is necessary to have a mechanism to prevent
> single application from abusing available PASIDs in system. With such
> consideration, this patch tracks the VFIO PASID allocation per-VM. There
> was a discussion to make quota to be per assigned devices. e.g. if a VM
> has many assigned devices, then it should have more quota. However, it
> is not sure how many PASIDs an assigned devices will use. e.g. it is
> possible that a VM with multiples assigned devices but requests less
> PASIDs. Therefore per-VM quota would be better.
>
> This patch uses struct mm pointer as a per-VM token. We also considered
> using task structure pointer and vfio_iommu structure pointer. However,
> task structure is per-thread, which means it cannot achieve per-VM PASID
> alloc tracking purpose. While for vfio_iommu structure, it is visible
> only within vfio. Therefore, structure mm pointer is selected. This patch
> adds a structure vfio_mm. A vfio_mm is created when the first vfio
> container is opened by a VM. On the reverse order, vfio_mm is free when
> the last vfio container is released. Each VM is assigned with a PASID
> quota, so that it is not able to request PASID beyond its quota. This
> patch adds a default quota of 1000. This quota could be tuned by
> administrator. Making PASID quota tunable will be added in another patch
> in this series.
>
> Previous discussions:
> https://patchwork.kernel.org/patch/11209429/
>
> Cc: Kevin Tian
> CC: Jacob Pan
> Cc: Alex Williamson
> Cc: Eric Auger
> Cc: Jean-Philippe Brucker
> Signed-off-by: Liu Yi L
> Signed-off-by: Yi Sun
> Signed-off-by: Jacob Pan
> ---
> drivers/vfio/vfio.c | 130
>
> drivers/vfio/vfio_iommu_type1.c | 104
> include/linux/vfio.h| 20 +++
> include/uapi/linux/vfio.h | 41 +
> 4 files changed, 295 insertions(+)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index c848262..d13b483 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
>
> #define DRIVER_VERSION "0.3"
> #define DRIVER_AUTHOR"Alex Williamson "
> @@ -46,6 +47,8 @@ static struct vfio {
> struct mutexgroup_lock;
> struct cdev group_cdev;
> dev_t group_devt;
> + struct list_headvfio_mm_list;
> + struct mutexvfio_mm_lock;
> wait_queue_head_t release_q;
> } vfio;
>
> @@ -2129,6 +2132,131 @@ int vfio_unregister_notifier(struct device *dev, enum
> vfio_notify_type type,
> EXPORT_SYMBOL(vfio_unregister_notifier);
>
> /**
> + * VFIO_MM objects - create, release, get, put, search
> + * Caller of the function should have held vfio.vfio_mm_lock.
> + */
> +static struct vfio_mm *vfio_create_mm(struct mm_struct *mm)
> +{
> + struct vfio_mm
Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
Hi Yi,
On Sun, Mar 22, 2020 at 05:31:58AM -0700, Liu, Yi L wrote:
> From: Liu Yi L
>
> For a long time, devices have only one DMA address space from platform
> IOMMU's point of view. This is true for both bare metal and directed-
> access in virtualization environment. Reason is the source ID of DMA in
> PCIe are BDF (bus/dev/fnc ID), which results in only device granularity
> DMA isolation. However, this is changing with the latest advancement in
> I/O technology area. More and more platform vendors are utilizing the PCIe
> PASID TLP prefix in DMA requests, thus to give devices with multiple DMA
> address spaces as identified by their individual PASIDs. For example,
> Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able to
> let device access multiple process virtual address space by binding the
> virtual address space with a PASID. Wherein the PASID is allocated in
> software and programmed to device per device specific manner. Devices
> which support PASID capability are called PASID-capable devices. If such
> devices are passed through to VMs, guest software are also able to bind
> guest process virtual address space on such devices. Therefore, the guest
> software could reuse the bare metal software programming model, which
> means guest software will also allocate PASID and program it to device
> directly. This is a dangerous situation since it has potential PASID
> conflicts and unauthorized address space access.
It's worth noting that this applies to Intel VT-d with scalable mode, not
IOMMUs that use one PASID space per VM
> It would be safer to
> let host intercept in the guest software's PASID allocation. Thus PASID
> are managed system-wide.
>
> This patch adds VFIO_IOMMU_PASID_REQUEST ioctl which aims to passdown
> PASID allocation/free request from the virtual IOMMU. Additionally, such
> requests are intended to be invoked by QEMU or other applications which
> are running in userspace, it is necessary to have a mechanism to prevent
> single application from abusing available PASIDs in system. With such
> consideration, this patch tracks the VFIO PASID allocation per-VM. There
> was a discussion to make quota to be per assigned devices. e.g. if a VM
> has many assigned devices, then it should have more quota. However, it
> is not sure how many PASIDs an assigned devices will use. e.g. it is
> possible that a VM with multiples assigned devices but requests less
> PASIDs. Therefore per-VM quota would be better.
>
> This patch uses struct mm pointer as a per-VM token. We also considered
> using task structure pointer and vfio_iommu structure pointer. However,
> task structure is per-thread, which means it cannot achieve per-VM PASID
> alloc tracking purpose. While for vfio_iommu structure, it is visible
> only within vfio. Therefore, structure mm pointer is selected. This patch
> adds a structure vfio_mm. A vfio_mm is created when the first vfio
> container is opened by a VM. On the reverse order, vfio_mm is free when
> the last vfio container is released. Each VM is assigned with a PASID
> quota, so that it is not able to request PASID beyond its quota. This
> patch adds a default quota of 1000. This quota could be tuned by
> administrator. Making PASID quota tunable will be added in another patch
> in this series.
>
> Previous discussions:
> https://patchwork.kernel.org/patch/11209429/
>
> Cc: Kevin Tian
> CC: Jacob Pan
> Cc: Alex Williamson
> Cc: Eric Auger
> Cc: Jean-Philippe Brucker
> Signed-off-by: Liu Yi L
> Signed-off-by: Yi Sun
> Signed-off-by: Jacob Pan
> ---
> drivers/vfio/vfio.c | 130
>
> drivers/vfio/vfio_iommu_type1.c | 104
> include/linux/vfio.h| 20 +++
> include/uapi/linux/vfio.h | 41 +
> 4 files changed, 295 insertions(+)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index c848262..d13b483 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
>
> #define DRIVER_VERSION "0.3"
> #define DRIVER_AUTHOR"Alex Williamson "
> @@ -46,6 +47,8 @@ static struct vfio {
> struct mutexgroup_lock;
> struct cdev group_cdev;
> dev_t group_devt;
> + struct list_headvfio_mm_list;
> + struct mutexvfio_mm_lock;
> wait_queue_head_t release_q;
> } vfio;
>
> @@ -2129,6 +2132,131 @@ int vfio_unregister_notifier(struct device *dev, enum
> vfio_notify_type type,
> EXPORT_SYMBOL(vfio_unregister_notifier);
>
> /**
> + * VFIO_MM objects - create, release, get, put, search
> + * Caller of the function should have held vfio.vfio_mm_lock.
> + */
> +static struct vfio_mm *vfio_create_mm(struct mm_struct *mm)
> +{
> + struct vfio_mm *vmm;
> + struct vfio_mm_token *token;
> + int
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Tian, Kevin
> Sent: Wednesday, April 1, 2020 1:43 PM
> To: Liu, Yi L ; alex.william...@redhat.com;
> Subject: RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
>
> > From: Liu, Yi L
> > Sent: Tuesday, March 31, 2020 9:22 PM
> >
> > > From: Tian, Kevin
> > > Sent: Tuesday, March 31, 2020 1:41 PM
> > > To: Liu, Yi L ; alex.william...@redhat.com;
> > > eric.au...@redhat.com
> > > Subject: RE: [PATCH v1 1/8] vfio: Add
> > VFIO_IOMMU_PASID_REQUEST(alloc/free)
> > >
> > > > From: Liu, Yi L
> > > > Sent: Monday, March 30, 2020 10:37 PM
> > > >
> > > > > From: Tian, Kevin
> > > > > Sent: Monday, March 30, 2020 4:32 PM
> > > > > To: Liu, Yi L ; alex.william...@redhat.com;
> > > > > Subject: RE: [PATCH v1 1/8] vfio: Add
> > > > VFIO_IOMMU_PASID_REQUEST(alloc/free)
> > > > >
> > > > > > From: Liu, Yi L
> > > > > > Sent: Sunday, March 22, 2020 8:32 PM
> > > > > >
> > > > > > From: Liu Yi L
> > > > > >
> > > > > > For a long time, devices have only one DMA address space from
> > > > > > platform IOMMU's point of view. This is true for both bare
> > > > > > metal and directed- access in virtualization environment.
> > > > > > Reason is the source ID of DMA in PCIe are BDF (bus/dev/fnc
> > > > > > ID), which results in only device granularity
[...]
> > >
> > > >
> > > > > > +
> > > > > > + switch (req.flags & VFIO_PASID_REQUEST_MASK) {
> > > > > > + case VFIO_IOMMU_PASID_ALLOC:
> > > > > > + {
> > > > > > + int ret = 0, result;
> > > > > > +
> > > > > > + result =
> > vfio_iommu_type1_pasid_alloc(iommu,
> > > > > > +
> > req.alloc_pasid.min,
> > > > > > +
> > req.alloc_pasid.max);
> > > > > > + if (result > 0) {
> > > > > > + offset = offsetof(
> > > > > > + struct
> > > > > > vfio_iommu_type1_pasid_request,
> > > > > > + alloc_pasid.result);
> > > > > > + ret = copy_to_user(
> > > > > > + (void __user *) (arg +
> > offset),
> > > > > > + , sizeof(result));
> > > > > > + } else {
> > > > > > + pr_debug("%s: PASID alloc failed\n",
> > > > > > __func__);
> > > > > > + ret = -EFAULT;
> > > > >
> > > > > no, this branch is not for copy_to_user error. it is about pasid
> > > > > alloc failure. you should handle both.
> > > >
> > > > Emmm, I just want to fail the IOCTL in such case, so the @result
> > > > field is meaningless in the user side. How about using another
> > > > return value (e.g. ENOSPC) to indicate the IOCTL failure?
> > >
> > > If pasid_alloc fails, you return its result to userspace if
> > > copy_to_user fails, then return -EFAULT.
> > >
> > > however, above you return -EFAULT for pasid_alloc failure, and then
> > > the number of not-copied bytes for copy_to_user.
> >
> > not quite get. Let me re-paste the code. :-)
> >
> > + case VFIO_IOMMU_PASID_ALLOC:
> > + {
> > + int ret = 0, result;
> > +
> > + result = vfio_iommu_type1_pasid_alloc(iommu,
> > + req.alloc_pasid.min,
> > + req.alloc_pasid.max);
> > + if (result > 0) {
> > + offset = offsetof(
> > + struct
> > vfio_iommu_type1_pasid_request,
> > + alloc_pasid.result);
> > + ret = copy_to_user(
> > + (void __user *) (arg + offset),
> > + , sizeof(result));
> > if copy_to_user failed, ret is the number of uncopied bytes and will
> > be returned to userspace to indicate failure. userspace will not use
> > the data in result field. perhaps, I should check the ret here and
> > return -EFAULT or another errno, instead of return the number of
> > un-copied bytes.
>
> here should return -EFAULT.
got it. so if any failure, the return value of this ioctl is a -ERROR_VAL.
>
> > + } else {
> > + pr_debug("%s: PASID alloc failed\n",
> > __func__);
> > + ret = -EFAULT;
> > if vfio_iommu_type1_pasid_alloc() failed, no doubt, return -EFAULT to
> > userspace to indicate failure.
>
> pasid_alloc has its own error types returned. why blindly replace it with
> -EFAULT?
right, should use its own error types.
Thanks,
Yi Liu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Liu, Yi L > Sent: Tuesday, March 31, 2020 9:22 PM > > > From: Tian, Kevin > > Sent: Tuesday, March 31, 2020 1:41 PM > > To: Liu, Yi L ; alex.william...@redhat.com; > > eric.au...@redhat.com > > Subject: RE: [PATCH v1 1/8] vfio: Add > VFIO_IOMMU_PASID_REQUEST(alloc/free) > > > > > From: Liu, Yi L > > > Sent: Monday, March 30, 2020 10:37 PM > > > > > > > From: Tian, Kevin > > > > Sent: Monday, March 30, 2020 4:32 PM > > > > To: Liu, Yi L ; alex.william...@redhat.com; > > > > Subject: RE: [PATCH v1 1/8] vfio: Add > > > VFIO_IOMMU_PASID_REQUEST(alloc/free) > > > > > > > > > From: Liu, Yi L > > > > > Sent: Sunday, March 22, 2020 8:32 PM > > > > > > > > > > From: Liu Yi L > > > > > > > > > > For a long time, devices have only one DMA address space from > > > > > platform IOMMU's point of view. This is true for both bare metal > > > > > and directed- access in virtualization environment. Reason is the > > > > > source ID of DMA in PCIe are BDF (bus/dev/fnc ID), which results > > > > > in only device granularity > > > > > > > > are->is > > > > > > thanks. > > > > > > > > DMA isolation. However, this is changing with the latest > > > > > advancement in I/O technology area. More and more platform > vendors > > > > > are utilizing the > > > PCIe > > > > > PASID TLP prefix in DMA requests, thus to give devices with > > > > > multiple DMA address spaces as identified by their individual > > > > > PASIDs. For example, Shared Virtual Addressing (SVA, a.k.a Shared > > > > > Virtual Memory) is able to let device access multiple process > > > > > virtual address space by binding the > > > > > > > > "address space" -> "address spaces" > > > > > > > > "binding the" -> "binding each" > > > > > > will correct both. > > > > > > > > virtual address space with a PASID. Wherein the PASID is allocated > > > > > in software and programmed to device per device specific manner. > > > > > Devices which support PASID capability are called PASID-capable > > > > > devices. If such devices are passed through to VMs, guest software > > > > > are also able to bind guest process virtual address space on such > > > > > devices. Therefore, the guest software could reuse the bare metal > > > > > software programming model, > > > which > > > > > means guest software will also allocate PASID and program it to > > > > > device directly. This is a dangerous situation since it has > > > > > potential PASID conflicts and unauthorized address space access. > > > > > It would be safer to let host intercept in the guest software's > > > > > PASID allocation. Thus PASID are managed system-wide. > > > > > > > > > > This patch adds VFIO_IOMMU_PASID_REQUEST ioctl which aims to > > > > > passdown PASID allocation/free request from the virtual IOMMU. > > > > > Additionally, such > > > > > > > > "Additionally, because such" > > > > > > > > > requests are intended to be invoked by QEMU or other applications > > > which > > > > > > > > simplify to "intended to be invoked from userspace" > > > > > > got it. > > > > > > > > are running in userspace, it is necessary to have a mechanism to > > > > > prevent single application from abusing available PASIDs in > > > > > system. With such consideration, this patch tracks the VFIO PASID > > > > > allocation per-VM. There was a discussion to make quota to be per > > > > > assigned devices. e.g. if a VM has many assigned devices, then it > > > > > should have more quota. However, it is not sure how many PASIDs an > > > > > assigned devices will use. e.g. it is > > > > > > > > devices -> device > > > > > > got it. > > > > > > > > possible that a VM with multiples assigned devices but requests > > > > > less PASIDs. Therefore per-VM quota would be better. > > > > > > > > > > This patch uses struct mm pointer as a per-VM to
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Tian, Kevin > Sent: Tuesday, March 31, 2020 1:41 PM > To: Liu, Yi L ; alex.william...@redhat.com; > eric.au...@redhat.com > Subject: RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free) > > > From: Liu, Yi L > > Sent: Monday, March 30, 2020 10:37 PM > > > > > From: Tian, Kevin > > > Sent: Monday, March 30, 2020 4:32 PM > > > To: Liu, Yi L ; alex.william...@redhat.com; > > > Subject: RE: [PATCH v1 1/8] vfio: Add > > VFIO_IOMMU_PASID_REQUEST(alloc/free) > > > > > > > From: Liu, Yi L > > > > Sent: Sunday, March 22, 2020 8:32 PM > > > > > > > > From: Liu Yi L > > > > > > > > For a long time, devices have only one DMA address space from > > > > platform IOMMU's point of view. This is true for both bare metal > > > > and directed- access in virtualization environment. Reason is the > > > > source ID of DMA in PCIe are BDF (bus/dev/fnc ID), which results > > > > in only device granularity > > > > > > are->is > > > > thanks. > > > > > > DMA isolation. However, this is changing with the latest > > > > advancement in I/O technology area. More and more platform vendors > > > > are utilizing the > > PCIe > > > > PASID TLP prefix in DMA requests, thus to give devices with > > > > multiple DMA address spaces as identified by their individual > > > > PASIDs. For example, Shared Virtual Addressing (SVA, a.k.a Shared > > > > Virtual Memory) is able to let device access multiple process > > > > virtual address space by binding the > > > > > > "address space" -> "address spaces" > > > > > > "binding the" -> "binding each" > > > > will correct both. > > > > > > virtual address space with a PASID. Wherein the PASID is allocated > > > > in software and programmed to device per device specific manner. > > > > Devices which support PASID capability are called PASID-capable > > > > devices. If such devices are passed through to VMs, guest software > > > > are also able to bind guest process virtual address space on such > > > > devices. Therefore, the guest software could reuse the bare metal > > > > software programming model, > > which > > > > means guest software will also allocate PASID and program it to > > > > device directly. This is a dangerous situation since it has > > > > potential PASID conflicts and unauthorized address space access. > > > > It would be safer to let host intercept in the guest software's > > > > PASID allocation. Thus PASID are managed system-wide. > > > > > > > > This patch adds VFIO_IOMMU_PASID_REQUEST ioctl which aims to > > > > passdown PASID allocation/free request from the virtual IOMMU. > > > > Additionally, such > > > > > > "Additionally, because such" > > > > > > > requests are intended to be invoked by QEMU or other applications > > which > > > > > > simplify to "intended to be invoked from userspace" > > > > got it. > > > > > > are running in userspace, it is necessary to have a mechanism to > > > > prevent single application from abusing available PASIDs in > > > > system. With such consideration, this patch tracks the VFIO PASID > > > > allocation per-VM. There was a discussion to make quota to be per > > > > assigned devices. e.g. if a VM has many assigned devices, then it > > > > should have more quota. However, it is not sure how many PASIDs an > > > > assigned devices will use. e.g. it is > > > > > > devices -> device > > > > got it. > > > > > > possible that a VM with multiples assigned devices but requests > > > > less PASIDs. Therefore per-VM quota would be better. > > > > > > > > This patch uses struct mm pointer as a per-VM token. We also > > > > considered using task structure pointer and vfio_iommu structure > > > > pointer. However, task structure is per-thread, which means it > > > > cannot achieve per-VM PASID alloc tracking purpose. While for > > > > vfio_iommu structure, it is visible only within vfio. Therefore, > > > > structure mm pointer is selected. This patch adds a structure > > > > vfio_mm. A vfio_mm is created when the first vfio container is > > > > op
Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
On Tue, Mar 31, 2020 at 08:36:32AM +, Liu, Yi L wrote: > > From: Liu, Yi L > > Sent: Tuesday, March 31, 2020 4:33 PM > > To: 'Christoph Hellwig' > > Subject: RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free) > > > > > From: Christoph Hellwig > > > Sent: Tuesday, March 31, 2020 3:54 PM > > > To: Liu, Yi L > > > Subject: Re: [PATCH v1 1/8] vfio: Add > > > VFIO_IOMMU_PASID_REQUEST(alloc/free) > > > > > > Who is going to use thse exports? Please submit them together with a > > > driver actually using them. > the user of the symbols are already in this patch. sorry for the split > answer.. Thanks, sorry for the noise! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Liu, Yi L > Sent: Tuesday, March 31, 2020 4:33 PM > To: 'Christoph Hellwig' > Subject: RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free) > > > From: Christoph Hellwig > > Sent: Tuesday, March 31, 2020 3:54 PM > > To: Liu, Yi L > > Subject: Re: [PATCH v1 1/8] vfio: Add > > VFIO_IOMMU_PASID_REQUEST(alloc/free) > > > > Who is going to use thse exports? Please submit them together with a > > driver actually using them. the user of the symbols are already in this patch. sorry for the split answer.. Regards, Yi Liu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Christoph Hellwig > Sent: Tuesday, March 31, 2020 3:54 PM > To: Liu, Yi L > Subject: Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free) > > Who is going to use thse exports? Please submit them together with > a driver actually using them. Hi Hellwig, Sorry, maybe I misunderstood your point. Do you mean the exported symbol below? They are used by the vfio_iommu_type1 driver which is a separate driver besides the vfio.ko driver. +EXPORT_SYMBOL_GPL(vfio_mm_put); +EXPORT_SYMBOL_GPL(vfio_mm_get_from_task); +EXPORT_SYMBOL_GPL(vfio_mm_pasid_alloc); +EXPORT_SYMBOL_GPL(vfio_mm_pasid_free); Regards, Yi Liu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Christoph Hellwig > Sent: Tuesday, March 31, 2020 3:54 PM > To: Liu, Yi L > Subject: Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free) > > Who is going to use thse exports? Please submit them together with > a driver actually using them. Hi Hellwig, These are exposed for SVA (Shared Virtual Addressing) usage in VMs. If say a driver who actually using them, it is the iommu driver running in guest. The flow is: guest iommu driver programs the virtual command interface and it traps to host. The virtual IOMMU device model lays in QEMU will utilize the exported ioctl to get PASIDs. Here is iommu kernel driver patch which utilizes virtual command interface to request pasid alloc/free. https://lkml.org/lkml/2020/3/20/1176 And, the below patch is one which utilizes the ioctl exported in this patch: https://patchwork.kernel.org/patch/11464601/ Regards, Yi Liu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
Who is going to use thse exports? Please submit them together with a driver actually using them. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Liu, Yi L > Sent: Monday, March 30, 2020 10:37 PM > > > From: Tian, Kevin > > Sent: Monday, March 30, 2020 4:32 PM > > To: Liu, Yi L ; alex.william...@redhat.com; > > Subject: RE: [PATCH v1 1/8] vfio: Add > VFIO_IOMMU_PASID_REQUEST(alloc/free) > > > > > From: Liu, Yi L > > > Sent: Sunday, March 22, 2020 8:32 PM > > > > > > From: Liu Yi L > > > > > > For a long time, devices have only one DMA address space from platform > > > IOMMU's point of view. This is true for both bare metal and directed- > > > access in virtualization environment. Reason is the source ID of DMA in > > > PCIe are BDF (bus/dev/fnc ID), which results in only device granularity > > > > are->is > > thanks. > > > > DMA isolation. However, this is changing with the latest advancement in > > > I/O technology area. More and more platform vendors are utilizing the > PCIe > > > PASID TLP prefix in DMA requests, thus to give devices with multiple DMA > > > address spaces as identified by their individual PASIDs. For example, > > > Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able to > > > let device access multiple process virtual address space by binding the > > > > "address space" -> "address spaces" > > > > "binding the" -> "binding each" > > will correct both. > > > > virtual address space with a PASID. Wherein the PASID is allocated in > > > software and programmed to device per device specific manner. Devices > > > which support PASID capability are called PASID-capable devices. If such > > > devices are passed through to VMs, guest software are also able to bind > > > guest process virtual address space on such devices. Therefore, the guest > > > software could reuse the bare metal software programming model, > which > > > means guest software will also allocate PASID and program it to device > > > directly. This is a dangerous situation since it has potential PASID > > > conflicts and unauthorized address space access. It would be safer to > > > let host intercept in the guest software's PASID allocation. Thus PASID > > > are managed system-wide. > > > > > > This patch adds VFIO_IOMMU_PASID_REQUEST ioctl which aims to > > > passdown > > > PASID allocation/free request from the virtual IOMMU. Additionally, such > > > > "Additionally, because such" > > > > > requests are intended to be invoked by QEMU or other applications > which > > > > simplify to "intended to be invoked from userspace" > > got it. > > > > are running in userspace, it is necessary to have a mechanism to prevent > > > single application from abusing available PASIDs in system. With such > > > consideration, this patch tracks the VFIO PASID allocation per-VM. There > > > was a discussion to make quota to be per assigned devices. e.g. if a VM > > > has many assigned devices, then it should have more quota. However, it > > > is not sure how many PASIDs an assigned devices will use. e.g. it is > > > > devices -> device > > got it. > > > > possible that a VM with multiples assigned devices but requests less > > > PASIDs. Therefore per-VM quota would be better. > > > > > > This patch uses struct mm pointer as a per-VM token. We also considered > > > using task structure pointer and vfio_iommu structure pointer. However, > > > task structure is per-thread, which means it cannot achieve per-VM PASID > > > alloc tracking purpose. While for vfio_iommu structure, it is visible > > > only within vfio. Therefore, structure mm pointer is selected. This patch > > > adds a structure vfio_mm. A vfio_mm is created when the first vfio > > > container is opened by a VM. On the reverse order, vfio_mm is free when > > > the last vfio container is released. Each VM is assigned with a PASID > > > quota, so that it is not able to request PASID beyond its quota. This > > > patch adds a default quota of 1000. This quota could be tuned by > > > administrator. Making PASID quota tunable will be added in another > patch > > > in this series. > > > > > > Previous discussions: > > > https://patchwork.kernel.org/patch/11209429/ > > > > > > Cc: Kevin Tian > > > CC: Jacob Pan > > > Cc: Alex Williamson > > > Cc: Eric Auger > > > Cc: Jean-Philippe Brucker > > > Signed-off-by: Liu
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Tian, Kevin > Sent: Monday, March 30, 2020 4:32 PM > To: Liu, Yi L ; alex.william...@redhat.com; > Subject: RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free) > > > From: Liu, Yi L > > Sent: Sunday, March 22, 2020 8:32 PM > > > > From: Liu Yi L > > > > For a long time, devices have only one DMA address space from platform > > IOMMU's point of view. This is true for both bare metal and directed- > > access in virtualization environment. Reason is the source ID of DMA in > > PCIe are BDF (bus/dev/fnc ID), which results in only device granularity > > are->is thanks. > > DMA isolation. However, this is changing with the latest advancement in > > I/O technology area. More and more platform vendors are utilizing the PCIe > > PASID TLP prefix in DMA requests, thus to give devices with multiple DMA > > address spaces as identified by their individual PASIDs. For example, > > Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able to > > let device access multiple process virtual address space by binding the > > "address space" -> "address spaces" > > "binding the" -> "binding each" will correct both. > > virtual address space with a PASID. Wherein the PASID is allocated in > > software and programmed to device per device specific manner. Devices > > which support PASID capability are called PASID-capable devices. If such > > devices are passed through to VMs, guest software are also able to bind > > guest process virtual address space on such devices. Therefore, the guest > > software could reuse the bare metal software programming model, which > > means guest software will also allocate PASID and program it to device > > directly. This is a dangerous situation since it has potential PASID > > conflicts and unauthorized address space access. It would be safer to > > let host intercept in the guest software's PASID allocation. Thus PASID > > are managed system-wide. > > > > This patch adds VFIO_IOMMU_PASID_REQUEST ioctl which aims to > > passdown > > PASID allocation/free request from the virtual IOMMU. Additionally, such > > "Additionally, because such" > > > requests are intended to be invoked by QEMU or other applications which > > simplify to "intended to be invoked from userspace" got it. > > are running in userspace, it is necessary to have a mechanism to prevent > > single application from abusing available PASIDs in system. With such > > consideration, this patch tracks the VFIO PASID allocation per-VM. There > > was a discussion to make quota to be per assigned devices. e.g. if a VM > > has many assigned devices, then it should have more quota. However, it > > is not sure how many PASIDs an assigned devices will use. e.g. it is > > devices -> device got it. > > possible that a VM with multiples assigned devices but requests less > > PASIDs. Therefore per-VM quota would be better. > > > > This patch uses struct mm pointer as a per-VM token. We also considered > > using task structure pointer and vfio_iommu structure pointer. However, > > task structure is per-thread, which means it cannot achieve per-VM PASID > > alloc tracking purpose. While for vfio_iommu structure, it is visible > > only within vfio. Therefore, structure mm pointer is selected. This patch > > adds a structure vfio_mm. A vfio_mm is created when the first vfio > > container is opened by a VM. On the reverse order, vfio_mm is free when > > the last vfio container is released. Each VM is assigned with a PASID > > quota, so that it is not able to request PASID beyond its quota. This > > patch adds a default quota of 1000. This quota could be tuned by > > administrator. Making PASID quota tunable will be added in another patch > > in this series. > > > > Previous discussions: > > https://patchwork.kernel.org/patch/11209429/ > > > > Cc: Kevin Tian > > CC: Jacob Pan > > Cc: Alex Williamson > > Cc: Eric Auger > > Cc: Jean-Philippe Brucker > > Signed-off-by: Liu Yi L > > Signed-off-by: Yi Sun > > Signed-off-by: Jacob Pan > > --- > > drivers/vfio/vfio.c | 130 > > > > drivers/vfio/vfio_iommu_type1.c | 104 > > > > include/linux/vfio.h| 20 +++ > > include/uapi/linux/vfio.h | 41 + > > 4 files changed, 295 insertions(+) > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > >
RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> From: Liu, Yi L
> Sent: Sunday, March 22, 2020 8:32 PM
>
> From: Liu Yi L
>
> For a long time, devices have only one DMA address space from platform
> IOMMU's point of view. This is true for both bare metal and directed-
> access in virtualization environment. Reason is the source ID of DMA in
> PCIe are BDF (bus/dev/fnc ID), which results in only device granularity
are->is
> DMA isolation. However, this is changing with the latest advancement in
> I/O technology area. More and more platform vendors are utilizing the PCIe
> PASID TLP prefix in DMA requests, thus to give devices with multiple DMA
> address spaces as identified by their individual PASIDs. For example,
> Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able to
> let device access multiple process virtual address space by binding the
"address space" -> "address spaces"
"binding the" -> "binding each"
> virtual address space with a PASID. Wherein the PASID is allocated in
> software and programmed to device per device specific manner. Devices
> which support PASID capability are called PASID-capable devices. If such
> devices are passed through to VMs, guest software are also able to bind
> guest process virtual address space on such devices. Therefore, the guest
> software could reuse the bare metal software programming model, which
> means guest software will also allocate PASID and program it to device
> directly. This is a dangerous situation since it has potential PASID
> conflicts and unauthorized address space access. It would be safer to
> let host intercept in the guest software's PASID allocation. Thus PASID
> are managed system-wide.
>
> This patch adds VFIO_IOMMU_PASID_REQUEST ioctl which aims to
> passdown
> PASID allocation/free request from the virtual IOMMU. Additionally, such
"Additionally, because such"
> requests are intended to be invoked by QEMU or other applications which
simplify to "intended to be invoked from userspace"
> are running in userspace, it is necessary to have a mechanism to prevent
> single application from abusing available PASIDs in system. With such
> consideration, this patch tracks the VFIO PASID allocation per-VM. There
> was a discussion to make quota to be per assigned devices. e.g. if a VM
> has many assigned devices, then it should have more quota. However, it
> is not sure how many PASIDs an assigned devices will use. e.g. it is
devices -> device
> possible that a VM with multiples assigned devices but requests less
> PASIDs. Therefore per-VM quota would be better.
>
> This patch uses struct mm pointer as a per-VM token. We also considered
> using task structure pointer and vfio_iommu structure pointer. However,
> task structure is per-thread, which means it cannot achieve per-VM PASID
> alloc tracking purpose. While for vfio_iommu structure, it is visible
> only within vfio. Therefore, structure mm pointer is selected. This patch
> adds a structure vfio_mm. A vfio_mm is created when the first vfio
> container is opened by a VM. On the reverse order, vfio_mm is free when
> the last vfio container is released. Each VM is assigned with a PASID
> quota, so that it is not able to request PASID beyond its quota. This
> patch adds a default quota of 1000. This quota could be tuned by
> administrator. Making PASID quota tunable will be added in another patch
> in this series.
>
> Previous discussions:
> https://patchwork.kernel.org/patch/11209429/
>
> Cc: Kevin Tian
> CC: Jacob Pan
> Cc: Alex Williamson
> Cc: Eric Auger
> Cc: Jean-Philippe Brucker
> Signed-off-by: Liu Yi L
> Signed-off-by: Yi Sun
> Signed-off-by: Jacob Pan
> ---
> drivers/vfio/vfio.c | 130
>
> drivers/vfio/vfio_iommu_type1.c | 104
>
> include/linux/vfio.h| 20 +++
> include/uapi/linux/vfio.h | 41 +
> 4 files changed, 295 insertions(+)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index c848262..d13b483 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
>
> #define DRIVER_VERSION "0.3"
> #define DRIVER_AUTHOR"Alex Williamson
> "
> @@ -46,6 +47,8 @@ static struct vfio {
> struct mutexgroup_lock;
> struct cdev group_cdev;
> dev_t group_devt;
> + struct list_headvfio_mm_list;
> + struct mutexvfio_mm_lock;
> wait_queue_head_t release_q;
> } vfio;
>
> @@ -2129,6 +2132,131 @@ int vfio_unregister_notifier(struct device *dev,
> enum vfio_notify_type type,
> EXPORT_SYMBOL(vfio_unregister_notifier);
>
> /**
> + * VFIO_MM objects - create, release, get, put, search
why capitalizing vfio_mm?
> + * Caller of the function should have held vfio.vfio_mm_lock.
> + */
> +static struct vfio_mm *vfio_create_mm(struct
Re: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
Hi Yi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on vfio/next]
[also build test WARNING on v5.6-rc6 next-20200320]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Liu-Yi-L/vfio-expose-virtual-Shared-Virtual-Addressing-to-VMs/20200322-213259
base: https://github.com/awilliam/linux-vfio.git next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.2.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All warnings (new ones prefixed by >>):
drivers/vfio/vfio.c: In function 'vfio_create_mm':
drivers/vfio/vfio.c:2149:8: error: implicit declaration of function
'ioasid_alloc_set'; did you mean 'ioasid_alloc'?
[-Werror=implicit-function-declaration]
2149 | ret = ioasid_alloc_set((struct ioasid_set *) mm,
|^~~~
|ioasid_alloc
>> drivers/vfio/vfio.c:2158:13: warning: assignment to 'long long unsigned int'
>> from 'struct mm_struct *' makes integer from pointer without a cast
>> [-Wint-conversion]
2158 | token->val = mm;
| ^
drivers/vfio/vfio.c: In function 'vfio_mm_unlock_and_free':
drivers/vfio/vfio.c:2170:2: error: implicit declaration of function
'ioasid_free_set'; did you mean 'ioasid_free'?
[-Werror=implicit-function-declaration]
2170 | ioasid_free_set(vmm->ioasid_sid, true);
| ^~~
| ioasid_free
drivers/vfio/vfio.c: In function 'vfio_mm_pasid_alloc':
drivers/vfio/vfio.c:2227:26: warning: passing argument 1 of 'ioasid_alloc'
makes pointer from integer without a cast [-Wint-conversion]
2227 | pasid = ioasid_alloc(vmm->ioasid_sid, min, max, NULL);
| ~~~^~~~
| |
| int
In file included from include/linux/iommu.h:16,
from drivers/vfio/vfio.c:20:
include/linux/ioasid.h:45:56: note: expected 'struct ioasid_set *' but
argument is of type 'int'
45 | static inline ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t
min,
| ~~~^~~
drivers/vfio/vfio.c: In function 'vfio_mm_pasid_free':
drivers/vfio/vfio.c:2246:25: warning: passing argument 1 of 'ioasid_find'
makes pointer from integer without a cast [-Wint-conversion]
2246 | pdata = ioasid_find(vmm->ioasid_sid, pasid, NULL);
| ~~~^~~~
| |
| int
In file included from include/linux/iommu.h:16,
from drivers/vfio/vfio.c:20:
include/linux/ioasid.h:55:52: note: expected 'struct ioasid_set *' but
argument is of type 'int'
55 | static inline void *ioasid_find(struct ioasid_set *set, ioasid_t
ioasid,
| ~~~^~~
cc1: some warnings being treated as errors
vim +2158 drivers/vfio/vfio.c
2133
2134 /**
2135 * VFIO_MM objects - create, release, get, put, search
2136 * Caller of the function should have held vfio.vfio_mm_lock.
2137 */
2138 static struct vfio_mm *vfio_create_mm(struct mm_struct *mm)
2139 {
2140 struct vfio_mm *vmm;
2141 struct vfio_mm_token *token;
2142 int ret = 0;
2143
2144 vmm = kzalloc(sizeof(*vmm), GFP_KERNEL);
2145 if (!vmm)
2146 return ERR_PTR(-ENOMEM);
2147
2148 /* Per mm IOASID set used for quota control and group
operations */
2149 ret = ioasid_alloc_set((struct ioasid_set *) mm,
2150 VFIO_DEFAULT_PASID_QUOTA,
>ioasid_sid);
2151 if (ret) {
2152 kfree(vmm);
2153 return ERR_PTR(ret);
2154 }
2155
2156 kref_init(>kref);
2157 token = >token;
> 2158 token->val = mm;
2159 vmm->pasid_quota = VFIO_DEFAULT_PASID_QUOTA;
2160 mutex_init(>pasid_lock);
2161
2162 list_add(>vfio_next, _mm_list);
2163
2164 return vmm;
2165 }
2166
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
iommu mailing list
iommu@lists.linux-foundation.org
[PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
From: Liu Yi L
For a long time, devices have only one DMA address space from platform
IOMMU's point of view. This is true for both bare metal and directed-
access in virtualization environment. Reason is the source ID of DMA in
PCIe are BDF (bus/dev/fnc ID), which results in only device granularity
DMA isolation. However, this is changing with the latest advancement in
I/O technology area. More and more platform vendors are utilizing the PCIe
PASID TLP prefix in DMA requests, thus to give devices with multiple DMA
address spaces as identified by their individual PASIDs. For example,
Shared Virtual Addressing (SVA, a.k.a Shared Virtual Memory) is able to
let device access multiple process virtual address space by binding the
virtual address space with a PASID. Wherein the PASID is allocated in
software and programmed to device per device specific manner. Devices
which support PASID capability are called PASID-capable devices. If such
devices are passed through to VMs, guest software are also able to bind
guest process virtual address space on such devices. Therefore, the guest
software could reuse the bare metal software programming model, which
means guest software will also allocate PASID and program it to device
directly. This is a dangerous situation since it has potential PASID
conflicts and unauthorized address space access. It would be safer to
let host intercept in the guest software's PASID allocation. Thus PASID
are managed system-wide.
This patch adds VFIO_IOMMU_PASID_REQUEST ioctl which aims to passdown
PASID allocation/free request from the virtual IOMMU. Additionally, such
requests are intended to be invoked by QEMU or other applications which
are running in userspace, it is necessary to have a mechanism to prevent
single application from abusing available PASIDs in system. With such
consideration, this patch tracks the VFIO PASID allocation per-VM. There
was a discussion to make quota to be per assigned devices. e.g. if a VM
has many assigned devices, then it should have more quota. However, it
is not sure how many PASIDs an assigned devices will use. e.g. it is
possible that a VM with multiples assigned devices but requests less
PASIDs. Therefore per-VM quota would be better.
This patch uses struct mm pointer as a per-VM token. We also considered
using task structure pointer and vfio_iommu structure pointer. However,
task structure is per-thread, which means it cannot achieve per-VM PASID
alloc tracking purpose. While for vfio_iommu structure, it is visible
only within vfio. Therefore, structure mm pointer is selected. This patch
adds a structure vfio_mm. A vfio_mm is created when the first vfio
container is opened by a VM. On the reverse order, vfio_mm is free when
the last vfio container is released. Each VM is assigned with a PASID
quota, so that it is not able to request PASID beyond its quota. This
patch adds a default quota of 1000. This quota could be tuned by
administrator. Making PASID quota tunable will be added in another patch
in this series.
Previous discussions:
https://patchwork.kernel.org/patch/11209429/
Cc: Kevin Tian
CC: Jacob Pan
Cc: Alex Williamson
Cc: Eric Auger
Cc: Jean-Philippe Brucker
Signed-off-by: Liu Yi L
Signed-off-by: Yi Sun
Signed-off-by: Jacob Pan
---
drivers/vfio/vfio.c | 130
drivers/vfio/vfio_iommu_type1.c | 104
include/linux/vfio.h| 20 +++
include/uapi/linux/vfio.h | 41 +
4 files changed, 295 insertions(+)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index c848262..d13b483 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#define DRIVER_VERSION "0.3"
#define DRIVER_AUTHOR "Alex Williamson "
@@ -46,6 +47,8 @@ static struct vfio {
struct mutexgroup_lock;
struct cdev group_cdev;
dev_t group_devt;
+ struct list_headvfio_mm_list;
+ struct mutexvfio_mm_lock;
wait_queue_head_t release_q;
} vfio;
@@ -2129,6 +2132,131 @@ int vfio_unregister_notifier(struct device *dev, enum
vfio_notify_type type,
EXPORT_SYMBOL(vfio_unregister_notifier);
/**
+ * VFIO_MM objects - create, release, get, put, search
+ * Caller of the function should have held vfio.vfio_mm_lock.
+ */
+static struct vfio_mm *vfio_create_mm(struct mm_struct *mm)
+{
+ struct vfio_mm *vmm;
+ struct vfio_mm_token *token;
+ int ret = 0;
+
+ vmm = kzalloc(sizeof(*vmm), GFP_KERNEL);
+ if (!vmm)
+ return ERR_PTR(-ENOMEM);
+
+ /* Per mm IOASID set used for quota control and group operations */
+ ret = ioasid_alloc_set((struct ioasid_set *) mm,
+ VFIO_DEFAULT_PASID_QUOTA, >ioasid_sid);
+ if (ret) {
+ kfree(vmm);
+
