[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c6i6lttkkeupbyfwy42byin7ccxh6rwznvgwsyjmvzeb5rbblv@ge3agfvfyn4e>
Date: Fri, 27 Jun 2025 14:35:30 +0300
From: "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>
To: "Huang, Kai" <kai.huang@...el.com>
Cc: "pbonzini@...hat.com" <pbonzini@...hat.com>,
"seanjc@...gle.com" <seanjc@...gle.com>, "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"Gao, Chao" <chao.gao@...el.com>, "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
"bp@...en8.de" <bp@...en8.de>, "x86@...nel.org" <x86@...nel.org>,
"mingo@...hat.com" <mingo@...hat.com>, "Zhao, Yan Y" <yan.y.zhao@...el.com>,
"tglx@...utronix.de" <tglx@...utronix.de>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>, "Yamahata, Isaku" <isaku.yamahata@...el.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv2 03/12] x86/virt/tdx: Allocate reference counters for
PAMT memory
On Thu, Jun 26, 2025 at 12:53:29AM +0000, Huang, Kai wrote:
>
> > +static int init_pamt_metadata(void)
> > +{
> > + size_t size = max_pfn / PTRS_PER_PTE * sizeof(*pamt_refcounts);
> > + struct vm_struct *area;
> > +
> > + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> > + return 0;
> > +
> > + /*
> > + * Reserve vmalloc range for PAMT reference counters. It covers all
> > + * physical address space up to max_pfn. It is going to be populated
> > + * from init_tdmr() only for present memory that available for TDX use.
> ^
> build_tdx_memlist()
Ack.
>
> > + */
> > + area = get_vm_area(size, VM_IOREMAP);
>
> I am not sure why VM_IOREMAP is used?
It follows vmap_pfn() pattern as usage is similar.
It seems the flag allows vread_iter() to work correct on sparse mappings.
> > + if (!area)
> > + return -ENOMEM;
> > +
> > + pamt_refcounts = area->addr;
> > + return 0;
> > +}
> > +
> > +static void free_pamt_metadata(void)
> > +{
> > + size_t size = max_pfn / PTRS_PER_PTE * sizeof(*pamt_refcounts);
> > +
> > + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> > + return;
> > +
> > + size = round_up(size, PAGE_SIZE);
> > + apply_to_existing_page_range(&init_mm,
> > + (unsigned long)pamt_refcounts,
> > + size, pamt_refcount_depopulate,
> > + NULL);
> > + vfree(pamt_refcounts);
> > + pamt_refcounts = NULL;
> > +}
> > +
> > /*
> > * Add a memory region as a TDX memory block. The caller must make sure
> > * all memory regions are added in address ascending order and don't
> > @@ -248,6 +347,10 @@ static int build_tdx_memlist(struct list_head *tmb_list)
> > ret = add_tdx_memblock(tmb_list, start_pfn, end_pfn, nid);
> > if (ret)
> > goto err;
> > +
> > + ret = alloc_pamt_refcount(start_pfn, end_pfn);
> > + if (ret)
> > + goto err;
>
> So this would goto the error path, which only calls free_tdx_memlist(),
> which frees all existing TDX memory blocks that have already created.
>
> Logically, it would be great to also free PAMT refcount pages too, but they
> all can be freed at free_pamt_metadata() eventually, so it's OK.
>
> But I think it would still be helpful to put a comment before
> free_tdx_memlist() in the error path to call out. Something like:
>
> err:
> /*
> * This only frees all TDX memory blocks that have been created.
> * All PAMT refcount pages will be freed when init_tdx_module()
> * calls free_pamt_metadata() eventually.
> */
> free_tdx_memlist(tmb_list);
> return ret;
Okay.
--
Kiryl Shutsemau / Kirill A. Shutemov
Powered by blists - more mailing lists