From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Alex Williamson <alex.williamson@redhat.com>
CC:     Matthew Rosato <mjrosato@linux.ibm.com>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "Liu, Yi L" <yi.l.liu@intel.com>,
        "jgg@nvidia.com" <jgg@nvidia.com>,
        "cohuck@redhat.com" <cohuck@redhat.com>,
        "farman@linux.ibm.com" <farman@linux.ibm.com>,
        "pmorel@linux.ibm.com" <pmorel@linux.ibm.com>,
        "borntraeger@linux.ibm.com" <borntraeger@linux.ibm.com>,
        "frankja@linux.ibm.com" <frankja@linux.ibm.com>,
        "imbrenda@linux.ibm.com" <imbrenda@linux.ibm.com>,
        "david@redhat.com" <david@redhat.com>,
        "akrowiak@linux.ibm.com" <akrowiak@linux.ibm.com>,
        "jjherne@linux.ibm.com" <jjherne@linux.ibm.com>,
        "pasic@linux.ibm.com" <pasic@linux.ibm.com>,
        "zhenyuw@linux.intel.com" <zhenyuw@linux.intel.com>,
        "Wang, Zhi A" <zhi.a.wang@intel.com>,
        "Christopherson,, Sean" <seanjc@google.com>,
        "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "intel-gvt-dev@lists.freedesktop.org" 
        <intel-gvt-dev@lists.freedesktop.org>,
        "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v3] vfio: fix deadlock between group lock and kvm lock
Thread-Topic: [PATCH v3] vfio: fix deadlock between group lock and kvm lock
Thread-Index: AQHZNyMj/DyFlYgOy0urx1B0EjoKIa68DpoAgAA1mqCAAAVXgIAALINA
Date:   Fri, 3 Feb 2023 02:00:21 +0000
Message-ID: <BN9PR11MB5276017F9CEBB4BAE58C40E88CD79@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20230202162442.78216-1-mjrosato@linux.ibm.com>
        <20230202124210.476adaf8.alex.williamson@redhat.com>
        <BN9PR11MB527618E281BEB8E479ABB0418CD69@BN9PR11MB5276.namprd11.prod.outlook.com>
 <20230202161307.0c6aa23e.alex.williamson@redhat.com>
