2022-08-18 17:18:50

by Pierre Morel

[permalink] [raw]
Subject: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO

We have a cross dependency between KVM and VFIO when using
s390 vfio_pci_zdev extensions for PCI passthrough
To be able to keep both subsystem modular we add a registering
hook inside the S390 core code.

This fixes a build problem when VFIO is built-in and KVM is built
as a module.

Reported-by: Randy Dunlap <[email protected]>
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Pierre Morel <[email protected]>
Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
Cc: <[email protected]>
---
arch/s390/include/asm/kvm_host.h | 17 ++++++-----------
arch/s390/kvm/pci.c | 10 ++++++----
arch/s390/pci/Makefile | 2 ++
arch/s390/pci/pci_kvm_hook.c | 11 +++++++++++
drivers/vfio/pci/vfio_pci_zdev.c | 8 ++++++--
5 files changed, 31 insertions(+), 17 deletions(-)
create mode 100644 arch/s390/pci/pci_kvm_hook.c

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index f39092e0ceaa..b1e98a9ed152 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -1038,16 +1038,11 @@ static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {}
#define __KVM_HAVE_ARCH_VM_FREE
void kvm_arch_free_vm(struct kvm *kvm);

-#ifdef CONFIG_VFIO_PCI_ZDEV_KVM
-int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm);
-void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev);
-#else
-static inline int kvm_s390_pci_register_kvm(struct zpci_dev *dev,
- struct kvm *kvm)
-{
- return -EPERM;
-}
-static inline void kvm_s390_pci_unregister_kvm(struct zpci_dev *dev) {}
-#endif
+struct zpci_kvm_hook {
+ int (*kvm_register)(void *opaque, struct kvm *kvm);
+ void (*kvm_unregister)(void *opaque);
+};
+
+extern struct zpci_kvm_hook zpci_kvm_hook;

#endif
diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 4946fb7757d6..22c025538323 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -431,8 +431,9 @@ static void kvm_s390_pci_dev_release(struct zpci_dev *zdev)
* available, enable them and let userspace indicate whether or not they will
* be used (specify SHM bit to disable).
*/
-int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm)
+static int kvm_s390_pci_register_kvm(void *opaque, struct kvm *kvm)
{
+ struct zpci_dev *zdev = opaque;
int rc;

if (!zdev)
@@ -510,10 +511,10 @@ int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm)
kvm_put_kvm(kvm);
return rc;
}
-EXPORT_SYMBOL_GPL(kvm_s390_pci_register_kvm);

-void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev)
+static void kvm_s390_pci_unregister_kvm(void *opaque)
{
+ struct zpci_dev *zdev = opaque;
struct kvm *kvm;

if (!zdev)
@@ -566,7 +567,6 @@ void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev)

kvm_put_kvm(kvm);
}
-EXPORT_SYMBOL_GPL(kvm_s390_pci_unregister_kvm);

void kvm_s390_pci_init_list(struct kvm *kvm)
{
@@ -678,6 +678,8 @@ int kvm_s390_pci_init(void)

spin_lock_init(&aift->gait_lock);
mutex_init(&aift->aift_lock);
+ zpci_kvm_hook.kvm_register = kvm_s390_pci_register_kvm;
+ zpci_kvm_hook.kvm_unregister = kvm_s390_pci_unregister_kvm;

return 0;
}
diff --git a/arch/s390/pci/Makefile b/arch/s390/pci/Makefile
index bf557a1b789c..c02dbfb415d9 100644
--- a/arch/s390/pci/Makefile
+++ b/arch/s390/pci/Makefile
@@ -7,3 +7,5 @@ obj-$(CONFIG_PCI) += pci.o pci_irq.o pci_dma.o pci_clp.o pci_sysfs.o \
pci_event.o pci_debug.o pci_insn.o pci_mmio.o \
pci_bus.o
obj-$(CONFIG_PCI_IOV) += pci_iov.o
+
+obj-y += pci_kvm_hook.o
diff --git a/arch/s390/pci/pci_kvm_hook.c b/arch/s390/pci/pci_kvm_hook.c
new file mode 100644
index 000000000000..ff34baf50a3e
--- /dev/null
+++ b/arch/s390/pci/pci_kvm_hook.c
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * VFIO ZPCI devices support
+ *
+ * Copyright (C) IBM Corp. 2022. All rights reserved.
+ * Author(s): Pierre Morel <[email protected]>
+ */
+#include <linux/kvm_host.h>
+
+struct zpci_kvm_hook zpci_kvm_hook;
+EXPORT_SYMBOL_GPL(zpci_kvm_hook);
diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
index e163aa9f6144..0cbdcd14f1c8 100644
--- a/drivers/vfio/pci/vfio_pci_zdev.c
+++ b/drivers/vfio/pci/vfio_pci_zdev.c
@@ -151,7 +151,10 @@ int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev)
if (!vdev->vdev.kvm)
return 0;

