[v7,14/20] x86/virt/tdx: Set up reserved areas for all TDMRs

Commit Message

Huang, Kai Nov. 21, 2022, 12:26 a.m. UTC

As the last step of constructing TDMRs, set up reserved areas for all
TDMRs.  For each TDMR, put all memory holes within this TDMR to the
reserved areas.  And for all PAMTs which overlap with this TDMR, put
all the overlapping parts to reserved areas too.

Reviewed-by: Isaku Yamahata <[e-mail]>
Signed-off-by: Kai Huang <[e-mail]>
---

v6 -> v7:
 - No change.

v5 -> v6:
 - Rebase due to using 'tdx_memblock' instead of memblock.
 - Split tdmr_set_up_rsvd_areas() into two functions to handle memory
   hole and PAMT respectively.
 - Added Isaku's Reviewed-by.


---
 arch/x86/virt/vmx/tdx/tdx.c | 190 +++++++++++++++++++++++++++++++++++-
 1 file changed, 188 insertions(+), 2 deletions(-)

Comments

Dave Hansen Nov. 23, 2022, 11:39 p.m. UTC | #1

> +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.

<snip>

Huang, Kai Nov. 28, 2022, 9:14 a.m. UTC | #2

On Wed, 2022-11-23 at 15:39 -0800, Dave Hansen wrote:
> > +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?

It is for one TDMR but not an array.  All functions in form of tdmr_xxx() are
operations for one TDMR.

But I agree it's not clear.

> 
> /*
>  * 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)

Thanks!

Should I also change below function 'tdmr_set_up_pamt_rsvd_areas()' to, i.e.
tdmr_populate_rsvd_pamts()?

Actually, there are two more functions in this patch: tdmr_set_up_rsvd_areas()
and tdmrs_set_up_rsvd_areas_all().  Should I also change them to
tdmr_populate_rsvd_areas() and tdmrs_populate_rsvd_areas_all()? 

> 
> > +	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.

Yes.

> 
> 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);

Right.   Sorry for the bad comment.

> 
> 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;

Should I use PFN_PHYS()?  Then looks we don't need this alignment.

> 
> 
> > +		/* 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.
> 		 */

I think so.  This actually also covers the "two contiguous memory regions" case,
which isn't mentioned in the original comment.

> 
> > +		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.
> 
> <snip>

Right.

Dave Hansen Nov. 28, 2022, 1:18 p.m. UTC | #3

