Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <94c8947f-4186-e399-a79f-6f94e91ed8b9@linux.ibm.com>
Date:   Tue, 31 May 2022 06:32:12 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v19 10/20] s390/vfio-ap: prepare for dynamic update of
 guest's APCB on assign/unassign
Content-Language: en-US
To:     jjherne@linux.ibm.com, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc:     freude@linux.ibm.com, borntraeger@de.ibm.com, cohuck@redhat.com,
        mjrosato@linux.ibm.com, pasic@linux.ibm.com,
        alex.williamson@redhat.com, kwankhede@nvidia.com,
        fiuczy@linux.ibm.com
References: <20220404221039.1272245-1-akrowiak@linux.ibm.com>
 <20220404221039.1272245-11-akrowiak@linux.ibm.com>
 <67f17a73-28e2-d458-a052-2782e16fe96d@linux.ibm.com>
From:   Tony Krowiak <akrowiak@linux.ibm.com>
In-Reply-To: <67f17a73-28e2-d458-a052-2782e16fe96d@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 5/27/22 9:18 AM, Jason J. Herne wrote:
> On 4/4/22 18:10, Tony Krowiak wrote:
>> The functions backing the matrix mdev's sysfs attribute interfaces to
>> assign/unassign adapters, domains and control domains must take and
>> release the locks required to perform a dynamic update of a guest's APCB
>> in the proper order.
>>
>> The proper order for taking the locks is:
>>
>> matrix_dev->guests_lock => kvm->lock => matrix_dev->mdevs_lock
>>
>> The proper order for releasing the locks is:
>>
>> matrix_dev->mdevs_lock => kvm->lock => matrix_dev->guests_lock
>>
>> Two new macros are introduced for this purpose: One to take the locks 
>> and
>> the other to release the locks. These macros will be used by the
>> assignment/unassignment functions to prepare for dynamic update of
>> the KVM guest's APCB.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 69 +++++++++++++++++++++++++------
>>   1 file changed, 57 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 757bbf449b04..2219b1069ceb 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -71,6 +71,51 @@ static const struct vfio_device_ops 
>> vfio_ap_matrix_dev_ops;
>>       mutex_unlock(&matrix_dev->guests_lock);    \
>>   })
>>   +/**
>> + * get_update_locks_for_mdev: Acquire the locks required to 
>> dynamically update a
>> + *                  KVM guest's APCB in the proper order.
>> + *
>> + * @matrix_mdev: a pointer to a struct ap_matrix_mdev object 
>> containing the AP
>> + *         configuration data to use to update a KVM guest's APCB.
>> + *
>> + * The proper locking order is:
>> + * 1. matrix_dev->guests_lock: required to use the KVM pointer to 
>> update a KVM
>> + *                   guest's APCB.
>> + * 2. matrix_mdev->kvm->lock:  required to update a guest's APCB
>> + * 3. matrix_dev->mdevs_lock:  required to access data stored in a 
>> matrix_mdev
>> + *
>> + * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, 
>> the KVM
>> + *     lock will not be taken.
>> + */
>
> Perhaps the locking order should be documented once at the top of all 
> of the locking
> functions instead of in each comment. The current method seems 
> needlessly verbose.

Perhaps, but I surmise this comment was motivated by the fact you are 
reviewing the
locking macros/functions en masse. On the other hand, someone debugging 
the code
may miss the locking order comments if their debug thread leads them to 
a locking
macro/function that does not have said comments. I think the value of 
leaving the
comments in place outweighs the value of limiting them as you suggested.

>
>> +#define get_update_locks_for_mdev(matrix_mdev) ({    \
>> +    mutex_lock(&matrix_dev->guests_lock);        \
>> +    if (matrix_mdev && matrix_mdev->kvm)        \
>> +        mutex_lock(&matrix_mdev->kvm->lock);    \
>> +    mutex_lock(&matrix_dev->mdevs_lock);        \
>> +})
>
> It does not make sense to reference matrix_dev on the first line of 
> this macro and
> then check it for a null value on the next line. If it can be null 
> then the check
> needs to come before the usage. If it cannot be null, then we can 
> remove the check.
> Same comment for the release macro.

You must have misread the code. The second line checks the value of 
matrix_mdev
for NULL, not matrix_dev. There are definitely cases where matrix_mdev 
can be
passed as NULL.

>
>> +/**
>> + * release_update_locks_for_mdev: Release the locks used to 
>> dynamically update a
>> + *                  KVM guest's APCB in the proper order.
>> + *
>> + * @matrix_mdev: a pointer to a struct ap_matrix_mdev object 
>> containing the AP
>> + *         configuration data to use to update a KVM guest's APCB.
>> + *
>> + * The proper unlocking order is:
>> + * 1. matrix_dev->mdevs_lock
>> + * 2. matrix_mdev->kvm->lock
>> + * 3. matrix_dev->guests_lock
>> + *
>> + * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, 
>> the KVM
>> + *     lock will not be released.
>> + */
>> +#define release_update_locks_for_mdev(matrix_mdev) ({    \
>> +    mutex_unlock(&matrix_dev->mdevs_lock);        \
>> +    if (matrix_mdev && matrix_mdev->kvm)        \
>> +        mutex_unlock(&matrix_mdev->kvm->lock); \
>> +    mutex_unlock(&matrix_dev->guests_lock);        \
>> +})
>> +