On 07/11/2024 23:25, Alex Williamson wrote:
On Thu, 7 Nov 2024 14:57:39 +0200
Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
On 07/11/2024 0:27, Alex Williamson wrote:
On Wed, 6 Nov 2024 09:59:09 -0400
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
On Tue, Nov 05, 2024 at 04:29:04PM -0700, Alex Williamson wrote:
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
config VIRTIO_VFIO_PCI
tristate "VFIO support for VIRTIO NET PCI devices"
- depends on VIRTIO_PCI && VIRTIO_PCI_ADMIN_LEGACY
+ depends on VIRTIO_PCI
select VFIO_PCI_CORE
help
This provides support for exposing VIRTIO NET VF devices which support
@@ -11,5 +11,7 @@ config VIRTIO_VFIO_PCI
As of that this driver emulates I/O BAR in software to let a VF be
seen as a transitional device by its users and let it work with
a legacy driver.
+ In addition, it provides migration support for VIRTIO NET VF devices
+ using the VFIO framework.
The first half of this now describes something that may or may not be
enabled by this config option and the additional help text for
migration is vague enough relative to PF requirements to get user
reports that the driver doesn't work as intended.
Yes, I think the help should be clearer
For the former, maybe we still want a separate config item that's
optionally enabled if VIRTIO_VFIO_PCI && VFIO_PCI_ADMIN_LEGACY.
If we are going to add a bunch of #ifdefs/etc for ADMIN_LEGACY we
may as well just use VIRTIO_PCI_ADMIN_LEGACY directly and not
introduce another kconfig for it?
I think that's what Yishai is proposing, but as we're adding a whole
new feature to the driver I'm concerned how the person configuring the
kernel knows which features from the description might be available in
the resulting driver.
We could maybe solve that with a completely re-written help text that
describes the legacy feature as X86-only and migration as a separate
architecture independent feature, but people aren't great at reading
and part of the audience is going to see "X86" in their peripheral
vision and disable it, and maybe even complain that the text was
presented to them.
OR, we can just add an optional sub-config bool that makes it easier to
describe the (new) main feature of the driver as supporting live
migration (on supported hardware) and the sub-config option as
providing legacy support (on supported hardware), and that sub-config
is only presented on X86, ie. ADMIN_LEGACY.
Ultimately the code already needs to support #ifdefs for the latter and
I think it's more user friendly and versatile to have the separate
config option.
NB. The sub-config should be default on for upgrade compatibility.
Is there any reason to compile out the migration support for virtio?
No other drivers were doing this?
No other vfio-pci variant driver provides multiple, independent
features, so for instance to compile out migration support from the
vfio-pci-mlx5 driver is to simply disable the driver altogether.
kconfig combinations are painful, it woudl be nice to not make too
many..
I'm not arguing for a legacy-only, non-migration version (please speak
up if someone wants that). The code already needs to support the
#ifdefs and I think reflecting that in a sub-config option helps
clarify what the driver is providing and conveniently makes it possible
to have a driver with exactly the same feature set across archs, if
desired. Thanks,
Since the live migration functionality is not architecture-dependent
(unlike legacy access, which requires X86) and is likely to be the
primary use of the driver, I would suggest keeping it outside of any
#ifdef directives, as initially introduced in V1.
To address the description issue and provide control for customers who
may need the legacy access functionality, we could introduce a bool
configuration option as a submenu under the driver’s main live migration
feature.
This approach will keep things simple and align with the typical use
case of the driver.
Something like the below [1] can do the job for that.
Alex,
Can that work for you ?
By the way, you have suggested calling the config entry
VFIO_PCI_ADMIN_LEGACY, don't we need to add here also the VIRTIO as a
prefix ? (i.e. VIRTIO_VFIO_PCI_ADMIN_LEGACY)
I think that was just a typo referring to VIRTIO_PCI_ADMIN_LEGACY.
Yes, appending _ADMIN_LEGACY to the main config option is fine.
[1]
# SPDX-License-Identifier: GPL-2.0-only
config VIRTIO_VFIO_PCI
tristate "VFIO support for live migration over VIRTIO NET PCI
devices"
Looking at other variant drivers, I think this should just be:
"VFIO support for VIRTIO NET PCI VF devices"
depends on VIRTIO_PCI
select VFIO_PCI_CORE
select IOMMUFD_DRIVER
IIUC, this is not a dependency, the device will just lack dirty page
tracking with either the type1 backend or when using iommufd when the
IOMMU hardware doesn't have dirty page tracking, therefore all VM
memory is perpetually dirty. Do I have that right?
IOMMUFD_DRIVER is selected to utilize the dirty tracking functionality
of IOMMU. Therefore, this is a select option rather than a dependency,
similar to how the pds and mlx5 VFIO drivers handle it in their Kconfig
files.
help
This provides migration support for VIRTIO NET PCI VF devices
using the VFIO framework.
This is still too open ended for me, there is specific PF support
required in the device to make this work. Maybe...
This provides migration support for VIRTIO NET PCI VF devices
using the VFIO framework. Migration support requires the
SR-IOV PF device to support specific VIRTIO extensions,
otherwise this driver provides no additional functionality
beyond vfio-pci.
Migration support in this driver relies on dirty page tracking
provided by the IOMMU hardware and exposed through IOMMUFD, any
other use cases are dis-recommended.
If you don't know what to do here, say N.
Looks good.
config VFIO_PCI_ADMIN_LEGACY
VIRTIO_VFIO_PCI_ADMIN_LEGACY
bool "VFIO support for legacy I/O access for VIRTIO NET PCI
devices"
Maybe:
"Legacy I/O support for VIRTIO NET PCI VF devices"
depends on VIRTIO_VFIO_PCI && VIRTIO_PCI_ADMIN_LEGACY
default y
help
This provides support for exposing VIRTIO NET VF devices which
support legacy IO access, using the VFIO framework that can
work with a legacy virtio driver in the guest.
Based on PCIe spec, VFs do not support I/O Space.
As of that this driver emulates I/O BAR in software to let a
VF be seen as a transitional device by its users and let it
work with a legacy driver.
Maybe:
This extends the virtio-vfio-pci driver to support legacy I/O
access, allowing use of legacy virtio drivers with VIRTIO NET
PCI VF devices. Legacy I/O support requires the SR-IOV PF
device to support and enable specific VIRTIO extensions,
otherwise this driver provides no additional functionality
beyond vfio-pci.
Looks good.
IMO, noting the PF requirement in each is important (feel free to
elaborate on specific VIRTIO extension requirements). It doesn't seem
necessary to explain how the legacy compatibility works, only that this
driver makes the VF compatible with the legacy driver.
Are both of these options configurable at the PF in either firmware or
software?
These options are configured in the firmware.
I used "support and enable" in the legacy section assuming
that there is such a knob, but for migration it seems less necessary
that there's an enable step. Please correct based on the actual
device behavior. Thanks,
Migration is such a basic functionality that we may expect it to be
enabled by default, so your suggestion seems reasonable. Let’s proceed
with it.
Thanks,
Yishai
