On 8/16/22 3:55 AM, Pierre Morel wrote: > > > On 8/16/22 08:04, Randy Dunlap wrote: >> Hi-- >> >> On 8/15/22 02:43, Pierre Morel wrote: >>> Thank you Randy for this good catch. >>> However forcing KVM to be include statically in the kernel when using VFIO_PCI extensions is not a good solution for us I think. >>> >>> I suggest we better do something like: >>> >>> ---- >>> >>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>> index 6287a843e8bc..1733339cc4eb 100644 >>> --- a/arch/s390/include/asm/kvm_host.h >>> +++ b/arch/s390/include/asm/kvm_host.h >>> @@ -1038,7 +1038,7 @@ 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 >>> +#if defined(CONFIG_VFIO_PCI_ZDEV_KVM) || defined(CONFIG_VFIO_PCI_ZDEV_KVM_MODULE) >> >> This all looks good except for the line above. >> It should be: >> >> #if IS_ENABLED(CONFIG_VFIO_PCI_ZDEV_KVM) >> >> Thanks. > > Yes, better, thanks. > How do we do? Should I repost it with reported-by you or do you want to post it? > > Pierre Thanks for looking into this while I was away. I think the issue is not just CONFIG_KVM=m && CONFIG_VFIO_PCI_ZDEV_KVM=y -- it also requires CONFIG_VFIO_PCI=y && CONFIG_VFIO_PCI_CORE=y. This combination results in building in vfio_pci (which tries to link the calls to kvm_s390_pci_register_kvm and kvm_s390_pci_unregister_kvm which is not built in). However... this tristate + IS_ENABLED(CONFIG_VFIO_PCI_ZDEV_KVM) check in kvm_host.h will not solve the issue. Rather, due to the #ifdef CONFIG_VFIO_PCI_ZDEV_KVM in include/linux/vfio_pci_core.h, this change will just cause us to never call kvm_s390_pci_register_kvm or kvm_s390_pci_unregister_kvm when CONFIG_VFIO_PCI_ZDEV_KVM=m, effectively treating CONFIG_VFIO_PCI_ZDEV_KVM=m as a 'n' and we don't get the zdev kvm extensions, which I don't think was the intent. I'm still thinking & am open to other ideas, but one solution to avoiding building in KVM would be to go back to using symbol_get for these 2 interfaces (kvm_s390_pci_register_kvm and kvm_s390_pci_unregister_kvm) as was done in a prior version of this series like virt/kvm/vfio.c does and otherwise leave CONFIG_VFIO_PCI_ZDEV_KVM as-is. diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c index e163aa9f6144..09c2758134c7 100644 --- a/drivers/vfio/pci/vfio_pci_zdev.c +++ b/drivers/vfio/pci/vfio_pci_zdev.c @@ -144,6 +144,8 @@ int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev, int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev) { struct zpci_dev *zdev = to_zpci(vdev->pdev); + int (*fn)(struct zpci_dev *zdev, struct kvm *kvm); + int rc; if (!zdev) return -ENODEV; @@ -151,15 +153,30 @@ 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); + fn = symbol_get(kvm_s390_pci_register_kvm); + if (!fn) + return -EPERM; + + rc = fn(zdev, vdev->vdev.kvm); + + symbol_put(kvm_s390_pci_register_kvm); + + return rc; } void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev) { struct zpci_dev *zdev = to_zpci(vdev->pdev); + void (*fn)(struct zpci_dev *zdev); if (!zdev || !vdev->vdev.kvm) return; - kvm_s390_pci_unregister_kvm(zdev); + fn = symbol_get(kvm_s390_pci_unregister_kvm); + if (!fn) + return; + + fn(zdev); + + symbol_put(kvm_s390_pci_unregister_kvm); }