From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 78D4EC11F65 for ; Wed, 30 Jun 2021 17:46:12 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 378606147D for ; Wed, 30 Jun 2021 17:46:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 378606147D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=xen.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.148221.273913 (Exim 4.92) (envelope-from ) id 1lyeHm-0001QH-5R; Wed, 30 Jun 2021 17:46:06 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 148221.273913; Wed, 30 Jun 2021 17:46:06 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lyeHm-0001QA-2X; Wed, 30 Jun 2021 17:46:06 +0000 Received: by outflank-mailman (input) for mailman id 148221; Wed, 30 Jun 2021 17:46:05 +0000 Received: from mail.xenproject.org ([104.130.215.37]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lyeHl-0001Pw-8d for xen-devel@lists.xenproject.org; Wed, 30 Jun 2021 17:46:05 +0000 Received: from xenbits.xenproject.org ([104.239.192.120]) by mail.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lyeHl-0002Us-3A; Wed, 30 Jun 2021 17:46:05 +0000 Received: from [54.239.6.186] (helo=a483e7b01a66.ant.amazon.com) by xenbits.xenproject.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1lyeHk-0005AX-Tc; Wed, 30 Jun 2021 17:46:05 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=xen.org; s=20200302mail; h=Content-Transfer-Encoding:Content-Type:In-Reply-To: MIME-Version:Date:Message-ID:From:References:Cc:To:Subject; bh=MMCmuWbYY1Iy80Nrq3l4pQcHIg2D0GPcfF2MBFVe11U=; b=khkMxgxBKEa5l9lOoDoVCpP6Xb L2zIrcjyrxoAAiYmq5g+53K7EDAUqK1GijqrCnHp5oIvPbTzX8kzu26nmlcOxmyhXc7b5GaRTwocH LtvNJWIilDgP0URMNURjyOMDSvGRlL6IfHA8cCofmDT4VuAM+boCfmD7ItOKn+47kh5o=; Subject: Re: [PATCH 4/9] xen/arm: static memory initialization To: Jan Beulich , Penny Zheng Cc: Bertrand.Marquis@arm.com, Wei.Chen@arm.com, xen-devel@lists.xenproject.org, sstabellini@kernel.org References: <20210607024318.3988467-1-penny.zheng@arm.com> <20210607024318.3988467-5-penny.zheng@arm.com> From: Julien Grall Message-ID: <7f77349f-015e-83d3-d646-af9897e31348@xen.org> Date: Wed, 30 Jun 2021 18:46:03 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Hi, On 10/06/2021 10:35, Jan Beulich wrote: > On 07.06.2021 04:43, Penny Zheng wrote: >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -611,6 +611,30 @@ static void __init init_pdx(void) >> } >> } >> >> +/* Static memory initialization */ >> +static void __init init_staticmem_pages(void) >> +{ >> + int bank; > > While I'm not a maintainer of this code, I'd still like to point out > that wherever possible we prefer "unsigned int" when dealing with > only non-negative values, and even more so when using them as array > indexes. +1. > >> + /* >> + * TODO: Considering NUMA-support scenario. >> + */ > > Nit: Comment style. > >> @@ -872,6 +896,9 @@ void __init start_xen(unsigned long boot_phys_offset, >> cmdline_parse(cmdline); >> >> setup_mm(); >> + /* If exists, Static Memory Initialization. */ >> + if ( bootinfo.static_mem.nr_banks > 0 ) >> + init_staticmem_pages(); > > I don't think the conditional is really needed here? > >> --- a/xen/common/page_alloc.c >> +++ b/xen/common/page_alloc.c >> @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void) >> return node_to_scrub(false) != NUMA_NO_NODE; >> } >> >> +static void free_page(struct page_info *pg, bool need_scrub) >> +{ >> + mfn_t mfn = page_to_mfn(pg); > > With pdx compression this is a non-trivial conversion. The function > being an internal helper and the caller already holding the MFN, I > think it would be preferable if the MFN was passed in here. If done > this way, you may want to consider adding an ASSERT() to double > check both passed in arguments match up. > >> + /* If a page has no owner it will need no safety TLB flush. */ >> + pg->u.free.need_tlbflush = (page_get_owner(pg) != NULL); >> + if ( pg->u.free.need_tlbflush ) >> + page_set_tlbflush_timestamp(pg); >> + >> + /* This page is not a guest frame any more. */ >> + page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */ >> + set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY); >> + >> +#ifdef CONFIG_ARM > > If avoidable there should be no arch-specific code added to this > file. Assuming another arch gained PGC_reserved, what's wrong with > enabling this code right away for them as well? I.e. use > PGC_reserved here instead of CONFIG_ARM? Alternatively this may > want to be CONFIG_STATIC_ALLOCATION, assuming we consider > PGC_reserved tied to it. > >> + if ( pg->count_info & PGC_reserved ) >> + { >> + /* TODO: asynchronous scrubbing. */ >> + if ( need_scrub ) >> + scrub_one_page(pg); >> + return; >> + } >> +#endif >> + if ( need_scrub ) > > Nit: Please have a blank line between these last two. > >> + { >> + pg->count_info |= PGC_need_scrub; >> + poison_one_page(pg); >> + } >> + >> + return; > > Please omit return statements at the end of functions returning void. > >> +} > > On the whole, bike shedding or not, I'm afraid the function's name > doesn't match what it does: There's no freeing of a page here. What > gets done is marking of a page as free. Hence maybe mark_page_free() > or mark_free_page() or some such? > >> @@ -1512,6 +1530,38 @@ static void free_heap_pages( >> spin_unlock(&heap_lock); >> } >> >> +#ifdef CONFIG_STATIC_ALLOCATION >> +/* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */ >> +void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, >> + bool need_scrub) >> +{ >> + mfn_t mfn = page_to_mfn(pg); >> + unsigned long i; >> + >> + for ( i = 0; i < nr_mfns; i++ ) >> + { >> + switch ( pg[i].count_info & PGC_state ) >> + { >> + case PGC_state_inuse: >> + BUG_ON(pg[i].count_info & PGC_broken); >> + /* Mark it free and reserved. */ >> + pg[i].count_info = PGC_state_free | PGC_reserved; >> + break; >> + >> + default: >> + printk(XENLOG_ERR >> + "Page state shall be only in PGC_state_inuse. " > > Why? A page (static or not) can become broken while in use. IOW I > don't think you can avoid handling PGC_state_offlining here. At which > point this code will match free_heap_pages()'es, and hence likely > will want folding as well. > >> --- a/xen/include/xen/mm.h >> +++ b/xen/include/xen/mm.h >> @@ -85,6 +85,12 @@ bool scrub_free_pages(void); >> } while ( false ) >> #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) >> >> +#ifdef CONFIG_ARM > > ITYM CONFIG_STATIC_ALLOCATION here? > > Jan > -- Julien Grall