On 18/10/2023 15:23, Jason Gunthorpe wrote:
> On Wed, Oct 18, 2023 at 01:48:04PM +0100, Joao Martins wrote:
>> On 18/10/2023 13:03, Jason Gunthorpe wrote:
>>> On Wed, Oct 18, 2023 at 11:19:07AM +0100, Joao Martins wrote:
>>>> On 16/10/2023 19:05, Jason Gunthorpe wrote:
>>>>> On Mon, Oct 16, 2023 at 06:52:50PM +0100, Joao Martins wrote:
>>>>>> On 16/10/2023 17:34, Jason Gunthorpe wrote:
>>>>>>> On Mon, Oct 16, 2023 at 05:25:16PM +0100, Joao Martins wrote:
>>>>>>>> diff --git a/drivers/iommu/iommufd/Kconfig b/drivers/iommu/iommufd/Kconfig
>>>>>>>> index 99d4b075df49..96ec013d1192 100644
>>>>>>>> --- a/drivers/iommu/iommufd/Kconfig
>>>>>>>> +++ b/drivers/iommu/iommufd/Kconfig
>>>>>>>> @@ -11,6 +11,13 @@ config IOMMUFD
>>>>>>>>
>>>>>>>> If you don't know what to do here, say N.
>>>>>>>>
>>>>>>>> +config IOMMUFD_DRIVER
>>>>>>>> + bool "IOMMUFD provides iommu drivers supporting functions"
>>>>>>>> + default IOMMU_API
>>>>>>>> + help
>>>>>>>> + IOMMUFD will provides supporting data structures and helpers to IOMMU
>>>>>>>> + drivers.
>>>>>>>
>>>>>>> It is not a 'user selectable' kconfig, just make it
>>>>>>>
>>>>>>> config IOMMUFD_DRIVER
>>>>>>> tristate
>>>>>>> default n
>>>>>>>
>>>>>> tristate? More like a bool as IOMMU drivers aren't modloadable
>>>>>
>>>>> tristate, who knows what people will select. If the modular drivers
>>>>> use it then it is forced to a Y not a M. It is the right way to use kconfig..
>>>>>
>>>> Making it tristate will break build bisection in this module with errors like this:
>>>>
>>>> [I say bisection, because aftewards when we put IOMMU drivers in the mix, these
>>>> are always builtin, so it ends up selecting IOMMU_DRIVER=y.]
>>>>
>>>> ERROR: modpost: missing MODULE_LICENSE() in drivers/iommu/iommufd/iova_bitmap.o
>>>>
>>>> iova_bitmap is no module, and making it tristate allows to build it as a module
>>>> as long as one of the selectors of is a module. 'bool' is actually more accurate
>>>> to what it is builtin or not.
>>>
>>> It is a module if you make it tristate, add the MODULE_LICENSE
>>
>> It's not just that. It can't work as a module when CONFIG_VFIO=y and another
>> user is CONFIG_MLX5_VFIO_PCI=m. CONFIG_VFIO uses the API so this is that case
>> where IS_ENABLED(CONFIG_IOMMUFD_DRIVER) evaluates to true but it is only
>> technically used by a module so it doesn't link it in.
>
> Ah! There is a well known kconfig technique for this too:
> depends on m || IOMMUFD_DRIVER != m
> or
> depends on IOMMUFD_DRIVER || IOMMUFD_DRIVER = n
>
These two lead to a recursive dependency:
drivers/vfio/Kconfig:2:error: recursive dependency detected!
drivers/vfio/Kconfig:2: symbol VFIO depends on IOMMUFD_DRIVER
drivers/iommu/iommufd/Kconfig:14: symbol IOMMUFD_DRIVER is selected by
MLX5_VFIO_PCI
drivers/vfio/pci/mlx5/Kconfig:2: symbol MLX5_VFIO_PCI depends on VFIO
For a resolution refer to Documentation/kbuild/kconfig-language.rst
subsection "Kconfig recursive dependency limitations"
Due to the end drivers being the ones actually selecting IOMMUFD_DRIVER. But
well, if we remove those, then no VF dirty tracking either.
> On the VFIO module.
>
The problem is VFIO module being =y with IOMMUFD_DRIVER=m because its end-user
being a module (MLX5_VFIO_PCI=m) which depends on it. If IOMMUFD_DRIVER
>> I would like to reiterate that there's no actual module user, making a bool is a
>> bit more clear on its usage on what it actually is (you would need IOMMU drivers
>> to be modules, which I think is a big gamble that is happening anytime soon?)
>
> This is all true too, but my thinking was to allow VFIO to use it
> without having an IOMMU driver compiled in that supports dirty
> tracking. eg for embedded cases.
OK, I see where you are coming from.
Honestly I think a simple 'select IOMMUFD_DRIVER' would fix it. If it's a module
it will select it as a module, and later if some IOMMU (if supported) selects it
it will make it builtin. The only it doesn't cover is when no VFIO PCI core
drivers are built or when non of the VFIO PCI core drivers do dirty tracking
i.e. it will still build it into CONFIG_VFIO -- but nothing new here as this is
how it works today.
I could restrict the scope with below and it would avoid having to do select in
mlx5/pds drivers:
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 1db519cce815..2abd1c598b65 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -7,7 +7,7 @@ menuconfig VFIO
select VFIO_GROUP if SPAPR_TCE_IOMMU || IOMMUFD=n
select VFIO_DEVICE_CDEV if !VFIO_GROUP
select VFIO_CONTAINER if IOMMUFD=n
- select IOMMUFD_DRIVER
+ select IOMMUFD_DRIVER if VFIO_PCI_CORE
help
VFIO provides a framework for secure userspace device drivers.
See Documentation/driver-api/vfio.rst for more details.
