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: <2c2d7563-c614-e4f8-7826-73deba6d489b@linux.ibm.com>
Date:   Fri, 27 May 2022 08:40:35 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Reply-To: jjherne@linux.ibm.com
Subject: Re: [PATCH v19 08/20] s390/vfio-ap: introduce new mutex to control
 access to the KVM pointer
Content-Language: en-US
To:     Tony Krowiak <akrowiak@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-9-akrowiak@linux.ibm.com>
From:   "Jason J. Herne" <jjherne@linux.ibm.com>
Organization: IBM
In-Reply-To: <20220404221039.1272245-9-akrowiak@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/4/22 18:10, Tony Krowiak wrote:
> The vfio_ap device driver registers for notification when the pointer to
> the KVM object for a guest is set. Recall that the KVM lock (kvm->lock)
> mutex must be taken outside of the matrix_dev->lock mutex to prevent the
> reporting by lockdep of a circular locking dependency (a.k.a., a lockdep
> splat):
> 
> * see commit 0cc00c8d4050 ("Fix circular lockdep when setting/clearing
>    crypto masks")
> 
> * see commit 86956e70761b ("replace open coded locks for
>    VFIO_GROUP_NOTIFY_SET_KVM notification")
> 
> With the introduction of support for hot plugging/unplugging AP devices
> passed through to a KVM guest, a new guests_lock mutex is introduced to
> ensure the proper locking order is maintained:
> 
> struct ap_matrix_dev {
>          ...
>          struct mutex guests_lock;
>         ...
> }
> 
> The matrix_dev->guests_lock controls access to the matrix_mdev instances
> that hold the state for AP devices that have been passed through to a
> KVM guest. This lock must be held to control access to the KVM pointer
> (matrix_mdev->kvm) while the vfio_ap device driver is using it to
> plug/unplug AP devices passed through to the KVM guest.
> 
> Keep in mind, the proper locking order must be maintained whenever
> dynamically updating a KVM guest's APCB to plug/unplug adapters, domains
> and control domains:
> 
>      1. matrix_dev->guests_lock: required to use the KVM pointer - stored in
>         a struct ap_matrix_mdev instance - 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
>         struct ap_matrix_mdev instance.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_drv.c     | 1 +
>   drivers/s390/crypto/vfio_ap_private.h | 6 ++++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 0a5acd151a9b..c258e5f7fdfc 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -161,6 +161,7 @@ static int vfio_ap_matrix_dev_create(void)
>   
>   	mutex_init(&matrix_dev->mdevs_lock);
>   	INIT_LIST_HEAD(&matrix_dev->mdev_list);
> +	mutex_init(&matrix_dev->guests_lock);
>   
>   	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
>   	matrix_dev->device.parent = root_device;
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 5262e02192a4..ec926f2f2930 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -39,6 +39,11 @@
>    *		single ap_matrix_mdev device. It's quite coarse but we don't
>    *		expect much contention.
>    * @vfio_ap_drv: the vfio_ap device driver
> + * @guests_lock: mutex for controlling access to a guest that is using AP
> + *		 devices passed through by the vfio_ap device driver. This lock
> + *		 will be taken when the AP devices are plugged into or unplugged
> + *		 from a guest, and when an ap_matrix_mdev device is added to or
> + *		 removed from @mdev_list or the list is iterated.
>    */
>   struct ap_matrix_dev {
>   	struct device device;
> @@ -47,6 +52,7 @@ struct ap_matrix_dev {
>   	struct list_head mdev_list;
>   	struct mutex mdevs_lock;
>   	struct ap_driver  *vfio_ap_drv;
> +	struct mutex guests_lock;
>   };
>   
>   extern struct ap_matrix_dev *matrix_dev;

In isolation... Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>


-- 
-- Jason J. Herne (jjherne@linux.ibm.com)