On Thu, Apr 19, 2018 at 5:40 PM, Michael S. Tsirkin [off-list ref] wrote:
On Tue, Apr 03, 2018 at 12:06:03PM -0700, Alexander Duyck wrote:
quoted
On Tue, Apr 3, 2018 at 11:27 AM, Michael S. Tsirkin [off-list ref] wrote:
quoted
On Tue, Apr 03, 2018 at 10:32:00AM -0700, Alexander Duyck wrote:
quoted
On Tue, Apr 3, 2018 at 6:12 AM, Michael S. Tsirkin [off-list ref] wrote:
quoted
On Fri, Mar 16, 2018 at 09:40:34AM -0700, Alexander Duyck wrote:
quoted
On Fri, Mar 16, 2018 at 9:34 AM, Michael S. Tsirkin [off-list ref] wrote:
quoted
On Thu, Mar 15, 2018 at 11:42:41AM -0700, Alexander Duyck wrote:
quoted
From: Alexander Duyck <redacted>
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 <redacted>
Signed-off-by: Alexander Duyck <redacted>
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.
28 more quoted lines
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.
quoted
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.
quoted
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.
quoted
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