Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <da1b73df-f4c0-0013-0a34-229fd6057285@linux.ibm.com>
Date:   Thu, 27 Jan 2022 09:41:49 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [RFC PATCH] s390: vfio-ap: Register the vfio_ap module for the
 "ap" parent bus
Content-Language: en-US
To:     Cornelia Huck <cohuck@redhat.com>, Thomas Huth <thuth@redhat.com>,
        Harald Freudenberger <freude@linux.ibm.com>,
        linux-s390@vger.kernel.org, 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>,
        Christian Borntraeger <borntraeger@de.ibm.com>
References: <20211201141110.94636-1-thuth@redhat.com>
 <210e3c57-bdc2-09ed-3e41-8dab57e8f36c@linux.ibm.com>
 <319a5ce7-873b-b8b9-1fa8-f2a69b418ac4@redhat.com>
 <15a39981-37ca-ed2d-3baf-c4cb74f8c343@linux.ibm.com>
 <0f1c6b3e-7e33-6fd1-11e7-f4857ec55105@redhat.com> <87a6hbtbqn.fsf@redhat.com>
From:   Tony Krowiak <akrowiak@linux.ibm.com>
In-Reply-To: <87a6hbtbqn.fsf@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 12/8/21 09:25, Cornelia Huck wrote:
> On Wed, Dec 08 2021, Thomas Huth <thuth@redhat.com> wrote:
>
>> On 02/12/2021 09.33, Harald Freudenberger wrote:
>>> On 02.12.21 08:13, Thomas Huth wrote:
>>>> On 01/12/2021 18.10, 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:
>>>>> Hello Thomas, interesting.
>>>>> Did you test this ? I mean did you build a kernel and have it run on a s390 with crypto cards available ?
>>>> Yes, I've tested it. Without the patch, the vfio_ap module does not get loaded automatically if a crypto card is available. With the patch applied, the vfio_ap module correctly gets loaded automatically on my system (similar to the vfio_ccw module).
>>>>
>>>>> My strong feeling is that this will make the AP bus code stumble as the code silently assumes there are exact
>>>>> two types of ap devices attached to the ap bus: ap cards and ap queues.
>>>> This is only about getting the module loaded automatically once such a device is available ... AFAIK it does not grab any of the devices automatically, so there shouldn't be any problems?
>>>>
>>>>    Thomas
>>>>
>>> Yes, of course for the automatic module load works this way. But you understand that now
>>> the vfio devices are childs of the ap bus and thus are siblings of the ap queue and ap card
>>> devices. As I wrote the ap bus code is not prepared to deal with a 3th type of devices
>>> dangling on the ap bus. So you should test what happens when there are real vfio ap devices
>>> in use together with 'regular' ap card and queue devices.
> Um, the queues/cards are devices on the bus, and just can have
> different drivers bound to them, right? The only device that the vfio-ap
> driver creates is the matrix device (which does not live on the ap bus),
> and this patch doesn't change that. It only correctly creates a table
> for a driver that already matched on the ap bus.
>
>> I pondered about this for a while, but I still do not quite understand. The
>> MODULE_DEVICE_TABLE macro only adds a __mod_something_device_table symbol to
>> the module, it does not change the hierarchy of the vfio devices ... so this
>> is really only about loading the module automatically. Or do you say that
>> there is already a problem if a user loads the module manually and thus it
>> should not get loaded automatically?
> Correct me if I'm wrong, but don't the devices on the ap bus need to be
> actually configured before they can attach to a non-default
> (i.e. vfio-ap) driver? IOW, it's not a simple bind operation, but extra
> configuration is required, so a loaded vfio-ap module should not affect
> any devices not configured to actually use it at all.

There are two bitmasks - apmask/aqmask - that identify the APQNs of the
queues to be bound to the default drivers. All others are bound to
the non-default driver (vfio_ap). So, you are correct, loading the vfio_ap
module will not affect any queue devices not configured to use it.

>
>>> However, I am still not sure if it is preferable to have the vfio ap module loaded automatically. The majority
>>> of customers will never use vfio ap devices - this is specific to kvm hosts only.
>> vfio-ccw also gets loaded automatically via MODULE_DEVICE_TABLE, so I think
>> vfio-ap should be handled the same way.
>> (Or should we maybe rather remove the MODULE_DEVICE_TABLE line from both
>> modules instead?)
> MODULE_DEVICE_TABLE declares "I can drive these devices", so it doesn't
> feel correct to remove them. If the modules should not be autoloaded,
> the system must be configured to not autoload them.
>
> Besides, is loading an extra module really causing that much harm? Does
> vfio-ap drag in too much other stuff?
>