| Message ID | 20180315184132.3102.90947.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 15, 2018 at 11:42:41AM -0700, Alexander Duyck wrote: > From: Alexander Duyck <alexander.h.duyck@intel.com> > > Hardware-realized virtio_pci devices can implement SR-IOV, so this > patch enables its use. The device in question is an upcoming Intel > NIC that implements both a virtio_net PF and virtio_net VFs. These > are hardware realizations of what has been up to now been a software > interface. > > The device in question has the following 4-part PCI IDs: > > PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe > VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe > > The patch currently needs no check for device ID, because the callback > will never be made for devices that do not assert the capability or > when run on a platform incapable of SR-IOV. > > One reason for this patch is because the hardware requires the > vendor ID of a VF to be the same as the vendor ID of the PF that > created it. So it seemed logical to simply have a fully-functioning > virtio_net PF create the VFs. This patch makes that possible. > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Mark Rustad <mark.d.rustad@intel.com> > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> So if and when virtio PFs can manage the VFs, then we can add a feature bit for that? Seems reasonable. Also, I am guessing that hardware implementations will want to add things like stong memory barriers - I guess we will add new feature bits for that too down the road? > --- > > v4: Dropped call to pci_disable_sriov in virtio_pci_remove function > v5: Replaced call to pci_sriov_configure_unmanaged with > pci_sriov_configure_simple > v6: Dropped "#ifdef" checks for IOV wrapping sriov_configure definition > v7: No code change, added Reviewed-by > > drivers/virtio/virtio_pci_common.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > index 48d4d1cf1cb6..67a227fd7aa0 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -596,6 +596,7 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) > #ifdef CONFIG_PM_SLEEP > .driver.pm = &virtio_pci_pm_ops, > #endif > + .sriov_configure = pci_sriov_configure_simple, > }; > > module_pci_driver(virtio_pci_driver); > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
On Fri, Mar 16, 2018 at 9:34 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Mar 15, 2018 at 11:42:41AM -0700, Alexander Duyck wrote: >> From: Alexander Duyck <alexander.h.duyck@intel.com> >> >> Hardware-realized virtio_pci devices can implement SR-IOV, so this >> patch enables its use. The device in question is an upcoming Intel >> NIC that implements both a virtio_net PF and virtio_net VFs. These >> are hardware realizations of what has been up to now been a software >> interface. >> >> The device in question has the following 4-part PCI IDs: >> >> PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe >> VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe >> >> The patch currently needs no check for device ID, because the callback >> will never be made for devices that do not assert the capability or >> when run on a platform incapable of SR-IOV. >> >> One reason for this patch is because the hardware requires the >> vendor ID of a VF to be the same as the vendor ID of the PF that >> created it. So it seemed logical to simply have a fully-functioning >> virtio_net PF create the VFs. This patch makes that possible. >> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com> >> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> > > So if and when virtio PFs can manage the VFs, then we can > add a feature bit for that? > Seems reasonable. Yes. If nothing else you may not even need a feature bit depending on how things go. One of the reasons why Mark called out the subvendor/subdevice was because that might be able to be used to identify the specific hardware that is providing the SR-IOV feature so in the future if it is added to virtio itself then you could exclude devices like this by just limiting things based on subvendor/subdevice IDs. > Also, I am guessing that hardware implementations will want > to add things like stong memory barriers - I guess we > will add new feature bits for that too down the road? That piece I don't have visibility into at this time. Perhaps Dan might have more visibility into future plans on what this might need. Thanks. - Alex
On Mar 15, 2018, at 11:42 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote: > From: Alexander Duyck <alexander.h.duyck@intel.com> > > Hardware-realized virtio_pci devices can implement SR-IOV, so this > patch enables its use. The device in question is an upcoming Intel > NIC that implements both a virtio_net PF and virtio_net VFs. These > are hardware realizations of what has been up to now been a software > interface. > > The device in question has the following 4-part PCI IDs: > > PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe > VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe > > The patch currently needs no check for device ID, because the callback > will never be made for devices that do not assert the capability or > when run on a platform incapable of SR-IOV. > > One reason for this patch is because the hardware requires the > vendor ID of a VF to be the same as the vendor ID of the PF that > created it. So it seemed logical to simply have a fully-functioning > virtio_net PF create the VFs. This patch makes that possible. > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Mark Rustad <mark.d.rustad@intel.com> > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> > --- > > v4: Dropped call to pci_disable_sriov in virtio_pci_remove function > v5: Replaced call to pci_sriov_configure_unmanaged with > pci_sriov_configure_simple > v6: Dropped "#ifdef" checks for IOV wrapping sriov_configure definition > v7: No code change, added Reviewed-by > > drivers/virtio/virtio_pci_common.c | 1 + > 1 file changed, 1 insertion(+) Tested with the identified device. Tested-by: Mark Rustad <mark.d.rustad@intel.com> -- Mark Rustad, Networking Division, Intel Corporation
On Thu, Mar 15, 2018 at 11:42:41AM -0700, Alexander Duyck wrote: > From: Alexander Duyck <alexander.h.duyck@intel.com> > > Hardware-realized virtio_pci devices can implement SR-IOV, so this > patch enables its use. The device in question is an upcoming Intel > NIC that implements both a virtio_net PF and virtio_net VFs. These > are hardware realizations of what has been up to now been a software > interface. > > The device in question has the following 4-part PCI IDs: > > PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe > VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe > > The patch currently needs no check for device ID, because the callback > will never be made for devices that do not assert the capability or > when run on a platform incapable of SR-IOV. > > One reason for this patch is because the hardware requires the > vendor ID of a VF to be the same as the vendor ID of the PF that > created it. So it seemed logical to simply have a fully-functioning > virtio_net PF create the VFs. This patch makes that possible. > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Mark Rustad <mark.d.rustad@intel.com> > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> I thought hard about this, and I think we need a feature bit for this. This way host can detect support, and we can also change our minds later if we need to modify the interface and manage VFs after all. It seems PCI specific so non pci transports would disable the feature for now. > --- > > v4: Dropped call to pci_disable_sriov in virtio_pci_remove function > v5: Replaced call to pci_sriov_configure_unmanaged with > pci_sriov_configure_simple > v6: Dropped "#ifdef" checks for IOV wrapping sriov_configure definition > v7: No code change, added Reviewed-by > > drivers/virtio/virtio_pci_common.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > index 48d4d1cf1cb6..67a227fd7aa0 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -596,6 +596,7 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) > #ifdef CONFIG_PM_SLEEP > .driver.pm = &virtio_pci_pm_ops, > #endif > + .sriov_configure = pci_sriov_configure_simple, > }; > > module_pci_driver(virtio_pci_driver); > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
On Fri, Mar 16, 2018 at 09:40:34AM -0700, Alexander Duyck wrote: > On Fri, Mar 16, 2018 at 9:34 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Mar 15, 2018 at 11:42:41AM -0700, Alexander Duyck wrote: > >> From: Alexander Duyck <alexander.h.duyck@intel.com> > >> > >> Hardware-realized virtio_pci devices can implement SR-IOV, so this > >> patch enables its use. The device in question is an upcoming Intel > >> NIC that implements both a virtio_net PF and virtio_net VFs. These > >> are hardware realizations of what has been up to now been a software > >> interface. > >> > >> The device in question has the following 4-part PCI IDs: > >> > >> PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe > >> VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe > >> > >> The patch currently needs no check for device ID, because the callback > >> will never be made for devices that do not assert the capability or > >> when run on a platform incapable of SR-IOV. > >> > >> One reason for this patch is because the hardware requires the > >> vendor ID of a VF to be the same as the vendor ID of the PF that > >> created it. So it seemed logical to simply have a fully-functioning > >> virtio_net PF create the VFs. This patch makes that possible. > >> > >> Reviewed-by: Christoph Hellwig <hch@lst.de> > >> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com> > >> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> > > > > So if and when virtio PFs can manage the VFs, then we can > > add a feature bit for that? > > Seems reasonable. > > Yes. If nothing else you may not even need a feature bit depending on > how things go. OTOH if the interface is changed in an incompatible way, and old Linux will attempt to drive the new device since there is no check. I think we should add a feature bit right away. > One of the reasons why Mark called out the > subvendor/subdevice was because that might be able to be used to > identify the specific hardware that is providing the SR-IOV feature so > in the future if it is added to virtio itself then you could exclude > devices like this by just limiting things based on subvendor/subdevice > IDs. > > > Also, I am guessing that hardware implementations will want > > to add things like stong memory barriers - I guess we > > will add new feature bits for that too down the road? > > That piece I don't have visibility into at this time. Perhaps Dan > might have more visibility into future plans on what this might need. > > Thanks. > > - Alex
On Tue, Apr 3, 2018 at 6:12 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Fri, Mar 16, 2018 at 09:40:34AM -0700, Alexander Duyck wrote: >> On Fri, Mar 16, 2018 at 9:34 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Thu, Mar 15, 2018 at 11:42:41AM -0700, Alexander Duyck wrote: >> >> From: Alexander Duyck <alexander.h.duyck@intel.com> >> >> >> >> Hardware-realized virtio_pci devices can implement SR-IOV, so this >> >> patch enables its use. The device in question is an upcoming Intel >> >> NIC that implements both a virtio_net PF and virtio_net VFs. These >> >> are hardware realizations of what has been up to now been a software >> >> interface. >> >> >> >> The device in question has the following 4-part PCI IDs: >> >> >> >> PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe >> >> VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe >> >> >> >> The patch currently needs no check for device ID, because the callback >> >> will never be made for devices that do not assert the capability or >> >> when run on a platform incapable of SR-IOV. >> >> >> >> One reason for this patch is because the hardware requires the >> >> vendor ID of a VF to be the same as the vendor ID of the PF that >> >> created it. So it seemed logical to simply have a fully-functioning >> >> virtio_net PF create the VFs. This patch makes that possible. >> >> >> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> >> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com> >> >> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> >> > >> > So if and when virtio PFs can manage the VFs, then we can >> > add a feature bit for that? >> > Seems reasonable. >> >> Yes. If nothing else you may not even need a feature bit depending on >> how things go. > > OTOH if the interface is changed in an incompatible way, > and old Linux will attempt to drive the new device > since there is no check. > > I think we should add a feature bit right away. I'm not sure why you would need a feature bit. The capability is controlled via PCI configuration space. If it is present the device has the capability. If it is not then it does not. Basically if the PCI configuration space is not present then the sysfs entries will not be spawned and nothing will attempt to use this function. - ALex
On Tue, Apr 03, 2018 at 10:32:00AM -0700, Alexander Duyck wrote: > On Tue, Apr 3, 2018 at 6:12 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Mar 16, 2018 at 09:40:34AM -0700, Alexander Duyck wrote: > >> On Fri, Mar 16, 2018 at 9:34 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > On Thu, Mar 15, 2018 at 11:42:41AM -0700, Alexander Duyck wrote: > >> >> From: Alexander Duyck <alexander.h.duyck@intel.com> > >> >> > >> >> Hardware-realized virtio_pci devices can implement SR-IOV, so this > >> >> patch enables its use. The device in question is an upcoming Intel > >> >> NIC that implements both a virtio_net PF and virtio_net VFs. These > >> >> are hardware realizations of what has been up to now been a software > >> >> interface. > >> >> > >> >> The device in question has the following 4-part PCI IDs: > >> >> > >> >> PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe > >> >> VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe > >> >> > >> >> The patch currently needs no check for device ID, because the callback > >> >> will never be made for devices that do not assert the capability or > >> >> when run on a platform incapable of SR-IOV. > >> >> > >> >> One reason for this patch is because the hardware requires the > >> >> vendor ID of a VF to be the same as the vendor ID of the PF that > >> >> created it. So it seemed logical to simply have a fully-functioning > >> >> virtio_net PF create the VFs. This patch makes that possible. > >> >> > >> >> Reviewed-by: Christoph Hellwig <hch@lst.de> > >> >> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com> > >> >> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> > >> > > >> > So if and when virtio PFs can manage the VFs, then we can > >> > add a feature bit for that? > >> > Seems reasonable. > >> > >> Yes. If nothing else you may not even need a feature bit depending on > >> how things go. > > > > OTOH if the interface is changed in an incompatible way, > > and old Linux will attempt to drive the new device > > since there is no check. > > > > I think we should add a feature bit right away. > > I'm not sure why you would need a feature bit. The capability is > controlled via PCI configuration space. If it is present the device > has the capability. If it is not then it does not. > > Basically if the PCI configuration space is not present then the sysfs > entries will not be spawned and nothing will attempt to use this > function. > > - ALex It's about compability with older guests which ignore the capability. The feature is thus helpful so host knows whether guest supports VFs.
On Tue, Apr 3, 2018 at 11:27 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, Apr 03, 2018 at 10:32:00AM -0700, Alexander Duyck wrote: >> On Tue, Apr 3, 2018 at 6:12 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Fri, Mar 16, 2018 at 09:40:34AM -0700, Alexander Duyck wrote: >> >> On Fri, Mar 16, 2018 at 9:34 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> > On Thu, Mar 15, 2018 at 11:42:41AM -0700, Alexander Duyck wrote: >> >> >> From: Alexander Duyck <alexander.h.duyck@intel.com> >> >> >> >> >> >> Hardware-realized virtio_pci devices can implement SR-IOV, so this >> >> >> patch enables its use. The device in question is an upcoming Intel >> >> >> NIC that implements both a virtio_net PF and virtio_net VFs. These >> >> >> are hardware realizations of what has been up to now been a software >> >> >> interface. >> >> >> >> >> >> The device in question has the following 4-part PCI IDs: >> >> >> >> >> >> PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe >> >> >> VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe >> >> >> >> >> >> The patch currently needs no check for device ID, because the callback >> >> >> will never be made for devices that do not assert the capability or >> >> >> when run on a platform incapable of SR-IOV. >> >> >> >> >> >> One reason for this patch is because the hardware requires the >> >> >> vendor ID of a VF to be the same as the vendor ID of the PF that >> >> >> created it. So it seemed logical to simply have a fully-functioning >> >> >> virtio_net PF create the VFs. This patch makes that possible. >> >> >> >> >> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> >> >> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com> >> >> >> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> >> >> > >> >> > So if and when virtio PFs can manage the VFs, then we can >> >> > add a feature bit for that? >> >> > Seems reasonable. >> >> >> >> Yes. If nothing else you may not even need a feature bit depending on >> >> how things go. >> > >> > OTOH if the interface is changed in an incompatible way, >> > and old Linux will attempt to drive the new device >> > since there is no check. >> > >> > I think we should add a feature bit right away. >> >> I'm not sure why you would need a feature bit. The capability is >> controlled via PCI configuration space. If it is present the device >> has the capability. If it is not then it does not. >> >> Basically if the PCI configuration space is not present then the sysfs >> entries will not be spawned and nothing will attempt to use this >> function. >> >> - ALex > > It's about compability with older guests which ignore the > capability. > > The feature is thus helpful so host knows whether guest supports VFs. The thing is if the capability is ignored then the feature isn't used. So for SR-IOV it isn't an uncommon thing for there to be drivers for the PF floating around that do not support SR-IOV. In such cases SR-IOV just isn't used while the hardware could support it. I would think in the case of virtio it would be the same kind of thing. Basically if SR-IOV is supported by the host then the capability would be present. If SR-IOV is supported by the guest then it would make use of the capability to spawn VFs. If either the capability isn't present, or the driver doesn't use it then you won't be able to spawn VFs in the guest. Maybe I am missing something. Do you support dynamically changing the PCI configuration space for Virtio devices based on the presence of feature bits provided by the guest? Also are you saying this patch set should wait on the feature bit to be added, or are you talking about doing this as some sort of follow-up? - Alex
On Apr 3, 2018, at 11:27 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> I'm not sure why you would need a feature bit. The capability is >> controlled via PCI configuration space. If it is present the device >> has the capability. If it is not then it does not. >> >> Basically if the PCI configuration space is not present then the sysfs >> entries will not be spawned and nothing will attempt to use this >> function. >> >> - ALex > > It's about compability with older guests which ignore the > capability. > > The feature is thus helpful so host knows whether guest supports VFs. This is not about a guest creating its own VFs. This is about a host PF that happens to have a virtio interface to be able to create virtio VFs that can be assigned to guests. Nothing changes at all from a guest perspective. Or maybe I am not understanding what you mean by "whether guest supports VFs". -- Mark Rustad, Networking Division, Intel Corporation
On Tue, Apr 03, 2018 at 12:06:03PM -0700, Alexander Duyck wrote: > On Tue, Apr 3, 2018 at 11:27 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Apr 03, 2018 at 10:32:00AM -0700, Alexander Duyck wrote: > >> On Tue, Apr 3, 2018 at 6:12 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > On Fri, Mar 16, 2018 at 09:40:34AM -0700, Alexander Duyck wrote: > >> >> On Fri, Mar 16, 2018 at 9:34 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > >> >> > On Thu, Mar 15, 2018 at 11:42:41AM -0700, Alexander Duyck wrote: > >> >> >> From: Alexander Duyck <alexander.h.duyck@intel.com> > >> >> >> > >> >> >> Hardware-realized virtio_pci devices can implement SR-IOV, so this > >> >> >> patch enables its use. The device in question is an upcoming Intel > >> >> >> NIC that implements both a virtio_net PF and virtio_net VFs. These > >> >> >> are hardware realizations of what has been up to now been a software > >> >> >> interface. > >> >> >> > >> >> >> The device in question has the following 4-part PCI IDs: > >> >> >> > >> >> >> PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe > >> >> >> VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe > >> >> >> > >> >> >> The patch currently needs no check for device ID, because the callback > >> >> >> will never be made for devices that do not assert the capability or > >> >> >> when run on a platform incapable of SR-IOV. > >> >> >> > >> >> >> One reason for this patch is because the hardware requires the > >> >> >> vendor ID of a VF to be the same as the vendor ID of the PF that > >> >> >> created it. So it seemed logical to simply have a fully-functioning > >> >> >> virtio_net PF create the VFs. This patch makes that possible. > >> >> >> > >> >> >> Reviewed-by: Christoph Hellwig <hch@lst.de> > >> >> >> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com> > >> >> >> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> > >> >> > > >> >> > So if and when virtio PFs can manage the VFs, then we can > >> >> > add a feature bit for that? > >> >> > Seems reasonable. > >> >> > >> >> Yes. If nothing else you may not even need a feature bit depending on > >> >> how things go. > >> > > >> > OTOH if the interface is changed in an incompatible way, > >> > and old Linux will attempt to drive the new device > >> > since there is no check. > >> > > >> > I think we should add a feature bit right away. > >> > >> I'm not sure why you would need a feature bit. The capability is > >> controlled via PCI configuration space. If it is present the device > >> has the capability. If it is not then it does not. > >> > >> Basically if the PCI configuration space is not present then the sysfs > >> entries will not be spawned and nothing will attempt to use this > >> function. > >> > >> - ALex > > > > It's about compability with older guests which ignore the > > capability. > > > > The feature is thus helpful so host knows whether guest supports VFs. > > The thing is if the capability is ignored then the feature isn't used. > So for SR-IOV it isn't an uncommon thing for there to be drivers for > the PF floating around that do not support SR-IOV. In such cases > SR-IOV just isn't used while the hardware could support it. Right but how come there are VF drivers but PF driver does not know about these? And are there PF drivers that intentially do not enable SRIOV because it's known to be broken in some way? Case in point I do think virtio want to limit this depending on a feature bit on general principles (the principle being that all extensions have feature bits). There are security implications here - we previously relied on whitelisting after all. Wouldn't it be safer to be a bit more careful and update the actual PF drivers? It's just one line per driver, but it can be done with an ack by driver maintainer. If/once we find out all drivers do have it, we can then change the default. > I would think in the case of virtio it would be the same kind of > thing. Basically if SR-IOV is supported by the host then the > capability would be present. If SR-IOV is supported by the guest then > it would make use of the capability to spawn VFs. If either the > capability isn't present, or the driver doesn't use it then you won't > be able to spawn VFs in the guest. > Maybe I am missing something. Do you support dynamically changing the > PCI configuration space for Virtio devices based on the presence of > feature bits provided by the guest? No. The point is that IMHO at least virtio - in absence of feature bit - to ignore VFs rather than assume they are safe to drive in an unmanaged way. > Also are you saying this patch set should wait on the feature bit to > be added, or are you talking about doing this as some sort of > follow-up? > > - Alex I think for virtio it should include the feature bit, yes. Adding feature bit is very easy - post a patch to the virtio TC mailing list, wait about a week to give people time to respond (two weeks if it is around holidays and such).
On Thu, Apr 19, 2018 at 5:40 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, Apr 03, 2018 at 12:06:03PM -0700, Alexander Duyck wrote: >> On Tue, Apr 3, 2018 at 11:27 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Tue, Apr 03, 2018 at 10:32:00AM -0700, Alexander Duyck wrote: >> >> On Tue, Apr 3, 2018 at 6:12 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> > On Fri, Mar 16, 2018 at 09:40:34AM -0700, Alexander Duyck wrote: >> >> >> On Fri, Mar 16, 2018 at 9:34 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> >> > On Thu, Mar 15, 2018 at 11:42:41AM -0700, Alexander Duyck wrote: >> >> >> >> From: Alexander Duyck <alexander.h.duyck@intel.com> >> >> >> >> >> >> >> >> Hardware-realized virtio_pci devices can implement SR-IOV, so this >> >> >> >> patch enables its use. The device in question is an upcoming Intel >> >> >> >> NIC that implements both a virtio_net PF and virtio_net VFs. These >> >> >> >> are hardware realizations of what has been up to now been a software >> >> >> >> interface. >> >> >> >> >> >> >> >> The device in question has the following 4-part PCI IDs: >> >> >> >> >> >> >> >> PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe >> >> >> >> VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe >> >> >> >> >> >> >> >> The patch currently needs no check for device ID, because the callback >> >> >> >> will never be made for devices that do not assert the capability or >> >> >> >> when run on a platform incapable of SR-IOV. >> >> >> >> >> >> >> >> One reason for this patch is because the hardware requires the >> >> >> >> vendor ID of a VF to be the same as the vendor ID of the PF that >> >> >> >> created it. So it seemed logical to simply have a fully-functioning >> >> >> >> virtio_net PF create the VFs. This patch makes that possible. >> >> >> >> >> >> >> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> >> >> >> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com> >> >> >> >> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> >> >> >> > >> >> >> > So if and when virtio PFs can manage the VFs, then we can >> >> >> > add a feature bit for that? >> >> >> > Seems reasonable. >> >> >> >> >> >> Yes. If nothing else you may not even need a feature bit depending on >> >> >> how things go. >> >> > >> >> > OTOH if the interface is changed in an incompatible way, >> >> > and old Linux will attempt to drive the new device >> >> > since there is no check. >> >> > >> >> > I think we should add a feature bit right away. >> >> >> >> I'm not sure why you would need a feature bit. The capability is >> >> controlled via PCI configuration space. If it is present the device >> >> has the capability. If it is not then it does not. >> >> >> >> Basically if the PCI configuration space is not present then the sysfs >> >> entries will not be spawned and nothing will attempt to use this >> >> function. >> >> >> >> - ALex >> > >> > It's about compability with older guests which ignore the >> > capability. >> > >> > The feature is thus helpful so host knows whether guest supports VFs. >> >> The thing is if the capability is ignored then the feature isn't used. >> So for SR-IOV it isn't an uncommon thing for there to be drivers for >> the PF floating around that do not support SR-IOV. In such cases >> SR-IOV just isn't used while the hardware could support it. > > Right but how come there are VF drivers but PF driver does not > know about these? I'm not sure what you mean here. The VF and PF drivers are the same driver. The only difference is that the PF has the extra SR-IOV configuration space. What this code is meant to enable is a form of SR-IOV where the VFs are essentially pre-allocated resources. So for example in our case the MMIO space is identical for a PF versus any of the VFs. It doesn't have any special controls in place to allow the PF to manipulate any of the resources belonging to the VFs. > And are there PF drivers that intentially do not enable SRIOV > because it's known to be broken in some way? In the Virtio IO case right now are there any devices that support SR-IOV? For now this is just an add-on bit to a function that is already emulating the Virtio in hardware. > Case in point I do think virtio want to limit this > depending on a feature bit on general principles > (the principle being that all extensions have feature bits). This part has me kind of scratching my head. In our setup the "PF" is really nothing more than a "VF" with the SR-IOV configuration space attached to it. There are already examples of similar designs for NVMe and the Amazon ENA devices. Giving the "PF" any functionality in MMIO space that controls the SR-IOV kind of defeats the whole point of allowing this function in the first place. Basically the PF isn't really controlling things, it is the kernel that is doing it. > There are security implications here - we previously relied on > whitelisting after all. Yes and no. The original patch set had issues as you could have a PF assigned to user space and the VFs managed by the host. When I changed things so that the function had to be in a kernel driver that issue went away. > Wouldn't it be safer to be a bit more careful and update the > actual PF drivers? It's just one line per driver, but it > can be done with an ack by driver maintainer. > If/once we find out all drivers do have it, we can then > change the default. I have no clue what you are talking about here. This is the more careful approach. Are you sure you are reviewing the v7 of the patches? My understanding is that no paravirtual interfaces currently expose SR-IOV. What we are looking at is hardware will want to emulate Virtio, specifically virtio_net in the future and as a part of that the PF ends up emulating it as well. What we would need to watch for going forward is that any device that enables SR-IOV support would need to also provide a 4 tuple ID so that if something goes wrong with it we could disable SR-IOV on the device via a PCI quirk later. >> I would think in the case of virtio it would be the same kind of >> thing. Basically if SR-IOV is supported by the host then the >> capability would be present. If SR-IOV is supported by the guest then >> it would make use of the capability to spawn VFs. If either the >> capability isn't present, or the driver doesn't use it then you won't >> be able to spawn VFs in the guest. > >> Maybe I am missing something. Do you support dynamically changing the >> PCI configuration space for Virtio devices based on the presence of >> feature bits provided by the guest? > > No. The point is that IMHO at least virtio - in absence of feature bit - > to ignore VFs rather than assume they are safe to drive > in an unmanaged way. > >> Also are you saying this patch set should wait on the feature bit to >> be added, or are you talking about doing this as some sort of >> follow-up? >> >> - Alex > > I think for virtio it should include the feature bit, yes. > Adding feature bit is very easy - post a patch to the virtio TC mailing > list, wait about a week to give people time to respond (two weeks if it > is around holidays and such). The problem is we are talking about hardware/FPGA, not software. Adding a feature bit means going back and updating RTL. The software side of things is easy, re-validating things after a hardware/FPGA change not so much. If this is a hard requirement I may just drop the virtio patch, push what I have, and leave it to Mark/Dan to deal with the necessary RTL and code changes needed to support Virtio as I don't expect the turnaround to be as easy as just a patch. Thanks. - Alex
On Fri, Apr 20, 2018 at 07:56:14AM -0700, Alexander Duyck wrote: > > I think for virtio it should include the feature bit, yes. > > Adding feature bit is very easy - post a patch to the virtio TC mailing > > list, wait about a week to give people time to respond (two weeks if it > > is around holidays and such). > > The problem is we are talking about hardware/FPGA, not software. > Adding a feature bit means going back and updating RTL. The software > side of things is easy, re-validating things after a hardware/FPGA > change not so much. > > If this is a hard requirement I may just drop the virtio patch, push > what I have, and leave it to Mark/Dan to deal with the necessary RTL > and code changes needed to support Virtio as I don't expect the > turnaround to be as easy as just a patch. > > Thanks. > > - Alex Let's focus on virtio in this thread. Involving the virtio TC in host/guest interface changes is a hard requirement. It's just too easy to create conflicts otherwise. So you guys should have just sent the proposal to the TC when you were doing your RTL and you would have been in the clear. Generally adding a feature bit with any extension is a good idea: this way you merely reserve a feature bit for your feature through the TC and are more or less sure of forward and backward compatibility. It's incredibly easy. But maybe it's not needed here. I am not making the decisions myself. Not too late: post to the TC list and let's see what the response is. Without a feature bit you are making a change affecting all future implementations without exception so the bar is a bit higher: you need to actually post a spec text proposal not just a patch showing how to use the feature, and TC needs to vote on it. Voting takes a week, review a week or two depending on change complexity. Hope this helps,
On Fri, Apr 20, 2018 at 8:28 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Fri, Apr 20, 2018 at 07:56:14AM -0700, Alexander Duyck wrote: >> > I think for virtio it should include the feature bit, yes. >> > Adding feature bit is very easy - post a patch to the virtio TC mailing >> > list, wait about a week to give people time to respond (two weeks if it >> > is around holidays and such). >> >> The problem is we are talking about hardware/FPGA, not software. >> Adding a feature bit means going back and updating RTL. The software >> side of things is easy, re-validating things after a hardware/FPGA >> change not so much. >> >> If this is a hard requirement I may just drop the virtio patch, push >> what I have, and leave it to Mark/Dan to deal with the necessary RTL >> and code changes needed to support Virtio as I don't expect the >> turnaround to be as easy as just a patch. >> >> Thanks. >> >> - Alex > > Let's focus on virtio in this thread. That is kind of what I was thinking, and why I was thinking it might make sense to make the virtio specific changes a separate patch set. I could get the PCI bits taken care of in the meantime since they effect genetic PCI, NVMe, and the Amazon ENA interfaces. > Involving the virtio TC in host/guest interface changes is a > hard requirement. It's just too easy to create conflicts otherwise. > > So you guys should have just sent the proposal to the TC when you > were doing your RTL and you would have been in the clear. Agreed. I believe I brought this up when I was originally asked to look into the coding for this. > Generally adding a feature bit with any extension is a good idea: > this way you merely reserve a feature bit for your feature through > the TC and are more or less sure of forward and backward compatibility. > It's incredibly easy. Agreed, though in this case I am not sure it makes sense since this isn't necessarily something that is a Virtio feature itself. It is just a side effect of the fact that they are adding SR-IOV support to a device that happens to emulate Virtio NET and apparently their PF has to be identical to the VF other than the PCIe extended config space. > But maybe it's not needed here. I am not making the decisions myself. > Not too late: post to the TC list and let's see what the response is. > Without a feature bit you are making a change affecting all future > implementations without exception so the bar is a bit higher: you need > to actually post a spec text proposal not just a patch showing how to > use the feature, and TC needs to vote on it. Voting takes a week, > review a week or two depending on change complexity. > > Hope this helps, > > -- > MST I think I will leave this for Dan and Mark to handle since I am still not all that familiar with the hardware in use here. Once a decision has been made him and Mark could look at pushing either the one line patch or something more complex involving a feature flag. Thanks. Alex
On Fri, Apr 20, 2018 at 09:08:51AM -0700, Alexander Duyck wrote: > On Fri, Apr 20, 2018 at 8:28 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Apr 20, 2018 at 07:56:14AM -0700, Alexander Duyck wrote: > >> > I think for virtio it should include the feature bit, yes. > >> > Adding feature bit is very easy - post a patch to the virtio TC mailing > >> > list, wait about a week to give people time to respond (two weeks if it > >> > is around holidays and such). > >> > >> The problem is we are talking about hardware/FPGA, not software. > >> Adding a feature bit means going back and updating RTL. The software > >> side of things is easy, re-validating things after a hardware/FPGA > >> change not so much. > >> > >> If this is a hard requirement I may just drop the virtio patch, push > >> what I have, and leave it to Mark/Dan to deal with the necessary RTL > >> and code changes needed to support Virtio as I don't expect the > >> turnaround to be as easy as just a patch. > >> > >> Thanks. > >> > >> - Alex > > > > Let's focus on virtio in this thread. > > That is kind of what I was thinking, and why I was thinking it might > make sense to make the virtio specific changes a separate patch set. I > could get the PCI bits taken care of in the meantime since they effect > genetic PCI, NVMe, and the Amazon ENA interfaces. > > > Involving the virtio TC in host/guest interface changes is a > > hard requirement. It's just too easy to create conflicts otherwise. > > > > So you guys should have just sent the proposal to the TC when you > > were doing your RTL and you would have been in the clear. > > Agreed. I believe I brought this up when I was originally asked to > look into the coding for this. > > > Generally adding a feature bit with any extension is a good idea: > > this way you merely reserve a feature bit for your feature through > > the TC and are more or less sure of forward and backward compatibility. > > It's incredibly easy. > > Agreed, though in this case I am not sure it makes sense since this > isn't necessarily something that is a Virtio feature itself. It is > just a side effect of the fact that they are adding SR-IOV support to > a device that happens to emulate Virtio NET and apparently their PF > has to be identical to the VF other than the PCIe extended config > space. I got that. My point is not everyone implementing SR-IOV will want to do it like this. Others might want to have VFs be different from PFs somehow. Feature bits ensure forward not just backward compatibility. > > But maybe it's not needed here. I am not making the decisions myself. > > Not too late: post to the TC list and let's see what the response is. > > Without a feature bit you are making a change affecting all future > > implementations without exception so the bar is a bit higher: you need > > to actually post a spec text proposal not just a patch showing how to > > use the feature, and TC needs to vote on it. Voting takes a week, > > review a week or two depending on change complexity. > > > > Hope this helps, > > > > -- > > MST > > I think I will leave this for Dan and Mark to handle since I am still > not all that familiar with the hardware in use here. Once a decision > has been made him and Mark could look at pushing either the one line > patch or something more complex involving a feature flag. > > Thanks. > > Alex As long as the TC is involved. I know it's a bit of a strange thing to block it at the driver level, the issue is with the device, but it's literally the only handle I have to prevent people from doing out of spec hacks then pushing it all on us to maintain.
On Fri, Apr 20, 2018 at 06:28:50PM +0300, Michael S. Tsirkin wrote: > But maybe it's not needed here. I am not making the decisions myself. > Not too late: post to the TC list and let's see what the response is. > Without a feature bit you are making a change affecting all future > implementations without exception so the bar is a bit higher: you need > to actually post a spec text proposal not just a patch showing how to > use the feature, and TC needs to vote on it. Voting takes a week, > review a week or two depending on change complexity. Also IFF the hardware already is out we can quirk it in the PCI ID table to manually set the feature in the driver as a workaround.
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c index 48d4d1cf1cb6..67a227fd7aa0 100644 --- a/drivers/virtio/virtio_pci_common.c +++ b/drivers/virtio/virtio_pci_common.c @@ -596,6 +596,7 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) #ifdef CONFIG_PM_SLEEP .driver.pm = &virtio_pci_pm_ops, #endif + .sriov_configure = pci_sriov_configure_simple, }; module_pci_driver(virtio_pci_driver);