Re: [PATCH v2 4/4] xen: add helpers to allocate unpopulated memory

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 7/24/20 10:34 AM, David Hildenbrand wrote:
> CCing Dan
>
> On 24.07.20 14:42, Roger Pau Monne wrote:
>> diff --git a/drivers/xen/unpopulated-alloc.c 
>> b/drivers/xen/unpopulated-alloc.c
>> new file mode 100644
>> index 000000000000..aaa91cefbbf9
>> --- /dev/null
>> +++ b/drivers/xen/unpopulated-alloc.c
>> @@ -0,0 +1,222 @@



>> + */
>> +
>> +#include <linux/errno.h>
>> +#include <linux/gfp.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mm.h>
>> +#include <linux/memremap.h>
>> +#include <linux/slab.h>
>> +
>> +#include <asm/page.h>
>> +
>> +#include <xen/page.h>
>> +#include <xen/xen.h>
>> +
>> +static DEFINE_MUTEX(lock);
>> +static LIST_HEAD(list);
>> +static unsigned int count;
>> +
>> +static int fill(unsigned int nr_pages)


Less generic names? How about  list_lock, pg_list, pg_count,
fill_pglist()? (But these are bad too, so maybe you can come up with
something better)


>> +{
>> +    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_DEVDAX;
>> +    pgmap->res.name = "XEN SCRATCH";


Typically iomem resources only capitalize first letters.


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


Are we not going to end up with a whole bunch of "Xen scratch" resource
ranges for each miss in the page list? Or do we expect them to get merged?


>> +    if (ret < 0) {
>> +            pr_err("Cannot allocate new IOMEM resource\n");
>> +            kfree(pgmap);
>> +            return ret;
>> +    }
>> +
>> +    nid = memory_add_physaddr_to_nid(pgmap->res.start);


Should we consider page range crossing node boundaries?


>> +
>> +#ifdef CONFIG_XEN_HAVE_PVMMU
>> +    /*
>> +     * We don't support PV MMU when Linux and Xen is using
>> +     * different page granularity.
>> +     */
>> +    BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);
>> +
>> +        /*
>> +         * 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);
>> +    if (IS_ERR(vaddr)) {
>> +            pr_err("Cannot remap memory range\n");
>> +            release_resource(&pgmap->res);
>> +            kfree(pgmap);
>> +            return PTR_ERR(vaddr);
>> +    }
>> +
>> +    for (i = 0; i < alloc_pages; i++) {
>> +            struct page *pg = virt_to_page(vaddr + PAGE_SIZE * i);
>> +
>> +            BUG_ON(!virt_addr_valid(vaddr + PAGE_SIZE * i));
>> +            list_add(&pg->lru, &list);
>> +            count++;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/**
>> + * xen_alloc_unpopulated_pages - alloc unpopulated pages
>> + * @nr_pages: Number of pages
>> + * @pages: pages returned
>> + * @return 0 on success, error otherwise
>> + */
>> +int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages)
>> +{
>> +    unsigned int i;
>> +    int ret = 0;
>> +
>> +    mutex_lock(&lock);
>> +    if (count < nr_pages) {
>> +            ret = fill(nr_pages);


(nr_pages - count) ?


>> +            if (ret)
>> +                    goto out;
>> +    }
>> +
>> +    for (i = 0; i < nr_pages; i++) {
>> +            struct page *pg = list_first_entry_or_null(&list, struct page,
>> +                                                       lru);
>> +
>> +            BUG_ON(!pg);
>> +            list_del(&pg->lru);
>> +            count--;
>> +            pages[i] = pg;
>> +
>> +#ifdef CONFIG_XEN_HAVE_PVMMU
>> +            /*
>> +             * We don't support PV MMU when Linux and Xen is using
>> +             * different page granularity.
>> +             */
>> +            BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);
>> +
>> +            if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>> +                    ret = xen_alloc_p2m_entry(page_to_pfn(pg));
>> +                    if (ret < 0) {
>> +                            unsigned int j;
>> +
>> +                            for (j = 0; j <= i; j++) {
>> +                                    list_add(&pages[j]->lru, &list);
>> +                                    count++;
>> +                            }
>> +                            goto out;
>> +                    }
>> +            }
>> +#endif
>> +    }
>> +
>> +out:
>> +    mutex_unlock(&lock);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL(xen_alloc_unpopulated_pages);
>> +



>> +
>> +#ifdef CONFIG_XEN_PV
>> +static int __init init(void)
>> +{
>> +    unsigned int i;
>> +
>> +    if (!xen_domain())
>> +            return -ENODEV;
>> +
>> +    /*
>> +     * Initialize with pages from the extra memory regions (see
>> +     * arch/x86/xen/setup.c).
>> +     */


This loop will be executing only for PV guests so we can just bail out
for non-PV guests here.


-boris


>> +    for (i = 0; i < XEN_EXTRA_MEM_MAX_REGIONS; i++) {
>> +            unsigned int j;
>> +
>> +            for (j = 0; j < xen_extra_mem[i].n_pfns; j++) {
>> +                    struct page *pg =
>> +                            pfn_to_page(xen_extra_mem[i].start_pfn + j);
>> +
>> +                    list_add(&pg->lru, &list);
>> +                    count++;
>> +            }
>> +    }
>> +
>> +    return 0;
>> +}
>> +subsys_initcall(init);