Subject: Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the
 "ap" parent bus
From:   Harald Freudenberger <freude@linux.ibm.com>
To:     Thomas Huth <thuth@redhat.com>, linux-s390@vger.kernel.org,
        Tony Krowiak <akrowiak@linux.ibm.com>,
        Halil Pasic <pasic@linux.ibm.com>,
        Jason Herne <jjherne@linux.ibm.com>
Cc:     linux-kernel@vger.kernel.org, Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>
References: <20211201141110.94636-1-thuth@redhat.com>
 <8512bb0a-a34a-09b0-65f3-781f3d092364@linux.ibm.com>
Message-ID: <80cf2be3-9b7e-896d-afc6-376b8f7f9414@linux.ibm.com>
Date:   Thu, 16 Dec 2021 10:50:13 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <8512bb0a-a34a-09b0-65f3-781f3d092364@linux.ibm.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

On 13.12.21 16:44, Harald Freudenberger wrote:
> On 01.12.21 15:11, Thomas Huth wrote:
>> The crypto devices that we can use with the vfio_ap module are sitting
>> on the "ap" bus, not on the "vfio_ap" bus that the module defines
>> itself. With this change, the vfio_ap module now gets automatically
>> loaded if a supported crypto adapter is available in the host.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  Note: Marked as "RFC" since I'm not 100% sure about it ...
>>        please review carefully!
>>
>>  drivers/s390/crypto/vfio_ap_drv.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>> index 4d2556bc7fe5..5580e40608a4 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -39,7 +39,7 @@ static struct ap_device_id ap_queue_ids[] = {
>>  	{ /* end of sibling */ },
>>  };
>>  
>> -MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>> +MODULE_DEVICE_TABLE(ap, ap_queue_ids);
>>  
>>  /**
>>   * vfio_ap_queue_dev_probe:
> I had a chance to check this now.
> First I have to apologize about the dispute with vfio devices appearing on the ap bus.
> That's not the case with this patch. As Connie states the MODULE_DEVICE_TABLE() does not
> change the parent of a device and vfio_ap_drv is a driver for ap devices and thus
> belongs to the ap bus anyway.
> So what's left is that with this change the vfio_ap kernel module is automatically loaded
> when an ap device type 10-13 is recognized by the ap bus. So the intention of the patch
> is fulfilled.
> Yet another kernel module which may occupy memory but will never get used by most customers.
> This may not be a problem but I had a glance at the list of kernel modules loaded on my
> LPAR with and without the patch and the difference is:
> ...
> kvm                   512000  1 vfio_ap
> vfio_ap                28672  0
> ...
> So the vfio_ap module has a dependency to the biggest kernel module ever - kvm.
> Do I need to say something more?
>
> If this dependency is removed then I would not hesitate to accept this patch. However
> this is up to Tony as he is the maintainer of the vfio ap device driver.
>
>
I need to throw in another point: with building initrd with dracut the kernel module
dependencies are evaluated. As of now this means that the zcrypt device driver
zoo is required in case you need to have crypto support for an encrypted root or
data disk at boot time. With vfio ap driver dependency enforced as requirement
from the AP bus there also comes the dependency to the kvm kernel module.
So the kvm kernel module needs to be part of the initrd now. I am not sure if this
is desired.