- return kvm_s390_pci_register_kvm(zdev, vdev->vdev.kvm);
+ if (zpci_kvm_hook.kvm_register)
+ return zpci_kvm_hook.kvm_register(zdev, vdev->vdev.kvm);
+
+ return -ENOENT;
}

void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev)
@@ -161,5 +164,6 @@ void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev)
if (!zdev || !vdev->vdev.kvm)
return;

- kvm_s390_pci_unregister_kvm(zdev);
+ if (zpci_kvm_hook.kvm_unregister)
+ zpci_kvm_hook.kvm_unregister(zdev);
}
--
2.31.1


2022-08-18 21:03:45

by Pierre Morel

[permalink] [raw]
Subject: Re: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO



On 8/18/22 22:38, Matthew Rosato wrote:
> On 8/18/22 12:46 PM, Pierre Morel wrote:
>> We have a cross dependency between KVM and VFIO when using
>> s390 vfio_pci_zdev extensions for PCI passthrough
>> To be able to keep both subsystem modular we add a registering
>> hook inside the S390 core code.
>>
>> This fixes a build problem when VFIO is built-in and KVM is built
>> as a module.
>>
>> Reported-by: Randy Dunlap <[email protected]>
>> Reported-by: kernel test robot <[email protected]>
>> Signed-off-by: Pierre Morel <[email protected]>
>> Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
>> Cc: <[email protected]>
>> ---
>> arch/s390/include/asm/kvm_host.h | 17 ++++++-----------
>> arch/s390/kvm/pci.c | 10 ++++++----
>> arch/s390/pci/Makefile | 2 ++
>> arch/s390/pci/pci_kvm_hook.c | 11 +++++++++++
>> drivers/vfio/pci/vfio_pci_zdev.c | 8 ++++++--
>> 5 files changed, 31 insertions(+), 17 deletions(-)
>> create mode 100644 arch/s390/pci/pci_kvm_hook.c
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index f39092e0ceaa..b1e98a9ed152 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -1038,16 +1038,11 @@ static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {}
>> #define __KVM_HAVE_ARCH_VM_FREE
>> void kvm_arch_free_vm(struct kvm *kvm);
>>
>> -#ifdef CONFIG_VFIO_PCI_ZDEV_KVM
>> -int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm);
>> -void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev);
>> -#else
>> -static inline int kvm_s390_pci_register_kvm(struct zpci_dev *dev,
>> - struct kvm *kvm)
>> -{
>> - return -EPERM;
>> -}
>> -static inline void kvm_s390_pci_unregister_kvm(struct zpci_dev *dev) {}
>> -#endif
>> +struct zpci_kvm_hook {
>> + int (*kvm_register)(void *opaque, struct kvm *kvm);
>> + void (*kvm_unregister)(void *opaque);
>> +};
>> +
>> +extern struct zpci_kvm_hook zpci_kvm_hook;
>>
>> #endif
>> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
>> index 4946fb7757d6..22c025538323 100644
>> --- a/arch/s390/kvm/pci.c
>> +++ b/arch/s390/kvm/pci.c
>> @@ -431,8 +431,9 @@ static void kvm_s390_pci_dev_release(struct zpci_dev *zdev)
>> * available, enable them and let userspace indicate whether or not they will
>> * be used (specify SHM bit to disable).
>> */
>> -int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm)
>> +static int kvm_s390_pci_register_kvm(void *opaque, struct kvm *kvm)
>> {
>> + struct zpci_dev *zdev = opaque;
>> int rc;
>>
>> if (!zdev)
>> @@ -510,10 +511,10 @@ int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm)
>> kvm_put_kvm(kvm);
>> return rc;
>> }
>> -EXPORT_SYMBOL_GPL(kvm_s390_pci_register_kvm);
>>
>> -void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev)
>> +static void kvm_s390_pci_unregister_kvm(void *opaque)
>> {
>> + struct zpci_dev *zdev = opaque;
>> struct kvm *kvm;
>>
>> if (!zdev)
>> @@ -566,7 +567,6 @@ void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev)
>>
>> kvm_put_kvm(kvm);
>> }
>> -EXPORT_SYMBOL_GPL(kvm_s390_pci_unregister_kvm);
>>
>> void kvm_s390_pci_init_list(struct kvm *kvm)
>> {
>> @@ -678,6 +678,8 @@ int kvm_s390_pci_init(void)
>>
>> spin_lock_init(&aift->gait_lock);
>> mutex_init(&aift->aift_lock);
>> + zpci_kvm_hook.kvm_register = kvm_s390_pci_register_kvm;
>> + zpci_kvm_hook.kvm_unregister = kvm_s390_pci_unregister_kvm;
>>
>> return 0;
>> }
>
> You should also set these to NULL in kvm_s390_pci_exit (which is called from kvm_arch_exit). In practice, the kvm module would need to be loaded again before we have a nonzero vdev->vdev.kvm so it should never be an issue - but we should clean up anyway when the module is removed.
>
> With that change:
>
> Reviewed-by: Matthew Rosato <[email protected]>
>

