Re: [PATCH v5 09/16] kexec: enable KHO support for memory preservation

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Mar 19, 2025 at 06:55:44PM -0700, Changyuan Lyu wrote:
> +/**
> + * kho_preserve_folio - preserve a folio across KHO.
> + * @folio: folio to preserve
> + *
> + * Records that the entire folio is preserved across KHO. The order
> + * will be preserved as well.
> + *
> + * Return: 0 on success, error code on failure
> + */
> +int kho_preserve_folio(struct folio *folio)
> +{
> +	unsigned long pfn = folio_pfn(folio);
> +	unsigned int order = folio_order(folio);
> +	int err;
> +
> +	if (!kho_enable)
> +		return -EOPNOTSUPP;
> +
> +	down_read(&kho_out.tree_lock);
> +	if (kho_out.fdt) {

What is the lock and fdt test for?

I'm getting the feeling that probably kho_preserve_folio() and the
like should accept some kind of 
'struct kho_serialization *' and then we don't need this to prove we
are within a valid serialization window. It could pass the pointer
through the notifiers

The global variables in this series are sort of ugly..

We want this to be fast, so try hard to avoid a lock..

> +void *kho_restore_phys(phys_addr_t phys, size_t size)
> +{
> +	unsigned long start_pfn, end_pfn, pfn;
> +	void *va = __va(phys);
> +
> +	start_pfn = PFN_DOWN(phys);
> +	end_pfn = PFN_UP(phys + size);
> +
> +	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> +		struct page *page = pfn_to_online_page(pfn);
> +
> +		if (!page)
> +			return NULL;
> +		kho_restore_page(page);
> +	}
> +
> +	return va;
> +}
> +EXPORT_SYMBOL_GPL(kho_restore_phys);

What do you imagine this is used for? I'm not sure what value there is
in returning a void *? How does the caller "free" this?


> +#define KHOSER_PTR(type)          \
> +	union {                   \
> +		phys_addr_t phys; \
> +		type ptr;         \
> +	}
> +#define KHOSER_STORE_PTR(dest, val)                 \
> +	({                                          \
> +		(dest).phys = virt_to_phys(val);    \
> +		typecheck(typeof((dest).ptr), val); \
> +	})
> +#define KHOSER_LOAD_PTR(src) \
> +	((src).phys ? (typeof((src).ptr))(phys_to_virt((src).phys)) : NULL)

I had imagined these macros would be in a header and usably by drivers
that also want to use structs to carry information.

> +static void deserialize_bitmap(unsigned int order,
> +			       struct khoser_mem_bitmap_ptr *elm)
> +{
> +	struct kho_mem_phys_bits *bitmap = KHOSER_LOAD_PTR(elm->bitmap);
> +	unsigned long bit;
> +
> +	for_each_set_bit(bit, bitmap->preserve, PRESERVE_BITS) {
> +		int sz = 1 << (order + PAGE_SHIFT);
> +		phys_addr_t phys =
> +			elm->phys_start + (bit << (order + PAGE_SHIFT));
> +		struct page *page = phys_to_page(phys);
> +
> +		memblock_reserve(phys, sz);
> +		memblock_reserved_mark_noinit(phys, sz);

Mike asked about this earlier, is it work combining runs of set bits
to increase sz? Or is this sort of temporary pending something better
that doesn't rely on memblock_reserve?

> +		page->private = order;

Can't just set the page order directly? Why use private?

> @@ -829,6 +1305,10 @@ static __init int kho_init(void)
>  
>  	kho_out.root.name = "";

?

>  	err = kho_add_string_prop(&kho_out.root, "compatible", "kho-v1");
> +	err |= kho_add_prop(&kho_out.preserved_memory, "metadata",
> +			    &kho_out.first_chunk_phys, sizeof(phys_addr_t));

metedata doesn't fee like a great a better name..

Please also document all the FDT schema thoroughly!

There should be yaml files just like in the normal DT case defining
all of this. This level of documentation and stability was one of the
selling reasons why FDT is being used here!

Jason