thr3ads.net - Linux Virtualization - [virtio-dev] Re: [PATCH v3] virtio

If this information is useful, please help other people find it:
Share via:

Tiwei Bie

2018-Jun-06 14:19 UTC

[virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

On Wed, Jun 06, 2018 at 03:44:11PM +0300, Michael S. Tsirkin
wrote:> On Wed, Jun 06, 2018 at 08:11:54PM +0800, Tiwei Bie wrote:
> > On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> > > > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin
wrote:
> > > > > On Fri, Jun 01, 2018 at 12:02:39PM +0800, Tiwei Bie
wrote:
> > > > > > There is a new feature bit allocated in virtio
spec to
> > > > > > support SR-IOV (Single Root I/O Virtualization):
> > > > > > 
> > > > > > https://github.com/oasis-tcs/virtio-spec/issues/11
> > > > > > 
> > > > > > This patch enables the support for this feature
bit in
> > > > > > virtio driver.
> > > > > > 
> > > > > > Signed-off-by: Tiwei Bie <tiwei.bie at
intel.com>
> > > > > > ---
> > > > > 
> > > > > OK but what about freeze/restore functions?
> > > 
> > > So for restore, don't you need to restore the
> > > sriov capability?
> > 
> > Currently I'm not familiar with the PM part.
> > But I still think the sriov capability should
> > be handled by PCI core.
> 
> OK but the point is restore looks just like power up for device.
> 
> > I'm trying to understand
> > all the relevant code..
> > For your question, based on what I found from
> > the code currently, I guess the sriov capability
> > will be restored by pci_restore_state() which
> > will be called by the ops in pci_dev_pm_ops.
> > The sriov_restore_state() will be called
> > eventually.
> > 
> > Best regards,
> > Tiwei Bie
> 
> Right but my point is during resume SRIOV gets enabled first before
> driver ok.
> 
> Maybe we should relax the requirements in the spec:
> - only require FEATURES_OK from device, not DRIVER_OK from driver
> - explain that it only has to happen once, not on each reset,
>   and driver can remember the result
I got your point now! I'd like to relax the
requirements in the spec.

Best regards,
Tiwei Bie
> 
> 
> > 
> > > 
> > > 
> > > > > I also wonder about kexec - virtio.c currently does:
> > > > > 
> > > > >         /* We always start by resetting the device, in
case a previous
> > > > >          * driver messed it up.  This also tests that
code path a little. */
> > > > >         dev->config->reset(dev);
> > > > > 
> > > > > Do we need to do something like this for sriov?
> > > > 
> > > > I think VFs are managed by PCI core. Once they are
> > > > allocated, virtio driver doesn't have to care too
> > > > much about how to manage them. The proposal for the
> > > > spec is just to provide a feature bit based virtio
> > > > way for virtio drivers to know whether a virtio
> > > > device is SR-IOV capable (and virtio drivers can
> > > > support configuring SR-IOV based on the feature
> > > > bit negotiation result).
> > > > 
> > > > > 
> > > > > I also wonder whether PCI core should disable sriov for
us.
> > > > > 
> > > > > 
> > > > > I wish there was a patch emulating this without vDPA
for QEMU,
> > > > > would make it easy to test your patches. Do you happen
> > > > > to have something like this?
> > > > 
> > > > Sorry, currently I don't have anything like this..
> > > > 
> > > > Best regards,
> > > > Tiwei Bie
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > 
> > > > > > v3:
> > > > > > - Drop the acks;
> > > > > > 
> > > > > > v2:
> > > > > > - Disable VFs when unbinding the driver (Alex,
MST);
> > > > > > - Don't use pci_sriov_configure_simple (Alex);
> > > > > > 
> > > > > >  drivers/virtio/virtio_pci_common.c | 30
++++++++++++++++++++++++++++++
> > > > > >  drivers/virtio/virtio_pci_modern.c | 14
++++++++++++++
> > > > > >  include/uapi/linux/virtio_config.h |  7 ++++++-
> > > > > >  3 files changed, 50 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
> > > > > > index 48d4d1cf1cb6..1d4467b2dc31 100644
> > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > @@ -577,6 +577,8 @@ static void
virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > >  	struct virtio_pci_device *vp_dev =
pci_get_drvdata(pci_dev);
> > > > > >  	struct device *dev =
get_device(&vp_dev->vdev.dev);
> > > > > >  
> > > > > > +	pci_disable_sriov(pci_dev);
> > > > > > +
> > > > > >  	unregister_virtio_device(&vp_dev->vdev);
> > > > > >  
> > > > > >  	if (vp_dev->ioaddr)
> > > > > > @@ -588,6 +590,33 @@ static void
virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > >  	put_device(dev);
> > > > > >  }
> > > > > >  
> > > > > > +static int virtio_pci_sriov_configure(struct
pci_dev *pci_dev, int num_vfs)
> > > > > > +{
> > > > > > +	struct virtio_pci_device *vp_dev =
pci_get_drvdata(pci_dev);
> > > > > > +	struct virtio_device *vdev =
&vp_dev->vdev;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	if (!(vdev->config->get_status(vdev) &
VIRTIO_CONFIG_S_DRIVER_OK))
> > > > > > +		return -EBUSY;
> > > > > > +
> > > > > > +	if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	if (pci_vfs_assigned(pci_dev))
> > > > > > +		return -EPERM;
> > > > > > +
> > > > > > +	if (num_vfs == 0) {
> > > > > > +		pci_disable_sriov(pci_dev);
> > > > > > +		return 0;
> > > > > > +	}
> > > > > > +
> > > > > > +	ret = pci_enable_sriov(pci_dev, num_vfs);
> > > > > > +	if (ret < 0)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	return num_vfs;
> > > > > > +}
> > > > > > +
> > > > > >  static struct pci_driver virtio_pci_driver = {
> > > > > >  	.name		= "virtio-pci",
> > > > > >  	.id_table	= virtio_pci_id_table,
> > > > > > @@ -596,6 +625,7 @@ static struct pci_driver
virtio_pci_driver = {
> > > > > >  #ifdef CONFIG_PM_SLEEP
> > > > > >  	.driver.pm	= &virtio_pci_pm_ops,
> > > > > >  #endif
> > > > > > +	.sriov_configure = virtio_pci_sriov_configure,
> > > > > >  };
> > > > > >  
> > > > > >  module_pci_driver(virtio_pci_driver);
> > > > > > diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
> > > > > > index 2555d80f6eec..07571daccfec 100644
> > > > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > > > @@ -153,14 +153,28 @@ static u64
vp_get_features(struct virtio_device *vdev)
> > > > > >  	return features;
> > > > > >  }
> > > > > >  
> > > > > > +static void vp_transport_features(struct
virtio_device *vdev, u64 features)
> > > > > > +{
> > > > > > +	struct virtio_pci_device *vp_dev =
to_vp_device(vdev);
> > > > > > +	struct pci_dev *pci_dev = vp_dev->pci_dev;
> > > > > > +
> > > > > > +	if ((features & BIT_ULL(VIRTIO_F_SR_IOV))
&&
> > > > > > +			pci_find_ext_capability(pci_dev,
PCI_EXT_CAP_ID_SRIOV))
> > > > > > +		__virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > > > > > +}
> > > > > > +
> > > > > >  /* virtio config->finalize_features()
implementation */
> > > > > >  static int vp_finalize_features(struct
virtio_device *vdev)
> > > > > >  {
> > > > > >  	struct virtio_pci_device *vp_dev =
to_vp_device(vdev);
> > > > > > +	u64 features = vdev->features;
> > > > > >  
> > > > > >  	/* Give virtio_ring a chance to accept features.
*/
> > > > > >  	vring_transport_features(vdev);
> > > > > >  
> > > > > > +	/* Give virtio_pci a chance to accept features.
*/
> > > > > > +	vp_transport_features(vdev, features);
> > > > > > +
> > > > > >  	if (!__virtio_test_bit(vdev,
VIRTIO_F_VERSION_1)) {
> > > > > >  		dev_err(&vdev->dev, "virtio: device
uses modern interface "
> > > > > >  			"but does not have
VIRTIO_F_VERSION_1\n");
> > > > > > diff --git a/include/uapi/linux/virtio_config.h
b/include/uapi/linux/virtio_config.h
> > > > > > index 308e2096291f..b7c1f4e7d59e 100644
> > > > > > --- a/include/uapi/linux/virtio_config.h
> > > > > > +++ b/include/uapi/linux/virtio_config.h
> > > > > > @@ -49,7 +49,7 @@
> > > > > >   * transport being used (eg. virtio_ring), the
rest are per-device feature
> > > > > >   * bits. */
> > > > > >  #define VIRTIO_TRANSPORT_F_START	28
> > > > > > -#define VIRTIO_TRANSPORT_F_END		34
> > > > > > +#define VIRTIO_TRANSPORT_F_END		38
> > > > > >  
> > > > > >  #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > > > >  /* Do we get callbacks when the ring is
completely used, even if we've
> > > > > > @@ -71,4 +71,9 @@
> > > > > >   * this is for compatibility with legacy systems.
> > > > > >   */
> > > > > >  #define VIRTIO_F_IOMMU_PLATFORM		33
> > > > > > +
> > > > > > +/*
> > > > > > + * Does the device support Single Root I/O
Virtualization?
> > > > > > + */
> > > > > > +#define VIRTIO_F_SR_IOV			37
> > > > > >  #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
> > > > > > -- 
> > > > > > 2.17.0
> > > > > 
> > > > >
---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe at
lists.oasis-open.org
> > > > > For additional commands, e-mail: virtio-dev-help at
lists.oasis-open.org
> > > > >

Michael S. Tsirkin

2018-Jun-06 14:27 UTC

head link

[virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

On Wed, Jun 06, 2018 at 10:19:43PM +0800, Tiwei Bie
wrote:> On Wed, Jun 06, 2018 at 03:44:11PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 06, 2018 at 08:11:54PM +0800, Tiwei Bie wrote:
> > > On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin
wrote:
> > > > On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> > > > > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S.
Tsirkin wrote:
> > > > > > On Fri, Jun 01, 2018 at 12:02:39PM +0800, Tiwei
Bie wrote:
> > > > > > > There is a new feature bit allocated in
virtio spec to
> > > > > > > support SR-IOV (Single Root I/O
Virtualization):
> > > > > > > 
> > > > > > >
https://github.com/oasis-tcs/virtio-spec/issues/11
> > > > > > > 
> > > > > > > This patch enables the support for this
feature bit in
> > > > > > > virtio driver.
> > > > > > > 
> > > > > > > Signed-off-by: Tiwei Bie <tiwei.bie at
intel.com>
> > > > > > > ---
> > > > > > 
> > > > > > OK but what about freeze/restore functions?
> > > > 
> > > > So for restore, don't you need to restore the
> > > > sriov capability?
> > > 
> > > Currently I'm not familiar with the PM part.
> > > But I still think the sriov capability should
> > > be handled by PCI core.
> > 
> > OK but the point is restore looks just like power up for device.
> > 
> > > I'm trying to understand
> > > all the relevant code..
> > > For your question, based on what I found from
> > > the code currently, I guess the sriov capability
> > > will be restored by pci_restore_state() which
> > > will be called by the ops in pci_dev_pm_ops.
> > > The sriov_restore_state() will be called
> > > eventually.
> > > 
> > > Best regards,
> > > Tiwei Bie
> > 
> > Right but my point is during resume SRIOV gets enabled first before
> > driver ok.
> > 
> > Maybe we should relax the requirements in the spec:
> > - only require FEATURES_OK from device, not DRIVER_OK from driver
> > - explain that it only has to happen once, not on each reset,
> >   and driver can remember the result
> 
> I got your point now! I'd like to relax the
> requirements in the spec.
> 
> Best regards,
> Tiwei Bie
Well the ballot approving your change closed.  I think we should apply
the first chunks reserving the feature bit then, and defer the rest, and
you can work on new wording documenting the actual behaviour with a new
github issue to track that - does this make sense?
Let's do it quickly though - I don't want to bother the
TC with re-voting the deferral, then the new patch.
> > 
> > 
> > > 
> > > > 
> > > > 
> > > > > > I also wonder about kexec - virtio.c currently
does:
> > > > > > 
> > > > > >         /* We always start by resetting the
device, in case a previous
> > > > > >          * driver messed it up.  This also tests
that code path a little. */
> > > > > >         dev->config->reset(dev);
> > > > > > 
> > > > > > Do we need to do something like this for sriov?
> > > > > 
> > > > > I think VFs are managed by PCI core. Once they are
> > > > > allocated, virtio driver doesn't have to care too
> > > > > much about how to manage them. The proposal for the
> > > > > spec is just to provide a feature bit based virtio
> > > > > way for virtio drivers to know whether a virtio
> > > > > device is SR-IOV capable (and virtio drivers can
> > > > > support configuring SR-IOV based on the feature
> > > > > bit negotiation result).
> > > > > 
> > > > > > 
> > > > > > I also wonder whether PCI core should disable
sriov for us.
> > > > > > 
> > > > > > 
> > > > > > I wish there was a patch emulating this without
vDPA for QEMU,
> > > > > > would make it easy to test your patches. Do you
happen
> > > > > > to have something like this?
> > > > > 
> > > > > Sorry, currently I don't have anything like this..
> > > > > 
> > > > > Best regards,
> > > > > Tiwei Bie
> > > > > 
> > > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > 
> > > > > > > v3:
> > > > > > > - Drop the acks;
> > > > > > > 
> > > > > > > v2:
> > > > > > > - Disable VFs when unbinding the driver
(Alex, MST);
> > > > > > > - Don't use pci_sriov_configure_simple
(Alex);
> > > > > > > 
> > > > > > >  drivers/virtio/virtio_pci_common.c | 30
++++++++++++++++++++++++++++++
> > > > > > >  drivers/virtio/virtio_pci_modern.c | 14
++++++++++++++
> > > > > > >  include/uapi/linux/virtio_config.h |  7
++++++-
> > > > > > >  3 files changed, 50 insertions(+), 1
deletion(-)
> > > > > > > 
> > > > > > > diff --git
a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > > > index 48d4d1cf1cb6..1d4467b2dc31 100644
> > > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > > @@ -577,6 +577,8 @@ static void
virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > > >  	struct virtio_pci_device *vp_dev =
pci_get_drvdata(pci_dev);
> > > > > > >  	struct device *dev =
get_device(&vp_dev->vdev.dev);
> > > > > > >  
> > > > > > > +	pci_disable_sriov(pci_dev);
> > > > > > > +
> > > > > > >  
unregister_virtio_device(&vp_dev->vdev);
> > > > > > >  
> > > > > > >  	if (vp_dev->ioaddr)
> > > > > > > @@ -588,6 +590,33 @@ static void
virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > > >  	put_device(dev);
> > > > > > >  }
> > > > > > >  
> > > > > > > +static int virtio_pci_sriov_configure(struct
pci_dev *pci_dev, int num_vfs)
> > > > > > > +{
> > > > > > > +	struct virtio_pci_device *vp_dev =
pci_get_drvdata(pci_dev);
> > > > > > > +	struct virtio_device *vdev =
&vp_dev->vdev;
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	if (!(vdev->config->get_status(vdev)
& VIRTIO_CONFIG_S_DRIVER_OK))
> > > > > > > +		return -EBUSY;
> > > > > > > +
> > > > > > > +	if (!__virtio_test_bit(vdev,
VIRTIO_F_SR_IOV))
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	if (pci_vfs_assigned(pci_dev))
> > > > > > > +		return -EPERM;
> > > > > > > +
> > > > > > > +	if (num_vfs == 0) {
> > > > > > > +		pci_disable_sriov(pci_dev);
> > > > > > > +		return 0;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	ret = pci_enable_sriov(pci_dev, num_vfs);
> > > > > > > +	if (ret < 0)
> > > > > > > +		return ret;
> > > > > > > +
> > > > > > > +	return num_vfs;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static struct pci_driver virtio_pci_driver =
{
> > > > > > >  	.name		= "virtio-pci",
> > > > > > >  	.id_table	= virtio_pci_id_table,
> > > > > > > @@ -596,6 +625,7 @@ static struct pci_driver
virtio_pci_driver = {
> > > > > > >  #ifdef CONFIG_PM_SLEEP
> > > > > > >  	.driver.pm	= &virtio_pci_pm_ops,
> > > > > > >  #endif
> > > > > > > +	.sriov_configure =
virtio_pci_sriov_configure,
> > > > > > >  };
> > > > > > >  
> > > > > > >  module_pci_driver(virtio_pci_driver);
> > > > > > > diff --git
a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > > > > > index 2555d80f6eec..07571daccfec 100644
> > > > > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > > > > @@ -153,14 +153,28 @@ static u64
vp_get_features(struct virtio_device *vdev)
> > > > > > >  	return features;
> > > > > > >  }
> > > > > > >  
> > > > > > > +static void vp_transport_features(struct
virtio_device *vdev, u64 features)
> > > > > > > +{
> > > > > > > +	struct virtio_pci_device *vp_dev =
to_vp_device(vdev);
> > > > > > > +	struct pci_dev *pci_dev =
vp_dev->pci_dev;
> > > > > > > +
> > > > > > > +	if ((features &
BIT_ULL(VIRTIO_F_SR_IOV)) &&
> > > > > > > +			pci_find_ext_capability(pci_dev,
PCI_EXT_CAP_ID_SRIOV))
> > > > > > > +		__virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > > > > > > +}
> > > > > > > +
> > > > > > >  /* virtio config->finalize_features()
implementation */
> > > > > > >  static int vp_finalize_features(struct
virtio_device *vdev)
> > > > > > >  {
> > > > > > >  	struct virtio_pci_device *vp_dev =
to_vp_device(vdev);
> > > > > > > +	u64 features = vdev->features;
> > > > > > >  
> > > > > > >  	/* Give virtio_ring a chance to accept
features. */
> > > > > > >  	vring_transport_features(vdev);
> > > > > > >  
> > > > > > > +	/* Give virtio_pci a chance to accept
features. */
> > > > > > > +	vp_transport_features(vdev, features);
> > > > > > > +
> > > > > > >  	if (!__virtio_test_bit(vdev,
VIRTIO_F_VERSION_1)) {
> > > > > > >  		dev_err(&vdev->dev, "virtio:
device uses modern interface "
> > > > > > >  			"but does not have
VIRTIO_F_VERSION_1\n");
> > > > > > > diff --git
a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > > > > index 308e2096291f..b7c1f4e7d59e 100644
> > > > > > > --- a/include/uapi/linux/virtio_config.h
> > > > > > > +++ b/include/uapi/linux/virtio_config.h
> > > > > > > @@ -49,7 +49,7 @@
> > > > > > >   * transport being used (eg. virtio_ring),
the rest are per-device feature
> > > > > > >   * bits. */
> > > > > > >  #define VIRTIO_TRANSPORT_F_START	28
> > > > > > > -#define VIRTIO_TRANSPORT_F_END		34
> > > > > > > +#define VIRTIO_TRANSPORT_F_END		38
> > > > > > >  
> > > > > > >  #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > > > > >  /* Do we get callbacks when the ring is
completely used, even if we've
> > > > > > > @@ -71,4 +71,9 @@
> > > > > > >   * this is for compatibility with legacy
systems.
> > > > > > >   */
> > > > > > >  #define VIRTIO_F_IOMMU_PLATFORM		33
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Does the device support Single Root I/O
Virtualization?
> > > > > > > + */
> > > > > > > +#define VIRTIO_F_SR_IOV			37
> > > > > > >  #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
> > > > > > > -- 
> > > > > > > 2.17.0
> > > > > > 
> > > > > >
---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe at
lists.oasis-open.org
> > > > > > For additional commands, e-mail: virtio-dev-help
at lists.oasis-open.org
> > > > > >

Tiwei Bie

2018-Jun-06 15:08 UTC

head link

[virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

On Wed, Jun 06, 2018 at 05:27:07PM +0300, Michael S. Tsirkin
wrote:> On Wed, Jun 06, 2018 at 10:19:43PM +0800, Tiwei Bie wrote:
> > On Wed, Jun 06, 2018 at 03:44:11PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 06, 2018 at 08:11:54PM +0800, Tiwei Bie wrote:
> > > > On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin
wrote:
> > > > > On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie
wrote:
> > > > > > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael
S. Tsirkin wrote:
> > > > > > > On Fri, Jun 01, 2018 at 12:02:39PM +0800,
Tiwei Bie wrote:
> > > > > > > > There is a new feature bit allocated in
virtio spec to
> > > > > > > > support SR-IOV (Single Root I/O
Virtualization):
> > > > > > > > 
> > > > > > > >
https://github.com/oasis-tcs/virtio-spec/issues/11
> > > > > > > > 
> > > > > > > > This patch enables the support for this
feature bit in
> > > > > > > > virtio driver.
> > > > > > > > 
> > > > > > > > Signed-off-by: Tiwei Bie <tiwei.bie
at intel.com>
> > > > > > > > ---
> > > > > > > 
> > > > > > > OK but what about freeze/restore functions?
> > > > > 
> > > > > So for restore, don't you need to restore the
> > > > > sriov capability?
> > > > 
> > > > Currently I'm not familiar with the PM part.
> > > > But I still think the sriov capability should
> > > > be handled by PCI core.
> > > 
> > > OK but the point is restore looks just like power up for device.
> > > 
> > > > I'm trying to understand
> > > > all the relevant code..
> > > > For your question, based on what I found from
> > > > the code currently, I guess the sriov capability
> > > > will be restored by pci_restore_state() which
> > > > will be called by the ops in pci_dev_pm_ops.
> > > > The sriov_restore_state() will be called
> > > > eventually.
> > > > 
> > > > Best regards,
> > > > Tiwei Bie
> > > 
> > > Right but my point is during resume SRIOV gets enabled first
before
> > > driver ok.
> > > 
> > > Maybe we should relax the requirements in the spec:
> > > - only require FEATURES_OK from device, not DRIVER_OK from driver
> > > - explain that it only has to happen once, not on each reset,
> > >   and driver can remember the result
> > 
> > I got your point now! I'd like to relax the
> > requirements in the spec.
> > 
> > Best regards,
> > Tiwei Bie
> 
> Well the ballot approving your change closed.  I think we should apply
> the first chunks reserving the feature bit then, and defer the rest, and
> you can work on new wording documenting the actual behaviour with a new
> github issue to track that - does this make sense?
> Let's do it quickly though - I don't want to bother the
> TC with re-voting the deferral, then the new patch.
Yeah. It makes sense! I think it's a good idea
to reserve the feature bit first and defer the
rest. I'll work on the new wording documenting
the actual behavior with a new github issue to
track it. Thanks a lot!

Best regards,
Tiwei Bie
> 
> > > 
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > > I also wonder about kexec - virtio.c
currently does:
> > > > > > > 
> > > > > > >         /* We always start by resetting the
device, in case a previous
> > > > > > >          * driver messed it up.  This also
tests that code path a little. */
> > > > > > >         dev->config->reset(dev);
> > > > > > > 
> > > > > > > Do we need to do something like this for
sriov?
> > > > > > 
> > > > > > I think VFs are managed by PCI core. Once they are
> > > > > > allocated, virtio driver doesn't have to care
too
> > > > > > much about how to manage them. The proposal for
the
> > > > > > spec is just to provide a feature bit based virtio
> > > > > > way for virtio drivers to know whether a virtio
> > > > > > device is SR-IOV capable (and virtio drivers can
> > > > > > support configuring SR-IOV based on the feature
> > > > > > bit negotiation result).
> > > > > > 
> > > > > > > 
> > > > > > > I also wonder whether PCI core should disable
sriov for us.
> > > > > > > 
> > > > > > > 
> > > > > > > I wish there was a patch emulating this
without vDPA for QEMU,
> > > > > > > would make it easy to test your patches. Do
you happen
> > > > > > > to have something like this?
> > > > > > 
> > > > > > Sorry, currently I don't have anything like
this..
> > > > > > 
> > > > > > Best regards,
> > > > > > Tiwei Bie
> > > > > > 
> > > > > > > 
> > > > > > > Thanks,
> > > > > > > 
> > > > > > > 
> > > > > > > > v3:
> > > > > > > > - Drop the acks;
> > > > > > > > 
> > > > > > > > v2:
> > > > > > > > - Disable VFs when unbinding the driver
(Alex, MST);
> > > > > > > > - Don't use
pci_sriov_configure_simple (Alex);
> > > > > > > > 
> > > > > > > >  drivers/virtio/virtio_pci_common.c | 30
++++++++++++++++++++++++++++++
> > > > > > > >  drivers/virtio/virtio_pci_modern.c | 14
++++++++++++++
> > > > > > > >  include/uapi/linux/virtio_config.h |  7
++++++-
> > > > > > > >  3 files changed, 50 insertions(+), 1
deletion(-)
> > > > > > > > 
> > > > > > > > diff --git
a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > > > > index 48d4d1cf1cb6..1d4467b2dc31 100644
> > > > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > > > @@ -577,6 +577,8 @@ static void
virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > > > >  	struct virtio_pci_device *vp_dev =
pci_get_drvdata(pci_dev);
> > > > > > > >  	struct device *dev =
get_device(&vp_dev->vdev.dev);
> > > > > > > >  
> > > > > > > > +	pci_disable_sriov(pci_dev);
> > > > > > > > +
> > > > > > > >  
unregister_virtio_device(&vp_dev->vdev);
> > > > > > > >  
> > > > > > > >  	if (vp_dev->ioaddr)
> > > > > > > > @@ -588,6 +590,33 @@ static void
virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > > > >  	put_device(dev);
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > +static int
virtio_pci_sriov_configure(struct pci_dev *pci_dev, int num_vfs)
> > > > > > > > +{
> > > > > > > > +	struct virtio_pci_device *vp_dev =
pci_get_drvdata(pci_dev);
> > > > > > > > +	struct virtio_device *vdev =
&vp_dev->vdev;
> > > > > > > > +	int ret;
> > > > > > > > +
> > > > > > > > +	if
(!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > > > > > > > +		return -EBUSY;
> > > > > > > > +
> > > > > > > > +	if (!__virtio_test_bit(vdev,
VIRTIO_F_SR_IOV))
> > > > > > > > +		return -EINVAL;
> > > > > > > > +
> > > > > > > > +	if (pci_vfs_assigned(pci_dev))
> > > > > > > > +		return -EPERM;
> > > > > > > > +
> > > > > > > > +	if (num_vfs == 0) {
> > > > > > > > +		pci_disable_sriov(pci_dev);
> > > > > > > > +		return 0;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	ret = pci_enable_sriov(pci_dev,
num_vfs);
> > > > > > > > +	if (ret < 0)
> > > > > > > > +		return ret;
> > > > > > > > +
> > > > > > > > +	return num_vfs;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static struct pci_driver
virtio_pci_driver = {
> > > > > > > >  	.name		= "virtio-pci",
> > > > > > > >  	.id_table	= virtio_pci_id_table,
> > > > > > > > @@ -596,6 +625,7 @@ static struct
pci_driver virtio_pci_driver = {
> > > > > > > >  #ifdef CONFIG_PM_SLEEP
> > > > > > > >  	.driver.pm	= &virtio_pci_pm_ops,
> > > > > > > >  #endif
> > > > > > > > +	.sriov_configure =
virtio_pci_sriov_configure,
> > > > > > > >  };
> > > > > > > >  
> > > > > > > >  module_pci_driver(virtio_pci_driver);
> > > > > > > > diff --git
a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > > > > > > index 2555d80f6eec..07571daccfec 100644
> > > > > > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > > > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > > > > > @@ -153,14 +153,28 @@ static u64
vp_get_features(struct virtio_device *vdev)
> > > > > > > >  	return features;
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > +static void
vp_transport_features(struct virtio_device *vdev, u64 features)
> > > > > > > > +{
> > > > > > > > +	struct virtio_pci_device *vp_dev =
to_vp_device(vdev);
> > > > > > > > +	struct pci_dev *pci_dev =
vp_dev->pci_dev;
> > > > > > > > +
> > > > > > > > +	if ((features &
BIT_ULL(VIRTIO_F_SR_IOV)) &&
> > > > > > > > +			pci_find_ext_capability(pci_dev,
PCI_EXT_CAP_ID_SRIOV))
> > > > > > > > +		__virtio_set_bit(vdev,
VIRTIO_F_SR_IOV);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  /* virtio
config->finalize_features() implementation */
> > > > > > > >  static int vp_finalize_features(struct
virtio_device *vdev)
> > > > > > > >  {
> > > > > > > >  	struct virtio_pci_device *vp_dev =
to_vp_device(vdev);
> > > > > > > > +	u64 features = vdev->features;
> > > > > > > >  
> > > > > > > >  	/* Give virtio_ring a chance to accept
features. */
> > > > > > > >  	vring_transport_features(vdev);
> > > > > > > >  
> > > > > > > > +	/* Give virtio_pci a chance to accept
features. */
> > > > > > > > +	vp_transport_features(vdev, features);
> > > > > > > > +
> > > > > > > >  	if (!__virtio_test_bit(vdev,
VIRTIO_F_VERSION_1)) {
> > > > > > > >  		dev_err(&vdev->dev,
"virtio: device uses modern interface "
> > > > > > > >  			"but does not have
VIRTIO_F_VERSION_1\n");
> > > > > > > > diff --git
a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > > > > > index 308e2096291f..b7c1f4e7d59e 100644
> > > > > > > > --- a/include/uapi/linux/virtio_config.h
> > > > > > > > +++ b/include/uapi/linux/virtio_config.h
> > > > > > > > @@ -49,7 +49,7 @@
> > > > > > > >   * transport being used (eg.
virtio_ring), the rest are per-device feature
> > > > > > > >   * bits. */
> > > > > > > >  #define VIRTIO_TRANSPORT_F_START	28
> > > > > > > > -#define VIRTIO_TRANSPORT_F_END		34
> > > > > > > > +#define VIRTIO_TRANSPORT_F_END		38
> > > > > > > >  
> > > > > > > >  #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > > > > > >  /* Do we get callbacks when the ring is
completely used, even if we've
> > > > > > > > @@ -71,4 +71,9 @@
> > > > > > > >   * this is for compatibility with
legacy systems.
> > > > > > > >   */
> > > > > > > >  #define VIRTIO_F_IOMMU_PLATFORM		33
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * Does the device support Single Root
I/O Virtualization?
> > > > > > > > + */
> > > > > > > > +#define VIRTIO_F_SR_IOV			37
> > > > > > > >  #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H
*/
> > > > > > > > -- 
> > > > > > > > 2.17.0
> > > > > > > 
> > > > > > >
---------------------------------------------------------------------
> > > > > > > To unsubscribe, e-mail:
virtio-dev-unsubscribe at lists.oasis-open.org
> > > > > > > For additional commands, e-mail:
virtio-dev-help at lists.oasis-open.org
> > > > > > >

Seemingly Similar Threads

Search for more reasonably related threads

Linux Virtualization - Jun 2018 - [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

[virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

[virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

[virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

Seemingly Similar Threads