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, Alex
