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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 22ECFC4332F for ; Wed, 23 Nov 2022 23:39:26 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2E7B26B0071; Wed, 23 Nov 2022 18:39:26 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2997C6B0072; Wed, 23 Nov 2022 18:39:26 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 13A4E6B0074; Wed, 23 Nov 2022 18:39:26 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 007336B0071 for ; Wed, 23 Nov 2022 18:39:25 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id AD2F11C6B94 for ; Wed, 23 Nov 2022 23:39:25 +0000 (UTC) X-FDA: 80166325890.14.5CF01C4 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by imf08.hostedemail.com (Postfix) with ESMTP id 893E2160008 for ; Wed, 23 Nov 2022 23:39:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1669246763; x=1700782763; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=NbNzIvJel5W7dEOjelcpJ9WyJAs1Bi0d1O8jBfYAwTs=; b=UaegnE6z1pXtWY9HBqOLtQRhgtlToioLSKaWBFKeHcgs3BWNpmdZSc/R 8rD2jW2gWxCSHJJowD3lIwdBBV4QWXrfbYpVRDybL5JXfULsLPZCoXLse bhE2F4LmYxQdzQ/f4/UTJV2C/PuPcwUn/VXJpVD8MM+bDlD+sq+SyMrFi 9fZem/FcTML5YAphWOm/R0okaomZ/uHs/LFZqMXySzlv+XS8e/nHp9e9r 4manfqNe2vXXIaNmeMom/CoNv4g1i0Z1TUVK5KBUPFWr5wquDXKUX1cHd Uw4cCO/JMTMmzEgcKmZlwAQQBKWd6r0i/5IE5YWi6Zd4+n1oadyHL5qj3 A==; X-IronPort-AV: E=McAfee;i="6500,9779,10540"; a="314214157" X-IronPort-AV: E=Sophos;i="5.96,187,1665471600"; d="scan'208";a="314214157" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Nov 2022 15:39:21 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10540"; a="674887505" X-IronPort-AV: E=Sophos;i="5.96,187,1665471600"; d="scan'208";a="674887505" Received: from vcbudden-mobl3.amr.corp.intel.com (HELO [10.212.129.67]) ([10.212.129.67]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Nov 2022 15:39:20 -0800 Message-ID: <5526fedd-fa54-0ad2-f356-94b167e6e290@intel.com> Date: Wed, 23 Nov 2022 15:39:19 -0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [PATCH v7 14/20] x86/virt/tdx: Set up reserved areas for all TDMRs Content-Language: en-US To: Kai Huang , linux-kernel@vger.kernel.org, kvm@vger.kernel.org Cc: linux-mm@kvack.org, seanjc@google.com, pbonzini@redhat.com, dan.j.williams@intel.com, rafael.j.wysocki@intel.com, kirill.shutemov@linux.intel.com, ying.huang@intel.com, reinette.chatre@intel.com, len.brown@intel.com, tony.luck@intel.com, peterz@infradead.org, ak@linux.intel.com, isaku.yamahata@intel.com, chao.gao@intel.com, sathyanarayanan.kuppuswamy@linux.intel.com, bagasdotme@gmail.com, sagis@google.com, imammedo@redhat.com References: <5a5644e691134dc72c5e3fb0fc22fa40d4aa0b34.1668988357.git.kai.huang@intel.com> From: Dave Hansen In-Reply-To: <5a5644e691134dc72c5e3fb0fc22fa40d4aa0b34.1668988357.git.kai.huang@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1669246765; a=rsa-sha256; cv=none; b=ZPaLKz/smTKNnDcQXAdduY/NtM/hWsE5NKUFTa8Zi2mxHwxZotZggPw8Hlp6U17Fv83Y2F ZFqTxMQoHvCGMgqldmX8VB7J/XmEs29tAY28DKwH8lmTmng7Woj0rc737CnaknJukYT//H tt8p5LQKVBFfRttOMKIQNEeXs6Agmw8= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=UaegnE6z; spf=pass (imf08.hostedemail.com: domain of dave.hansen@intel.com designates 192.55.52.115 as permitted sender) smtp.mailfrom=dave.hansen@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1669246765; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=PelLGl3ySBdlVI92lqLNw6Vuf4qIev2lVZ0JveaDh7I=; b=71QQz74AQVi4bJlIIVGiUDXgKEra43ZsAamn29QGp+aPqrXoRDTFaqYhWloMGLSE9w18k3 Fu+DrxrdSILfYHWyY5qHpGIAHVI2Rhw38QKyyzprEuPG5M9SqxTQNQd1ysmJx4DvPd5xYA ja3ywkipLRQozVc+jOK7XKIHYHq8avY= X-Rspam-User: X-Rspamd-Queue-Id: 893E2160008 Authentication-Results: imf08.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=UaegnE6z; spf=pass (imf08.hostedemail.com: domain of dave.hansen@intel.com designates 192.55.52.115 as permitted sender) smtp.mailfrom=dave.hansen@intel.com; dmarc=pass (policy=none) header.from=intel.com X-Stat-Signature: o1b3powkqdrabodcsfcerukhsqkosfsx X-Rspamd-Server: rspam10 X-HE-Tag: 1669246763-980834 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: > +static int tdmr_set_up_memory_hole_rsvd_areas(struct tdmr_info *tdmr, > + int *rsvd_idx) > +{ This needs a comment. This is another case where it's confusing to be passing around 'struct tdmr_info *'. Is this *A* TDMR or an array? /* * Go through tdx_memlist to find holes between memory areas. If any of * those holes fall within @tdmr, set up a TDMR reserved area to cover * the hole. */ static int tdmr_populate_rsvd_holes(struct list_head *tdx_memlist, struct tdmr_info *tdmr, int *rsvd_idx) > + struct tdx_memblock *tmb; > + u64 prev_end; > + int ret; > + > + /* Mark holes between memory regions as reserved */ > + prev_end = tdmr_start(tdmr); I'm having a hard time following this, especially the mixing of semantics between 'prev_end' both pointing to tdmr and to tmb addresses. Here, 'prev_end' logically represents the last address which we know has been handled. All of the holes in the addresses below it have been dealt with. It is safe to set here to tdmr_start() because this function call is uniquely tasked with setting up reserved areas in 'tdmr'. So, it can safely consider any holes before tdmr_start(tdmr) as being handled. But, dang, there's a lot of complexity there. First, the: /* Mark holes between memory regions as reserved */ comment is misleading. It has *ZILCH* to do with the "prev_end = tdmr_start(tdmr);" assignment. This at least needs: /* Start looking for reserved blocks at the beginning of the TDMR: */ prev_end = tdmr_start(tdmr); but I also get the feeling that 'prev_end' is a crummy variable name. I don't have any better suggestions at the moment. > + list_for_each_entry(tmb, &tdx_memlist, list) { > + u64 start, end; > + > + start = tmb->start_pfn << PAGE_SHIFT; > + end = tmb->end_pfn << PAGE_SHIFT; > + More alignment opportunities: start = tmb->start_pfn << PAGE_SHIFT; end = tmb->end_pfn << PAGE_SHIFT; > + /* Break if this region is after the TDMR */ > + if (start >= tdmr_end(tdmr)) > + break; > + > + /* Exclude regions before this TDMR */ > + if (end < tdmr_start(tdmr)) > + continue; > + > + /* > + * Skip if no hole exists before this region. "<=" is > + * used because one memory region might span two TDMRs > + * (when the previous TDMR covers part of this region). > + * In this case the start address of this region is > + * smaller than the start address of the second TDMR. > + * > + * Update the prev_end to the end of this region where > + * the possible memory hole starts. > + */ Can't this just be: /* * Skip over memory areas that * have already been dealt with. */ > + if (start <= prev_end) { > + prev_end = end; > + continue; > + } > + > + /* Add the hole before this region */ > + ret = tdmr_add_rsvd_area(tdmr, rsvd_idx, prev_end, > + start - prev_end); > + if (ret) > + return ret; > + > + prev_end = end; > + } > + > + /* Add the hole after the last region if it exists. */ > + if (prev_end < tdmr_end(tdmr)) { > + ret = tdmr_add_rsvd_area(tdmr, rsvd_idx, prev_end, > + tdmr_end(tdmr) - prev_end); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static int tdmr_set_up_pamt_rsvd_areas(struct tdmr_info *tdmr, int *rsvd_idx, > + struct tdmr_info *tdmr_array, > + int tdmr_num) > +{ > + int i, ret; > + > + /* > + * If any PAMT overlaps with this TDMR, the overlapping part > + * must also be put to the reserved area too. Walk over all > + * TDMRs to find out those overlapping PAMTs and put them to > + * reserved areas. > + */ > + for (i = 0; i < tdmr_num; i++) { > + struct tdmr_info *tmp = tdmr_array_entry(tdmr_array, i); > + unsigned long pamt_start_pfn, pamt_npages; > + u64 pamt_start, pamt_end; > + > + tdmr_get_pamt(tmp, &pamt_start_pfn, &pamt_npages); > + /* Each TDMR must already have PAMT allocated */ > + WARN_ON_ONCE(!pamt_npages || !pamt_start_pfn); > + > + pamt_start = pamt_start_pfn << PAGE_SHIFT; > + pamt_end = pamt_start + (pamt_npages << PAGE_SHIFT); > + > + /* Skip PAMTs outside of the given TDMR */ > + if ((pamt_end <= tdmr_start(tdmr)) || > + (pamt_start >= tdmr_end(tdmr))) > + continue; > + > + /* Only mark the part within the TDMR as reserved */ > + if (pamt_start < tdmr_start(tdmr)) > + pamt_start = tdmr_start(tdmr); > + if (pamt_end > tdmr_end(tdmr)) > + pamt_end = tdmr_end(tdmr); > + > + ret = tdmr_add_rsvd_area(tdmr, rsvd_idx, pamt_start, > + pamt_end - pamt_start); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +/* Compare function called by sort() for TDMR reserved areas */ > +static int rsvd_area_cmp_func(const void *a, const void *b) > +{ > + struct tdmr_reserved_area *r1 = (struct tdmr_reserved_area *)a; > + struct tdmr_reserved_area *r2 = (struct tdmr_reserved_area *)b; > + > + if (r1->offset + r1->size <= r2->offset) > + return -1; > + if (r1->offset >= r2->offset + r2->size) > + return 1; > + > + /* Reserved areas cannot overlap. The caller should guarantee. */ > + WARN_ON_ONCE(1); > + return -1; > +} > + > +/* Set up reserved areas for a TDMR, including memory holes and PAMTs */ > +static int tdmr_set_up_rsvd_areas(struct tdmr_info *tdmr, > + struct tdmr_info *tdmr_array, > + int tdmr_num) > +{ > + int ret, rsvd_idx = 0; > + > + /* Put all memory holes within the TDMR into reserved areas */ > + ret = tdmr_set_up_memory_hole_rsvd_areas(tdmr, &rsvd_idx); > + if (ret) > + return ret; > + > + /* Put all (overlapping) PAMTs within the TDMR into reserved areas */ > + ret = tdmr_set_up_pamt_rsvd_areas(tdmr, &rsvd_idx, tdmr_array, tdmr_num); > + if (ret) > + return ret; > + > + /* TDX requires reserved areas listed in address ascending order */ > + sort(tdmr->reserved_areas, rsvd_idx, sizeof(struct tdmr_reserved_area), > + rsvd_area_cmp_func, NULL); Ugh, and I guess we don't know where the PAMTs will be ordered with respect to holes, so sorting is the easiest way to do this.