Re: [PATCH v1 06/26] x86/virt/tdx: Add tdx_page_array helpers for new TDX Module objects

[Xu Yilun <yilun.xu@linux.intel.com>]

> > +static bool tdx_page_array_validate_release(struct tdx_page_array *array,
> > +					    unsigned int offset,
> > +					    unsigned int nr_released,
> > +					    u64 released_hpa)
> > +{
> > +	unsigned int nents;
> > +	u64 *entries;
> > +	int i;
> > +
> > +	if (offset >= array->nr_pages)
> > +		return false;
> > +
> > +	nents = umin(array->nr_pages - offset, TDX_PAGE_ARRAY_MAX_NENTS);
> > +
> > +	if (nents != nr_released) {
> > +		pr_err("%s nr_released [%d] doesn't match page array nents [%d]\n",
> > +		       __func__, nr_released, nents);
> > +		return false;
> > +	}
> > +
> > +	/*
> > +	 * Unfortunately TDX has multiple page allocation protocols, check the
> > +	 * "singleton" case required for HPA_ARRAY_T.
> > +	 */
> > +	if (page_to_phys(array->pages[0]) == released_hpa &&
> > +	    array->nr_pages == 1)
> > +		return true;
> > +
> > +	/* Then check the "non-singleton" case */
> > +	if (page_to_phys(array->root) == released_hpa) {
> > +		entries = (u64 *)page_address(array->root);
> 
> page_address() returns a void * so the cast here isn't needed and (to me
> at least) doesn't add value from readability point of view.

These casts disappear during the refactoring from alloc_page() to
kzalloc(PAGE_SIZE, ...) for my v2.

> 
> I haven't checked later patches, but if this code doesn't change to use
> entries outside this scope then,
> 		u64 *entries = page_address(array->root);
> would be nice to restrict the scope and make the type here immediately
> visible.

Yes. Also for int i.

Thanks,
Yilun

> 
> > +		for (i = 0; i < nents; i++) {
> > +			struct page *page = array->pages[offset + i];
> > +			u64 val = page_to_phys(page);
> > +
> > +			if (val != entries[i]) {
> > +				pr_err("%s entry[%d] [0x%llx] doesn't match page hpa [0x%llx]\n",
> > +				       __func__, i, entries[i], val);
> > +				return false;
> > +			}
> > +		}
> > +
> > +		return true;
> > +	}
> > +
> > +	pr_err("%s failed to validate, released_hpa [0x%llx], root page hpa [0x%llx], page0 hpa [%#llx], number pages %u\n",
> > +	       __func__, released_hpa, page_to_phys(array->root),
> > +	       page_to_phys(array->pages[0]), array->nr_pages);
> > +
> > +	return false;
> > +}
> 
>