| Message ID | 20230923012511.10379-3-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IOMMUFD Dirty Tracking | expand |
On Sat, Sep 23, 2023 at 02:24:54AM +0100, Joao Martins wrote: > Both VFIO and IOMMUFD will need iova bitmap for storing dirties and walking > the user bitmaps, so move to the common dependency into IOMMU core. IOMMUFD > can't exactly host it given that VFIO dirty tracking can be used without > IOMMUFD. Hum, this seems strange. Why not just make those VFIO drivers depends on iommufd? That seems harmless to me. However, I think the real issue is that iommu drivers need to use this API too for their part? IMHO would I would like to get to is a part of iommufd that used by iommu drivers (and thus built-in) and the current part that is modular. Basically, I think you should put this in the iommufd directory. Make the vfio side kconfig depend on iommufd at this point Later when the iommu drivers need it make some CONFIG_IOMMUFD_DRIVER_SUPPORT to build another module (that will be built in) and make the drivers that need it select it so it becomes built in. Jason
On 13/10/2023 16:48, Jason Gunthorpe wrote: > On Sat, Sep 23, 2023 at 02:24:54AM +0100, Joao Martins wrote: >> Both VFIO and IOMMUFD will need iova bitmap for storing dirties and walking >> the user bitmaps, so move to the common dependency into IOMMU core. IOMMUFD >> can't exactly host it given that VFIO dirty tracking can be used without >> IOMMUFD. > > Hum, this seems strange. Why not just make those VFIO drivers depends > on iommufd? That seems harmless to me. > IF you and Alex are OK with it then I can move to IOMMUFD. > However, I think the real issue is that iommu drivers need to use this > API too for their part? > Exactly. > IMHO would I would like to get to is a part of iommufd that used by > iommu drivers (and thus built-in) and the current part that is > modular. > > Basically, I think you should put this in the iommufd directory. Make > the vfio side kconfig depend on iommufd at this point > > Later when the iommu drivers need it make some > CONFIG_IOMMUFD_DRIVER_SUPPORT to build another module (that will be > built in) and make the drivers that need it select it so it becomes > built in. That's a good idea; you want me to do this (CONFIG_IOMMUFD_DRIVER_SUPPORT) in the context of this series, or as a follow-up (assuming I make it depend on iommufd as you suggested earlier) ?
On Fri, Oct 13, 2023 at 05:00:14PM +0100, Joao Martins wrote: > > Later when the iommu drivers need it make some > > CONFIG_IOMMUFD_DRIVER_SUPPORT to build another module (that will be > > built in) and make the drivers that need it select it so it becomes > > built in. > > That's a good idea; you want me to do this (CONFIG_IOMMUFD_DRIVER_SUPPORT) in > the context of this series, or as a follow-up (assuming I make it depend on > iommufd as you suggested earlier) ? I think it needs to be in this series, maybe as the next patch since io pagetable code starts to use it? Jason
On 13/10/2023 17:04, Jason Gunthorpe wrote: > On Fri, Oct 13, 2023 at 05:00:14PM +0100, Joao Martins wrote: > >>> Later when the iommu drivers need it make some >>> CONFIG_IOMMUFD_DRIVER_SUPPORT to build another module (that will be >>> built in) and make the drivers that need it select it so it becomes >>> built in. >> >> That's a good idea; you want me to do this (CONFIG_IOMMUFD_DRIVER_SUPPORT) in >> the context of this series, or as a follow-up (assuming I make it depend on >> iommufd as you suggested earlier) ? > > I think it needs to be in this series, maybe as the next patch since > io pagetable code starts to use it? OK I'll see how it looks like
On 13/10/2023 17:00, Joao Martins wrote: > On 13/10/2023 16:48, Jason Gunthorpe wrote: >> On Sat, Sep 23, 2023 at 02:24:54AM +0100, Joao Martins wrote: >>> Both VFIO and IOMMUFD will need iova bitmap for storing dirties and walking >>> the user bitmaps, so move to the common dependency into IOMMU core. IOMMUFD >>> can't exactly host it given that VFIO dirty tracking can be used without >>> IOMMUFD. >> >> Hum, this seems strange. Why not just make those VFIO drivers depends >> on iommufd? That seems harmless to me. >> > > IF you and Alex are OK with it then I can move to IOMMUFD. > >> However, I think the real issue is that iommu drivers need to use this >> API too for their part? >> > > Exactly. > My other concern into moving to IOMMUFD instead of core was VFIO_IOMMU_TYPE1, and if we always make it depend on IOMMUFD then we can't have what is today something supported because of VFIO_IOMMU_TYPE1 stuff with migration drivers (i.e. vfio-iommu-type1 with the live migration stuff). But if it's exists an IOMMUFD_DRIVER kconfig, then VFIO_CONTAINER can instead select the IOMMUFD_DRIVER alone so long as CONFIG_IOMMUFD isn't required? I am essentially talking about: # SPDX-License-Identifier: GPL-2.0-only menuconfig VFIO tristate "VFIO Non-Privileged userspace driver framework" select IOMMU_API depends on IOMMUFD || !IOMMUFD select INTERVAL_TREE select VFIO_GROUP if SPAPR_TCE_IOMMU || IOMMUFD=n select VFIO_DEVICE_CDEV if !VFIO_GROUP select VFIO_CONTAINER if IOMMUFD=n help VFIO provides a framework for secure userspace device drivers. See Documentation/driver-api/vfio.rst for more details. If you don't know what to do here, say N. ... and the fact that VFIO_IOMMU_TYPE1 requires VFIO_GROUP: config VFIO_CONTAINER bool "Support for the VFIO container /dev/vfio/vfio" select VFIO_IOMMU_TYPE1 if MMU && (X86 || S390 || ARM || ARM64) depends on VFIO_GROUP default y help The VFIO container is the classic interface to VFIO for establishing IOMMU mappings. If N is selected here then IOMMUFD must be used to manage the mappings. Unless testing IOMMUFD say Y here. if VFIO_CONTAINER config VFIO_IOMMU_TYPE1 tristate default n [...]
On Fri, Oct 13, 2023 at 06:10:04PM +0100, Joao Martins wrote: > On 13/10/2023 17:00, Joao Martins wrote: > > On 13/10/2023 16:48, Jason Gunthorpe wrote: > >> On Sat, Sep 23, 2023 at 02:24:54AM +0100, Joao Martins wrote: > >>> Both VFIO and IOMMUFD will need iova bitmap for storing dirties and walking > >>> the user bitmaps, so move to the common dependency into IOMMU core. IOMMUFD > >>> can't exactly host it given that VFIO dirty tracking can be used without > >>> IOMMUFD. > >> > >> Hum, this seems strange. Why not just make those VFIO drivers depends > >> on iommufd? That seems harmless to me. > >> > > > > IF you and Alex are OK with it then I can move to IOMMUFD. > > > >> However, I think the real issue is that iommu drivers need to use this > >> API too for their part? > >> > > > > Exactly. > > > > My other concern into moving to IOMMUFD instead of core was VFIO_IOMMU_TYPE1, > and if we always make it depend on IOMMUFD then we can't have what is today > something supported because of VFIO_IOMMU_TYPE1 stuff with migration drivers > (i.e. vfio-iommu-type1 with the live migration stuff). I plan to remove the live migration stuff from vfio-iommu-type1, it is all dead code now. > But if it's exists an IOMMUFD_DRIVER kconfig, then VFIO_CONTAINER can instead > select the IOMMUFD_DRIVER alone so long as CONFIG_IOMMUFD isn't required? I am > essentially talking about: Not VFIO_CONTAINER, the dirty tracking code is in vfio_main: vfio_main.c:#include <linux/iova_bitmap.h> vfio_main.c:static int vfio_device_log_read_and_clear(struct iova_bitmap *iter, vfio_main.c: struct iova_bitmap *iter; vfio_main.c: iter = iova_bitmap_alloc(report.iova, report.length, vfio_main.c: ret = iova_bitmap_for_each(iter, device, vfio_main.c: iova_bitmap_free(iter); And in various vfio device drivers. So the various drivers can select IOMMUFD_DRIVER And the core code should just gain a if (!IS_SUPPORTED(CONFIG_IOMMUFD_DRIVER)) return -EOPNOTSUPP On the two functions grep found above so the compiler eliminates all the symbols. No kconfig needed. Jason
On 13/10/2023 18:16, Jason Gunthorpe wrote: > On Fri, Oct 13, 2023 at 06:10:04PM +0100, Joao Martins wrote: >> On 13/10/2023 17:00, Joao Martins wrote: >>> On 13/10/2023 16:48, Jason Gunthorpe wrote: >>>> On Sat, Sep 23, 2023 at 02:24:54AM +0100, Joao Martins wrote: >>>>> Both VFIO and IOMMUFD will need iova bitmap for storing dirties and walking >>>>> the user bitmaps, so move to the common dependency into IOMMU core. IOMMUFD >>>>> can't exactly host it given that VFIO dirty tracking can be used without >>>>> IOMMUFD. >>>> >>>> Hum, this seems strange. Why not just make those VFIO drivers depends >>>> on iommufd? That seems harmless to me. >>>> >>> >>> IF you and Alex are OK with it then I can move to IOMMUFD. >>> >>>> However, I think the real issue is that iommu drivers need to use this >>>> API too for their part? >>>> >>> >>> Exactly. >>> >> >> My other concern into moving to IOMMUFD instead of core was VFIO_IOMMU_TYPE1, >> and if we always make it depend on IOMMUFD then we can't have what is today >> something supported because of VFIO_IOMMU_TYPE1 stuff with migration drivers >> (i.e. vfio-iommu-type1 with the live migration stuff). > > I plan to remove the live migration stuff from vfio-iommu-type1, it is > all dead code now. > I wasn't referring to the type1 dirty tracking stuff -- I was referring the stuff related to vfio devices, used *together* with type1 (for DMA map/unmap). >> But if it's exists an IOMMUFD_DRIVER kconfig, then VFIO_CONTAINER can instead >> select the IOMMUFD_DRIVER alone so long as CONFIG_IOMMUFD isn't required? I am >> essentially talking about: > > Not VFIO_CONTAINER, the dirty tracking code is in vfio_main: > > vfio_main.c:#include <linux/iova_bitmap.h> > vfio_main.c:static int vfio_device_log_read_and_clear(struct iova_bitmap *iter, > vfio_main.c: struct iova_bitmap *iter; > vfio_main.c: iter = iova_bitmap_alloc(report.iova, report.length, > vfio_main.c: ret = iova_bitmap_for_each(iter, device, > vfio_main.c: iova_bitmap_free(iter); > > And in various vfio device drivers. > > So the various drivers can select IOMMUFD_DRIVER > It isn't so much that type1 requires IOMMUFD, but more that it is used together with the core code that allows the vfio drivers to do migration. So the concern is if we make VFIO core depend on IOMMU that we prevent VFIO_CONTAINER/VFIO_GROUP to not be selected. My kconfig read was that we either select VFIO_GROUP or VFIO_DEVICE_CDEV but not both > And the core code should just gain a > > if (!IS_SUPPORTED(CONFIG_IOMMUFD_DRIVER)) > return -EOPNOTSUPP > > On the two functions grep found above so the compiler eliminates all > the symbols. No kconfig needed. > > Jason
On Fri, Oct 13, 2023 at 06:23:09PM +0100, Joao Martins wrote: > > > On 13/10/2023 18:16, Jason Gunthorpe wrote: > > On Fri, Oct 13, 2023 at 06:10:04PM +0100, Joao Martins wrote: > >> On 13/10/2023 17:00, Joao Martins wrote: > >>> On 13/10/2023 16:48, Jason Gunthorpe wrote: > >>>> On Sat, Sep 23, 2023 at 02:24:54AM +0100, Joao Martins wrote: > >>>>> Both VFIO and IOMMUFD will need iova bitmap for storing dirties and walking > >>>>> the user bitmaps, so move to the common dependency into IOMMU core. IOMMUFD > >>>>> can't exactly host it given that VFIO dirty tracking can be used without > >>>>> IOMMUFD. > >>>> > >>>> Hum, this seems strange. Why not just make those VFIO drivers depends > >>>> on iommufd? That seems harmless to me. > >>>> > >>> > >>> IF you and Alex are OK with it then I can move to IOMMUFD. > >>> > >>>> However, I think the real issue is that iommu drivers need to use this > >>>> API too for their part? > >>>> > >>> > >>> Exactly. > >>> > >> > >> My other concern into moving to IOMMUFD instead of core was VFIO_IOMMU_TYPE1, > >> and if we always make it depend on IOMMUFD then we can't have what is today > >> something supported because of VFIO_IOMMU_TYPE1 stuff with migration drivers > >> (i.e. vfio-iommu-type1 with the live migration stuff). > > > > I plan to remove the live migration stuff from vfio-iommu-type1, it is > > all dead code now. > > > > I wasn't referring to the type1 dirty tracking stuff -- I was referring the > stuff related to vfio devices, used *together* with type1 (for DMA > map/unmap). Ah, well, I guess that is true > >> But if it's exists an IOMMUFD_DRIVER kconfig, then VFIO_CONTAINER can instead > >> select the IOMMUFD_DRIVER alone so long as CONFIG_IOMMUFD isn't required? I am > >> essentially talking about: > > > > Not VFIO_CONTAINER, the dirty tracking code is in vfio_main: > > > > vfio_main.c:#include <linux/iova_bitmap.h> > > vfio_main.c:static int vfio_device_log_read_and_clear(struct iova_bitmap *iter, > > vfio_main.c: struct iova_bitmap *iter; > > vfio_main.c: iter = iova_bitmap_alloc(report.iova, report.length, > > vfio_main.c: ret = iova_bitmap_for_each(iter, device, > > vfio_main.c: iova_bitmap_free(iter); > > > > And in various vfio device drivers. > > > > So the various drivers can select IOMMUFD_DRIVER > > > > It isn't so much that type1 requires IOMMUFD, but more that it is used together > with the core code that allows the vfio drivers to do migration. So the concern > is if we make VFIO core depend on IOMMU that we prevent > VFIO_CONTAINER/VFIO_GROUP to not be selected. My kconfig read was that we either > select VFIO_GROUP or VFIO_DEVICE_CDEV but not both Doing it as I said is still the right thing. If someone has turned on one of the drivers that actually implements dirty tracking it will turn on IOMMUFD_DRIVER and that will cause the supporting core code to compile in the support functions. Jason
On 13/10/2023 18:28, Jason Gunthorpe wrote: > On Fri, Oct 13, 2023 at 06:23:09PM +0100, Joao Martins wrote: >> On 13/10/2023 18:16, Jason Gunthorpe wrote: >>> On Fri, Oct 13, 2023 at 06:10:04PM +0100, Joao Martins wrote: >>>> On 13/10/2023 17:00, Joao Martins wrote: >>>>> On 13/10/2023 16:48, Jason Gunthorpe wrote: >>>>>> On Sat, Sep 23, 2023 at 02:24:54AM +0100, Joao Martins wrote: >>>> But if it's exists an IOMMUFD_DRIVER kconfig, then VFIO_CONTAINER can instead >>>> select the IOMMUFD_DRIVER alone so long as CONFIG_IOMMUFD isn't required? I am >>>> essentially talking about: >>> >>> Not VFIO_CONTAINER, the dirty tracking code is in vfio_main: >>> >>> vfio_main.c:#include <linux/iova_bitmap.h> >>> vfio_main.c:static int vfio_device_log_read_and_clear(struct iova_bitmap *iter, >>> vfio_main.c: struct iova_bitmap *iter; >>> vfio_main.c: iter = iova_bitmap_alloc(report.iova, report.length, >>> vfio_main.c: ret = iova_bitmap_for_each(iter, device, >>> vfio_main.c: iova_bitmap_free(iter); >>> >>> And in various vfio device drivers. >>> >>> So the various drivers can select IOMMUFD_DRIVER >>> >> >> It isn't so much that type1 requires IOMMUFD, but more that it is used together >> with the core code that allows the vfio drivers to do migration. So the concern >> is if we make VFIO core depend on IOMMU that we prevent >> VFIO_CONTAINER/VFIO_GROUP to not be selected. My kconfig read was that we either >> select VFIO_GROUP or VFIO_DEVICE_CDEV but not both > > Doing it as I said is still the right thing. > > If someone has turned on one of the drivers that actually implements > dirty tracking it will turn on IOMMUFD_DRIVER and that will cause the > supporting core code to compile in the support functions. > Yeap. And as long as CONFIG_IOMMUFD_DRIVER does not select/enable CONFIG_IOMMUFD then we should be fine
On Fri, 13 Oct 2023 18:23:09 +0100 Joao Martins <joao.m.martins@oracle.com> wrote: > On 13/10/2023 18:16, Jason Gunthorpe wrote: > > On Fri, Oct 13, 2023 at 06:10:04PM +0100, Joao Martins wrote: > >> On 13/10/2023 17:00, Joao Martins wrote: > >>> On 13/10/2023 16:48, Jason Gunthorpe wrote: > >>>> On Sat, Sep 23, 2023 at 02:24:54AM +0100, Joao Martins wrote: > >>>>> Both VFIO and IOMMUFD will need iova bitmap for storing dirties and walking > >>>>> the user bitmaps, so move to the common dependency into IOMMU core. IOMMUFD > >>>>> can't exactly host it given that VFIO dirty tracking can be used without > >>>>> IOMMUFD. > >>>> > >>>> Hum, this seems strange. Why not just make those VFIO drivers depends > >>>> on iommufd? That seems harmless to me. > >>>> > >>> > >>> IF you and Alex are OK with it then I can move to IOMMUFD. It's only strange in that we don't actually have a hard dependency on IOMMUFD currently and won't until we remove container support, which is some ways down the road. Ultimately we expect to get to the same place, so I don't have a particular issue with it. > >>>> However, I think the real issue is that iommu drivers need to use this > >>>> API too for their part? > >>>> > >>> > >>> Exactly. > >>> > >> > >> My other concern into moving to IOMMUFD instead of core was VFIO_IOMMU_TYPE1, > >> and if we always make it depend on IOMMUFD then we can't have what is today > >> something supported because of VFIO_IOMMU_TYPE1 stuff with migration drivers > >> (i.e. vfio-iommu-type1 with the live migration stuff). > > > > I plan to remove the live migration stuff from vfio-iommu-type1, it is > > all dead code now. > > > > I wasn't referring to the type1 dirty tracking stuff -- I was referring the > stuff related to vfio devices, used *together* with type1 (for DMA map/unmap). > > >> But if it's exists an IOMMUFD_DRIVER kconfig, then VFIO_CONTAINER can instead > >> select the IOMMUFD_DRIVER alone so long as CONFIG_IOMMUFD isn't required? I am > >> essentially talking about: > > > > Not VFIO_CONTAINER, the dirty tracking code is in vfio_main: > > > > vfio_main.c:#include <linux/iova_bitmap.h> > > vfio_main.c:static int vfio_device_log_read_and_clear(struct iova_bitmap *iter, > > vfio_main.c: struct iova_bitmap *iter; > > vfio_main.c: iter = iova_bitmap_alloc(report.iova, report.length, > > vfio_main.c: ret = iova_bitmap_for_each(iter, device, > > vfio_main.c: iova_bitmap_free(iter); > > > > And in various vfio device drivers. > > > > So the various drivers can select IOMMUFD_DRIVER > > > > It isn't so much that type1 requires IOMMUFD, but more that it is used together > with the core code that allows the vfio drivers to do migration. So the concern > is if we make VFIO core depend on IOMMU that we prevent > VFIO_CONTAINER/VFIO_GROUP to not be selected. My kconfig read was that we either > select VFIO_GROUP or VFIO_DEVICE_CDEV but not both That's not true. We can have both. In fact we rely on having both to support a smooth transition to the cdev interface. Thanks, Alex
On 13/10/2023 21:41, Alex Williamson wrote: > On Fri, 13 Oct 2023 18:23:09 +0100 > Joao Martins <joao.m.martins@oracle.com> wrote: > >> On 13/10/2023 18:16, Jason Gunthorpe wrote: >>> On Fri, Oct 13, 2023 at 06:10:04PM +0100, Joao Martins wrote: >>>> On 13/10/2023 17:00, Joao Martins wrote: >>>>> On 13/10/2023 16:48, Jason Gunthorpe wrote: >>>> But if it's exists an IOMMUFD_DRIVER kconfig, then VFIO_CONTAINER can instead >>>> select the IOMMUFD_DRIVER alone so long as CONFIG_IOMMUFD isn't required? I am >>>> essentially talking about: >>> >>> Not VFIO_CONTAINER, the dirty tracking code is in vfio_main: >>> >>> vfio_main.c:#include <linux/iova_bitmap.h> >>> vfio_main.c:static int vfio_device_log_read_and_clear(struct iova_bitmap *iter, >>> vfio_main.c: struct iova_bitmap *iter; >>> vfio_main.c: iter = iova_bitmap_alloc(report.iova, report.length, >>> vfio_main.c: ret = iova_bitmap_for_each(iter, device, >>> vfio_main.c: iova_bitmap_free(iter); >>> >>> And in various vfio device drivers. >>> >>> So the various drivers can select IOMMUFD_DRIVER >>> >> >> It isn't so much that type1 requires IOMMUFD, but more that it is used together >> with the core code that allows the vfio drivers to do migration. So the concern >> is if we make VFIO core depend on IOMMU that we prevent >> VFIO_CONTAINER/VFIO_GROUP to not be selected. My kconfig read was that we either >> select VFIO_GROUP or VFIO_DEVICE_CDEV but not both > > That's not true. We can have both. In fact we rely on having both to > support a smooth transition to the cdev interface. Thanks, On a triple look, mixed defaults[0] vs manual config: having IOMMUFD=y|m today it won't select VFIO_CONTAINER, nobody stops one from actually selecting it both. Unless I missed something [0] Ref: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/vfio/Kconfig menuconfig VFIO [...] select VFIO_GROUP if SPAPR_TCE_IOMMU || IOMMUFD=n select VFIO_DEVICE_CDEV if !VFIO_GROUP select VFIO_CONTAINER if IOMMUFD=n [...] if VFIO config VFIO_DEVICE_CDEV [...] depends on IOMMUFD && !SPAPR_TCE_IOMMU default !VFIO_GROUP [...] config VFIO_GROUP default y [...] config VFIO_CONTAINER [...] select VFIO_IOMMU_TYPE1 if MMU && (X86 || S390 || ARM || ARM64) depends on VFIO_GROUP default y
On Fri, 13 Oct 2023 22:20:31 +0100 Joao Martins <joao.m.martins@oracle.com> wrote: > On 13/10/2023 21:41, Alex Williamson wrote: > > On Fri, 13 Oct 2023 18:23:09 +0100 > > Joao Martins <joao.m.martins@oracle.com> wrote: > > > >> On 13/10/2023 18:16, Jason Gunthorpe wrote: > >>> On Fri, Oct 13, 2023 at 06:10:04PM +0100, Joao Martins wrote: > >>>> On 13/10/2023 17:00, Joao Martins wrote: > >>>>> On 13/10/2023 16:48, Jason Gunthorpe wrote: > >>>> But if it's exists an IOMMUFD_DRIVER kconfig, then VFIO_CONTAINER can instead > >>>> select the IOMMUFD_DRIVER alone so long as CONFIG_IOMMUFD isn't required? I am > >>>> essentially talking about: > >>> > >>> Not VFIO_CONTAINER, the dirty tracking code is in vfio_main: > >>> > >>> vfio_main.c:#include <linux/iova_bitmap.h> > >>> vfio_main.c:static int vfio_device_log_read_and_clear(struct iova_bitmap *iter, > >>> vfio_main.c: struct iova_bitmap *iter; > >>> vfio_main.c: iter = iova_bitmap_alloc(report.iova, report.length, > >>> vfio_main.c: ret = iova_bitmap_for_each(iter, device, > >>> vfio_main.c: iova_bitmap_free(iter); > >>> > >>> And in various vfio device drivers. > >>> > >>> So the various drivers can select IOMMUFD_DRIVER > >>> > >> > >> It isn't so much that type1 requires IOMMUFD, but more that it is used together > >> with the core code that allows the vfio drivers to do migration. So the concern > >> is if we make VFIO core depend on IOMMU that we prevent > >> VFIO_CONTAINER/VFIO_GROUP to not be selected. My kconfig read was that we either > >> select VFIO_GROUP or VFIO_DEVICE_CDEV but not both > > > > That's not true. We can have both. In fact we rely on having both to > > support a smooth transition to the cdev interface. Thanks, > > On a triple look, mixed defaults[0] vs manual config: having IOMMUFD=y|m today > it won't select VFIO_CONTAINER, nobody stops one from actually selecting it > both. Unless I missed something Oh! I misunderstood your comment, you're referring to default selections rather than possible selections. So yes, if VFIO depends on IOMMUFD then suddenly our default configs shift to IOMMUFD/CDEV rather than legacy CONTAINER/GROUP. So perhaps if VFIO selects IOMMUFD, that's not exactly harmless currently. I think Jason is describing this would eventually be in a built-in portion of IOMMUFD, but I think currently that built-in portion is IOMMU. So until we have this IOMMUFD_DRIVER that enables that built-in portion, it seems unnecessarily disruptive to make VFIO select IOMMUFD to get this iova bitmap support. Thanks, Alex > [0] Ref: > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/vfio/Kconfig > > menuconfig VFIO > [...] > select VFIO_GROUP if SPAPR_TCE_IOMMU || IOMMUFD=n > select VFIO_DEVICE_CDEV if !VFIO_GROUP > select VFIO_CONTAINER if IOMMUFD=n > [...] > > if VFIO > config VFIO_DEVICE_CDEV > [...] > depends on IOMMUFD && !SPAPR_TCE_IOMMU > default !VFIO_GROUP > [...] > config VFIO_GROUP > default y > [...] > config VFIO_CONTAINER > [...] > select VFIO_IOMMU_TYPE1 if MMU && (X86 || S390 || ARM || ARM64) > depends on VFIO_GROUP > default y >
On Fri, Oct 13, 2023 at 03:51:34PM -0600, Alex Williamson wrote: > I think Jason is describing this would eventually be in a built-in > portion of IOMMUFD, but I think currently that built-in portion is > IOMMU. So until we have this IOMMUFD_DRIVER that enables that built-in > portion, it seems unnecessarily disruptive to make VFIO select IOMMUFD > to get this iova bitmap support. Thanks, Right, I'm saying Joao may as well make IOMMUFD_DRIVER right now for this Jason
On 14/10/2023 01:02, Jason Gunthorpe wrote: > On Fri, Oct 13, 2023 at 03:51:34PM -0600, Alex Williamson wrote: > >> I think Jason is describing this would eventually be in a built-in >> portion of IOMMUFD, but I think currently that built-in portion is >> IOMMU. So until we have this IOMMUFD_DRIVER that enables that built-in >> portion, it seems unnecessarily disruptive to make VFIO select IOMMUFD >> to get this iova bitmap support. Thanks, > > Right, I'm saying Joao may as well make IOMMUFD_DRIVER right now for > this So far I have this snip at the end. Though given that there are struct iommu_domain changes that set a dirty_ops (which require iova-bitmap). Do we just ifdef around IOMMUFD_DRIVER or we always include it if CONFIG_IOMMU_API=y ? Thus far I'm going towards the latter 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. + if IOMMUFD config IOMMUFD_VFIO_CONTAINER bool "IOMMUFD provides the VFIO container /dev/vfio/vfio" diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile index 8aeba81800c5..34b446146961 100644 --- a/drivers/iommu/iommufd/Makefile +++ b/drivers/iommu/iommufd/Makefile @@ -11,3 +11,4 @@ iommufd-y := \ iommufd-$(CONFIG_IOMMUFD_TEST) += selftest.o obj-$(CONFIG_IOMMUFD) += iommufd.o +obj-$(CONFIG_IOMMUFD_DRIVER) += iova_bitmap.o diff --git a/drivers/vfio/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c similarity index 100% rename from drivers/vfio/iova_bitmap.c rename to drivers/iommu/iommufd/iova_bitmap.c diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig index 6bda6dbb4878..1db519cce815 100644 --- a/drivers/vfio/Kconfig +++ b/drivers/vfio/Kconfig @@ -7,6 +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 help VFIO provides a framework for secure userspace device drivers. See Documentation/driver-api/vfio.rst for more details. diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile index c82ea032d352..68c05705200f 100644 --- a/drivers/vfio/Makefile +++ b/drivers/vfio/Makefile @@ -1,8 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_VFIO) += vfio.o -vfio-y += vfio_main.o \ - iova_bitmap.o +vfio-y += vfio_main.o vfio-$(CONFIG_VFIO_DEVICE_CDEV) += device_cdev.o vfio-$(CONFIG_VFIO_GROUP) += group.o
On Mon, Oct 16, 2023 at 05:25:16PM +0100, Joao Martins wrote: > >> I think Jason is describing this would eventually be in a built-in > >> portion of IOMMUFD, but I think currently that built-in portion is > >> IOMMU. So until we have this IOMMUFD_DRIVER that enables that built-in > >> portion, it seems unnecessarily disruptive to make VFIO select IOMMUFD > >> to get this iova bitmap support. Thanks, > > > > Right, I'm saying Joao may as well make IOMMUFD_DRIVER right now for > > this > > So far I have this snip at the end. > > Though given that there are struct iommu_domain changes that set a dirty_ops > (which require iova-bitmap). Drivers which set those ops need to select IOMMUFD_DRIVER.. Perhaps (at least for ARM) they should even be coded select IOMMUFD_DRIVER if IOMMUFD And then #ifdef out the dirty tracking bits so embedded systems don't get the bloat with !IOMMUFD > 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 ie the only way to get it is to build a driver that will consume it. > diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile > index 8aeba81800c5..34b446146961 100644 > --- a/drivers/iommu/iommufd/Makefile > +++ b/drivers/iommu/iommufd/Makefile > @@ -11,3 +11,4 @@ iommufd-y := \ > iommufd-$(CONFIG_IOMMUFD_TEST) += selftest.o > > obj-$(CONFIG_IOMMUFD) += iommufd.o > +obj-$(CONFIG_IOMMUFD_DRIVER) += iova_bitmap.o Right.. > diff --git a/drivers/vfio/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c > similarity index 100% > rename from drivers/vfio/iova_bitmap.c > rename to drivers/iommu/iommufd/iova_bitmap.c > diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig > index 6bda6dbb4878..1db519cce815 100644 > --- a/drivers/vfio/Kconfig > +++ b/drivers/vfio/Kconfig > @@ -7,6 +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 As discussed use a if (IS_ENABLED) here and just disable the bitmap code if something else didn't enable it. VFIO isn't a consumer of it The question you are asking is on the driver side implementing it, and it should be conditional if IOMMUFD is turned on. Jason
On 16/10/2023 17:34, Jason Gunthorpe wrote: > On Mon, Oct 16, 2023 at 05:25:16PM +0100, Joao Martins wrote: >>>> I think Jason is describing this would eventually be in a built-in >>>> portion of IOMMUFD, but I think currently that built-in portion is >>>> IOMMU. So until we have this IOMMUFD_DRIVER that enables that built-in >>>> portion, it seems unnecessarily disruptive to make VFIO select IOMMUFD >>>> to get this iova bitmap support. Thanks, >>> >>> Right, I'm saying Joao may as well make IOMMUFD_DRIVER right now for >>> this >> >> So far I have this snip at the end. >> >> Though given that there are struct iommu_domain changes that set a dirty_ops >> (which require iova-bitmap). > > Drivers which set those ops need to select IOMMUFD_DRIVER.. > My problem is more of the generic/vfio side (headers and structures of iommu core) not really IOMMU driver nor IOMMUFD. > Perhaps (at least for ARM) they should even be coded > > select IOMMUFD_DRIVER if IOMMUFD > > And then #ifdef out the dirty tracking bits so embedded systems don't > get the bloat with !IOMMUFD > Right >> 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 > ie the only way to get it is to build a driver that will consume it. > >> diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile >> index 8aeba81800c5..34b446146961 100644 >> --- a/drivers/iommu/iommufd/Makefile >> +++ b/drivers/iommu/iommufd/Makefile >> @@ -11,3 +11,4 @@ iommufd-y := \ >> iommufd-$(CONFIG_IOMMUFD_TEST) += selftest.o >> >> obj-$(CONFIG_IOMMUFD) += iommufd.o >> +obj-$(CONFIG_IOMMUFD_DRIVER) += iova_bitmap.o > > Right.. > >> diff --git a/drivers/vfio/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c >> similarity index 100% >> rename from drivers/vfio/iova_bitmap.c >> rename to drivers/iommu/iommufd/iova_bitmap.c >> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig >> index 6bda6dbb4878..1db519cce815 100644 >> --- a/drivers/vfio/Kconfig >> +++ b/drivers/vfio/Kconfig >> @@ -7,6 +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 > > As discussed use a if (IS_ENABLED) here and just disable the > bitmap code if something else didn't enable it. > I'm adding this to vfio_main: if (!IS_ENABLED(CONFIG_IOMMUFD_DRIVER)) return -EOPNOTSUPP; > VFIO isn't a consumer of it > (...) The select IOMMUFD_DRIVER was there because of VFIO PCI vendor drivers not VFIO core. for the 'disable bitmap code' I can add ifdef-ry in iova_bitmap.h to add scalfold definitions to error-out/nop if CONFIG_IOMMUFD_DRIVER=n when moving to iommufd/ > The question you are asking is on the driver side implementing it, and > it should be conditional if IOMMUFD is turned on. For the IOMMU driver side sure IOMMUFD should be required as it's the sole entry point of this But as discussed earlier IOMMUFD=m|y dependency really only matters for IOMMU dirty tracking, but not for VFIO device dirty tracking. For your suggested scheme to work VFIO PCI drivers still need to select IOMMUFD_DRIVER as they require it
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: > >>>> I think Jason is describing this would eventually be in a built-in > >>>> portion of IOMMUFD, but I think currently that built-in portion is > >>>> IOMMU. So until we have this IOMMUFD_DRIVER that enables that built-in > >>>> portion, it seems unnecessarily disruptive to make VFIO select IOMMUFD > >>>> to get this iova bitmap support. Thanks, > >>> > >>> Right, I'm saying Joao may as well make IOMMUFD_DRIVER right now for > >>> this > >> > >> So far I have this snip at the end. > >> > >> Though given that there are struct iommu_domain changes that set a dirty_ops > >> (which require iova-bitmap). > > > > Drivers which set those ops need to select IOMMUFD_DRIVER.. > > > > My problem is more of the generic/vfio side (headers and structures of iommu > core) not really IOMMU driver nor IOMMUFD. As I said, just don't compile that stuff. If nothing else selects IOMMFD_DRIVER then the core code has nothing to do. > >> 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.. > >> --- a/drivers/vfio/Kconfig > >> +++ b/drivers/vfio/Kconfig > >> @@ -7,6 +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 > > > > As discussed use a if (IS_ENABLED) here and just disable the > > bitmap code if something else didn't enable it. > > > > I'm adding this to vfio_main: > > if (!IS_ENABLED(CONFIG_IOMMUFD_DRIVER)) > return -EOPNOTSUPP; Seems right > > VFIO isn't a consumer of it > > > > (...) The select IOMMUFD_DRIVER was there because of VFIO PCI vendor drivers not > VFIO core. Those driver should individually select IOMMUFD_DRIVER for the 'disable bitmap code' I can add ifdef-ry in iova_bitmap.h to > add scalfold definitions to error-out/nop if CONFIG_IOMMUFD_DRIVER=n when moving > to iommufd/ Yes that could also be a good approach > For your suggested scheme to work VFIO PCI drivers still need to select > IOMMUFD_DRIVER as they require it Yes, of course Jason
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.. > Got it (and thanks for the patience) >>>> --- a/drivers/vfio/Kconfig >>>> +++ b/drivers/vfio/Kconfig >>>> @@ -7,6 +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 >>> >>> As discussed use a if (IS_ENABLED) here and just disable the >>> bitmap code if something else didn't enable it. >>> >> >> I'm adding this to vfio_main: >> >> if (!IS_ENABLED(CONFIG_IOMMUFD_DRIVER)) >> return -EOPNOTSUPP; > > Seems right > >>> VFIO isn't a consumer of it >>> >> >> (...) The select IOMMUFD_DRIVER was there because of VFIO PCI vendor drivers not >> VFIO core. > > Those driver should individually select IOMMUFD_DRIVER > OK -- this is the part that wasn't clear straight away. So individually per driver not on VFIO_PCI_CORE in which these drivers depend on? A lot of the dirty tracking stuff gets steered via what VFIO_PCI_CORE allows, perhaps I can put the IOMMUFD_DRIVER selection there? >> for the 'disable bitmap code' I can add ifdef-ry in iova_bitmap.h to >> add scalfold definitions to error-out/nop if CONFIG_IOMMUFD_DRIVER=n when moving >> to iommufd/ > > Yes that could also be a good approach > >> For your suggested scheme to work VFIO PCI drivers still need to select >> IOMMUFD_DRIVER as they require it > > Yes, of course Here's a diff, naturally AMD/Intel kconfigs would get a select IOMMUFD_DRIVER as well later in the series diff --git a/drivers/iommu/iommufd/Kconfig b/drivers/iommu/iommufd/Kconfig index 99d4b075df49..4c6cb96a4b28 100644 --- a/drivers/iommu/iommufd/Kconfig +++ b/drivers/iommu/iommufd/Kconfig @@ -11,6 +11,10 @@ config IOMMUFD If you don't know what to do here, say N. +config IOMMUFD_DRIVER + tristate + default n + if IOMMUFD config IOMMUFD_VFIO_CONTAINER bool "IOMMUFD provides the VFIO container /dev/vfio/vfio" diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile index 8aeba81800c5..34b446146961 100644 --- a/drivers/iommu/iommufd/Makefile +++ b/drivers/iommu/iommufd/Makefile @@ -11,3 +11,4 @@ iommufd-y := \ iommufd-$(CONFIG_IOMMUFD_TEST) += selftest.o obj-$(CONFIG_IOMMUFD) += iommufd.o +obj-$(CONFIG_IOMMUFD_DRIVER) += iova_bitmap.o diff --git a/drivers/vfio/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c similarity index 100% rename from drivers/vfio/iova_bitmap.c rename to drivers/iommu/iommufd/iova_bitmap.c diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile index c82ea032d352..68c05705200f 100644 --- a/drivers/vfio/Makefile +++ b/drivers/vfio/Makefile @@ -1,8 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_VFIO) += vfio.o -vfio-y += vfio_main.o \ - iova_bitmap.o +vfio-y += vfio_main.o vfio-$(CONFIG_VFIO_DEVICE_CDEV) += device_cdev.o vfio-$(CONFIG_VFIO_GROUP) += group.o vfio-$(CONFIG_IOMMUFD) += iommufd.o diff --git a/drivers/vfio/pci/mlx5/Kconfig b/drivers/vfio/pci/mlx5/Kconfig index 7088edc4fb28..c3ced56b7787 100644 --- a/drivers/vfio/pci/mlx5/Kconfig +++ b/drivers/vfio/pci/mlx5/Kconfig @@ -3,6 +3,7 @@ config MLX5_VFIO_PCI tristate "VFIO support for MLX5 PCI devices" depends on MLX5_CORE select VFIO_PCI_CORE + select IOMMUFD_DRIVER help This provides migration support for MLX5 devices using the VFIO framework. diff --git a/drivers/vfio/pci/pds/Kconfig b/drivers/vfio/pci/pds/Kconfig index 407b3fd32733..fff368a8183b 100644 --- a/drivers/vfio/pci/pds/Kconfig +++ b/drivers/vfio/pci/pds/Kconfig @@ -5,6 +5,7 @@ config PDS_VFIO_PCI tristate "VFIO support for PDS PCI devices" depends on PDS_CORE select VFIO_PCI_CORE + select IOMMUFD_DRIVER help This provides generic PCI support for PDS devices using the VFIO framework. diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index 40732e8ed4c6..93b0c2b377e1 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -1095,6 +1095,9 @@ static int vfio_device_log_read_and_clear(struct iova_bitmap *iter, { struct vfio_device *device = opaque; + if (!IS_ENABLED(CONFIG_IOMMUFD_DRIVER)) + return -EOPNOTSUPP; + return device->log_ops->log_read_and_clear(device, iova, length, iter); } @@ -1111,6 +1114,9 @@ vfio_ioctl_device_feature_logging_report(struct vfio_device *device, u64 iova_end; int ret; + if (!IS_ENABLED(CONFIG_IOMMUFD_DRIVER)) + return -EOPNOTSUPP; + if (!device->log_ops) return -ENOTTY; diff --git a/include/linux/iova_bitmap.h b/include/linux/iova_bitmap.h index c006cf0a25f3..1c338f5e5b7a 100644 --- a/include/linux/iova_bitmap.h +++ b/include/linux/iova_bitmap.h @@ -7,6 +7,7 @@ #define _IOVA_BITMAP_H_ #include <linux/types.h> +#include <linux/errno.h> struct iova_bitmap; @@ -14,6 +15,7 @@ typedef int (*iova_bitmap_fn_t)(struct iova_bitmap *bitmap, unsigned long iova, size_t length, void *opaque); +#if IS_ENABLED(CONFIG_IOMMUFD_DRIVER) struct iova_bitmap *iova_bitmap_alloc(unsigned long iova, size_t length, unsigned long page_size, u64 __user *data); @@ -22,5 +24,29 @@ int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque, iova_bitmap_fn_t fn); void iova_bitmap_set(struct iova_bitmap *bitmap, unsigned long iova, size_t length); +#else +static inline struct iova_bitmap *iova_bitmap_alloc(unsigned long iova, + size_t length, + unsigned long page_size, + u64 __user *data) +{ + return NULL; +} + +static inline void iova_bitmap_free(struct iova_bitmap *bitmap) +{ +} + +static inline int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque, + iova_bitmap_fn_t fn) +{ + return -EOPNOTSUPP; +} + +static inline void iova_bitmap_set(struct iova_bitmap *bitmap, + unsigned long iova, size_t length) +{ +} +#endif #endif
On Mon, Oct 16, 2023 at 07:15:10PM +0100, Joao Martins wrote: > Here's a diff, naturally AMD/Intel kconfigs would get a select IOMMUFD_DRIVER as > well later in the series It looks OK, the IS_ENABLES are probably overkill once you have changed the .h file, just saves a few code bytes, not sure we care? Jason
On 16/10/2023 19:20, Jason Gunthorpe wrote: > On Mon, Oct 16, 2023 at 07:15:10PM +0100, Joao Martins wrote: > >> Here's a diff, naturally AMD/Intel kconfigs would get a select IOMMUFD_DRIVER as >> well later in the series > > It looks OK, the IS_ENABLES are probably overkill once you have > changed the .h file, just saves a few code bytes, not sure we care? I can remove them
On 16/10/2023 19:37, Joao Martins wrote: > On 16/10/2023 19:20, Jason Gunthorpe wrote: >> On Mon, Oct 16, 2023 at 07:15:10PM +0100, Joao Martins wrote: >> >>> Here's a diff, naturally AMD/Intel kconfigs would get a select IOMMUFD_DRIVER as >>> well later in the series >> >> It looks OK, the IS_ENABLES are probably overkill once you have >> changed the .h file, just saves a few code bytes, not sure we care? > > I can remove them Additionally, I don't think I can use the symbol namespace for IOMMUFD, as iova-bitmap can be build builtin with a module iommufd, otherwise we get into errors like this: ERROR: modpost: module iommufd uses symbol iova_bitmap_for_each from namespace IOMMUFD, but does not import it. ERROR: modpost: module iommufd uses symbol iova_bitmap_free from namespace IOMMUFD, but does not import it. ERROR: modpost: module iommufd uses symbol iova_bitmap_alloc from namespace IOMMUFD, but does not import it.
On Mon, Oct 16, 2023 at 07:50:25PM +0100, Joao Martins wrote: > On 16/10/2023 19:37, Joao Martins wrote: > > On 16/10/2023 19:20, Jason Gunthorpe wrote: > >> On Mon, Oct 16, 2023 at 07:15:10PM +0100, Joao Martins wrote: > >> > >>> Here's a diff, naturally AMD/Intel kconfigs would get a select IOMMUFD_DRIVER as > >>> well later in the series > >> > >> It looks OK, the IS_ENABLES are probably overkill once you have > >> changed the .h file, just saves a few code bytes, not sure we care? > > > > I can remove them > > Additionally, I don't think I can use the symbol namespace for IOMMUFD, as > iova-bitmap can be build builtin with a module iommufd, otherwise we get into > errors like this: > > ERROR: modpost: module iommufd uses symbol iova_bitmap_for_each from namespace > IOMMUFD, but does not import it. > ERROR: modpost: module iommufd uses symbol iova_bitmap_free from namespace > IOMMUFD, but does not import it. > ERROR: modpost: module iommufd uses symbol iova_bitmap_alloc from namespace > IOMMUFD, but does not import it. You cannot self-import the namespace? I'm not that familiar with this stuff Jason
On 17/10/2023 13:58, Jason Gunthorpe wrote: > On Mon, Oct 16, 2023 at 07:50:25PM +0100, Joao Martins wrote: >> On 16/10/2023 19:37, Joao Martins wrote: >>> On 16/10/2023 19:20, Jason Gunthorpe wrote: >>>> On Mon, Oct 16, 2023 at 07:15:10PM +0100, Joao Martins wrote: >>>> >>>>> Here's a diff, naturally AMD/Intel kconfigs would get a select IOMMUFD_DRIVER as >>>>> well later in the series >>>> >>>> It looks OK, the IS_ENABLES are probably overkill once you have >>>> changed the .h file, just saves a few code bytes, not sure we care? >>> >>> I can remove them >> >> Additionally, I don't think I can use the symbol namespace for IOMMUFD, as >> iova-bitmap can be build builtin with a module iommufd, otherwise we get into >> errors like this: >> >> ERROR: modpost: module iommufd uses symbol iova_bitmap_for_each from namespace >> IOMMUFD, but does not import it. >> ERROR: modpost: module iommufd uses symbol iova_bitmap_free from namespace >> IOMMUFD, but does not import it. >> ERROR: modpost: module iommufd uses symbol iova_bitmap_alloc from namespace >> IOMMUFD, but does not import it. > > You cannot self-import the namespace? I'm not that familiar with this stuff Neither do I. But self-importing looks to work. An alternative is to have an alternative namespace (e.g. IOMMUFD_DRIVER) in similar fashion to IOMMUFD_INTERNAL. But I fear this patch is already doing too much at the late stage. Are you keen on getting this moved with namespaces right now, or it can be a post-merge cleanup? diff --git a/drivers/iommu/iommufd/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c index f54b56388e00..0aaf2218bf61 100644 --- a/drivers/iommu/iommufd/iova_bitmap.c +++ b/drivers/iommu/iommufd/iova_bitmap.c @@ -7,6 +7,7 @@ #include <linux/mm.h> #include <linux/slab.h> #include <linux/highmem.h> +#include <linux/module.h> #define BITS_PER_PAGE (PAGE_SIZE * BITS_PER_BYTE) @@ -268,7 +269,7 @@ struct iova_bitmap *iova_bitmap_alloc(unsigned long iova, size_t length, iova_bitmap_free(bitmap); return ERR_PTR(rc); } -EXPORT_SYMBOL_GPL(iova_bitmap_alloc); +EXPORT_SYMBOL_NS_GPL(iova_bitmap_alloc, IOMMUFD); /** * iova_bitmap_free() - Frees an IOVA bitmap object @@ -290,7 +291,7 @@ void iova_bitmap_free(struct iova_bitmap *bitmap) kfree(bitmap); } -EXPORT_SYMBOL_GPL(iova_bitmap_free); +EXPORT_SYMBOL_NS_GPL(iova_bitmap_free, IOMMUFD); /* * Returns the remaining bitmap indexes from mapped_total_index to process for @@ -389,7 +390,7 @@ int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque, return ret; } -EXPORT_SYMBOL_GPL(iova_bitmap_for_each); +EXPORT_SYMBOL_NS_GPL(iova_bitmap_for_each, IOMMUFD); /** * iova_bitmap_set() - Records an IOVA range in bitmap diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c index 25cfbf67031b..30f1656ac5da 100644 --- a/drivers/iommu/iommufd/main.c +++ b/drivers/iommu/iommufd/main.c @@ -558,5 +558,6 @@ MODULE_ALIAS_MISCDEV(VFIO_MINOR); MODULE_ALIAS("devname:vfio/vfio"); #endif MODULE_IMPORT_NS(IOMMUFD_INTERNAL); +MODULE_IMPORT_NS(IOMMUFD); MODULE_DESCRIPTION("I/O Address Space Management for passthrough devices"); MODULE_LICENSE("GPL"); diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index 40732e8ed4c6..a96d97da367d 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -1693,6 +1693,7 @@ static void __exit vfio_cleanup(void) module_init(vfio_init); module_exit(vfio_cleanup); +MODULE_IMPORT_NS(IOMMUFD); MODULE_VERSION(DRIVER_VERSION); MODULE_LICENSE("GPL v2"); MODULE_AUTHOR(DRIVER_AUTHOR);
On Tue, Oct 17, 2023 at 04:20:22PM +0100, Joao Martins wrote: > On 17/10/2023 13:58, Jason Gunthorpe wrote: > > On Mon, Oct 16, 2023 at 07:50:25PM +0100, Joao Martins wrote: > >> On 16/10/2023 19:37, Joao Martins wrote: > >>> On 16/10/2023 19:20, Jason Gunthorpe wrote: > >>>> On Mon, Oct 16, 2023 at 07:15:10PM +0100, Joao Martins wrote: > >>>> > >>>>> Here's a diff, naturally AMD/Intel kconfigs would get a select IOMMUFD_DRIVER as > >>>>> well later in the series > >>>> > >>>> It looks OK, the IS_ENABLES are probably overkill once you have > >>>> changed the .h file, just saves a few code bytes, not sure we care? > >>> > >>> I can remove them > >> > >> Additionally, I don't think I can use the symbol namespace for IOMMUFD, as > >> iova-bitmap can be build builtin with a module iommufd, otherwise we get into > >> errors like this: > >> > >> ERROR: modpost: module iommufd uses symbol iova_bitmap_for_each from namespace > >> IOMMUFD, but does not import it. > >> ERROR: modpost: module iommufd uses symbol iova_bitmap_free from namespace > >> IOMMUFD, but does not import it. > >> ERROR: modpost: module iommufd uses symbol iova_bitmap_alloc from namespace > >> IOMMUFD, but does not import it. > > > > You cannot self-import the namespace? I'm not that familiar with this stuff > > Neither do I. But self-importing looks to work. An alternative is to have an > alternative namespace (e.g. IOMMUFD_DRIVER) in similar fashion to IOMMUFD_INTERNAL. > > But I fear this patch is already doing too much at the late stage. Are you keen > on getting this moved with namespaces right now, or it can be a post-merge cleanup? It is our standard, if you want to make two patches in this series that is OK too Jason
On 17/10/2023 16:23, Jason Gunthorpe wrote: > On Tue, Oct 17, 2023 at 04:20:22PM +0100, Joao Martins wrote: >> On 17/10/2023 13:58, Jason Gunthorpe wrote: >>> On Mon, Oct 16, 2023 at 07:50:25PM +0100, Joao Martins wrote: >>>> On 16/10/2023 19:37, Joao Martins wrote: >>>>> On 16/10/2023 19:20, Jason Gunthorpe wrote: >>>>>> On Mon, Oct 16, 2023 at 07:15:10PM +0100, Joao Martins wrote: >>>>>> >>>>>>> Here's a diff, naturally AMD/Intel kconfigs would get a select IOMMUFD_DRIVER as >>>>>>> well later in the series >>>>>> >>>>>> It looks OK, the IS_ENABLES are probably overkill once you have >>>>>> changed the .h file, just saves a few code bytes, not sure we care? >>>>> >>>>> I can remove them >>>> >>>> Additionally, I don't think I can use the symbol namespace for IOMMUFD, as >>>> iova-bitmap can be build builtin with a module iommufd, otherwise we get into >>>> errors like this: >>>> >>>> ERROR: modpost: module iommufd uses symbol iova_bitmap_for_each from namespace >>>> IOMMUFD, but does not import it. >>>> ERROR: modpost: module iommufd uses symbol iova_bitmap_free from namespace >>>> IOMMUFD, but does not import it. >>>> ERROR: modpost: module iommufd uses symbol iova_bitmap_alloc from namespace >>>> IOMMUFD, but does not import it. >>> >>> You cannot self-import the namespace? I'm not that familiar with this stuff >> >> Neither do I. But self-importing looks to work. An alternative is to have an >> alternative namespace (e.g. IOMMUFD_DRIVER) in similar fashion to IOMMUFD_INTERNAL. >> >> But I fear this patch is already doing too much at the late stage. Are you keen >> on getting this moved with namespaces right now, or it can be a post-merge cleanup? > > It is our standard, if you want to make two patches in this series that is OK too > This is what I have now (as a separate patch). It is a little more intrusive as I need to change exist module users (mlx5-vfio-pci, pds, vfio). --->8--- From: Joao Martins <joao.m.martins@oracle.com> Date: Tue, 17 Oct 2023 11:12:28 -0400 Subject: [PATCH v4 03/20] iommufd/iova_bitmap: Move symbols to IOMMUFD namespace The IOVA bitmap helpers were not using any namespaces, so to adhere with IOMMUFD symbol export convention, use the IOMMUFD and import in the right places. This today means to self-import in iommufd/main.c, VFIO and the vfio-pci drivers that use iova_bitmap_set(). Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- drivers/iommu/iommufd/iova_bitmap.c | 8 ++++---- drivers/iommu/iommufd/main.c | 1 + drivers/vfio/pci/mlx5/main.c | 1 + drivers/vfio/pci/pds/pci_drv.c | 1 + drivers/vfio/vfio_main.c | 1 + 5 files changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/iommu/iommufd/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c index f54b56388e00..0a92c9eeaf7f 100644 --- a/drivers/iommu/iommufd/iova_bitmap.c +++ b/drivers/iommu/iommufd/iova_bitmap.c @@ -268,7 +268,7 @@ struct iova_bitmap *iova_bitmap_alloc(unsigned long iova, size_t length, iova_bitmap_free(bitmap); return ERR_PTR(rc); } -EXPORT_SYMBOL_GPL(iova_bitmap_alloc); +EXPORT_SYMBOL_NS_GPL(iova_bitmap_alloc, IOMMUFD); /** * iova_bitmap_free() - Frees an IOVA bitmap object @@ -290,7 +290,7 @@ void iova_bitmap_free(struct iova_bitmap *bitmap) kfree(bitmap); } -EXPORT_SYMBOL_GPL(iova_bitmap_free); +EXPORT_SYMBOL_NS_GPL(iova_bitmap_free, IOMMUFD); /* * Returns the remaining bitmap indexes from mapped_total_index to process for @@ -389,7 +389,7 @@ int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque, return ret; } -EXPORT_SYMBOL_GPL(iova_bitmap_for_each); +EXPORT_SYMBOL_NS_GPL(iova_bitmap_for_each, IOMMUFD); /** * iova_bitmap_set() - Records an IOVA range in bitmap @@ -423,4 +423,4 @@ void iova_bitmap_set(struct iova_bitmap *bitmap, cur_bit += nbits; } while (cur_bit <= last_bit); } -EXPORT_SYMBOL_GPL(iova_bitmap_set); +EXPORT_SYMBOL_NS_GPL(iova_bitmap_set, IOMMUFD); diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c index e71523cbd0de..9b2c18d7af1e 100644 --- a/drivers/iommu/iommufd/main.c +++ b/drivers/iommu/iommufd/main.c @@ -552,5 +552,6 @@ MODULE_ALIAS_MISCDEV(VFIO_MINOR); MODULE_ALIAS("devname:vfio/vfio"); #endif MODULE_IMPORT_NS(IOMMUFD_INTERNAL); +MODULE_IMPORT_NS(IOMMUFD); MODULE_DESCRIPTION("I/O Address Space Management for passthrough devices"); MODULE_LICENSE("GPL"); diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c index 42ec574a8622..5cf2b491d15a 100644 --- a/drivers/vfio/pci/mlx5/main.c +++ b/drivers/vfio/pci/mlx5/main.c @@ -1376,6 +1376,7 @@ static struct pci_driver mlx5vf_pci_driver = { module_pci_driver(mlx5vf_pci_driver); +MODULE_IMPORT_NS(IOMMUFD); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Max Gurtovoy <mgurtovoy@nvidia.com>"); MODULE_AUTHOR("Yishai Hadas <yishaih@nvidia.com>"); diff --git a/drivers/vfio/pci/pds/pci_drv.c b/drivers/vfio/pci/pds/pci_drv.c index ab4b5958e413..dd8c00c895a2 100644 --- a/drivers/vfio/pci/pds/pci_drv.c +++ b/drivers/vfio/pci/pds/pci_drv.c @@ -204,6 +204,7 @@ static struct pci_driver pds_vfio_pci_driver = { module_pci_driver(pds_vfio_pci_driver); +MODULE_IMPORT_NS(IOMMUFD); MODULE_DESCRIPTION(PDS_VFIO_DRV_DESCRIPTION); MODULE_AUTHOR("Brett Creeley <brett.creeley@amd.com>"); MODULE_LICENSE("GPL"); diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index 40732e8ed4c6..a96d97da367d 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -1693,6 +1693,7 @@ static void __exit vfio_cleanup(void) module_init(vfio_init); module_exit(vfio_cleanup); +MODULE_IMPORT_NS(IOMMUFD); MODULE_VERSION(DRIVER_VERSION); MODULE_LICENSE("GPL v2"); MODULE_AUTHOR(DRIVER_AUTHOR); -- 2.17.2
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. Joao
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 Jason
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. You could have the header file test for its presence of being a module instead of just IS_ENABLED() . But then this is useless as CONFIG_VFIO code is what drives the whole VFIO driver dirty tracking so it must override it as =y. This extra kconfig change at the end should fix it. But I am a bit skeptical of these last minute module-user changes, as it is getting a bit too nuanced from what was a relatively non invasive change. 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?) diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig index 6bda6dbb4878..1db519cce815 100644 --- a/drivers/vfio/Kconfig +++ b/drivers/vfio/Kconfig @@ -7,6 +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 help VFIO provides a framework for secure userspace device drivers. See Documentation/driver-api/vfio.rst for more details. diff --git a/drivers/iommu/iommufd/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c index f54b56388e00..350f6b615e91 100644 --- a/drivers/iommu/iommufd/iova_bitmap.c +++ b/drivers/iommu/iommufd/iova_bitmap.c @@ -7,6 +7,7 @@ #include <linux/mm.h> #include <linux/slab.h> #include <linux/highmem.h> +#include <linux/module.h> #define BITS_PER_PAGE (PAGE_SIZE * BITS_PER_BYTE) @@ -424,3 +425,5 @@ void iova_bitmap_set(struct iova_bitmap *bitmap, } while (cur_bit <= last_bit); } EXPORT_SYMBOL_GPL(iova_bitmap_set); + +MODULE_LICENSE("GPL v2");
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 On the VFIO module. > 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. Jason
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.
On Wed, Oct 18, 2023 at 04:34:17PM +0100, Joao Martins wrote: > 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. Oh I'm surprised by this > 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 I think you are better to stick with the bool non-modular in this case Jason
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 769e43d780ce..9d9dfbd2dfc2 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o obj-$(CONFIG_IOMMU_IO_PGTABLE_DART) += io-pgtable-dart.o obj-$(CONFIG_IOMMU_IOVA) += iova.o +obj-$(CONFIG_IOMMU_IOVA) += iova_bitmap.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o obj-$(CONFIG_IPMMU_VMSA) += ipmmu-vmsa.o diff --git a/drivers/vfio/iova_bitmap.c b/drivers/iommu/iova_bitmap.c similarity index 100% rename from drivers/vfio/iova_bitmap.c rename to drivers/iommu/iova_bitmap.c diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile index c82ea032d352..68c05705200f 100644 --- a/drivers/vfio/Makefile +++ b/drivers/vfio/Makefile @@ -1,8 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_VFIO) += vfio.o -vfio-y += vfio_main.o \ - iova_bitmap.o +vfio-y += vfio_main.o vfio-$(CONFIG_VFIO_DEVICE_CDEV) += device_cdev.o vfio-$(CONFIG_VFIO_GROUP) += group.o vfio-$(CONFIG_IOMMUFD) += iommufd.o
Both VFIO and IOMMUFD will need iova bitmap for storing dirties and walking the user bitmaps, so move to the common dependency into IOMMU core. IOMMUFD can't exactly host it given that VFIO dirty tracking can be used without IOMMUFD. Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- drivers/iommu/Makefile | 1 + drivers/{vfio => iommu}/iova_bitmap.c | 0 drivers/vfio/Makefile | 3 +-- 3 files changed, 2 insertions(+), 2 deletions(-) rename drivers/{vfio => iommu}/iova_bitmap.c (100%)