Re: [PATCH v5 3/3] xen: add helpers to allocate unpopulated memory

[Jürgen Groß]

On 01.09.20 16:45, Roger Pau Monné wrote:
> On Tue, Sep 01, 2020 at 10:33:26AM +0200, Roger Pau Monne wrote:
> > +static int fill_list(unsigned int nr_pages)
> > +{
> > +	struct dev_pagemap *pgmap;
> > +	void *vaddr;
> > +	unsigned int i, alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
> > +	int nid, ret;
> > +
> > +	pgmap = kzalloc(sizeof(*pgmap), GFP_KERNEL);
> > +	if (!pgmap)
> > +		return -ENOMEM;
> > +
> > +	pgmap->type = MEMORY_DEVICE_GENERIC;
> > +	pgmap->res.name = "Xen scratch";
> > +	pgmap->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> > +
> > +	ret = allocate_resource(&iomem_resource, &pgmap->res,
> > +				alloc_pages * PAGE_SIZE, 0, -1,
> > +				PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
> > +	if (ret < 0) {
> > +		pr_err("Cannot allocate new IOMEM resource\n");
> > +		kfree(pgmap);
> > +		return ret;
> > +	}
> > +
> > +	nid = memory_add_physaddr_to_nid(pgmap->res.start);
>
> I think this is not needed ...
>
> > +
> > +#ifdef CONFIG_XEN_HAVE_PVMMU
> > +        /*
> > +         * memremap will build page tables for the new memory so
> > +         * the p2m must contain invalid entries so the correct
> > +         * non-present PTEs will be written.
> > +         *
> > +         * If a failure occurs, the original (identity) p2m entries
> > +         * are not restored since this region is now known not to
> > +         * conflict with any devices.
> > +         */
> > +	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> > +		xen_pfn_t pfn = PFN_DOWN(pgmap->res.start);
> > +
> > +		for (i = 0; i < alloc_pages; i++) {
> > +			if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) {
> > +				pr_warn("set_phys_to_machine() failed, no memory added\n");
> > +				release_resource(&pgmap->res);
> > +				kfree(pgmap);
> > +				return -ENOMEM;
> > +			}
> > +                }
> > +	}
> > +#endif
> > +
> > +	vaddr = memremap_pages(pgmap, nid);
>
> ... and NUMA_NO_NODE should be used here instead, as this memory is just
> fictitious space to map foreign memory, and shouldn't be related to
> any NUMA node.
>
> The following chunk should be folded in, or I can resend.

I can fold it in.


Juergen