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;
Date:   Sat, 6 May 2023 18:57:59 +0800
From:   Yan Zhao <yan.y.zhao@intel.com>
To:     Sean Christopherson <seanjc@google.com>, <kvm@vger.kernel.org>,
        <intel-gfx@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        "Zhenyu Wang" <zhenyuw@linux.intel.com>,
        Ben Gardon <bgardon@google.com>,
        "Paolo Bonzini" <pbonzini@redhat.com>,
        <intel-gvt-dev@lists.freedesktop.org>,
        "Zhi Wang" <zhi.a.wang@intel.com>
Subject: Re: [PATCH v2 05/27] drm/i915/gvt: Verify VFIO-pinned page is THP
 when shadowing 2M gtt entry
Message-ID: <ZFYyt2fF6alyKlzO@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20230311002258.852397-1-seanjc@google.com>
 <20230311002258.852397-6-seanjc@google.com>
 <ZBP7oZ1lkJhlSNpY@yzhao56-desk.sh.intel.com>
 <ZFQYbHTYgG4HJ+ac@google.com>
 <ZFX1PaKIa44WtSNX@yzhao56-desk.sh.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZFX1PaKIa44WtSNX@yzhao56-desk.sh.intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?/hGutdhRQ8khcerP+enzKhI7thet5pfU9otFegNa7Dp+fN10OKdSRPDzfRz8?=
 =?us-ascii?Q?QPJMKm16m40wzlyJBWBsJagxKT7pP3J4izlwa2yoz3HLU0PKAJiZSNAszVTl?=
 =?us-ascii?Q?uUrbZaQl2B1WbK31zlmlH1c99mgaojihuojo/sFrxGi5uDIgt04PkOWA937u?=
 =?us-ascii?Q?060yLULtsFaa5wQL9bBCeJTQo7q0axDAQUg//fIsSj1N3AO5nPi5bRGYvcxw?=
 =?us-ascii?Q?Nz27pzwx7o+WOQ1eZufxw+wBuxHKLY8ZF1juM1l2XXOf3YUPW0u0Wboo+VVE?=
 =?us-ascii?Q?kDoXHcOuGwK1hN3sLhIqCx9d8QH/G39d7wKcns2d91dUAq4hYz1kuCzPsUP5?=
 =?us-ascii?Q?H3WJrWVtOorfqhpl7HWzIUBy7plKrvnfnrw8SXZjUBbPDCaXooZHs84eJC6U?=
 =?us-ascii?Q?zRlZtweIac/JgvAe6xnpGFd7ean7WiW/1pl5EBx6PtBzHH0qP8NKkMbL+0n8?=
 =?us-ascii?Q?NYFBSJBqTGRudFZlrDdD9otlnCVnPp5aGwwJM4mtwXsCzsfLKi58UUDW+6RR?=
 =?us-ascii?Q?kqgTJLXtLzUf7lL5bWAk4V/rIZoYgRzSkoeqO9sAWvwSA+lP+UM2uZGGb4b3?=
 =?us-ascii?Q?YLuZheXcUFzJ6VrTI4Gh13RIq9L/kV32MnfrCb5ZdaqzkC/La+c/RkvItfu9?=
 =?us-ascii?Q?BJAtG3BwC0WrcI8GtFv8P6i2zZLLWrRQQaMe+dYlQCgAiJBq0Qtjv5NGbcwr?=
 =?us-ascii?Q?2RRdoXzjz0NGBjqXc8sLVLLwUHErCKZK2GXQq2hUQ9bpYxN9b93mLFCoDOfa?=
 =?us-ascii?Q?o/yunZlc9HqaUgV3/9n6P7k/o+2aOgDd4KsVqjXMZIld6B5WmcDc9/3kjBaV?=
 =?us-ascii?Q?RhhtEBvibPX2uDGmFIJ+7YzakHCjQ6sqnMm7zviyLHnGDaz3O8x/bo2vtWxR?=
 =?us-ascii?Q?kjadvBYxces+YoKMKAIK1kEOUcsOX8/bHT0yuzg2Oq1EytYVAcp/2p6AD8vR?=
 =?us-ascii?Q?weOeG6HxdhXM3FXOJ27YDoRMx05oIRyWjy3gdK6EW9DkGylkGz/0u3eXWoDO?=
 =?us-ascii?Q?98ZwMjnbkN5tb+OY9k2tLSA7C9kGoojrsKPG3bpzJzlvOKkmjHL+u53FLiwa?=
 =?us-ascii?Q?kTcZjeW38Mk5QyCvBkOa+XxxVVwB4xGgNhuI+ezb43/aBnuBuYr9epmu75Mm?=
 =?us-ascii?Q?iEW3tBicpQiYiXqWAyQzsxW1V4njxKHBheeKqZgIQxYO/J7/mfllwdrU9S60?=
 =?us-ascii?Q?Y/LeZWG9ghRkaPNJfx6mLWEF+1MW4EVpnX8GjitjOuog27/laZQJwIErpOiF?=
 =?us-ascii?Q?FYnLcG7Eo5/gON+tdOFVoLNystbq1OGwzLJ+omjM/nDlNBQh3WLBBWuDoYnW?=
 =?us-ascii?Q?b3Ifo8phf8n5pwfMBiYeXCTiFGrPpF/XIWxszFfMBfNqQJmQ200PNhsU0uLZ?=
 =?us-ascii?Q?EgL2A42XE+uNu8fw/Ml5DiTiYqiCzL0p6Vb2rQIDx0s3mpgXOM44pUjUO1tp?=
 =?us-ascii?Q?2GzyA2OgAe+ci94HsUjdgpr1wCaI+z5HL+SlrtaiDYymPbGsJgoFTGZhrOVY?=
 =?us-ascii?Q?NEfu75yQojsgUUDHvnSPNpO3z4AK8Yc+PRo1O2ByhsqiA+aynMSeAgRivitc?=
 =?us-ascii?Q?XC/Kj2EYO2EOA0SaDKRDhrrP0Jj6DcIU3wSpKA6y?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8ad565e1-5547-46f7-3d32-08db4e244074
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 May 2023 11:23:01.2243
 (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: h77bHBYumedar2cse2LIp1luQDujlb/d43MZ/+sm74RxVDK1n+8DTXSHDOL4IDLwFe4Hw/9/KHX8SXjg/81UxQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7913
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_PASS,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sat, May 06, 2023 at 02:35:41PM +0800, Yan Zhao wrote:
> > > Maybe the checking of PageTransHuge(cur_page) and bailing out is not necessary.
> > > If a page is not transparent huge, but there are 512 contigous 4K
> > > pages, I think it's still good to map them in IOMMU in 2M.
> > > See vfio_pin_map_dma() who does similar things.
> > 
> > I agree that bailing isn't strictly necessary, and processing "blindly" should
> > Just Work for HugeTLB and other hugepage types.  I was going to argue that it
> > would be safer to add this and then drop it at the end, but I think that's a
> > specious argument.  If not checking the page type is unsafe, then the existing
> > code is buggy, and this changelog literally states that the check for contiguous
> > pages guards against any such problems.
> > 
> > I do think there's a (very, very theoretical) issue though.  For "CONFIG_SPARSEMEM=y
> > && CONFIG_SPARSEMEM_VMEMMAP=n", struct pages aren't virtually contiguous with respect
> > to their pfns, i.e. it's possible (again, very theoretically) that two struct pages
> > could be virtually contiguous but physically discontiguous.  I suspect I'm being
> > ridiculously paranoid, but for the efficient cases where pages are guaranteed to
> > be contiguous, the extra page_to_pfn() checks should be optimized away by the
> > compiler, i.e. there's no meaningful downside to the paranoia.
> To make sure I understand it correctly:
> There are 3 conditions:
> (1) Two struct pages aren't virtually contiguous, but there PFNs are contiguous.
> (2) Two struct pages are virtually contiguous but their PFNs aren't contiguous.
>     (Looks this will not happen?)
> (3) Two struct pages are virtually contiguous, and their PFNs are contiguous, too.
>     But they have different backends, e.g.
>     PFN 1 and PFN 2 are contiguous, while PFN 1 belongs to RAM, and PFN 2
>     belongs to DEVMEM.
> 
> I think you mean condition (3) is problematic, am I right?
Oh, I got it now.
You are saying about condition (2), with "CONFIG_SPARSEMEM=y &&
CONFIG_SPARSEMEM_VMEMMAP=n".
Two struct pages are contiguous if one is at one section's tail and another at
another section's head, but the two sections aren't for contiguous PFNs.

> > 
> > TL;DR: My plan is to drop this patch and instead harden the continuity check.
> 
> So you want to check page zone?