All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Raj, Ashok" <ashok.raj@intel.com>
To: Yan Zhao <yan.y.zhao@intel.com>
Cc: "Lu, Baolu" <baolu.lu@intel.com>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>,
	"alex.williamson@redhat.com" <alex.williamson@redhat.com>,
	"eric.auger@redhat.com" <eric.auger@redhat.com>,
	"jacob.jun.pan@linux.intel.com" <jacob.jun.pan@linux.intel.com>,
	"joro@8bytes.org" <joro@8bytes.org>,
	"Tian, Jun J" <jun.j.tian@intel.com>,
	"Sun, Yi Y" <yi.y.sun@intel.com>,
	"jean-philippe@linaro.org" <jean-philippe@linaro.org>,
	"peterx@redhat.com" <peterx@redhat.com>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Wu, Hao" <hao.wu@intel.com>, Ashok Raj <ashok.raj@intel.com>
Subject: Re: [PATCH v1 0/2] vfio/pci: expose device's PASID capability to VMs
Date: Thu, 16 Apr 2020 15:33:54 -0700	[thread overview]
Message-ID: <20200416223353.GC45480@otc-nc-03> (raw)
In-Reply-To: <20200416221224.GA16688@joy-OptiPlex-7040>

Hi Zhao


On Thu, Apr 16, 2020 at 06:12:26PM -0400, Yan Zhao wrote:
> On Tue, Mar 31, 2020 at 03:08:25PM +0800, Lu, Baolu wrote:
> > On 2020/3/31 14:35, Tian, Kevin wrote:
> > >> From: Liu, Yi L<yi.l.liu@intel.com>
> > >> Sent: Sunday, March 22, 2020 8:33 PM
> > >>
> > >> From: Liu Yi L<yi.l.liu@intel.com>
> > >>
> > >> Shared Virtual Addressing (SVA), a.k.a, Shared Virtual Memory (SVM) on
> > >> Intel platforms allows address space sharing between device DMA and
> > >> applications. SVA can reduce programming complexity and enhance security.
> > >>
> > >> To enable SVA, device needs to have PASID capability, which is a key
> > >> capability for SVA. This patchset exposes the device's PASID capability
> > >> to guest instead of hiding it from guest.
> > >>
> > >> The second patch emulates PASID capability for VFs (Virtual Function) since
> > >> VFs don't implement such capability per PCIe spec. This patch emulates such
> > >> capability and expose to VM if the capability is enabled in PF (Physical
> > >> Function).
> > >>
> > >> However, there is an open for PASID emulation. If PF driver disables PASID
> > >> capability at runtime, then it may be an issue. e.g. PF should not disable
> > >> PASID capability if there is guest using this capability on any VF related
> > >> to this PF. To solve it, may need to introduce a generic communication
> > >> framework between vfio-pci driver and PF drivers. Please feel free to give
> > >> your suggestions on it.
> > > I'm not sure how this is addressed on bate metal today, i.e. between normal
> > > kernel PF and VF drivers. I look at pasid enable/disable code in intel-iommu.c.
> > > There is no check on PF/VF dependency so far. The cap is toggled when
> > > attaching/detaching the PF to its domain. Let's see how IOMMU guys
> > > respond, and if there is a way for VF driver to block PF driver from disabling
> > > the pasid cap when it's being actively used by VF driver, then we may
> > > leverage the same trick in VFIO when emulation is provided to guest.
> > 
> > IOMMU subsystem doesn't expose any APIs for pasid enabling/disabling.
> > The PCI subsystem does. It handles VF/PF like below.
> > 
> > /**
> >   * pci_enable_pasid - Enable the PASID capability
> >   * @pdev: PCI device structure
> >   * @features: Features to enable
> >   *
> >   * Returns 0 on success, negative value on error. This function checks
> >   * whether the features are actually supported by the device and returns
> >   * an error if not.
> >   */
> > int pci_enable_pasid(struct pci_dev *pdev, int features)
> > {
> >          u16 control, supported;
> >          int pasid = pdev->pasid_cap;
> > 
> >          /*
> >           * VFs must not implement the PASID Capability, but if a PF
> >           * supports PASID, its VFs share the PF PASID configuration.
> >           */
> >          if (pdev->is_virtfn) {
> >                  if (pci_physfn(pdev)->pasid_enabled)
> >                          return 0;
> >                  return -EINVAL;
> >          }
> > 
> > /**
> >   * pci_disable_pasid - Disable the PASID capability
> >   * @pdev: PCI device structure
> >   */
> > void pci_disable_pasid(struct pci_dev *pdev)
> > {
> >          u16 control = 0;
> >          int pasid = pdev->pasid_cap;
> > 
> >          /* VFs share the PF PASID configuration */
> >          if (pdev->is_virtfn)
> >                  return;
> > 
> > 
> > It doesn't block disabling PASID on PF even VFs are possibly using it.
> >
> hi
> I'm not sure, but is it possible for pci_enable_pasid() and
> pci_disable_pasid() to do the same thing as pdev->driver->sriov_configure,
> e.g. pci_sriov_configure_simple() below.
> 
> It checks whether there are VFs are assigned in pci_vfs_assigned(dev).
> and we can set the VF in assigned status if vfio_pci_open() is performed
> on the VF.

But you can still unbind the PF driver that magically causes the VF's to be
removed from the guest image too correct? 

Only the IOMMU mucks with pasid_enable/disable. And it doesn't look like
we have a path to disable without tearing down the PF binding. 

We originally had some refcounts and such and would do the real disable only
when the refcount drops to 0, but we found it wasn't actually necessary 
to protect these resources like that.

> 
> 
> int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn)
> {
>         int rc;
> 
>         might_sleep();
> 
>         if (!dev->is_physfn)
>                 return -ENODEV;
> 
>         if (pci_vfs_assigned(dev)) {
>                 pci_warn(dev, "Cannot modify SR-IOV while VFs are assigned\n");
>                 return -EPERM;
>         }
> 
>         if (nr_virtfn == 0) {
>                 sriov_disable(dev);
>                 return 0;
>         }
> 
>         rc = sriov_enable(dev, nr_virtfn);
>         if (rc < 0)
>                 return rc;
> 
>         return nr_virtfn;
> }
> 
> Thanks
> Yan

WARNING: multiple messages have this Message-ID (diff)
From: "Raj, Ashok" <ashok.raj@intel.com>
To: Yan Zhao <yan.y.zhao@intel.com>
Cc: "jean-philippe@linaro.org" <jean-philippe@linaro.org>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	"Lu, Baolu" <baolu.lu@intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	"Sun, Yi Y" <yi.y.sun@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"alex.williamson@redhat.com" <alex.williamson@redhat.com>,
	Ashok Raj <ashok.raj@intel.com>,
	"Tian, Jun J" <jun.j.tian@intel.com>,
	"Wu, Hao" <hao.wu@intel.com>
Subject: Re: [PATCH v1 0/2] vfio/pci: expose device's PASID capability to VMs
Date: Thu, 16 Apr 2020 15:33:54 -0700	[thread overview]
Message-ID: <20200416223353.GC45480@otc-nc-03> (raw)
In-Reply-To: <20200416221224.GA16688@joy-OptiPlex-7040>

Hi Zhao


On Thu, Apr 16, 2020 at 06:12:26PM -0400, Yan Zhao wrote:
> On Tue, Mar 31, 2020 at 03:08:25PM +0800, Lu, Baolu wrote:
> > On 2020/3/31 14:35, Tian, Kevin wrote:
> > >> From: Liu, Yi L<yi.l.liu@intel.com>
> > >> Sent: Sunday, March 22, 2020 8:33 PM
> > >>
> > >> From: Liu Yi L<yi.l.liu@intel.com>
> > >>
> > >> Shared Virtual Addressing (SVA), a.k.a, Shared Virtual Memory (SVM) on
> > >> Intel platforms allows address space sharing between device DMA and
> > >> applications. SVA can reduce programming complexity and enhance security.
> > >>
> > >> To enable SVA, device needs to have PASID capability, which is a key
> > >> capability for SVA. This patchset exposes the device's PASID capability
> > >> to guest instead of hiding it from guest.
> > >>
> > >> The second patch emulates PASID capability for VFs (Virtual Function) since
> > >> VFs don't implement such capability per PCIe spec. This patch emulates such
> > >> capability and expose to VM if the capability is enabled in PF (Physical
> > >> Function).
> > >>
> > >> However, there is an open for PASID emulation. If PF driver disables PASID
> > >> capability at runtime, then it may be an issue. e.g. PF should not disable
> > >> PASID capability if there is guest using this capability on any VF related
> > >> to this PF. To solve it, may need to introduce a generic communication
> > >> framework between vfio-pci driver and PF drivers. Please feel free to give
> > >> your suggestions on it.
> > > I'm not sure how this is addressed on bate metal today, i.e. between normal
> > > kernel PF and VF drivers. I look at pasid enable/disable code in intel-iommu.c.
> > > There is no check on PF/VF dependency so far. The cap is toggled when
> > > attaching/detaching the PF to its domain. Let's see how IOMMU guys
> > > respond, and if there is a way for VF driver to block PF driver from disabling
> > > the pasid cap when it's being actively used by VF driver, then we may
> > > leverage the same trick in VFIO when emulation is provided to guest.
> > 
> > IOMMU subsystem doesn't expose any APIs for pasid enabling/disabling.
> > The PCI subsystem does. It handles VF/PF like below.
> > 
> > /**
> >   * pci_enable_pasid - Enable the PASID capability
> >   * @pdev: PCI device structure
> >   * @features: Features to enable
> >   *
> >   * Returns 0 on success, negative value on error. This function checks
> >   * whether the features are actually supported by the device and returns
> >   * an error if not.
> >   */
> > int pci_enable_pasid(struct pci_dev *pdev, int features)
> > {
> >          u16 control, supported;
> >          int pasid = pdev->pasid_cap;
> > 
> >          /*
> >           * VFs must not implement the PASID Capability, but if a PF
> >           * supports PASID, its VFs share the PF PASID configuration.
> >           */
> >          if (pdev->is_virtfn) {
> >                  if (pci_physfn(pdev)->pasid_enabled)
> >                          return 0;
> >                  return -EINVAL;
> >          }
> > 
> > /**
> >   * pci_disable_pasid - Disable the PASID capability
> >   * @pdev: PCI device structure
> >   */
> > void pci_disable_pasid(struct pci_dev *pdev)
> > {
> >          u16 control = 0;
> >          int pasid = pdev->pasid_cap;
> > 
> >          /* VFs share the PF PASID configuration */
> >          if (pdev->is_virtfn)
> >                  return;
> > 
> > 
> > It doesn't block disabling PASID on PF even VFs are possibly using it.
> >
> hi
> I'm not sure, but is it possible for pci_enable_pasid() and
> pci_disable_pasid() to do the same thing as pdev->driver->sriov_configure,
> e.g. pci_sriov_configure_simple() below.
> 
> It checks whether there are VFs are assigned in pci_vfs_assigned(dev).
> and we can set the VF in assigned status if vfio_pci_open() is performed
> on the VF.

But you can still unbind the PF driver that magically causes the VF's to be
removed from the guest image too correct? 

Only the IOMMU mucks with pasid_enable/disable. And it doesn't look like
we have a path to disable without tearing down the PF binding. 

We originally had some refcounts and such and would do the real disable only
when the refcount drops to 0, but we found it wasn't actually necessary 
to protect these resources like that.

> 
> 
> int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn)
> {
>         int rc;
> 
>         might_sleep();
> 
>         if (!dev->is_physfn)
>                 return -ENODEV;
> 
>         if (pci_vfs_assigned(dev)) {
>                 pci_warn(dev, "Cannot modify SR-IOV while VFs are assigned\n");
>                 return -EPERM;
>         }
> 
>         if (nr_virtfn == 0) {
>                 sriov_disable(dev);
>                 return 0;
>         }
> 
>         rc = sriov_enable(dev, nr_virtfn);
>         if (rc < 0)
>                 return rc;
> 
>         return nr_virtfn;
> }
> 
> Thanks
> Yan
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  reply	other threads:[~2020-04-16 22:33 UTC|newest]

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-22 12:33 [PATCH v1 0/2] vfio/pci: expose device's PASID capability to VMs Liu, Yi L
2020-03-22 12:33 ` Liu, Yi L
2020-03-22 12:33 ` [PATCH v1 1/2] vfio/pci: Expose PCIe PASID capability to guest Liu, Yi L
2020-03-22 12:33   ` Liu, Yi L
2020-03-31  6:39   ` Tian, Kevin
2020-03-31  6:39     ` Tian, Kevin
2020-03-31  6:42     ` Liu, Yi L
2020-03-31  6:42       ` Liu, Yi L
2020-03-22 12:33 ` [PATCH v1 2/2] vfio/pci: Emulate PASID/PRI capability for VFs Liu, Yi L
2020-03-22 12:33   ` Liu, Yi L
2020-04-02 22:59   ` Alex Williamson
2020-04-02 22:59     ` Alex Williamson
2020-04-03  7:53     ` Liu, Yi L
2020-04-03  7:53       ` Liu, Yi L
2020-04-03 17:25       ` Alex Williamson
2020-04-03 17:25         ` Alex Williamson
2020-04-07  4:26         ` Tian, Kevin
2020-04-07  4:26           ` Tian, Kevin
2020-04-07 15:58           ` Alex Williamson
2020-04-07 15:58             ` Alex Williamson
2020-04-08  0:27             ` Tian, Kevin
2020-04-08  0:27               ` Tian, Kevin
2020-04-08  4:00             ` Raj, Ashok
2020-04-08  4:00               ` Raj, Ashok
2020-04-08 16:19               ` Alex Williamson
2020-04-08 16:19                 ` Alex Williamson
2020-04-08 16:33                 ` Raj, Ashok
2020-04-08 16:33                   ` Raj, Ashok
2020-04-09  7:35                 ` Jean-Philippe Brucker
2020-04-09  7:35                   ` Jean-Philippe Brucker
2020-04-13 19:44                   ` Alex Williamson
2020-04-13 19:44                     ` Alex Williamson
2020-04-13  3:10                 ` Raj, Ashok
2020-04-13  3:10                   ` Raj, Ashok
2020-04-13  3:29                   ` Raj, Ashok
2020-04-13  3:29                     ` Raj, Ashok
2020-04-13 19:10                     ` Alex Williamson
2020-04-13 19:10                       ` Alex Williamson
2020-04-13  7:54                   ` Tian, Kevin
2020-04-13  7:54                     ` Tian, Kevin
2020-04-13  8:05                   ` Tian, Kevin
2020-04-13  8:05                     ` Tian, Kevin
2020-04-13 19:21                     ` Alex Williamson
2020-04-13 19:21                       ` Alex Williamson
2020-04-14  2:40                       ` Tian, Kevin
2020-04-14  2:40                         ` Tian, Kevin
2020-04-14  3:28                         ` Alex Williamson
2020-04-14  3:28                           ` Alex Williamson
2020-04-14  3:42                           ` Tian, Kevin
2020-04-14  3:42                             ` Tian, Kevin
2020-04-14 15:24                             ` Alex Williamson
2020-04-14 15:24                               ` Alex Williamson
2020-04-14 23:57                               ` Tian, Kevin
2020-04-14 23:57                                 ` Tian, Kevin
2020-04-15  0:36                                 ` Alex Williamson
2020-04-15  0:36                                   ` Alex Williamson
2020-04-15  1:01                                   ` Tian, Kevin
2020-04-15  1:01                                     ` Tian, Kevin
2020-04-03 11:42     ` Liu, Yi L
2020-04-03 11:42       ` Liu, Yi L
2020-03-31  6:35 ` [PATCH v1 0/2] vfio/pci: expose device's PASID capability to VMs Tian, Kevin
2020-03-31  6:35   ` Tian, Kevin
2020-03-31  7:08   ` Lu, Baolu
2020-03-31  7:08     ` Lu, Baolu
2020-04-16 22:12     ` Yan Zhao
2020-04-16 22:12       ` Yan Zhao
2020-04-16 22:33       ` Raj, Ashok [this message]
2020-04-16 22:33         ` Raj, Ashok
2020-04-17  1:13         ` Yan Zhao
2020-04-17  1:13           ` Yan Zhao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200416223353.GC45480@otc-nc-03 \
    --to=ashok.raj@intel.com \
    --cc=alex.williamson@redhat.com \
    --cc=baolu.lu@intel.com \
    --cc=eric.auger@redhat.com \
    --cc=hao.wu@intel.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jacob.jun.pan@linux.intel.com \
    --cc=jean-philippe@linaro.org \
    --cc=joro@8bytes.org \
    --cc=jun.j.tian@intel.com \
    --cc=kevin.tian@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterx@redhat.com \
    --cc=yan.y.zhao@intel.com \
    --cc=yi.l.liu@intel.com \
    --cc=yi.y.sun@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.