Received-SPF: pass (google.com: domain of linux-kernel+bounces-72682-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 20 Feb 2024 16:52:09 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: Sean Christopherson <seanjc@google.com>
CC: <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-kselftest@vger.kernel.org>, <pbonzini@redhat.com>, <shuah@kernel.org>,
	<stevensd@chromium.org>
Subject: Re: [RFC PATCH v2 1/3] KVM: allow mapping of compound tail pages for
 IO or PFNMAP mapping
Message-ID: <ZdRoOVA2jv+DXAzL@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20240103084327.19955-1-yan.y.zhao@intel.com>
 <20240103084424.20014-1-yan.y.zhao@intel.com>
 <ZcrfQS883NfOso4r@google.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZcrfQS883NfOso4r@google.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?DJAH5wyzXyx2AaPzWl0MP/tYltSmziWogyohxFunfJWmKQO8OR9FFtAaNXKR?=
 =?us-ascii?Q?mv9ajrRoxVkOlDW+N9hZpADfPCTtn+TnyUOWHvgYFyUyU9TNBS/FbBxFA0if?=
 =?us-ascii?Q?WcemlUEyQcnGUHKCUVSEJK9kOUwOV3fdbzWWagmd1hHeKByZv/CU+7D1CTCA?=
 =?us-ascii?Q?+RqqLuadi4atLx7PAz52fhXm1SJtDrbYl+3DRy06VfiqPtR1I7xotXUIsQ1c?=
 =?us-ascii?Q?u0LBMCYHRE2LUWYYmLJ3fLvdXQY8zxUeAGrenJd+RqsJTlXtmkkbLe00R6wz?=
 =?us-ascii?Q?rPPCGc66+n8s2KYvGS7nVghtFjTg/5Zb6iqJPmSxDqncnLm/FnbS7HTx9V9o?=
 =?us-ascii?Q?73NtgsqxLxAfiXFSfIita1hAwoerdUhVtFRjjsu+Auakh+TAn5WCmUYAWqvP?=
 =?us-ascii?Q?cO2gmOoKu4BuGMsWWyfUxN44Lt2sl7s+TvsoB3co/bgrhULLd594ORpSjG/O?=
 =?us-ascii?Q?e740Fr3NVXkmZf1TtE4swspDyLTLV1zCtm4CoPHZzLfBhh9LEMb3BhuEmXXn?=
 =?us-ascii?Q?qgucV8q/0xV5gJzMb7nP/80uTlQayMVnfkfkBRQW4QHakWdDvhf4ZfU2uCUr?=
 =?us-ascii?Q?7g8EvYYKwE5mwH6njnUMOamcPoT5SQLuwEZBr3HFKAZePKiEEwUNmrm0EXmF?=
 =?us-ascii?Q?iaApjaWzEaKD/dzReWlSwmbAcaPrBXxERMgwsyT3jHdBh05cDTG5+nHNra/D?=
 =?us-ascii?Q?Jf3+liUphUPF42dJibp0atQEwQyHlbEr5pMSrS4Fh9XQ0OMP4ZG6Z1a6g+f0?=
 =?us-ascii?Q?N/fvWelqBaTS3bFVvmgbspTEkrJkBiDZbWewa0Z5vVP54icynWqOPg/5jJw0?=
 =?us-ascii?Q?fHm4L7vF3yAdIiHQa5ATBhWTXtgD7EBOpYprR1IEcfwCdZhsOSA/52cxP2dH?=
 =?us-ascii?Q?E4d+lQpw4F6//yo7Vp53uHWCTRhM7pmOSDZ7amXdHbRONWF7/OofhmyLOHiz?=
 =?us-ascii?Q?Pfg1jUvQezHSZCqPbIVk0yCFbHQ4PkDa98E9UxprYc/9GbZ4ajFyyLi1K/WH?=
 =?us-ascii?Q?sm2KXVPpC1PEJ3O5q7IZYiSC0LU7cFw7qN1Q81eCcZg3sktqp65P8E2ITe2U?=
 =?us-ascii?Q?/bClxaEfh7zre6hK7O+NhKlRS4JLoCfLwNiNRmZkH9JPvPW1CGInRnqIivON?=
 =?us-ascii?Q?7F9EU1nSX/SqCnrVWVFXlnybMFkPc0lTGsZPkmm12vwg46h2cwK1QDVyyJ8r?=
 =?us-ascii?Q?niE5RVgmfunErO7449qW47FKKmJDZ8gjiX1bY17mtDeK5PzJrQo1JAvkKa3e?=
 =?us-ascii?Q?QRAzZ8CWi5DRKHKt72gthyDvDQVpmCQWzcOKPwq8nTMSjA2sNhLQlqIRDG2r?=
 =?us-ascii?Q?n2Dqd10ZJVLVv1etgJDWjSe3lBAeGxNjGnIUvEx/lOxGc7zFYDd5gIm06qQQ?=
 =?us-ascii?Q?mAUTDM/qAbbN+WXmRDx3L9bYndmWSZ7VJla0YrL8WUq0xgxsl1sN3yEtjq+b?=
 =?us-ascii?Q?LgdWXYdZpUVwG+Zzpalj09b1juA4JiaMgM719m4FAoYSrw5FvCFie4TM54xE?=
 =?us-ascii?Q?QS0F0cWnUlAIxhLEWfM67uVYjup6jTQhCUGD9Bpp25GOmmI8/xt3OGPQ2bq+?=
 =?us-ascii?Q?bhZdHF0jDIFdhFskdBIryq9P6XLSL6j1Ajuj/FOn?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 41a254a4-87ca-4268-f3d5-08dc31f55e1f
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2024 09:21:49.7101
 (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: YBHWf/N+tP2DdVmA9WFEh9lBKdfOzWB8BS9wvc5QMw+k7Pf20W6YjV8l7Sf7zocjlnDI7skgQLa0hBQuZgXTqw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4738
X-OriginatorOrg: intel.com

On Mon, Feb 12, 2024 at 07:17:21PM -0800, Sean Christopherson wrote:
> On Wed, Jan 03, 2024, Yan Zhao wrote:
> > Allow mapping of tail pages of compound pages for IO or PFNMAP mapping
> > by trying and getting ref count of its head page.
> > 
> > For IO or PFNMAP mapping, sometimes it's backed by compound pages.
> > KVM will just return error on mapping of tail pages of the compound pages,
> > as ref count of the tail pages are always 0.
> > 
> > So, rather than check and add ref count of a tail page, check and add ref
> > count of its folio (head page) to allow mapping of the compound tail pages.
> 
> Can you add a blurb to call out that this is effectively what gup() does in
> try_get_folio()?  That knowledge give me a _lot_ more confidence that this is
> correct (I didn't think too deeply about what this patch was doing when I looked
> at v1).
Sure.

> 
> > This will not break the origial intention to disallow mapping of tail pages
> > of non-compound higher order allocations as the folio of a non-compound
> > tail page is the same as the page itself.
> > 
> > On the other side, put_page() has already converted page to folio before
> > putting page ref.
> > 
> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> > ---
> >  virt/kvm/kvm_main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index acd67fb40183..f53b58446ac7 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -2892,7 +2892,7 @@ static int kvm_try_get_pfn(kvm_pfn_t pfn)
> >  	if (!page)
> >  		return 1;
> >  
> > -	return get_page_unless_zero(page);
> > +	return folio_try_get(page_folio(page));
> 
> This seems like it needs retry logic, a la try_get_folio(), to guard against a
> race with the folio being split.  From page_folio():
>   
>  If the caller* does not hold a reference, this call may race with a folio split,
>  so it should re-check the folio still contains this page after gaining a
>  reference on the folio.
> 
> I assume that splitting one of these folios is extremely unlikely, but I don't
> see any harm in being paranoid (unless this really truly cannot race).
Yes, you are right!
Will do the retry. Thanks!