Yes indeed, will do.
Thanks

Pierre

--
Pierre Morel
IBM Lab Boeblingen

2022-08-18 21:41:30

by Matthew Rosato

[permalink] [raw]
Subject: Re: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO

On 8/18/22 12:46 PM, Pierre Morel wrote:
> We have a cross dependency between KVM and VFIO when using
> s390 vfio_pci_zdev extensions for PCI passthrough
> To be able to keep both subsystem modular we add a registering
> hook inside the S390 core code.
>
> This fixes a build problem when VFIO is built-in and KVM is built
> as a module.
>
> Reported-by: Randy Dunlap <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Pierre Morel <[email protected]>
> Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
> Cc: <[email protected]>
> ---
> arch/s390/include/asm/kvm_host.h | 17 ++++++-----------
> arch/s390/kvm/pci.c | 10 ++++++----
> arch/s390/pci/Makefile | 2 ++
> arch/s390/pci/pci_kvm_hook.c | 11 +++++++++++
> drivers/vfio/pci/vfio_pci_zdev.c | 8 ++++++--
> 5 files changed, 31 insertions(+), 17 deletions(-)
> create mode 100644 arch/s390/pci/pci_kvm_hook.c
>
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index f39092e0ceaa..b1e98a9ed152 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -1038,16 +1038,11 @@ static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {}
> #define __KVM_HAVE_ARCH_VM_FREE
> void kvm_arch_free_vm(struct kvm *kvm);
>
> -#ifdef CONFIG_VFIO_PCI_ZDEV_KVM
> -int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm);
> -void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev);
> -#else
> -static inline int kvm_s390_pci_register_kvm(struct zpci_dev *dev,
> - struct kvm *kvm)
> -{
> - return -EPERM;
> -}
> -static inline void kvm_s390_pci_unregister_kvm(struct zpci_dev *dev) {}
> -#endif
> +struct zpci_kvm_hook {
> + int (*kvm_register)(void *opaque, struct kvm *kvm);
> + void (*kvm_unregister)(void *opaque);
> +};
> +
> +extern struct zpci_kvm_hook zpci_kvm_hook;
>
> #endif
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 4946fb7757d6..22c025538323 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -431,8 +431,9 @@ static void kvm_s390_pci_dev_release(struct zpci_dev *zdev)
> * available, enable them and let userspace indicate whether or not they will
> * be used (specify SHM bit to disable).
> */
> -int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm)
> +static int kvm_s390_pci_register_kvm(void *opaque, struct kvm *kvm)
> {
> + struct zpci_dev *zdev = opaque;
> int rc;
>
> if (!zdev)
> @@ -510,10 +511,10 @@ int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm)
> kvm_put_kvm(kvm);
> return rc;
> }
> -EXPORT_SYMBOL_GPL(kvm_s390_pci_register_kvm);
>
> -void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev)
> +static void kvm_s390_pci_unregister_kvm(void *opaque)
> {
> + struct zpci_dev *zdev = opaque;
> struct kvm *kvm;
>
> if (!zdev)
> @@ -566,7 +567,6 @@ void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev)
>
> kvm_put_kvm(kvm);
> }
> -EXPORT_SYMBOL_GPL(kvm_s390_pci_unregister_kvm);
>
> void kvm_s390_pci_init_list(struct kvm *kvm)
> {
> @@ -678,6 +678,8 @@ int kvm_s390_pci_init(void)
>
> spin_lock_init(&aift->gait_lock);
> mutex_init(&aift->aift_lock);
> + zpci_kvm_hook.kvm_register = kvm_s390_pci_register_kvm;
> + zpci_kvm_hook.kvm_unregister = kvm_s390_pci_unregister_kvm;
>
> return 0;
> }

