Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1f2dd65e-b79b-44df-cc6a-8b3aa8fd61af@linux.ibm.com>
Date:   Tue, 16 Aug 2022 22:22:02 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH] vfio-pci/zdev: require KVM to be built-in
Content-Language: en-US
To:     Matthew Rosato <mjrosato@linux.ibm.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        linux-kernel@vger.kernel.org
Cc:     kernel test robot <lkp@intel.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Eric Farman <farman@linux.ibm.com>, linux-s390@vger.kernel.org,
        kvm@vger.kernel.org
References: <20220814215154.32112-1-rdunlap@infradead.org>
 <663c7595-1c18-043e-5f12-b0ce880b84bf@linux.ibm.com>
 <5530ed1f-90ec-ce84-2348-80e484fa48cb@infradead.org>
 <47cfc72d-62f6-2bd3-db91-99f91591fc30@linux.ibm.com>
 <8809c67b-a9f6-07a6-307c-369cd391e9b5@linux.ibm.com>
From:   Pierre Morel <pmorel@linux.ibm.com>
In-Reply-To: <8809c67b-a9f6-07a6-307c-369cd391e9b5@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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