Messages in this thread | | | Date | Tue, 16 Aug 2022 22:22:02 +0200 | Subject | Re: [PATCH] vfio-pci/zdev: require KVM to be built-in | From | Pierre Morel <> |
| |
On 8/16/22 21:46, Matthew Rosato wrote: > 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); > } > >
Hello Matt,
In between I came to another solution that seems to satisfy the dependencies. Still need to check that the functionality is still intact :)
I send you the proposition as a reply.
Regards, Pierre
-- Pierre Morel IBM Lab Boeblingen
| |