In-Reply-To: <20230202161307.0c6aa23e.alex.williamson@redhat.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?B4VWbod3lorMFyHUfjgeU/7m7kzZArNOE0LuLx8KMekDxw6qTo9g6tT/hRVQ?=
 =?us-ascii?Q?ScyOAJcCqwdbT2gGL/TcoVHRLZOtcYiCipTai/XSD0lBZY3jyHrtmUGItfXF?=
 =?us-ascii?Q?Equ/YftueCiuwoOlFhQcxiWWyRtmsPWLIkMhArwVZmqOXtP62q19yPcdRfSl?=
 =?us-ascii?Q?gDJUO2GJgHxwfyXo3dUmvPY2UvH7Z4PELODBGplgsFSSR3qwb7xBXL1c7B3F?=
 =?us-ascii?Q?SvcxwNpzRa+oGqE6HvkXy0hEFc/2VJviRpmsFHpOj+E/B9nxrWQcv+6HfoTx?=
 =?us-ascii?Q?QbN8AsQK+yDhfkzphptqatiIvlihr8AXvn6+M4i2AOlBTAyJ1g00FuVyzPWJ?=
 =?us-ascii?Q?GR3oq6r5BdI5annGwhbt1tDiFxe5+qlUbw5r/rlYdPTvuGutUe1wnvYCVQnx?=
 =?us-ascii?Q?r8jbvU3ta0HkAbWPPoEETFybfHKIsR9CvHMc7Of1GlLKnFtV+wt2qOU+6wWZ?=
 =?us-ascii?Q?Dei2mebXJZJmHAnu6mLACxVqSrBLc0uZ5nm05PF1wHfBg1l0f3IhEQCHQEOI?=
 =?us-ascii?Q?Gl3Ns+Os0hAS0j0bIaTF13WlOQnACgKZIz9D05FqczwytLC39flG5M5ubmPS?=
 =?us-ascii?Q?al943ez8iu4DOV0SZHZLANolDXVHBgEhbiSY0hWUmnYDEebJN0hyJbiJFFaR?=
 =?us-ascii?Q?lo2hstn5x6P0IJJoxuk6TuwXHvfwoKP22M/SyqMpBttUkLpKDAO9GAGaeP32?=
 =?us-ascii?Q?I9ALW+EXcx6WhshBfsiAVXbb0KS71xMH4c7A25bO47MI0o4Ke/OoTs67VKmZ?=
 =?us-ascii?Q?okIqEEakKNRlRJBXqt73TbqHp9kZzwgGhP0MpIC5eBfFz2gWlknwVsU30T7p?=
 =?us-ascii?Q?zWzLEZAuvfxTo/PgwunFh1i72j2icXkRsbl8rA37NfJP2e4ZlZFfdYrUtEUR?=
 =?us-ascii?Q?eyCM4QH9X9Hhcbq1kqwNel6iOtTApvdhsCw15hHkgLXLrKZxQnW4UwNpTqSY?=
 =?us-ascii?Q?qw073dROx2DWKI0/KN1itIeRs2XNiva01N7kdg17gLrnLUpxKK+L0TolPuvr?=
 =?us-ascii?Q?kTdkXd2kuP5DDY5kox7lmHeJK90qUeP+7hw6Z2Hz8ZQ9vzPaA/mqIq+7XAtG?=
 =?us-ascii?Q?vxfy7jOyNR7cgHi3Mn2/kmQTZek6VW+qYEiwC9ALL/PEO6ov9rihQffY+etq?=
 =?us-ascii?Q?ke+5kAMpHO77ysZ8FpAbcKmxhRpanw9iaSRdVwoXlm1iA/Tx812/CBBt53/V?=
 =?us-ascii?Q?3YjkCYOcGjb6PlNLFUSj45JnnokecOYjD5DlpRMILFezpopyhqzWbd7PD2PR?=
 =?us-ascii?Q?+1XGnmn8X2jOPug7lZlQZhnsU4ahAj/EePJsmFXVuR/EUVQS7juv0o8aJbRE?=
 =?us-ascii?Q?bp08TrdCeZ5eQl80cWH70tRWbC0eVujPcupuMM4GssY1k/Q7EPvMSHwCHMRo?=
 =?us-ascii?Q?WAbIdot3HjW05uV1EcksIkegTjCxbTpdh7U8VZl25H1dZ8tONHh6zJLdV28n?=
 =?us-ascii?Q?WEv5FBpeNBXmOkMkWAlceBTWtir26QDXba5FYmItTDsfUIuO/dXdla50v2I8?=
 =?us-ascii?Q?G5YmPtg1Lw2NypWWUG8kzgvGiBFmhkVQOOcw8ut4kEMNjXmIIEwtb9Nioyfc?=
 =?us-ascii?Q?RyAgU7jodwRg6aoi5Aqlxe2XXw2Ugma0qP2nzI3Q?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c1a4f503-9901-4ca6-2388-08db058a686c
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Feb 2023 02:00:21.8444
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: BE5KNnAIQhLv4gElSrkVufuR5Dv/zwV+SxOYWBJEe1FVPhbwR3A006c0vhIvboxbyXsmPAi1QmpDEZfsCmLeOA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4835
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Friday, February 3, 2023 7:13 AM
>=20
> On Thu, 2 Feb 2023 23:04:10 +0000
> "Tian, Kevin" <kevin.tian@intel.com> wrote:
>=20
> > > From: Alex Williamson <alex.williamson@redhat.com>
> > > Sent: Friday, February 3, 2023 3:42 AM
> > >
> > >
> > > LGTM.  I'm not sure moving the functions to vfio_main really buys us
> > > anything since we're making so much use of group fields.  The cdev
> > > approach will necessarily be different, so the bulk of the get code w=
ill
> > > likely need to move back to group.c anyway.
> > >
> >
> > well my last comment was based on Matthew's v2 where the get code
> > gets a kvm passed in instead of implicitly retrieving group ref_lock
> > internally. In that case the get/put helpers only contain device logic
> > thus fit in vfio_main.c.
> >
> > with v3 then they have to be in group.c since we don't want to use
> > group fields in vfio_main.c.
> >
> > but I still think v2 of the helpers is slightly better. The only differ=
ence
> > between cdev and group when handling this race is using different
> > ref_lock. the symbol get/put part is exactly same. So even if we
> > merge v3 like this, very likely Yi has to change it back to v2 style
> > to share the get/put helpers while just leaving the ref_lock part
> > handled differently between the two path.
>=20
> I'm not really a fan of the asymmetry of the v2 version where the get
> helper needs to be called under the new kvm_ref_lock, but the put
> helper does not.  Having the get helper handle that makes the caller
> much cleaner.  Thanks,
>=20

What about passing the lock pointer into the helper? it's still slightly
asymmetry as the put helper doesn't carry the lock pointer but it
could also be interpreted as if the pointer has been saved in the get
then if it needs to be referenced by the put there is no need to pass
it in again.