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

[Huang, Kai]

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.