You should also set these to NULL in kvm_s390_pci_exit (which is called from kvm_arch_exit). In practice, the kvm module would need to be loaded again before we have a nonzero vdev->vdev.kvm so it should never be an issue - but we should clean up anyway when the module is removed.

With that change:

Reviewed-by: Matthew Rosato <[email protected]>

> diff --git a/arch/s390/pci/Makefile b/arch/s390/pci/Makefile
> index bf557a1b789c..c02dbfb415d9 100644
> --- a/arch/s390/pci/Makefile
> +++ b/arch/s390/pci/Makefile
> @@ -7,3 +7,5 @@ obj-$(CONFIG_PCI) += pci.o pci_irq.o pci_dma.o pci_clp.o pci_sysfs.o \
> pci_event.o pci_debug.o pci_insn.o pci_mmio.o \
> pci_bus.o
> obj-$(CONFIG_PCI_IOV) += pci_iov.o
> +
> +obj-y += pci_kvm_hook.o
> diff --git a/arch/s390/pci/pci_kvm_hook.c b/arch/s390/pci/pci_kvm_hook.c
> new file mode 100644
> index 000000000000..ff34baf50a3e
> --- /dev/null
> +++ b/arch/s390/pci/pci_kvm_hook.c
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * VFIO ZPCI devices support
> + *
> + * Copyright (C) IBM Corp. 2022. All rights reserved.
> + * Author(s): Pierre Morel <[email protected]>
> + */
> +#include <linux/kvm_host.h>
> +
> +struct zpci_kvm_hook zpci_kvm_hook;
> +EXPORT_SYMBOL_GPL(zpci_kvm_hook);
> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
> index e163aa9f6144..0cbdcd14f1c8 100644
> --- a/drivers/vfio/pci/vfio_pci_zdev.c
> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
> @@ -151,7 +151,10 @@ int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev)
> if (!vdev->vdev.kvm)
> return 0;
>
> - return kvm_s390_pci_register_kvm(zdev, vdev->vdev.kvm);
> + if (zpci_kvm_hook.kvm_register)
> + return zpci_kvm_hook.kvm_register(zdev, vdev->vdev.kvm);
> +
> + return -ENOENT;
> }
>
> void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev)
> @@ -161,5 +164,6 @@ void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev)
> if (!zdev || !vdev->vdev.kvm)
> return;
>
> - kvm_s390_pci_unregister_kvm(zdev);
> + if (zpci_kvm_hook.kvm_unregister)
> + zpci_kvm_hook.kvm_unregister(zdev);
> }

2022-08-19 07:49:43

