Message ID | 20230607152308.125787-2-janusz.krzysztofik@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp288296vqr; Wed, 7 Jun 2023 08:35:57 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6QE0LqpW4kGvT8S+srtV+VGENmNuoFHQ3xqeYc9c7y5xvNJ0hnjXq+zBHXnWtonrL4gP5i X-Received: by 2002:a17:902:8c86:b0:1af:b678:5168 with SMTP id t6-20020a1709028c8600b001afb6785168mr2233957plo.67.1686152157484; Wed, 07 Jun 2023 08:35:57 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1686152157; cv=none; d=google.com; s=arc-20160816; b=B3z418i/IztjW+uahFl8bfWXckjznzifN2djWu0TNj59C6cnFs5uwPsnShrOsehSIu 7Xj1hXH2pZxkmg37wIrPJP/DyvbfinpJ3sEKDibalOcLLpGnEdbYGzt5jie3pMa3ihN5 37bnLGVSVKSC4fOSdhpJ0N4YBFyFRJg6cvEhbM4w2FimIa61AixJ/rErtngfgSfYS1Ov uF/Tq+lCZWaSPi+k+9TZpVj/ZMR8vzSBbfNfshBiQ/hmaIKamdSvsySWDtDtcNsfICVt KnfEvCuUH8DkFOxm23W1Lg6g9WMp7nwknbimJQtJkUBhSjQHBYhOmeVU36jGzMQ0EIP+ QnYA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=MkTODXGXDekeTEqv7Sti47WNoyEYb/I2D3YVBFpEvt0=; b=BX/duMDpGBKrrGGKxygRJkMytExTIHl/2hKh3LoyGlUb8fFMDKUUln2bHa+ZTaxXCI eseWs2ehA7Mr2DpWbHbm3ZPFO66sDwgW3wggd+vfFGslGWhlzCxWRo1k47n0tAnbvJlR W7Jh6jl55/ad3euaysorHyD5fvhN5xoXACxBYaNhG6jKhVxh3HD7kjqxJdIHI+L/ML7T VrB+hfL+jFsD8me6fgM5aN5YKV67IPOy11EYhotoewKTO/0oSwPSAlA+1yKktkfuBTKI iZNxuU8kUeG8VUKTeLzo20pGl5uSnbz96mznJ3XNdyprQCIvJMVbwvXGvpHACu5QKfRD lZog== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b="A2mvcJi/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id p13-20020a170902e74d00b001b23d4573b5si2430032plf.3.2023.06.07.08.35.41; Wed, 07 Jun 2023 08:35:57 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b="A2mvcJi/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241396AbjFGP0J (ORCPT <rfc822;xxoosimple@gmail.com> + 99 others); Wed, 7 Jun 2023 11:26:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34346 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S241218AbjFGPZk (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 7 Jun 2023 11:25:40 -0400 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3FB241BE2 for <linux-kernel@vger.kernel.org>; Wed, 7 Jun 2023 08:25:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1686151515; x=1717687515; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=ARZjNToUiO85DLbaOveTz4aJbWaU0n8lP8ieuY9WBAg=; b=A2mvcJi/gZezCXpPtX4airtaCl2TnYEKg+ctc8gocffxIWVDOJM1W8Dm WqzIit5+dOgpDCMwii9wGHS//aYtOgxyUF7ZvwccajBuCei/5a2Leig/K hILp/SIiiQzpBHrpZ9p1ph5fKiPqjWSMu1k76QLI86uLwcimNUFX2gDOF q8lpOozzI3lhSh9cXSmwsgZg/5INGPUPXfsRsvLvyq6xwpLywirkgR2ak xWrXD0k3RJX/+1RFnEyjruQC9Fc5VenV5BjGPXpmiGOIt6NS3iqTjxRLZ rArzy/foOgUH6xrnGTO9MwTidkGcc+Uz3RoJSLwXeOCnXAzKp4yK2adPk A==; X-IronPort-AV: E=McAfee;i="6600,9927,10734"; a="337383278" X-IronPort-AV: E=Sophos;i="6.00,224,1681196400"; d="scan'208";a="337383278" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Jun 2023 08:24:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10734"; a="956327825" X-IronPort-AV: E=Sophos;i="6.00,224,1681196400"; d="scan'208";a="956327825" Received: from jkrzyszt-mobl2.ger.corp.intel.com ([10.213.10.174]) by fmsmga006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Jun 2023 08:24:00 -0700 From: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> To: Borislav Petkov <bp@alien8.de> Cc: Juergen Gross <jgross@suse.com>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org, Andi Shyti <andi.shyti@linux.intel.com>, intel-gfx@lists.freedesktop.org, Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> Subject: [PATCH v2] x86/mm: Fix PAT bit missing from page protection modify mask Date: Wed, 7 Jun 2023 17:23:09 +0200 Message-ID: <20230607152308.125787-2-janusz.krzysztofik@linux.intel.com> X-Mailer: git-send-email 2.41.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, 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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1768058684483825445?= X-GMAIL-MSGID: =?utf-8?q?1768058684483825445?= |
Series |
[v2] x86/mm: Fix PAT bit missing from page protection modify mask
|
|
Commit Message
Janusz Krzysztofik
June 7, 2023, 3:23 p.m. UTC
Visible glitches have been observed when running graphics applications on Linux under Xen hypervisor. Those observations have been confirmed with failures from kms_pwrite_crc Intel GPU test that verifies data coherency of DRM frame buffer objects using hardware CRC checksums calculated by display controllers, exposed to userspace via debugfs. Affected processing paths have then been identified with new IGT test variants that mmap the objects using different methods and caching modes [1]. When running as a Xen PV guest, Linux uses Xen provided PAT configuration which is different from its native one. In particular, Xen specific PTE encoding of write-combining caching, likely used by graphics applications, differs from the Linux default one found among statically defined minimal set of supported modes. Since Xen defines PTE encoding of the WC mode as _PAGE_PAT, it no longer belongs to the minimal set, depends on correct handling of _PAGE_PAT bit, and can be mismatched with write-back caching. When a user calls mmap() for a DRM buffer object, DRM device specific .mmap file operation, called from mmap_region(), takes care of setting PTE encoding bits in a vm_page_prot field of an associated virtual memory area structure. Unfortunately, _PAGE_PAT bit is not preserved when the vma's .vm_flags are then applied to .vm_page_prot via vm_set_page_prot(). Bits to be preserved are determined with _PAGE_CHG_MASK symbol that doesn't cover _PAGE_PAT. As a consequence, WB caching is requested instead of WC when running under Xen (also, WP is silently changed to WT, and UC downgraded to UC_MINUS). When running on bare metal, WC is not affected, but WP and WT extra modes are unintentionally replaced with WC and UC, respectively. WP and WT modes, encoded with _PAGE_PAT bit set, were introduced by commit 281d4078bec3 ("x86: Make page cache mode a real type"). Care was taken to extend _PAGE_CACHE_MASK symbol with that additional bit, but that symbol has never been used for identification of bits preserved when applying page protection flags. Support for all cache modes under Xen, including the problematic WC mode, was then introduced by commit 47591df50512 ("xen: Support Xen pv-domains using PAT"). Extend bitmask used by pgprot_modify() for selecting bits to be preserved with _PAGE_PAT bit. However, since that bit can be reused as _PAGE_PSE, and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is likely intentionally defined with that bit not set, keep that symbol unchanged. v2: Keep pgprot_modify() untouched, make _PAGE_PAT part of _PAGE_CHG_MASK instead (Borislav), - also add _PAGE_PAT_LARGE to _HPAGE_CHG_MASK (Juergen). [1] https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/0f0754413f14 Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/7648 Fixes: 281d4078bec3 ("x86: Make page cache mode a real type") Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Cc: Juergen Gross <jgross@suse.com> Cc: Borislav Petkov <bp@alien8.de> Cc: stable@vger.kernel.org # v3.19+ --- arch/x86/include/asm/pgtable_types.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Comments
On 6/7/23 08:23, Janusz Krzysztofik wrote: > > Extend bitmask used by pgprot_modify() for selecting bits to be preserved > with _PAGE_PAT bit. However, since that bit can be reused as _PAGE_PSE, > and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is likely > intentionally defined with that bit not set, keep that symbol unchanged. I'm really having a hard time parsing what that last sentence is saying. Could you try again, please?
On Wednesday, 7 June 2023 17:31:24 CEST Dave Hansen wrote: > On 6/7/23 08:23, Janusz Krzysztofik wrote: > > > > Extend bitmask used by pgprot_modify() for selecting bits to be preserved > > with _PAGE_PAT bit. However, since that bit can be reused as _PAGE_PSE, > > and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is likely > > intentionally defined with that bit not set, keep that symbol unchanged. > > I'm really having a hard time parsing what that last sentence is saying. > > Could you try again, please? OK, but then I need to get my doubts addressed by someone first, otherwise I'm not able to provide a better justification from my heart. The issue needs to be fixed by including _PAGE_PAT bit into a bitmask used by pgprot_modify() for selecting bits to be preserved. We can either do that internally to pgprot_modify() (my initial proposal, which my poorly worded paragraph was still trying to describe and justify), or by making _PAGE_PAT a part of _PAGE_CHG_MASK, as suggested by Borislav and reflected in my v2 changelog. But for the latter, I think we need to make sure that we don't break other users of _PAGE_CHG_MASK. Maybe Borislav can confirm that's the case. Since _PAGE_PAT is the same as _PAGE_PSE, _HPAGE_CHG_MASK -- a huge pmds' counterpart of _PAGE_CHG_MASK, introduced by commit c489f1257b8c ("thp: add pmd_modify"), defined as (_PAGE_CHG_MASK | _PAGE_PSE) -- will no longer differ from _PAGE_CHG_MASK as soon as we add _PAGE_PAT bit to the latter. If such modification of _PAGE_CHG_MASK was irrelevant to its users then one may ask why a new symbol was introduced instead of reusing the existing one with that otherwise irrelevant bit (_PAGE_PSE in that case) added. I've initially assumed that keeping _PAGE_CHG_MASK without _PAGE_PSE (vel _PAGE_PAT) included into it was intentional for some reason. Maybe Johannes Weiner, the author of that patch (adding him to Cc:), could shed more light on that. Thanks, Janusz
On Wed, 2023-06-07 at 19:11 +0200, Janusz Krzysztofik wrote: > On Wednesday, 7 June 2023 17:31:24 CEST Dave Hansen wrote: > > On 6/7/23 08:23, Janusz Krzysztofik wrote: > > > > > > Extend bitmask used by pgprot_modify() for selecting bits to be > > > preserved > > > with _PAGE_PAT bit. However, since that bit can be reused as > > > _PAGE_PSE, > > > and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is > > > likely > > > intentionally defined with that bit not set, keep that symbol > > > unchanged. > > > > I'm really having a hard time parsing what that last sentence is > > saying. > > > > Could you try again, please? > > OK, but then I need to get my doubts addressed by someone first, > otherwise I'm > not able to provide a better justification from my heart. > > The issue needs to be fixed by including _PAGE_PAT bit into a bitmask > used > by pgprot_modify() for selecting bits to be preserved. We can either > do > that internally to pgprot_modify() (my initial proposal, which my > poorly > worded paragraph was still trying to describe and justify), or by > making > _PAGE_PAT a part of _PAGE_CHG_MASK, as suggested by Borislav and > reflected in > my v2 changelog. But for the latter, I think we need to make sure > that we > don't break other users of _PAGE_CHG_MASK. Maybe Borislav can > confirm that's > the case. > > Since _PAGE_PAT is the same as _PAGE_PSE, _HPAGE_CHG_MASK -- a huge > pmds' > counterpart of _PAGE_CHG_MASK, introduced by commit c489f1257b8c > ("thp: add > pmd_modify"), defined as (_PAGE_CHG_MASK | _PAGE_PSE) -- will no > longer differ > from _PAGE_CHG_MASK as soon as we add _PAGE_PAT bit to the latter. > If such > modification of _PAGE_CHG_MASK was irrelevant to its users then one > may ask > why a new symbol was introduced instead of reusing the existing one > with that > otherwise irrelevant bit (_PAGE_PSE in that case) added. I've > initially > assumed that keeping _PAGE_CHG_MASK without _PAGE_PSE (vel _PAGE_PAT) > included > into it was intentional for some reason. Maybe Johannes Weiner, the > author of > that patch (adding him to Cc:), could shed more light on that. So since _PAGE_PSE is actually the same value as _PAGE_PAT, you don't actually need to have _PAGE_PSE in _HPAGE_CHG_MASK in order to get functional correctness. Is that right? I think it is still a little hidden (even before this) and I wonder about separating out the common bits into, like, _COMMON_PAGE_CHG_MASK or something. Then setting specific PAGE and HPAGE bits (like _PAGE_PAT, _PAGE_PSE and _PAGE_PAT_LARGE) in their specific define. Would it be more readable that way?
On Wednesday, 7 June 2023 23:12:13 CEST Edgecombe, Rick P wrote: > On Wed, 2023-06-07 at 19:11 +0200, Janusz Krzysztofik wrote: > > On Wednesday, 7 June 2023 17:31:24 CEST Dave Hansen wrote: > > > On 6/7/23 08:23, Janusz Krzysztofik wrote: > > > > > > > > Extend bitmask used by pgprot_modify() for selecting bits to be > > > > preserved > > > > with _PAGE_PAT bit. However, since that bit can be reused as > > > > _PAGE_PSE, > > > > and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is > > > > likely > > > > intentionally defined with that bit not set, keep that symbol > > > > unchanged. > > > > > > I'm really having a hard time parsing what that last sentence is > > > saying. > > > > > > Could you try again, please? > > > > OK, but then I need to get my doubts addressed by someone first, > > otherwise I'm > > not able to provide a better justification from my heart. > > > > The issue needs to be fixed by including _PAGE_PAT bit into a bitmask > > used > > by pgprot_modify() for selecting bits to be preserved. We can either > > do > > that internally to pgprot_modify() (my initial proposal, which my > > poorly > > worded paragraph was still trying to describe and justify), or by > > making > > _PAGE_PAT a part of _PAGE_CHG_MASK, as suggested by Borislav and > > reflected in > > my v2 changelog. But for the latter, I think we need to make sure > > that we > > don't break other users of _PAGE_CHG_MASK. Maybe Borislav can > > confirm that's > > the case. > > > > Since _PAGE_PAT is the same as _PAGE_PSE, _HPAGE_CHG_MASK -- a huge > > pmds' > > counterpart of _PAGE_CHG_MASK, introduced by commit c489f1257b8c > > ("thp: add > > pmd_modify"), defined as (_PAGE_CHG_MASK | _PAGE_PSE) -- will no > > longer differ > > from _PAGE_CHG_MASK as soon as we add _PAGE_PAT bit to the latter. > > If such > > modification of _PAGE_CHG_MASK was irrelevant to its users then one > > may ask > > why a new symbol was introduced instead of reusing the existing one > > with that > > otherwise irrelevant bit (_PAGE_PSE in that case) added. I've > > initially > > assumed that keeping _PAGE_CHG_MASK without _PAGE_PSE (vel _PAGE_PAT) > > included > > into it was intentional for some reason. Maybe Johannes Weiner, the > > author of > > that patch (adding him to Cc:), could shed more light on that. > > So since _PAGE_PSE is actually the same value as _PAGE_PAT, you don't > actually need to have _PAGE_PSE in _HPAGE_CHG_MASK in order to get > functional correctness. Is that right? As soon as we add _PAGE_PAT to _PAGE_CHG_MASK -- yes, that's right. But we may still want to add _PAGE_PSE to _HPAGE_CHG_MASK to have the need for that bit explicitly documented. > > I think it is still a little hidden (even before this) and I wonder > about separating out the common bits into, like, _COMMON_PAGE_CHG_MASK > or something. Then setting specific PAGE and HPAGE bits (like > _PAGE_PAT, _PAGE_PSE and _PAGE_PAT_LARGE) in their specific define. > Would it be more readable that way? Yes, I think that's a good idea, and I can use it in my patch. The question if _PAGE_PAT vel _PAGE_PSE added to _PAGE_CHG_MASK is really harmless for pte_modify() and its users is still open for me though. Thanks, Janusz
On Wed, 2023-06-07 at 23:33 +0200, Janusz Krzysztofik wrote: > > So since _PAGE_PSE is actually the same value as _PAGE_PAT, you > > don't > > actually need to have _PAGE_PSE in _HPAGE_CHG_MASK in order to get > > functional correctness. Is that right? > > As soon as we add _PAGE_PAT to _PAGE_CHG_MASK -- yes, that's right. > But we > may still want to add _PAGE_PSE to _HPAGE_CHG_MASK to have the need > for that > bit explicitly documented. _PAGE_PSE is already in _HPAGE_CHG_MASK though, right? I'm confused. > > > > > I think it is still a little hidden (even before this) and I wonder > > about separating out the common bits into, like, > > _COMMON_PAGE_CHG_MASK > > or something. Then setting specific PAGE and HPAGE bits (like > > _PAGE_PAT, _PAGE_PSE and _PAGE_PAT_LARGE) in their specific define. > > Would it be more readable that way? > > Yes, I think that's a good idea, and I can use it in my patch. > > The question if _PAGE_PAT vel _PAGE_PSE added to _PAGE_CHG_MASK is > really > harmless for pte_modify() and its users is still open for me though. When you say "vel", this is similar to the english acronym "AKA" I think? So I think you mean, when you add _PAGE_PAT to _PAGE_CHG_MASK, you are also adding _PAGE_PSE to it. So does that cause any problems? I see, good question... vm_page_prot is used when creating PTEs and huge PMDs, and the setter only uses _PAGE_CHG_MASK, even though it won't actually know where that prot is going to end up. Having _PAGE_PAT/PSE in _PAGE_CHG_MASK certainly doesn't make it easier to think about. One thing it's favor though is vm_page_prot is not applied to page table entries that are pointing to other page table entries (PSE = 0). So you shouldn't accidentally set PSE=1. And _PAGE_PSE shouldn't be being set in there, so you also shouldn't accidentally be setting PAT=1. But yea, I see why you are concerned. I would /guess/ it would be ok functionally. That probably doesn't help much...
On 07.06.23 23:12, Edgecombe, Rick P wrote: > On Wed, 2023-06-07 at 19:11 +0200, Janusz Krzysztofik wrote: >> On Wednesday, 7 June 2023 17:31:24 CEST Dave Hansen wrote: >>> On 6/7/23 08:23, Janusz Krzysztofik wrote: >>>> >>>> Extend bitmask used by pgprot_modify() for selecting bits to be >>>> preserved >>>> with _PAGE_PAT bit. However, since that bit can be reused as >>>> _PAGE_PSE, >>>> and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is >>>> likely >>>> intentionally defined with that bit not set, keep that symbol >>>> unchanged. >>> >>> I'm really having a hard time parsing what that last sentence is >>> saying. >>> >>> Could you try again, please? >> >> OK, but then I need to get my doubts addressed by someone first, >> otherwise I'm >> not able to provide a better justification from my heart. >> >> The issue needs to be fixed by including _PAGE_PAT bit into a bitmask >> used >> by pgprot_modify() for selecting bits to be preserved. We can either >> do >> that internally to pgprot_modify() (my initial proposal, which my >> poorly >> worded paragraph was still trying to describe and justify), or by >> making >> _PAGE_PAT a part of _PAGE_CHG_MASK, as suggested by Borislav and >> reflected in >> my v2 changelog. But for the latter, I think we need to make sure >> that we >> don't break other users of _PAGE_CHG_MASK. Maybe Borislav can >> confirm that's >> the case. >> >> Since _PAGE_PAT is the same as _PAGE_PSE, _HPAGE_CHG_MASK -- a huge >> pmds' >> counterpart of _PAGE_CHG_MASK, introduced by commit c489f1257b8c >> ("thp: add >> pmd_modify"), defined as (_PAGE_CHG_MASK | _PAGE_PSE) -- will no >> longer differ >> from _PAGE_CHG_MASK as soon as we add _PAGE_PAT bit to the latter. >> If such >> modification of _PAGE_CHG_MASK was irrelevant to its users then one >> may ask >> why a new symbol was introduced instead of reusing the existing one >> with that >> otherwise irrelevant bit (_PAGE_PSE in that case) added. I've >> initially >> assumed that keeping _PAGE_CHG_MASK without _PAGE_PSE (vel _PAGE_PAT) >> included >> into it was intentional for some reason. Maybe Johannes Weiner, the >> author of >> that patch (adding him to Cc:), could shed more light on that. > > So since _PAGE_PSE is actually the same value as _PAGE_PAT, you don't > actually need to have _PAGE_PSE in _HPAGE_CHG_MASK in order to get > functional correctness. Is that right? > > I think it is still a little hidden (even before this) and I wonder > about separating out the common bits into, like, _COMMON_PAGE_CHG_MASK > or something. Then setting specific PAGE and HPAGE bits (like > _PAGE_PAT, _PAGE_PSE and _PAGE_PAT_LARGE) in their specific define. > Would it be more readable that way? I'd go that route. I don't think we should rely on _PAGE_PSE == _PAGE_PAT here. Juergen
On Thursday, 8 June 2023 00:47:36 CEST Edgecombe, Rick P wrote: > On Wed, 2023-06-07 at 23:33 +0200, Janusz Krzysztofik wrote: > > > So since _PAGE_PSE is actually the same value as _PAGE_PAT, you > > > don't > > > actually need to have _PAGE_PSE in _HPAGE_CHG_MASK in order to get > > > functional correctness. Is that right? > > > > As soon as we add _PAGE_PAT to _PAGE_CHG_MASK -- yes, that's right. > > But we > > may still want to add _PAGE_PSE to _HPAGE_CHG_MASK to have the need > > for that > > bit explicitly documented. > > _PAGE_PSE is already in _HPAGE_CHG_MASK though, right? I'm confused. Yes, it is, sorry for confusion. I should have said: we may still want to keep _PAGE_PSE explicitly added to _HPAGE_CHK_MASK to have the reason for including that bit documented. Thanks, Janusz > > > > > > > > > I think it is still a little hidden (even before this) and I wonder > > > about separating out the common bits into, like, > > > _COMMON_PAGE_CHG_MASK > > > or something. Then setting specific PAGE and HPAGE bits (like > > > _PAGE_PAT, _PAGE_PSE and _PAGE_PAT_LARGE) in their specific define. > > > Would it be more readable that way? > > > > Yes, I think that's a good idea, and I can use it in my patch. > > > > The question if _PAGE_PAT vel _PAGE_PSE added to _PAGE_CHG_MASK is > > really > > harmless for pte_modify() and its users is still open for me though. > > When you say "vel", this is similar to the english acronym "AKA" I > think? > > So I think you mean, when you add _PAGE_PAT to _PAGE_CHG_MASK, you are > also adding _PAGE_PSE to it. So does that cause any problems? I see, > good question... > > vm_page_prot is used when creating PTEs and huge PMDs, and the setter > only uses _PAGE_CHG_MASK, even though it won't actually know where that > prot is going to end up. > > Having _PAGE_PAT/PSE in _PAGE_CHG_MASK certainly doesn't make it easier > to think about. One thing it's favor though is vm_page_prot is not > applied to page table entries that are pointing to other page table > entries (PSE = 0). So you shouldn't accidentally set PSE=1. And > _PAGE_PSE shouldn't be being set in there, so you also shouldn't > accidentally be setting PAT=1. > > But yea, I see why you are concerned. I would /guess/ it would be ok > functionally. That probably doesn't help much... >
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h index 447d4bee25c48..0432ed9e59e3e 100644 --- a/arch/x86/include/asm/pgtable_types.h +++ b/arch/x86/include/asm/pgtable_types.h @@ -125,11 +125,11 @@ * instance, and is *not* included in this mask since * pte_modify() does modify it. */ -#define _PAGE_CHG_MASK (PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT | \ - _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY | \ - _PAGE_SOFT_DIRTY | _PAGE_DEVMAP | _PAGE_ENC | \ +#define _PAGE_CHG_MASK (PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT | _PAGE_PAT | \ + _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY | \ + _PAGE_SOFT_DIRTY | _PAGE_DEVMAP | _PAGE_ENC | \ _PAGE_UFFD_WP) -#define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE) +#define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE | _PAGE_PAT_LARGE) /* * The cache modes defined here are used to translate between pure SW usage