* [PATCH] vfio-pci/zdev: require KVM to be built-in
@ 2022-08-14 21:51 Randy Dunlap
2022-08-15 9:43 ` Pierre Morel
0 siblings, 1 reply; 17+ messages in thread
From: Randy Dunlap @ 2022-08-14 21:51 UTC (permalink / raw)
To: linux-kernel
Cc: Randy Dunlap, kernel test robot, Matthew Rosato,
Christian Borntraeger, Pierre Morel, Eric Farman, linux-s390,
kvm
Fix build errors when CONFIG_KVM=m:
s390-linux-ld: drivers/vfio/pci/vfio_pci_zdev.o: in function `vfio_pci_zdev_open_device':
vfio_pci_zdev.c:(.text+0x242): undefined reference to `kvm_s390_pci_register_kvm'
s390-linux-ld: drivers/vfio/pci/vfio_pci_zdev.o: in function `vfio_pci_zdev_close_device':
vfio_pci_zdev.c:(.text+0x296): undefined reference to `kvm_s390_pci_unregister_kvm'
Having a bool Kconfig symbol depend on a tristate symbol can often
lead to problems like this.
Fixes: 8061d1c31f1a ("vfio-pci/zdev: add open/close device hooks")
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Reported-by: kernel test robot <lkp@intel.com>
Cc: Matthew Rosato <mjrosato@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Pierre Morel <pmorel@linux.ibm.com>
Cc: Eric Farman <farman@linux.ibm.com>
Cc: linux-s390@vger.kernel.org
Cc: kvm@vger.kernel.org
---
drivers/vfio/pci/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -46,7 +46,7 @@ endif
config VFIO_PCI_ZDEV_KVM
bool "VFIO PCI extensions for s390x KVM passthrough"
- depends on S390 && KVM
+ depends on S390 && KVM=y
default y
help
Support s390x-specific extensions to enable support for enhancements
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] vfio-pci/zdev: require KVM to be built-in
2022-08-14 21:51 [PATCH] vfio-pci/zdev: require KVM to be built-in Randy Dunlap
@ 2022-08-15 9:43 ` Pierre Morel
2022-08-16 6:04 ` Randy Dunlap
0 siblings, 1 reply; 17+ messages in thread
From: Pierre Morel @ 2022-08-15 9:43 UTC (permalink / raw)
To: Randy Dunlap, linux-kernel
Cc: kernel test robot, Matthew Rosato, Christian Borntraeger,
Eric Farman, linux-s390, kvm
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)
int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm);
void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev);
#else
diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index f9d0c908e738..bbc375b028ef 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -45,9 +45,9 @@ config VFIO_PCI_IGD
endif
config VFIO_PCI_ZDEV_KVM
- bool "VFIO PCI extensions for s390x KVM passthrough"
+ def_tristate y
+ prompt "VFIO PCI extensions for s390x KVM passthrough"
depends on S390 && KVM
- default y
help
Support s390x-specific extensions to enable support for
enhancements
to KVM passthrough capabilities, such as interpretive
execution of
----
What do you think? It seems to me it solves the problem, what do you think?
Regards,
Pierre
On 8/14/22 23:51, Randy Dunlap wrote:
> Fix build errors when CONFIG_KVM=m:
>
> s390-linux-ld: drivers/vfio/pci/vfio_pci_zdev.o: in function `vfio_pci_zdev_open_device':
> vfio_pci_zdev.c:(.text+0x242): undefined reference to `kvm_s390_pci_register_kvm'
> s390-linux-ld: drivers/vfio/pci/vfio_pci_zdev.o: in function `vfio_pci_zdev_close_device':
> vfio_pci_zdev.c:(.text+0x296): undefined reference to `kvm_s390_pci_unregister_kvm'
>
> Having a bool Kconfig symbol depend on a tristate symbol can often
> lead to problems like this.
>
> Fixes: 8061d1c31f1a ("vfio-pci/zdev: add open/close device hooks")
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Cc: Matthew Rosato <mjrosato@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> Cc: Pierre Morel <pmorel@linux.ibm.com>
> Cc: Eric Farman <farman@linux.ibm.com>
> Cc: linux-s390@vger.kernel.org
> Cc: kvm@vger.kernel.org
> ---
> drivers/vfio/pci/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -46,7 +46,7 @@ endif
>
> config VFIO_PCI_ZDEV_KVM
> bool "VFIO PCI extensions for s390x KVM passthrough"
> - depends on S390 && KVM
> + depends on S390 && KVM=y
> default y
> help
> Support s390x-specific extensions to enable support for enhancements
>
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] vfio-pci/zdev: require KVM to be built-in
2022-08-15 9:43 ` Pierre Morel
@ 2022-08-16 6:04 ` Randy Dunlap
2022-08-16 7:55 ` Pierre Morel
0 siblings, 1 reply; 17+ messages in thread
From: Randy Dunlap @ 2022-08-16 6:04 UTC (permalink / raw)
To: Pierre Morel, linux-kernel
Cc: kernel test robot, Matthew Rosato, Christian Borntraeger,
Eric Farman, linux-s390, kvm
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.
> int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm);
> void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev);
> #else
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index f9d0c908e738..bbc375b028ef 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -45,9 +45,9 @@ config VFIO_PCI_IGD
> endif
>
> config VFIO_PCI_ZDEV_KVM
> - bool "VFIO PCI extensions for s390x KVM passthrough"
> + def_tristate y
> + prompt "VFIO PCI extensions for s390x KVM passthrough"
> depends on S390 && KVM
> - default y
> help
> Support s390x-specific extensions to enable support for enhancements
> to KVM passthrough capabilities, such as interpretive execution of
>
> ----
>
> What do you think? It seems to me it solves the problem, what do you think?
>
> Regards,
> Pierre
--
~Randy
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] vfio-pci/zdev: require KVM to be built-in
2022-08-16 6:04 ` Randy Dunlap
@ 2022-08-16 7:55 ` Pierre Morel
2022-08-16 13:47 ` Pierre Morel
2022-08-16 19:46 ` Matthew Rosato
0 siblings, 2 replies; 17+ messages in thread
From: Pierre Morel @ 2022-08-16 7:55 UTC (permalink / raw)
To: Randy Dunlap, linux-kernel
Cc: kernel test robot, Matthew Rosato, Christian Borntraeger,
Eric Farman, linux-s390, kvm
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
>
>
>> int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm);
>> void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev);
>> #else
>> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
>> index f9d0c908e738..bbc375b028ef 100644
>> --- a/drivers/vfio/pci/Kconfig
>> +++ b/drivers/vfio/pci/Kconfig
>> @@ -45,9 +45,9 @@ config VFIO_PCI_IGD
>> endif
>>
>> config VFIO_PCI_ZDEV_KVM
>> - bool "VFIO PCI extensions for s390x KVM passthrough"
>> + def_tristate y
>> + prompt "VFIO PCI extensions for s390x KVM passthrough"
>> depends on S390 && KVM
>> - default y
>> help
>> Support s390x-specific extensions to enable support for enhancements
>> to KVM passthrough capabilities, such as interpretive execution of
>>
>> ----
>>
>> What do you think? It seems to me it solves the problem, what do you think?
>>
>> Regards,
>> Pierre
>
>
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] vfio-pci/zdev: require KVM to be built-in
2022-08-16 7:55 ` Pierre Morel
@ 2022-08-16 13:47 ` Pierre Morel
2022-08-16 18:06 ` Randy Dunlap
2022-08-16 19:46 ` Matthew Rosato
1 sibling, 1 reply; 17+ messages in thread
From: Pierre Morel @ 2022-08-16 13:47 UTC (permalink / raw)
To: Randy Dunlap, linux-kernel
Cc: kernel test robot, Matthew Rosato, Christian Borntraeger,
Eric Farman, linux-s390, kvm
Randy,
I need to provide the correction patch rapidly.
Without answer I will propose the patch.
Regards,
Pierre
On 8/16/22 09:55, 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
>
>
>>
>>
>>> int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm);
>>> void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev);
>>> #else
>>> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
>>> index f9d0c908e738..bbc375b028ef 100644
>>> --- a/drivers/vfio/pci/Kconfig
>>> +++ b/drivers/vfio/pci/Kconfig
>>> @@ -45,9 +45,9 @@ config VFIO_PCI_IGD
>>> endif
>>>
>>> config VFIO_PCI_ZDEV_KVM
>>> - bool "VFIO PCI extensions for s390x KVM passthrough"
>>> + def_tristate y
>>> + prompt "VFIO PCI extensions for s390x KVM passthrough"
>>> depends on S390 && KVM
>>> - default y
>>> help
>>> Support s390x-specific extensions to enable support for
>>> enhancements
>>> to KVM passthrough capabilities, such as interpretive
>>> execution of
>>>
>>> ----
>>>
>>> What do you think? It seems to me it solves the problem, what do you
>>> think?
>>>
>>> Regards,
>>> Pierre
>>
>>
>
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] vfio-pci/zdev: require KVM to be built-in
2022-08-16 13:47 ` Pierre Morel
@ 2022-08-16 18:06 ` Randy Dunlap
0 siblings, 0 replies; 17+ messages in thread
From: Randy Dunlap @ 2022-08-16 18:06 UTC (permalink / raw)
To: Pierre Morel, linux-kernel
Cc: kernel test robot, Matthew Rosato, Christian Borntraeger,
Eric Farman, linux-s390, kvm
On 8/16/22 06:47, Pierre Morel wrote:
> Randy,
>
> I need to provide the correction patch rapidly.
> Without answer I will propose the patch.
>
> Regards,
> Pierre
Please go ahead with it.
Thanks.
>
> On 8/16/22 09:55, 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
>>
>>
>>>
>>>
>>>> int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm);
>>>> void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev);
>>>> #else
>>>> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
>>>> index f9d0c908e738..bbc375b028ef 100644
>>>> --- a/drivers/vfio/pci/Kconfig
>>>> +++ b/drivers/vfio/pci/Kconfig
>>>> @@ -45,9 +45,9 @@ config VFIO_PCI_IGD
>>>> endif
>>>>
>>>> config VFIO_PCI_ZDEV_KVM
>>>> - bool "VFIO PCI extensions for s390x KVM passthrough"
>>>> + def_tristate y
>>>> + prompt "VFIO PCI extensions for s390x KVM passthrough"
>>>> depends on S390 && KVM
>>>> - default y
>>>> help
>>>> Support s390x-specific extensions to enable support for enhancements
>>>> to KVM passthrough capabilities, such as interpretive execution of
>>>>
>>>> ----
>>>>
>>>> What do you think? It seems to me it solves the problem, what do you think?
>>>>
>>>> Regards,
>>>> Pierre
>>>
>>>
>>
>
--
~Randy
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] vfio-pci/zdev: require KVM to be built-in
2022-08-16 7:55 ` Pierre Morel
2022-08-16 13:47 ` Pierre Morel
@ 2022-08-16 19:46 ` Matthew Rosato
2022-08-16 20:22 ` Pierre Morel
1 sibling, 1 reply; 17+ messages in thread
From: Matthew Rosato @ 2022-08-16 19:46 UTC (permalink / raw)
To: Pierre Morel, Randy Dunlap, linux-kernel
Cc: kernel test robot, Christian Borntraeger, Eric Farman, linux-s390, kvm
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);
}
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] vfio-pci/zdev: require KVM to be built-in
2022-08-16 19:46 ` Matthew Rosato
@ 2022-08-16 20:22 ` Pierre Morel
2022-08-16 20:28 ` [PATCH] KVM: s390: pci: VFIO_PCI ZDEV configuration fix Pierre Morel
2022-08-18 10:23 ` [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO Pierre Morel
0 siblings, 2 replies; 17+ messages in thread
From: Pierre Morel @ 2022-08-16 20:22 UTC (permalink / raw)
To: Matthew Rosato, Randy Dunlap, linux-kernel
Cc: kernel test robot, Christian Borntraeger, Eric Farman, linux-s390, kvm
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] KVM: s390: pci: VFIO_PCI ZDEV configuration fix
2022-08-16 20:22 ` Pierre Morel
@ 2022-08-16 20:28 ` Pierre Morel
2022-08-16 22:15 ` Matthew Rosato
2022-08-18 10:23 ` [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO Pierre Morel
1 sibling, 1 reply; 17+ messages in thread
From: Pierre Morel @ 2022-08-16 20:28 UTC (permalink / raw)
To: mjrosato; +Cc: rdunlap, linux-kernel, lkp, borntraeger, farman, linux-s390, kvm
Fixing configuration for VFIO PCI interpretation.
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
Fixes: c435c54639aa5 ("vfio/pci: introduce CONFIG_VFIO_PCI_ZDEV_KVM..")
Cc: <stable@vger.kernel.org>
---
arch/s390/include/asm/kvm_host.h | 9 ---------
arch/s390/kvm/Makefile | 2 +-
arch/s390/kvm/pci.c | 4 ++--
drivers/vfio/pci/Kconfig | 4 ++--
include/linux/vfio_pci_core.h | 2 +-
5 files changed, 6 insertions(+), 15 deletions(-)
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index f39092e0ceaa..f6cf961731af 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -1038,16 +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
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
#endif
diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
index 02217fb4ae10..be36afcfd6ff 100644
--- a/arch/s390/kvm/Makefile
+++ b/arch/s390/kvm/Makefile
@@ -9,6 +9,6 @@ ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
kvm-y += kvm-s390.o intercept.o interrupt.o priv.o sigp.o
kvm-y += diag.o gaccess.o guestdbg.o vsie.o pv.o
+kvm-y += pci.o
-kvm-$(CONFIG_VFIO_PCI_ZDEV_KVM) += pci.o
obj-$(CONFIG_KVM) += kvm.o
diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 4946fb7757d6..cf8ab72a2109 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -435,7 +435,7 @@ int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm)
{
int rc;
- if (!zdev)
+ if (!IS_ENABLED(CONFIG_VFIO_PCI_ZDEV_KVM) || !zdev)
return -EINVAL;
mutex_lock(&zdev->kzdev_lock);
@@ -516,7 +516,7 @@ void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev)
{
struct kvm *kvm;
- if (!zdev)
+ if (!IS_ENABLED(CONFIG_VFIO_PCI_ZDEV_KVM) || !zdev)
return;
mutex_lock(&zdev->kzdev_lock);
diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index f9d0c908e738..bbc375b028ef 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -45,9 +45,9 @@ config VFIO_PCI_IGD
endif
config VFIO_PCI_ZDEV_KVM
- bool "VFIO PCI extensions for s390x KVM passthrough"
+ def_tristate y
+ prompt "VFIO PCI extensions for s390x KVM passthrough"
depends on S390 && KVM
- default y
help
Support s390x-specific extensions to enable support for enhancements
to KVM passthrough capabilities, such as interpretive execution of
diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
index 5579ece4347b..7db3bb8129b1 100644
--- a/include/linux/vfio_pci_core.h
+++ b/include/linux/vfio_pci_core.h
@@ -205,7 +205,7 @@ static inline int vfio_pci_igd_init(struct vfio_pci_core_device *vdev)
}
#endif
-#ifdef CONFIG_VFIO_PCI_ZDEV_KVM
+#if IS_ENABLED(CONFIG_VFIO_PCI_ZDEV_KVM)
int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev,
struct vfio_info_cap *caps);
int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev);
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] KVM: s390: pci: VFIO_PCI ZDEV configuration fix
2022-08-16 20:28 ` [PATCH] KVM: s390: pci: VFIO_PCI ZDEV configuration fix Pierre Morel
@ 2022-08-16 22:15 ` Matthew Rosato
2022-08-17 7:10 ` Pierre Morel
0 siblings, 1 reply; 17+ messages in thread
From: Matthew Rosato @ 2022-08-16 22:15 UTC (permalink / raw)
To: Pierre Morel
Cc: rdunlap, linux-kernel, lkp, borntraeger, farman, linux-s390, kvm
On 8/16/22 4:28 PM, Pierre Morel wrote:
> Fixing configuration for VFIO PCI interpretation.
>
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
> Fixes: c435c54639aa5 ("vfio/pci: introduce CONFIG_VFIO_PCI_ZDEV_KVM..")
> Cc: <stable@vger.kernel.org>
> ---
> arch/s390/include/asm/kvm_host.h | 9 ---------
> arch/s390/kvm/Makefile | 2 +-
> arch/s390/kvm/pci.c | 4 ++--
> drivers/vfio/pci/Kconfig | 4 ++--
> include/linux/vfio_pci_core.h | 2 +-
> 5 files changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index f39092e0ceaa..f6cf961731af 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -1038,16 +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
> 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
>
> #endif
> diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
> index 02217fb4ae10..be36afcfd6ff 100644
> --- a/arch/s390/kvm/Makefile
> +++ b/arch/s390/kvm/Makefile
> @@ -9,6 +9,6 @@ ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
>
> kvm-y += kvm-s390.o intercept.o interrupt.o priv.o sigp.o
> kvm-y += diag.o gaccess.o guestdbg.o vsie.o pv.o
> +kvm-y += pci.o
>
> -kvm-$(CONFIG_VFIO_PCI_ZDEV_KVM) += pci.o
You would need to switch this to CONFIG_PCI at least, else we get build errors with CONFIG_PCI=n.
> obj-$(CONFIG_KVM) += kvm.o
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 4946fb7757d6..cf8ab72a2109 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -435,7 +435,7 @@ int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm)
> {
> int rc;
>
> - if (!zdev)
> + if (!IS_ENABLED(CONFIG_VFIO_PCI_ZDEV_KVM) || !zdev)
> return -EINVAL;
>
> mutex_lock(&zdev->kzdev_lock);
> @@ -516,7 +516,7 @@ void kvm_s390_pci_unregister_kvm(struct zpci_dev *zdev)
> {
> struct kvm *kvm;
>
> - if (!zdev)
> + if (!IS_ENABLED(CONFIG_VFIO_PCI_ZDEV_KVM) || !zdev)
> return;
>
> mutex_lock(&zdev->kzdev_lock);
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index f9d0c908e738..bbc375b028ef 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -45,9 +45,9 @@ config VFIO_PCI_IGD
> endif
>
> config VFIO_PCI_ZDEV_KVM
> - bool "VFIO PCI extensions for s390x KVM passthrough"
> + def_tristate y
> + prompt "VFIO PCI extensions for s390x KVM passthrough"
> depends on S390 && KVM
> - default y
> help
> Support s390x-specific extensions to enable support for enhancements
> to KVM passthrough capabilities, such as interpretive execution of
> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index 5579ece4347b..7db3bb8129b1 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -205,7 +205,7 @@ static inline int vfio_pci_igd_init(struct vfio_pci_core_device *vdev)
> }
> #endif
>
> -#ifdef CONFIG_VFIO_PCI_ZDEV_KVM
> +#if IS_ENABLED(CONFIG_VFIO_PCI_ZDEV_KVM)
> int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev,
> struct vfio_info_cap *caps);
> int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev);
This still doesn't seem quite right... I tried some variations:
1)
CONFIG_KVM=m
CONFIG_VFIO_PCI_CORE=m
CONFIG_VFIO_PCI=m
CONFIG_VFIO_PCI_ZDEV_KVM=m
compiles, works with a small change:
diff --git a/include/linux/sched/user.h b/include/linux/sched/user.h
index f054d0360a75..99734e135420 100644
--- a/include/linux/sched/user.h
+++ b/include/linux/sched/user.h
@@ -25,7 +25,7 @@ struct user_struct {
#if defined(CONFIG_PERF_EVENTS) || defined(CONFIG_BPF_SYSCALL) || \
defined(CONFIG_NET) || defined(CONFIG_IO_URING) || \
- defined(CONFIG_VFIO_PCI_ZDEV_KVM)
+ IS_ENABLED(CONFIG_VFIO_PCI_ZDEV_KVM)
2)
CONFIG_KVM=y
CONFIG_VFIO_PCI_CORE=m
CONFIG_VFIO_PCI=m
CONFIG_VFIO_PCI_ZDEV_KVM=m
compiles, works with above user.h change
3)
CONFIG_KVM=y
CONFIG_VFIO_PCI_CORE=y
CONFIG_VFIO_PCI=y
CONFIG_VFIO_PCI_ZDEV_KVM=y
compiles, works with above user.h change
4)
CONFIG_KVM=m
CONFIG_VFIO_PCI_CORE=y
CONFIG_VFIO_PCI=y
CONFIG_VFIO_PCI_ZDEV_KVM=m
fails with:
ld: drivers/vfio/pci/vfio_pci_core.o: in function `vfio_pci_core_enable':
/usr/src/linux/drivers/vfio/pci/vfio_pci_core.c:320: undefined reference to `vfio_pci_zdev_open_device'
ld: /usr/src/linux/drivers/vfio/pci/vfio_pci_core.c:349: undefined reference to `vfio_pci_zdev_close_device'
ld: drivers/vfio/pci/vfio_pci_core.o: in function `vfio_pci_core_disable':
/usr/src/linux/drivers/vfio/pci/vfio_pci_core.c:428: undefined reference to `vfio_pci_zdev_close_device'
ld: drivers/vfio/pci/vfio_pci_core.o: in function `vfio_pci_core_ioctl':
/usr/src/linux/drivers/vfio/pci/vfio_pci_core.c:712: undefined reference to `vfio_pci_info_zdev_add_caps'
5)
CONFIG_KVM=m
CONFIG_VFIO_PCI_CORE=y
CONFIG_VFIO_PCI=y
CONFIG_VFIO_PCI_ZDEV_KVM=y
This forces CONFIG_VFIO_PCI_ZDEV_KVM to 'm' and fails as above.
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] KVM: s390: pci: VFIO_PCI ZDEV configuration fix
2022-08-16 22:15 ` Matthew Rosato
@ 2022-08-17 7:10 ` Pierre Morel
0 siblings, 0 replies; 17+ messages in thread
From: Pierre Morel @ 2022-08-17 7:10 UTC (permalink / raw)
To: Matthew Rosato
Cc: rdunlap, linux-kernel, lkp, borntraeger, farman, linux-s390, kvm
On 8/17/22 00:15, Matthew Rosato wrote:
> On 8/16/22 4:28 PM, Pierre Morel wrote:
>> Fixing configuration for VFIO PCI interpretation.
>>
> 4)
> CONFIG_KVM=m
> CONFIG_VFIO_PCI_CORE=y
> CONFIG_VFIO_PCI=y
> CONFIG_VFIO_PCI_ZDEV_KVM=m
>
> fails with:
>
> ld: drivers/vfio/pci/vfio_pci_core.o: in function `vfio_pci_core_enable':
> /usr/src/linux/drivers/vfio/pci/vfio_pci_core.c:320: undefined reference to `vfio_pci_zdev_open_device'
> ld: /usr/src/linux/drivers/vfio/pci/vfio_pci_core.c:349: undefined reference to `vfio_pci_zdev_close_device'
> ld: drivers/vfio/pci/vfio_pci_core.o: in function `vfio_pci_core_disable':
> /usr/src/linux/drivers/vfio/pci/vfio_pci_core.c:428: undefined reference to `vfio_pci_zdev_close_device'
> ld: drivers/vfio/pci/vfio_pci_core.o: in function `vfio_pci_core_ioctl':
> /usr/src/linux/drivers/vfio/pci/vfio_pci_core.c:712: undefined reference to `vfio_pci_info_zdev_add_caps'
>
>
> 5)
> CONFIG_KVM=m
> CONFIG_VFIO_PCI_CORE=y
> CONFIG_VFIO_PCI=y
> CONFIG_VFIO_PCI_ZDEV_KVM=y
>
> This forces CONFIG_VFIO_PCI_ZDEV_KVM to 'm' and fails as above.
>
>
>
yes right
I was really too stupid yesterday
I think you are right and you should go as you proposed or use some
pointer hook inside S390 to register with KVM.
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO
2022-08-16 20:22 ` Pierre Morel
2022-08-16 20:28 ` [PATCH] KVM: s390: pci: VFIO_PCI ZDEV configuration fix Pierre Morel
@ 2022-08-18 10:23 ` Pierre Morel
2022-08-18 13:33 ` Matthew Rosato
1 sibling, 1 reply; 17+ messages in thread
From: Pierre Morel @ 2022-08-18 10:23 UTC (permalink / raw)
To: mjrosato
Cc: rdunlap, linux-kernel, lkp, borntraeger, farman, linux-s390, kvm,
gor, hca, schnelle
We have a cross dependency between KVM and VFIO.
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 or excluded.
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
Cc: <stable@vger.kernel.org>
---
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..8312ed9d1937 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 kvm_register_hook {
+ int (*kvm_register)(void *opaque, struct kvm *kvm);
+ void (*kvm_unregister)(void *opaque);
+};
+
+extern struct kvm_register_hook kvm_pci_hook;
#endif
diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 4946fb7757d6..e173fce64c4f 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);
+ kvm_pci_hook.kvm_register = kvm_s390_pci_register_kvm;
+ kvm_pci_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..9d8799b72dbf
--- /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 <pmorel@linux.ibm.com>
+ */
+#include <linux/kvm_host.h>
+
+struct kvm_register_hook kvm_pci_hook;
+EXPORT_SYMBOL_GPL(kvm_pci_hook);
diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
index e163aa9f6144..3b7a707e2fe5 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 (kvm_pci_hook.kvm_register)
+ return kvm_pci_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 (kvm_pci_hook.kvm_unregister)
+ return kvm_pci_hook.kvm_unregister(zdev);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO
2022-08-18 10:23 ` [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO Pierre Morel
@ 2022-08-18 13:33 ` Matthew Rosato
2022-08-18 14:06 ` Pierre Morel
2022-08-18 14:20 ` Niklas Schnelle
0 siblings, 2 replies; 17+ messages in thread
From: Matthew Rosato @ 2022-08-18 13:33 UTC (permalink / raw)
To: Pierre Morel
Cc: rdunlap, linux-kernel, lkp, borntraeger, farman, linux-s390, kvm,
gor, hca, schnelle
On 8/18/22 6:23 AM, Pierre Morel wrote:
> We have a cross dependency between KVM and VFIO.
maybe add something like '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 or excluded.
s/or excluded//
There's no problem when KVM is excluded, that forces CONFIG_VFIO_PCI_ZDEV_KVM=n because of the 'depends on S390 && KVM'.
>
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
> Cc: <stable@vger.kernel.org>
> ---
> 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..8312ed9d1937 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 kvm_register_hook {
Nit: zpci_kvm_register_hook ? Just to make it clear it's for zpci.
> + int (*kvm_register)(void *opaque, struct kvm *kvm);
> + void (*kvm_unregister)(void *opaque);
> +};
> +
> +extern struct kvm_register_hook kvm_pci_hook;
Nit: kvm_zpci_hook ?
>
> #endif
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 4946fb7757d6..e173fce64c4f 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);
> + kvm_pci_hook.kvm_register = kvm_s390_pci_register_kvm;
> + kvm_pci_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 guess it doesn't harm anything to add this unconditionally, but I think it would also be OK to just include this in the CONFIG_PCI list - vfio_pci_zdev and arch/s390/kvm/pci all rely on CONFIG_PCI via CONFIG_VFIO_PCI_ZDEV_KVM which implies PCI via VFIO_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..9d8799b72dbf
> --- /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 <pmorel@linux.ibm.com>
> + */
> +#include <linux/kvm_host.h>
> +
> +struct kvm_register_hook kvm_pci_hook;
> +EXPORT_SYMBOL_GPL(kvm_pci_hook);
Following the comments above, zpci_kvm_register_hook, kvm_zpci_hook ?
I'm not sure if this really needs to be in a separate file or if it could just go into arch/s390/pci.c with the zpci_aipb -- If going the route of a separate file, up to Niklas whether he wants this under the S390 PCI maintainership or added to the list for s390 vfio-pci like arch/kvm/pci* and vfio_pci_zdev.
> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
> index e163aa9f6144..3b7a707e2fe5 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 (kvm_pci_hook.kvm_register)
> + return kvm_pci_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 (kvm_pci_hook.kvm_unregister)
> + return kvm_pci_hook.kvm_unregister(zdev);
No need for the return here, this is a void function calling a void function.
Overall, this looks good to me and survives a series of compile and device passthrough tests on my end, just a matter of a few of these minor comments above. Thanks for tackling this Pierre!
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO
2022-08-18 13:33 ` Matthew Rosato
@ 2022-08-18 14:06 ` Pierre Morel
2022-08-18 14:20 ` Niklas Schnelle
1 sibling, 0 replies; 17+ messages in thread
From: Pierre Morel @ 2022-08-18 14:06 UTC (permalink / raw)
To: Matthew Rosato
Cc: rdunlap, linux-kernel, lkp, borntraeger, farman, linux-s390, kvm,
gor, hca, schnelle
On 8/18/22 15:33, Matthew Rosato wrote:
> On 8/18/22 6:23 AM, Pierre Morel wrote:
>> We have a cross dependency between KVM and VFIO.
>
> maybe add something like '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 or excluded.
>
> s/or excluded//
>
> There's no problem when KVM is excluded, that forces CONFIG_VFIO_PCI_ZDEV_KVM=n because of the 'depends on S390 && KVM'.
OK
>
>>
>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
>> Cc: <stable@vger.kernel.org>
>> ---
>> 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..8312ed9d1937 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 kvm_register_hook {
>
> Nit: zpci_kvm_register_hook ? Just to make it clear it's for zpci.
OK
>
>> + int (*kvm_register)(void *opaque, struct kvm *kvm);
>> + void (*kvm_unregister)(void *opaque);
>> +};
>> +
>> +extern struct kvm_register_hook kvm_pci_hook;
>
> Nit: kvm_zpci_hook ?
OK too,
>
>>
>> #endif
>> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
>> index 4946fb7757d6..e173fce64c4f 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);
>> + kvm_pci_hook.kvm_register = kvm_s390_pci_register_kvm;
>> + kvm_pci_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 guess it doesn't harm anything to add this unconditionally, but I think it would also be OK to just include this in the CONFIG_PCI list - vfio_pci_zdev and arch/s390/kvm/pci all rely on CONFIG_PCI via CONFIG_VFIO_PCI_ZDEV_KVM which implies PCI via VFIO_PCI.
Right,CONFIG_PCI is a bool so we can put the hook in arch/s390/pci/pci.c
and use a defined(CONFIG_PCI) to protect the initialization inside KVM.
>
>> diff --git a/arch/s390/pci/pci_kvm_hook.c b/arch/s390/pci/pci_kvm_hook.c
>> new file mode 100644
>> index 000000000000..9d8799b72dbf
>> --- /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 <pmorel@linux.ibm.com>
>> + */
>> +#include <linux/kvm_host.h>
>> +
>> +struct kvm_register_hook kvm_pci_hook;
>> +EXPORT_SYMBOL_GPL(kvm_pci_hook);
>
> Following the comments above, zpci_kvm_register_hook, kvm_zpci_hook ?
OK
>
> I'm not sure if this really needs to be in a separate file or if it could just go into arch/s390/pci.c with the zpci_aipb -- If going the route of a separate file, up to Niklas whether he wants this under the S390 PCI maintainership or added to the list for s390 vfio-pci like arch/kvm/pci* and vfio_pci_zdev.
agreed no need for a separate file, it is much better.
>
>> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
>> index e163aa9f6144..3b7a707e2fe5 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 (kvm_pci_hook.kvm_register)
>> + return kvm_pci_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 (kvm_pci_hook.kvm_unregister)
>> + return kvm_pci_hook.kvm_unregister(zdev);
>
> No need for the return here, this is a void function calling a void function.
right.
>
>
> Overall, this looks good to me and survives a series of compile and device passthrough tests on my end, just a matter of a few of these minor comments above. Thanks for tackling this Pierre!
>
Thanks,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO
2022-08-18 13:33 ` Matthew Rosato
2022-08-18 14:06 ` Pierre Morel
@ 2022-08-18 14:20 ` Niklas Schnelle
2022-08-18 15:13 ` Matthew Rosato
1 sibling, 1 reply; 17+ messages in thread
From: Niklas Schnelle @ 2022-08-18 14:20 UTC (permalink / raw)
To: Matthew Rosato, Pierre Morel
Cc: rdunlap, linux-kernel, lkp, farman, linux-s390, kvm, gor, hca,
Janosch Frank
On Thu, 2022-08-18 at 09:33 -0400, Matthew Rosato wrote:
> On 8/18/22 6:23 AM, Pierre Morel wrote:
> > We have a cross dependency between KVM and VFIO.
>
> maybe add something like '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 or excluded.
>
> s/or excluded//
>
> There's no problem when KVM is excluded, that forces CONFIG_VFIO_PCI_ZDEV_KVM=n because of the 'depends on S390 && KVM'.
>
> > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
> > Cc: <stable@vger.kernel.org>
> > ---
> > 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..8312ed9d1937 100644
> > --- a/arch/s390/include/asm/kvm_host.h
> > +++ b/arch/s390/include/asm/kvm_host.h
I added Janosch as second S390 KVM maintainer in case he wants to chime
in.
> > @@ -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 kvm_register_hook {
>
> Nit: zpci_kvm_register_hook ? Just to make it clear it's for zpci.
Hmm, I guess one could re-use the same struct for another such KVM
dependency but I lean towards the same thinking as Matt, for now this
is for zpci so stay specific we can always generalize later.
Nit: For me hook and register together sound a bit redudant, maybe
"zpci_kvm_register"? Also question for Matt as a native speaker, should
it rather be "registration" when used as a noun here?
>
> > + int (*kvm_register)(void *opaque, struct kvm *kvm);
> > + void (*kvm_unregister)(void *opaque);
I do wonder if this needs to be opague "struct zpci_dev" should be
defined even if CONFIG_PCI is unset.
> > +};
> > +
> > +extern struct kvm_register_hook kvm_pci_hook;
>
> Nit: kvm_zpci_hook ?
Analogous to zpci_kvm_regist(er|ration) I would call the variable
simply zpci_kvm i.e. the type is a registration and the variable is the
instance of it that links zpci and kvm.
>
> >
> > #endif
> > diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> > index 4946fb7757d6..e173fce64c4f 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);
> > + kvm_pci_hook.kvm_register = kvm_s390_pci_register_kvm;
> > + kvm_pci_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 guess it doesn't harm anything to add this unconditionally, but I think it would also be OK to just include this in the CONFIG_PCI list - vfio_pci_zdev and arch/s390/kvm/pci all rely on CONFIG_PCI via CONFIG_VFIO_PCI_ZDEV_KVM which implies PCI via VFIO_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..9d8799b72dbf
> > --- /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 <pmorel@linux.ibm.com>
> > + */
> > +#include <linux/kvm_host.h>
> > +
> > +struct kvm_register_hook kvm_pci_hook;
> > +EXPORT_SYMBOL_GPL(kvm_pci_hook);
>
> Following the comments above, zpci_kvm_register_hook, kvm_zpci_hook ?
>
> I'm not sure if this really needs to be in a separate file or if it could just go into arch/s390/pci.c with the zpci_aipb -- If going the route of a separate file, up to Niklas whether he wants this under the S390 PCI maintainership or added to the list for s390 vfio-pci like arch/kvm/pci* and vfio_pci_zdev.
I'm fine with a separate file, pci.c is long enough as it is. I also
don't have a problem with having it maintained as part of S390 PCI but
logically I think it does fall more under arch/kvm/pci* so one could
argue it should be added in the MAINTAINERS file in that section.
If you change the struct name as I proposed above I would probably go
with "pci_kvm_register.c"
>
> > diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
> > index e163aa9f6144..3b7a707e2fe5 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 (kvm_pci_hook.kvm_register)
> > + return kvm_pci_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 (kvm_pci_hook.kvm_unregister)
> > + return kvm_pci_hook.kvm_unregister(zdev);
>
> No need for the return here, this is a void function calling a void function.
>
>
> Overall, this looks good to me and survives a series of compile and device passthrough tests on my end, just a matter of a few of these minor comments above. Thanks for tackling this Pierre!
Yes I agree, overall this looks good to me though I'm admittedly not
very knowledgable about how to best handle module dependencies like
this. It does look cleaner than the symbol_get() alternative we
discussed.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO
2022-08-18 14:20 ` Niklas Schnelle
@ 2022-08-18 15:13 ` Matthew Rosato
2022-08-18 15:22 ` Niklas Schnelle
0 siblings, 1 reply; 17+ messages in thread
From: Matthew Rosato @ 2022-08-18 15:13 UTC (permalink / raw)
To: Niklas Schnelle, Pierre Morel
Cc: rdunlap, linux-kernel, lkp, farman, linux-s390, kvm, gor, hca,
Janosch Frank
On 8/18/22 10:20 AM, Niklas Schnelle wrote:
> On Thu, 2022-08-18 at 09:33 -0400, Matthew Rosato wrote:
>> On 8/18/22 6:23 AM, Pierre Morel wrote:
>>> We have a cross dependency between KVM and VFIO.
>>
>> maybe add something like '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 or excluded.
>>
>> s/or excluded//
>>
>> There's no problem when KVM is excluded, that forces CONFIG_VFIO_PCI_ZDEV_KVM=n because of the 'depends on S390 && KVM'.
>>
>>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
>>> Cc: <stable@vger.kernel.org>
>>> ---
>>> 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..8312ed9d1937 100644
>>> --- a/arch/s390/include/asm/kvm_host.h
>>> +++ b/arch/s390/include/asm/kvm_host.h
>
> I added Janosch as second S390 KVM maintainer in case he wants to chime
> in.
>
>>> @@ -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 kvm_register_hook {
>>
>> Nit: zpci_kvm_register_hook ? Just to make it clear it's for zpci.
>
> Hmm, I guess one could re-use the same struct for another such KVM
> dependency but I lean towards the same thinking as Matt, for now this
> is for zpci so stay specific we can always generalize later.
Yes, let's keep this zpci-specific.
>
> Nit: For me hook and register together sound a bit redudant, maybe
> "zpci_kvm_register"? Also question for Matt as a native speaker, should
> it rather be "registration" when used as a noun here?
>
Maybe just drop the 'register'. If there is a need for a 3rd function later, for example, it might not be related to registration.
e.g. struct kvm_zpci_hook {
...
};
extern struct kvm_zpci_hook zpci_kvm;
>
>>
>>> + int (*kvm_register)(void *opaque, struct kvm *kvm);
>>> + void (*kvm_unregister)(void *opaque);
>
> I do wonder if this needs to be opague "struct zpci_dev" should be
> defined even if CONFIG_PCI is unset.
>
>
>>> +};
>>> +
>>> +extern struct kvm_register_hook kvm_pci_hook;
>>
>> Nit: kvm_zpci_hook ?
>
> Analogous to zpci_kvm_regist(er|ration) I would call the variable
> simply zpci_kvm i.e. the type is a registration and the variable is the
> instance of it that links zpci and kvm.
>
Yeah, see above.
>>
>>>
>>> #endif
>>> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
>>> index 4946fb7757d6..e173fce64c4f 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);
>>> + kvm_pci_hook.kvm_register = kvm_s390_pci_register_kvm;
>>> + kvm_pci_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 guess it doesn't harm anything to add this unconditionally, but I think it would also be OK to just include this in the CONFIG_PCI list - vfio_pci_zdev and arch/s390/kvm/pci all rely on CONFIG_PCI via CONFIG_VFIO_PCI_ZDEV_KVM which implies PCI via VFIO_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..9d8799b72dbf
>>> --- /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 <pmorel@linux.ibm.com>
>>> + */
>>> +#include <linux/kvm_host.h>
>>> +
>>> +struct kvm_register_hook kvm_pci_hook;
>>> +EXPORT_SYMBOL_GPL(kvm_pci_hook);
>>
>> Following the comments above, zpci_kvm_register_hook, kvm_zpci_hook ?
>>
>> I'm not sure if this really needs to be in a separate file or if it could just go into arch/s390/pci.c with the zpci_aipb -- If going the route of a separate file, up to Niklas whether he wants this under the S390 PCI maintainership or added to the list for s390 vfio-pci like arch/kvm/pci* and vfio_pci_zdev.
>
> I'm fine with a separate file, pci.c is long enough as it is. I also
> don't have a problem with having it maintained as part of S390 PCI but
> logically I think it does fall more under arch/kvm/pci* so one could
> argue it should be added in the MAINTAINERS file in that section.
> If you change the struct name as I proposed above I would probably go
> with "pci_kvm_register.c"
OK, no problem with me for a separate file then, or maintaining said file. But I guess not pci_kvm_register.c per my comments above
>
>>
>>> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
>>> index e163aa9f6144..3b7a707e2fe5 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 (kvm_pci_hook.kvm_register)
>>> + return kvm_pci_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 (kvm_pci_hook.kvm_unregister)
>>> + return kvm_pci_hook.kvm_unregister(zdev);
>>
>> No need for the return here, this is a void function calling a void function.
>>
>>
>> Overall, this looks good to me and survives a series of compile and device passthrough tests on my end, just a matter of a few of these minor comments above. Thanks for tackling this Pierre!
>
> Yes I agree, overall this looks good to me though I'm admittedly not
> very knowledgable about how to best handle module dependencies like
> this. It does look cleaner than the symbol_get() alternative we
> discussed.
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO
2022-08-18 15:13 ` Matthew Rosato
@ 2022-08-18 15:22 ` Niklas Schnelle
0 siblings, 0 replies; 17+ messages in thread
From: Niklas Schnelle @ 2022-08-18 15:22 UTC (permalink / raw)
To: Matthew Rosato, Pierre Morel
Cc: rdunlap, linux-kernel, lkp, farman, linux-s390, kvm, gor, hca,
Janosch Frank
On Thu, 2022-08-18 at 11:13 -0400, Matthew Rosato wrote:
> On 8/18/22 10:20 AM, Niklas Schnelle wrote:
> > On Thu, 2022-08-18 at 09:33 -0400, Matthew Rosato wrote:
> > > On 8/18/22 6:23 AM, Pierre Morel wrote:
> > > > We have a cross dependency between KVM and VFIO.
> > >
> > > maybe add something like '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 or excluded.
> > >
> > > s/or excluded//
> > >
> > > There's no problem when KVM is excluded, that forces CONFIG_VFIO_PCI_ZDEV_KVM=n because of the 'depends on S390 && KVM'.
> > >
> > > > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > > Fixes: 09340b2fca007 ("KVM: s390: pci: add routines to start/stop inter..")
> > > > Cc: <stable@vger.kernel.org>
> > > > ---
> > > > 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..8312ed9d1937 100644
> > > > --- a/arch/s390/include/asm/kvm_host.h
> > > > +++ b/arch/s390/include/asm/kvm_host.h
> >
> > I added Janosch as second S390 KVM maintainer in case he wants to chime
> > in.
> >
> > > > @@ -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 kvm_register_hook {
> > >
> > > Nit: zpci_kvm_register_hook ? Just to make it clear it's for zpci.
> >
> > Hmm, I guess one could re-use the same struct for another such KVM
> > dependency but I lean towards the same thinking as Matt, for now this
> > is for zpci so stay specific we can always generalize later.
>
> Yes, let's keep this zpci-specific.
>
> > Nit: For me hook and register together sound a bit redudant, maybe
> > "zpci_kvm_register"? Also question for Matt as a native speaker, should
> > it rather be "registration" when used as a noun here?
> >
>
> Maybe just drop the 'register'. If there is a need for a 3rd function later, for example, it might not be related to registration.
Yes, that sounds good and makes sense so "zpci_kvm_hook".
>
> e.g. struct kvm_zpci_hook {
> ...
> };
>
> extern struct kvm_zpci_hook zpci_kvm;
>
---8<---
> > >
> > > > diff --git a/arch/s390/pci/pci_kvm_hook.c b/arch/s390/pci/pci_kvm_hook.c
> > > > new file mode 100644
> > > > index 000000000000..9d8799b72dbf
> > > > --- /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 <pmorel@linux.ibm.com>
> > > > + */
> > > > +#include <linux/kvm_host.h>
> > > > +
> > > > +struct kvm_register_hook kvm_pci_hook;
> > > > +EXPORT_SYMBOL_GPL(kvm_pci_hook);
> > >
> > > Following the comments above, zpci_kvm_register_hook, kvm_zpci_hook ?
> > >
> > > I'm not sure if this really needs to be in a separate file or if it could just go into arch/s390/pci.c with the zpci_aipb -- If going the route of a separate file, up to Niklas whether he wants this under the S390 PCI maintainership or added to the list for s390 vfio-pci like arch/kvm/pci* and vfio_pci_zdev.
> >
> > I'm fine with a separate file, pci.c is long enough as it is. I also
> > don't have a problem with having it maintained as part of S390 PCI but
> > logically I think it does fall more under arch/kvm/pci* so one could
> > argue it should be added in the MAINTAINERS file in that section.
> > If you change the struct name as I proposed above I would probably go
> > with "pci_kvm_register.c"
>
> OK, no problem with me for a separate file then, or maintaining said file. But I guess not pci_kvm_register.c per my comments above
Yes, let's go with pci_kvm_hook.c then
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-08-18 15:23 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-14 21:51 [PATCH] vfio-pci/zdev: require KVM to be built-in Randy Dunlap
2022-08-15 9:43 ` Pierre Morel
2022-08-16 6:04 ` Randy Dunlap
2022-08-16 7:55 ` Pierre Morel
2022-08-16 13:47 ` Pierre Morel
2022-08-16 18:06 ` Randy Dunlap
2022-08-16 19:46 ` Matthew Rosato
2022-08-16 20:22 ` Pierre Morel
2022-08-16 20:28 ` [PATCH] KVM: s390: pci: VFIO_PCI ZDEV configuration fix Pierre Morel
2022-08-16 22:15 ` Matthew Rosato
2022-08-17 7:10 ` Pierre Morel
2022-08-18 10:23 ` [PATCH] KVM: s390: pci: Hook to access KVM lowlevel from VFIO Pierre Morel
2022-08-18 13:33 ` Matthew Rosato
2022-08-18 14:06 ` Pierre Morel
2022-08-18 14:20 ` Niklas Schnelle
2022-08-18 15:13 ` Matthew Rosato
2022-08-18 15:22 ` Niklas Schnelle
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).