by Niklas Schnelle

[permalink] [raw]
Subject: Re: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO

On Thu, 2022-08-18 at 18:46 +0200, Pierre Morel wrote:
> We have a cross dependency between KVM and VFIO when using
> s390 vfio_pci_zdev extensions for PCI passthrough
> To be able to keep both subsystem modular we add a registering
> hook inside the S390 core code.
>
> This fixes a build problem when VFIO is built-in and KVM is built
> as a module.
>
> Reported-by: Randy Dunlap <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Pierre Morel <[email protected]>
> Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")

Please don't shorten the Fixes tag, the subject line is likely also
checked by some automated tools. It's okay for this line to be over the
column limit and checkpatch.pl --strict also accepts it.

> Cc: <[email protected]>
> ---
> arch/s390/include/asm/kvm_host.h | 17 ++++++-----------
> arch/s390/kvm/pci.c | 10 ++++++----
> arch/s390/pci/Makefile | 2 ++
> arch/s390/pci/pci_kvm_hook.c | 11 +++++++++++
> drivers/vfio/pci/vfio_pci_zdev.c | 8 ++++++--
> 5 files changed, 31 insertions(+), 17 deletions(-)
> create mode 100644 arch/s390/pci/pci_kvm_hook.c
>
>
---8<---
>
> kvm_put_kvm(kvm);
> }
> -EXPORT_SYMBOL_GPL(kvm_s390_pci_unregister_kvm);
>
> void kvm_s390_pci_init_list(struct kvm *kvm)
> {
> @@ -678,6 +678,8 @@ int kvm_s390_pci_init(void)
>
> spin_lock_init(&aift->gait_lock);
> mutex_init(&aift->aift_lock);
> + zpci_kvm_hook.kvm_register = kvm_s390_pci_register_kvm;
> + zpci_kvm_hook.kvm_unregister = kvm_s390_pci_unregister_kvm;
>
> return 0;
> }
> diff --git a/arch/s390/pci/Makefile b/arch/s390/pci/Makefile
> index bf557a1b789c..c02dbfb415d9 100644
> --- a/arch/s390/pci/Makefile
> +++ b/arch/s390/pci/Makefile
> @@ -7,3 +7,5 @@ obj-$(CONFIG_PCI) += pci.o pci_irq.o pci_dma.o pci_clp.o pci_sysfs.o \
> pci_event.o pci_debug.o pci_insn.o pci_mmio.o \
> pci_bus.o
> obj-$(CONFIG_PCI_IOV) += pci_iov.o
> +
> +obj-y += pci_kvm_hook.o

I thought we wanted to compile this only for CONFIG_PCI?

> diff --git a/arch/s390/pci/pci_kvm_hook.c b/arch/s390/pci/pci_kvm_hook.c
> new file mode 100644
> index 000000000000..ff34baf50a3e
---8<---

2022-08-19 09:10:33

by Pierre Morel

[permalink] [raw]
Subject: Re: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO



On 8/19/22 09:14, Niklas Schnelle wrote:
> On Thu, 2022-08-18 at 18:46 +0200, Pierre Morel wrote:
>> We have a cross dependency between KVM and VFIO when using
>> s390 vfio_pci_zdev extensions for PCI passthrough
>> To be able to keep both subsystem modular we add a registering
>> hook inside the S390 core code.
>>
>> This fixes a build problem when VFIO is built-in and KVM is built
>> as a module.
>>
>> Reported-by: Randy Dunlap <[email protected]>
>> Reported-by: kernel test robot <[email protected]>
>> Signed-off-by: Pierre Morel <[email protected]>
>> Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
>
> Please don't shorten the Fixes tag, the subject line is likely also
> checked by some automated tools. It's okay for this line to be over the
> column limit and checkpatch.pl --strict also accepts it.
>

OK

