References: <20211020170305.376118-1-ankur.a.arora@oracle.com>
 <20211020170305.376118-8-ankur.a.arora@oracle.com>
 <b955c5c4-bc4b-9f43-be1c-3a45973de259@linux.alibaba.com>
 <87czm5ulcc.fsf@oracle.com>
 <acc75c62-b919-12da-25b4-2b65ecf89ab6@linux.alibaba.com>
User-agent: mu4e 1.4.10; emacs 27.1
From:   Ankur Arora <ankur.a.arora@oracle.com>
To:     Yu Xu <xuyu@linux.alibaba.com>
Cc:     Ankur Arora <ankur.a.arora@oracle.com>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org, x86@kernel.org,
        mingo@kernel.org, bp@alien8.de, luto@kernel.org,
        akpm@linux-foundation.org, mike.kravetz@oracle.com,
        jon.grimm@amd.com, kvm@vger.kernel.org, konrad.wilk@oracle.com,
        boris.ostrovsky@oracle.com
Subject: Re: [PATCH v2 07/14] x86/clear_page: add clear_page_uncached()
In-reply-to: <acc75c62-b919-12da-25b4-2b65ecf89ab6@linux.alibaba.com>
Date:   Fri, 10 Dec 2021 06:26:58 -0800
Message-ID: <87wnkcsfgt.fsf@oracle.com>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?TX5gdpeWGG5piOAfa2y/GHv36ydWsLytvtlUYjoIiSpWtcfjhcGfHy1ioJlQ?=
 =?us-ascii?Q?iGrH3H9Lnj2DFH3DmbIKJwOwD5UahQj7HK+bRfjxyaU9LtUGo5aU1EyIXh6l?=
 =?us-ascii?Q?OLG3qMbg4OKplcbD5vYFFYswAhNMgT12y/Kn0CcR8QIq7i5kmGv0YDnAkPoJ?=
 =?us-ascii?Q?HSVmbDihJRqgYhdovHIaKTf7iWJAw+HHNFlLffZKhQ6LzhIAhcUd6vqjm5IT?=
 =?us-ascii?Q?h+zQDgqZuUadem9IxVBGg6t4yulqEZGYPMxQEeKXMc0GGNQomaHmZuUkxpZh?=
 =?us-ascii?Q?aooc4W8HOatsm+XS0Zc8LwPytZ3bebNwg+4RgdjOk6xr9kldxpL/fej6tenv?=
 =?us-ascii?Q?4i0Cb8uFgtM12IJlSYCu9o+AXofVkHBIoF8Pv3n+z1jLNxdheX56HeYEEUbp?=
 =?us-ascii?Q?A0mv6RLRyQAPSvl0VNVmyxT8g5krAtDiAygyECAO1AW9p9EN7+ENQ9EmmUon?=
 =?us-ascii?Q?bL3NM7Y2YUo8JPrFyd53ERLFODW5yBn6VH/pcyQT8Vs8q2kML+YbCBEsHGQt?=
 =?us-ascii?Q?YB8lG9e2++qnInpGqW8SmWAIieUMWQonZhDmlcwL7B5K1X2DG0NQP5/yR1b3?=
 =?us-ascii?Q?Ac1VhUpcVL60Zw+/sKYK1+h4SXHoeo14RXkJDv86qLNQrT8hdpJzJ/SCRb+y?=
 =?us-ascii?Q?S+ygwo/0lk2/t0fAQOF9rI8Qeva3XQ6wtxXy6JiwZIs6MkS6PrvoHvdIqg4a?=
 =?us-ascii?Q?k8bvOIkzMxPRjgRZLHwcFscbCAjUAWjIqUAJcptCF1FujBL3tPv/IDI49GM/?=
 =?us-ascii?Q?yAQS+2GrJN4md3bzRqh1p18uHOvPAgn8xk21Jm977L5vljfY2oCAHOb/3fND?=
 =?us-ascii?Q?r3YrckJxrPM2ajTCu7f33jeB6oNuYzzR8nb+Ad4QTnGg3zpjA610nAY1kfsH?=
 =?us-ascii?Q?3VxwGTLP54K1PG7LZ9gjCy2boG2ECUReaDlqLHIL53jEvTZICYCxghqiLp26?=
 =?us-ascii?Q?3j7BIVIHm0vO3IqaasiLZs/YPGthj+dpmzC09LSW3r/HWsHHq0jtZvpsR3UU?=
 =?us-ascii?Q?uXLA+F3HIyZlaeqGXnn+bf8lntNORZVaB1l9NLrQ3AwufDFw0f8YozTA/GOc?=
 =?us-ascii?Q?BKr273CK6NhsZhqS9WBX78OhLFkV9Ex7w43al06VKy4aH2Ca8G0KHBbk+sWg?=
 =?us-ascii?Q?n7TkckEEJjurYNPBcbOPfuZfj/HSDwH0llUxkbfd5rPDMMVw+oaaXMX8VCpu?=
 =?us-ascii?Q?Vao/sh1ujeIgo0e1e821jns3TVr23qXul9evLHq60QD7sidKRMl6mGUi51vc?=
 =?us-ascii?Q?mxJ0djdHLakP61WmUn12sfaCIxyFhvwGWOYDOg1RtjnK69Are/wUQgs1KFsU?=
 =?us-ascii?Q?rhYyhIwIrH5hgp6i3d5F9KLanWrEUlumqEZxz34L9JjjyoCb39VFzWwsEJLr?=
 =?us-ascii?Q?G5BI4nVX88SCrkYBctxf2e2J2nUyuPX1H34ine0JBJMAL/qOIP2fOmzFKVMi?=
 =?us-ascii?Q?h1BzjaNYfHQpuuaGrGdifR2HtTs8W+yyoTO0TX5zTWo6dQSytLb/v1yatRRF?=
 =?us-ascii?Q?72RsXcLJ8Z1j4smUJto1INi955AkH1FdpqRgkyE53v+8x1JOWpW/J1Og3Wa9?=
 =?us-ascii?Q?Sfa9MN9OrEpS/XN7Y+CIgi8YK+uluan57a0ilxaPFdz6BzQt3pNhVkpfg2qj?=
 =?us-ascii?Q?qF21F9403nIz7/mNk6jK01ghOAv6MwqG+hVY5J/0TvT+Swf+9cQ8FSGNItea?=
 =?us-ascii?Q?gETyiw=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 944d27b1-d2f5-4f4b-c049-08d9bbe9261c