On 11/28/22 01:14, Huang, Kai wrote:
> On Wed, 2022-11-23 at 15:39 -0800, Dave Hansen wrote:
...
>> /*
>>  * 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)
> 
> Thanks!
> 
> Should I also change below function 'tdmr_set_up_pamt_rsvd_areas()' to, i.e.
> tdmr_populate_rsvd_pamts()?
> 
> Actually, there are two more functions in this patch: tdmr_set_up_rsvd_areas()
> and tdmrs_set_up_rsvd_areas_all().  Should I also change them to
> tdmr_populate_rsvd_areas() and tdmrs_populate_rsvd_areas_all()? 

I don't know.  I'll look at the naming again once I see it all together.

>> 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;
> 
> Should I use PFN_PHYS()?  Then looks we don't need this alignment.

Sure.

Huang, Kai Nov. 28, 2022, 10:24 p.m. UTC | #4

On Mon, 2022-11-28 at 05:18 -0800, Dave Hansen wrote:
> On 11/28/22 01:14, Huang, Kai wrote:
> > On Wed, 2022-11-23 at 15:39 -0800, Dave Hansen wrote:
> ...
> > > /*
> > >  * 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)
> > 
> > Thanks!
> > 
> > Should I also change below function 'tdmr_set_up_pamt_rsvd_areas()' to, i.e.
> > tdmr_populate_rsvd_pamts()?
> > 
> > Actually, there are two more functions in this patch: tdmr_set_up_rsvd_areas()
> > and tdmrs_set_up_rsvd_areas_all().  Should I also change them to
> > tdmr_populate_rsvd_areas() and tdmrs_populate_rsvd_areas_all()? 
> 
> I don't know.  I'll look at the naming again once I see it all together.

Sure. I'll only change the one you mentioned above and keep the others for the
next version.

Dave Hansen Nov. 28, 2022, 10:58 p.m. UTC | #5

On 11/28/22 14:24, Huang, Kai wrote:
>> I don't know.  I'll look at the naming again once I see it all together.
> Sure. I'll only change the one you mentioned above and keep the others for the
> next version.

The alternative is that you can look at what I suggested, try and learn
something from it, and try to apply that elsewhere in the series before
you post again.

Huang, Kai Nov. 28, 2022, 11:10 p.m. UTC | #6

On Mon, 2022-11-28 at 14:58 -0800, Hansen, Dave wrote:
> On 11/28/22 14:24, Huang, Kai wrote:
> > > I don't know.  I'll look at the naming again once I see it all together.
> > Sure. I'll only change the one you mentioned above and keep the others for the
> > next version.
> 
> The alternative is that you can look at what I suggested, try and learn
> something from it, and try to apply that elsewhere in the series before
> you post again.

Yeah surely will do.

Patch

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 9d76e70de46e..1fbf33f2f210 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -21,6 +21,7 @@ 
 #include <linux/memblock.h>
 #include <linux/minmax.h>
 #include <linux/sizes.h>
+#include <linux/sort.h>
 #include <asm/msr-index.h>
 #include <asm/msr.h>
 #include <asm/apic.h>
@@ -767,6 +768,187 @@  static unsigned long tdmrs_count_pamt_pages(struct tdmr_info *tdmr_array,
 	return pamt_npages;
 }
 
+static int tdmr_add_rsvd_area(struct tdmr_info *tdmr, int *p_idx,
+			      u64 addr, u64 size)
+{
+	struct tdmr_reserved_area *rsvd_areas = tdmr->reserved_areas;
+	int idx = *p_idx;
+
+	/* Reserved area must be 4K aligned in offset and size */
+	if (WARN_ON(addr & ~PAGE_MASK || size & ~PAGE_MASK))
+		return -EINVAL;
+
+	/* Cannot exceed maximum reserved areas supported by TDX */
+	if (idx >= tdx_sysinfo.max_reserved_per_tdmr)
+		return -E2BIG;
+
+	rsvd_areas[idx].offset = addr - tdmr->base;
+	rsvd_areas[idx].size = size;
+
+	*p_idx = idx + 1;
+
+	return 0;
+}
+
+static int tdmr_set_up_memory_hole_rsvd_areas(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);
+	list_for_each_entry(tmb, &tdx_memlist, list) {
+		u64 start, end;
+
+		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.
+		 */
+		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);
+
+	return 0;
+}
+
+static int tdmrs_set_up_rsvd_areas_all(struct tdmr_info *tdmr_array,
+				       int tdmr_num)
+{
+	int i;
+
+	for (i = 0; i < tdmr_num; i++) {
+		int ret;
+
+		ret = tdmr_set_up_rsvd_areas(tdmr_array_entry(tdmr_array, i),
+				tdmr_array, tdmr_num);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /*
  * Construct an array of TDMRs to cover all TDX memory ranges.
  * The actual number of TDMRs is kept to @tdmr_num.
@@ -783,8 +965,12 @@  static int construct_tdmrs(struct tdmr_info *tdmr_array, int *tdmr_num)
 	if (ret)
 		goto err;
 
-	/* Return -EINVAL until constructing TDMRs is done */
-	ret = -EINVAL;
+	ret = tdmrs_set_up_rsvd_areas_all(tdmr_array, *tdmr_num);
+	if (ret)
+		goto err_free_pamts;
+
+	return 0;
+err_free_pamts:
 	tdmrs_free_pamt_all(tdmr_array, *tdmr_num);
 err:
 	return ret;