>> Cc: <[email protected]>
>> ---
>> arch/s390/include/asm/kvm_host.h | 17 ++++++-----------
>> arch/s390/kvm/pci.c | 10 ++++++----
>> arch/s390/pci/Makefile | 2 ++
>> arch/s390/pci/pci_kvm_hook.c | 11 +++++++++++
>> drivers/vfio/pci/vfio_pci_zdev.c | 8 ++++++--
>> 5 files changed, 31 insertions(+), 17 deletions(-)
>> create mode 100644 arch/s390/pci/pci_kvm_hook.c
>>
>>
> ---8<---
>>
>> kvm_put_kvm(kvm);
>> }
>> -EXPORT_SYMBOL_GPL(kvm_s390_pci_unregister_kvm);
>>
>> void kvm_s390_pci_init_list(struct kvm *kvm)
>> {
>> @@ -678,6 +678,8 @@ int kvm_s390_pci_init(void)
>>
>> spin_lock_init(&aift->gait_lock);
>> mutex_init(&aift->aift_lock);
>> + zpci_kvm_hook.kvm_register = kvm_s390_pci_register_kvm;
>> + zpci_kvm_hook.kvm_unregister = kvm_s390_pci_unregister_kvm;
>>
>> return 0;
>> }
>> diff --git a/arch/s390/pci/Makefile b/arch/s390/pci/Makefile
>> index bf557a1b789c..c02dbfb415d9 100644
>> --- a/arch/s390/pci/Makefile
>> +++ b/arch/s390/pci/Makefile
>> @@ -7,3 +7,5 @@ obj-$(CONFIG_PCI) += pci.o pci_irq.o pci_dma.o pci_clp.o pci_sysfs.o \
>> pci_event.o pci_debug.o pci_insn.o pci_mmio.o \
>> pci_bus.o
>> obj-$(CONFIG_PCI_IOV) += pci_iov.o
>> +
>> +obj-y += pci_kvm_hook.o
>
> I thought we wanted to compile this only for CONFIG_PCI?

Ah sorry, that is indeed what I understood with Matt but then I
misunderstood your own answer from yesterday.
I change to
obj-$(CONFIG_PCI) += pci_kvm_hook.o

>
>> diff --git a/arch/s390/pci/pci_kvm_hook.c b/arch/s390/pci/pci_kvm_hook.c
>> new file mode 100644
>> index 000000000000..ff34baf50a3e
> ---8<---
>

--
Pierre Morel
IBM Lab Boeblingen

2022-08-19 11:23:00

by Niklas Schnelle

[permalink] [raw]
Subject: Re: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO

On Fri, 2022-08-19 at 10:44 +0200, Pierre Morel wrote:
>
> On 8/19/22 09:14, Niklas Schnelle wrote:
> > On Thu, 2022-08-18 at 18:46 +0200, Pierre Morel wrote:
> > > We have a cross dependency between KVM and VFIO when using
> > > s390 vfio_pci_zdev extensions for PCI passthrough
> > > To be able to keep both subsystem modular we add a registering
> > > hook inside the S390 core code.
> > >
> > > This fixes a build problem when VFIO is built-in and KVM is built
> > > as a module.
> > >
> > > Reported-by: Randy Dunlap <[email protected]>
> > > Reported-by: kernel test robot <[email protected]>
> > > Signed-off-by: Pierre Morel <[email protected]>
> > > Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
> >
> > Please don't shorten the Fixes tag, the subject line is likely also
> > checked by some automated tools. It's okay for this line to be over the
> > column limit and checkpatch.pl --strict also accepts it.
> >
>
> OK
>
> > > Cc: <[email protected]>
> > > ---
> > > arch/s390/include/asm/kvm_host.h | 17 ++++++-----------
> > > arch/s390/kvm/pci.c | 10 ++++++----
> > > arch/s390/pci/Makefile | 2 ++
> > > arch/s390/pci/pci_kvm_hook.c | 11 +++++++++++
> > > drivers/vfio/pci/vfio_pci_zdev.c | 8 ++++++--
> > > 5 files changed, 31 insertions(+), 17 deletions(-)
> > > create mode 100644 arch/s390/pci/pci_kvm_hook.c
> > >
> > >
> > ---8<---
> > >
> > > kvm_put_kvm(kvm);
> > > }
> > > -EXPORT_SYMBOL_GPL(kvm_s390_pci_unregister_kvm);
> > >
> > > void kvm_s390_pci_init_list(struct kvm *kvm)
> > > {
> > > @@ -678,6 +678,8 @@ int kvm_s390_pci_init(void)
> > >
> > > spin_lock_init(&aift->gait_lock);
> > > mutex_init(&aift->aift_lock);
> > > + zpci_kvm_hook.kvm_register = kvm_s390_pci_register_kvm;
> > > + zpci_kvm_hook.kvm_unregister = kvm_s390_pci_unregister_kvm;
> > >
> > > return 0;
> > > }
> > > diff --git a/arch/s390/pci/Makefile b/arch/s390/pci/Makefile
> > > index bf557a1b789c..c02dbfb415d9 100644
> > > --- a/arch/s390/pci/Makefile
> > > +++ b/arch/s390/pci/Makefile
> > > @@ -7,3 +7,5 @@ obj-$(CONFIG_PCI) += pci.o pci_irq.o pci_dma.o pci_clp.o pci_sysfs.o \
> > > pci_event.o pci_debug.o pci_insn.o pci_mmio.o \
> > > pci_bus.o
> > > obj-$(CONFIG_PCI_IOV) += pci_iov.o
> > > +
> > > +obj-y += pci_kvm_hook.o
> >
> > I thought we wanted to compile this only for CONFIG_PCI?
>
> Ah sorry, that is indeed what I understood with Matt but then I
> misunderstood your own answer from yesterday.
> I change to
> obj-$(CONFIG_PCI) += pci_kvm_hook.o
>
> > > diff --git a/arch/s390/pci/pci_kvm_hook.c b/arch/s390/pci/pci_kvm_hook.c
> > > new file mode 100644
> > > index 000000000000..ff34baf50a3e
> > ---8<---
> >

Ok with the two things above plus the comment by Matt incorporated:

Reviewed-by: Niklas Schnelle <[email protected]>

2022-08-19 12:03:19

by Pierre Morel

[permalink] [raw]
Subject: Re: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO



On 8/19/22 12:42, Niklas Schnelle wrote:
> On Fri, 2022-08-19 at 10:44 +0200, Pierre Morel wrote:
>>
>> On 8/19/22 09:14, Niklas Schnelle wrote:
>>> On Thu, 2022-08-18 at 18:46 +0200, Pierre Morel wrote:
>>>> We have a cross dependency between KVM and VFIO when using
>>>> s390 vfio_pci_zdev extensions for PCI passthrough
>>>> To be able to keep both subsystem modular we add a registering
>>>> hook inside the S390 core code.
>>>>
>>>> This fixes a build problem when VFIO is built-in and KVM is built
>>>> as a module.
>>>>
>>>> Reported-by: Randy Dunlap <[email protected]>
>>>> Reported-by: kernel test robot <[email protected]>
>>>> Signed-off-by: Pierre Morel <[email protected]>
>>>> Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
>>>
>>> Please don't shorten the Fixes tag, the subject line is likely also
>>> checked by some automated tools. It's okay for this line to be over the
>>> column limit and checkpatch.pl --strict also accepts it.
>>>
>>
>> OK
>>
>>>> Cc: <[email protected]>
>>>> ---
>>>> arch/s390/include/asm/kvm_host.h | 17 ++++++-----------
>>>> arch/s390/kvm/pci.c | 10 ++++++----
>>>> arch/s390/pci/Makefile | 2 ++
>>>> arch/s390/pci/pci_kvm_hook.c | 11 +++++++++++
>>>> drivers/vfio/pci/vfio_pci_zdev.c | 8 ++++++--
>>>> 5 files changed, 31 insertions(+), 17 deletions(-)
>>>> create mode 100644 arch/s390/pci/pci_kvm_hook.c
>>>>
>>>>
>>> ---8<---
>>>>
>>>> kvm_put_kvm(kvm);
>>>> }
>>>> -EXPORT_SYMBOL_GPL(kvm_s390_pci_unregister_kvm);
>>>>
>>>> void kvm_s390_pci_init_list(struct kvm *kvm)
>>>> {
>>>> @@ -678,6 +678,8 @@ int kvm_s390_pci_init(void)
>>>>
>>>> spin_lock_init(&aift->gait_lock);
>>>> mutex_init(&aift->aift_lock);
>>>> + zpci_kvm_hook.kvm_register = kvm_s390_pci_register_kvm;
>>>> + zpci_kvm_hook.kvm_unregister = kvm_s390_pci_unregister_kvm;
>>>>
>>>> return 0;
>>>> }
>>>> diff --git a/arch/s390/pci/Makefile b/arch/s390/pci/Makefile
>>>> index bf557a1b789c..c02dbfb415d9 100644
>>>> --- a/arch/s390/pci/Makefile
>>>> +++ b/arch/s390/pci/Makefile
>>>> @@ -7,3 +7,5 @@ obj-$(CONFIG_PCI) += pci.o pci_irq.o pci_dma.o pci_clp.o pci_sysfs.o \
>>>> pci_event.o pci_debug.o pci_insn.o pci_mmio.o \
>>>> pci_bus.o
>>>> obj-$(CONFIG_PCI_IOV) += pci_iov.o
>>>> +
>>>> +obj-y += pci_kvm_hook.o
>>>
>>> I thought we wanted to compile this only for CONFIG_PCI?
>>
>> Ah sorry, that is indeed what I understood with Matt but then I
>> misunderstood your own answer from yesterday.
>> I change to
>> obj-$(CONFIG_PCI) += pci_kvm_hook.o
>>
>>>> diff --git a/arch/s390/pci/pci_kvm_hook.c b/arch/s390/pci/pci_kvm_hook.c
>>>> new file mode 100644
>>>> index 000000000000..ff34baf50a3e
>>> ---8<---
>>>
>
> Ok with the two things above plus the comment by Matt incorporated:
>
> Reviewed-by: Niklas Schnelle <[email protected]>
>