X-MS-Exchange-CrossTenant-AuthSource: CO6PR10MB5409.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Dec 2021 14:27:09.2871
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 6R09OEeqqOxwfrV6ZwoJpdliplDBf8kDwmd2G+brwZDpdwWk8ewTxQt3UwRalaAkhXhUL8TGopA//fcfzAiOpy3eSmX41PnVYmyt7KfADds=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1776
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10193 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 mlxscore=0 spamscore=0
 phishscore=0 bulkscore=0 suspectscore=0 malwarescore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112100082
X-Proofpoint-ORIG-GUID: knPKZex8pKECZlOskrHu78mpbU-FHmfY
X-Proofpoint-GUID: knPKZex8pKECZlOskrHu78mpbU-FHmfY
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Yu Xu <xuyu@linux.alibaba.com> writes:

> On 12/10/21 12:37 PM, Ankur Arora wrote:
>> Yu Xu <xuyu@linux.alibaba.com> writes:
>>
>>> On 10/21/21 1:02 AM, Ankur Arora wrote:
>>>> Expose the low-level uncached primitives (clear_page_movnt(),
>>>> clear_page_clzero()) as alternatives via clear_page_uncached().
>>>> Also fallback to clear_page(), if X86_FEATURE_MOVNT_SLOW is set
>>>> and the CPU does not have X86_FEATURE_CLZERO.
>>>> Both the uncached primitives use stores which are weakly ordered
>>>> with respect to other instructions accessing the memory hierarchy.
>>>> To ensure that callers don't mix accesses to different types of
>>>> address_spaces, annotate clear_user_page_uncached(), and
>>>> clear_page_uncached() as taking __incoherent pointers as arguments.
>>>> Also add clear_page_uncached_make_coherent() which provides the
>>>> necessary store fence to flush out the uncached regions.
>>>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>>>> ---
>>>> Notes:
>>>>       This patch adds the fallback definitions of clear_user_page_uncached()
>>>>       etc in include/linux/mm.h which is likely not the right place for it.
>>>>       I'm guessing these should be moved to include/asm-generic/page.h
>>>>       (or maybe a new include/asm-generic/page_uncached.h) and for
>>>>       architectures that do have arch/$arch/include/asm/page.h (which
>>>>       seems like all of them), also replicate there?
>>>>       Anyway, wanted to first check if that's the way to do it, before
>>>>       doing that.
>>>>    arch/x86/include/asm/page.h    | 10 ++++++++++
>>>>    arch/x86/include/asm/page_32.h |  9 +++++++++
>>>>    arch/x86/include/asm/page_64.h | 32 ++++++++++++++++++++++++++++++++
>>>>    include/linux/mm.h             | 14 ++++++++++++++
>>>>    4 files changed, 65 insertions(+)
>>>> diff --git a/arch/x86/include/asm/page_32.h b/arch/x86/include/asm/page_32.h
>>>> index 94dbd51df58f..163be03ac422 100644
>>>> --- a/arch/x86/include/asm/page_32.h
>>>> +++ b/arch/x86/include/asm/page_32.h
>>>> @@ -39,6 +39,15 @@ static inline void clear_page(void *page)
>>>>    	memset(page, 0, PAGE_SIZE);
>>>>    }
>>>>    +static inline void clear_page_uncached(__incoherent void *page)
>>>> +{
>>>> +	clear_page((__force void *) page);
>>>> +}
>>>> +
>>>> +static inline void clear_page_uncached_make_coherent(void)
>>>> +{
>>>> +}
>>>> +
>>>>    static inline void copy_page(void *to, void *from)
>>>>    {
>>>>    	memcpy(to, from, PAGE_SIZE);
>>>> diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
>>>> index 3c53f8ef8818..d7946047c70f 100644
>>>> --- a/arch/x86/include/asm/page_64.h
>>>> +++ b/arch/x86/include/asm/page_64.h
>>>> @@ -56,6 +56,38 @@ static inline void clear_page(void *page)
>>>>    			   : "cc", "memory", "rax", "rcx");
>>>>    }
>>>>    +/*
>>>> + * clear_page_uncached: only allowed on __incoherent memory regions.
>>>> + */
>>>> +static inline void clear_page_uncached(__incoherent void *page)
>>>> +{
>>>> +	alternative_call_2(clear_page_movnt,
>>>> +			   clear_page, X86_FEATURE_MOVNT_SLOW,
>>>> +			   clear_page_clzero, X86_FEATURE_CLZERO,
>>>> +			   "=D" (page),
>>>> +			   "0" (page)
>>>> +			   : "cc", "memory", "rax", "rcx");
>>>> +}
>>>> +
>>>> +/*
>>>> + * clear_page_uncached_make_coherent: executes the necessary store
>>>> + * fence after which __incoherent regions can be safely accessed.
>>>> + */
>>>> +static inline void clear_page_uncached_make_coherent(void)
>>>> +{
>>>> +	/*
>>>> +	 * Keep the sfence for oldinstr and clzero separate to guard against
>>>> +	 * the possibility that a cpu-model both has X86_FEATURE_MOVNT_SLOW
>>>> +	 * and X86_FEATURE_CLZERO.
>>>> +	 *
>>>> +	 * The alternatives need to be in the same order as the ones
>>>> +	 * in clear_page_uncached().
>>>> +	 */
>>>> +	alternative_2("sfence",
>>>> +		      "", X86_FEATURE_MOVNT_SLOW,
>>>> +		      "sfence", X86_FEATURE_CLZERO);
>>>> +}
>>>> +
>>>>    void copy_page(void *to, void *from);
>>>>      #ifdef CONFIG_X86_5LEVEL
>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>> index 73a52aba448f..b88069d1116c 100644
>>>> --- a/include/linux/mm.h
>>>> +++ b/include/linux/mm.h
>>>> @@ -3192,6 +3192,20 @@ static inline bool vma_is_special_huge(const struct vm_area_struct *vma)
>>>>      #endif /* CONFIG_TRANSPARENT_HUGEPAGE || CONFIG_HUGETLBFS */
>>>>    +#ifndef clear_user_page_uncached
>>>
>>> Hi Ankur Arora,
>>>
>>> I've been looking for where clear_user_page_uncached is defined in this
>>> patchset, but failed.
>>>
>>> There should be something like follows in arch/x86, right?
>>>
>>> static inline void clear_user_page_uncached(__incoherent void *page,
>>>                                 unsigned long vaddr, struct page *pg)
>>> {
>>>          clear_page_uncached(page);
>>> }
>>>
>>>
>>> Did I miss something?
>>>
>> Hi Yu Xu,
>> Defined in include/linux/mm.h. Just below :).
>
> Thanks for your reply :)
>
> This is the version when #ifndef clear_user_page_uncached, i.e., fall
> back to standard clear_user_page.
>
> But where is the uncached version of clear_user_page? I am looking for
> this.

