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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6bd01b60-2625-c46e-f9ff-95247700a8cc@redhat.com>
Date:   Mon, 27 Jul 2020 10:00:44 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Roger Pau Monne <roger.pau@...rix.com>,
        linux-kernel@...r.kernel.org
Cc:     Juergen Gross <jgross@...e.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Wei Liu <wl@....org>,
        Oleksandr Andrushchenko <oleksandr_andrushchenko@...m.com>,
        David Airlie <airlied@...ux.ie>,
        Yan Yankovskyi <yyankovskyi@...il.com>,
        dri-devel@...ts.freedesktop.org, Michal Hocko <mhocko@...nel.org>,
        linux-mm@...ck.org, Daniel Vetter <daniel@...ll.ch>,
        xen-devel@...ts.xenproject.org,
        Dan Williams <dan.j.williams@...el.com>,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH v2 4/4] xen: add helpers to allocate unpopulated memory

On 24.07.20 18:36, Boris Ostrovsky wrote:
> 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?
> 

AFAIK, no resources will get merged (and it's in the general case not
safe to do). The old approach (add_memory_resource()) will end up with
the same situation ("Xen Scratch" vs. "System RAM") one new resource per
added memory block/section.

FWIW, I am looking into merging selected resources in the context of
virtio-mem _after_ adding succeeded (not directly when adding the
resource to the tree). Interface might look something like

void merge_child_mem_resources(struct resource *parent, const char *name);

So I can, for example, trigger merging of all "System RAM (virtio_mem)"
resources, that are located under a device node (e.g., "virtio0").

I also thought about tagging each mergeable resource via something like
"IORESOURCE_MERGEABLE" - whereby the user agrees that it does not hold
any pointers to such a resource. But I don't see yet a copelling reason
to sacrifice space for a new flag.

So with this in place, this code could call once adding succeeded

merge_child_mem_resources(&iomem_resource, "Xen Scratch");

-- 
Thanks,

David / dhildenb