Just a little correction, it changes nothing if the pci_kvm_hook.c goes
on same lines as other CONFIG_PCI depending files.
So I put it on the same line.

Thanks

Pierre

--
Pierre Morel
IBM Lab Boeblingen

2022-08-19 12:16:39

by Niklas Schnelle

[permalink] [raw]
Subject: Re: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO

---8<---
> > > > > diff --git a/arch/s390/pci/Makefile b/arch/s390/pci/Makefile
> > > > > index bf557a1b789c..c02dbfb415d9 100644
> > > > > --- a/arch/s390/pci/Makefile
> > > > > +++ b/arch/s390/pci/Makefile
> > > > > @@ -7,3 +7,5 @@ obj-$(CONFIG_PCI) += pci.o pci_irq.o pci_dma.o pci_clp.o pci_sysfs.o \
> > > > > pci_event.o pci_debug.o pci_insn.o pci_mmio.o \
> > > > > pci_bus.o
> > > > > obj-$(CONFIG_PCI_IOV) += pci_iov.o
> > > > > +
> > > > > +obj-y += pci_kvm_hook.o
> > > >
> > > > I thought we wanted to compile this only for CONFIG_PCI?
> > >
> > > Ah sorry, that is indeed what I understood with Matt but then I
> > > misunderstood your own answer from yesterday.
> > > I change to
> > > obj-$(CONFIG_PCI) += pci_kvm_hook.o
> > >
> > > > > diff --git a/arch/s390/pci/pci_kvm_hook.c b/arch/s390/pci/pci_kvm_hook.c
> > > > > new file mode 100644
> > > > > index 000000000000..ff34baf50a3e
> > > > ---8<---
> > > >
> >
> > Ok with the two things above plus the comment by Matt incorporated:
> >
> > Reviewed-by: Niklas Schnelle <[email protected]>
> >
>
> Just a little correction, it changes nothing if the pci_kvm_hook.c goes
> on same lines as other CONFIG_PCI depending files.
> So I put it on the same line.
>
> Thanks
>
> Pierre
>

Of course yes. Thanks for fixing this and I'm assuming this would
either go through the KVM or vfio trees, correct?