Sorry, my bad. There is a hunk missing. Not sure how, but this hunk
(from patch 7) got dropped in version that I sent out:

    diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
    index 4d5810c8fab7..89229f2db34a 100644
    --- a/arch/x86/include/asm/page.h
    +++ b/arch/x86/include/asm/page.h
    @@ -28,6 +28,16 @@ static inline void clear_user_page(void *page, unsigned long vaddr,
            clear_page(page);
    }

    +#define clear_user_page_uncached clear_user_page_uncached
    +/*
    + * clear_page_uncached: allowed on only __incoherent memory regions.
    + */
    +static inline void clear_user_page_uncached(__incoherent void *page,
    +                                       unsigned long vaddr, struct page *pg)
    +{
    +       clear_page_uncached(page);
    +}
    +
    static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
                                    struct page *topage)
    {

It is identical to the version that you surmised was missing. Thanks for
the close reading.

Ankur

>>
>>>> +/*
>>>> + * clear_user_page_uncached: fallback to the standard clear_user_page().
>>>> + */
>>>> +static inline void clear_user_page_uncached(__incoherent void *page,
>>>> +					unsigned long vaddr, struct page *pg)
>>>> +{
>>>> +	clear_user_page((__force void *)page, vaddr, pg);
>>>> +}
>> That said, as this note in the patch mentions, this isn't really a great
>> place for this definition. As you also mention, the right place for this
>> would be somewhere in the arch/.../include and include/asm-generic hierarchy.
>>
>>>>       This patch adds the fallback definitions of clear_user_page_uncached()
>>>>       etc in include/linux/mm.h which is likely not the right place for it.
>>>>       I'm guessing these should be moved to include/asm-generic/page.h
>>>>       (or maybe a new include/asm-generic/page_uncached.h) and for
>>>>       architectures that do have arch/$arch/include/asm/page.h (which
>>>>       seems like all of them), also replicate there?
>>>>       Anyway, wanted to first check if that's the way to do it, before
>>>>       doing that.
>> Recommendations on how to handle this, welcome.
>> Thanks
>> --
>> ankur
>>