[PATCH v3 0/3] virtio-mem: disallow mapping virtio-mem memory via /dev/mem

[PATCH v3 0/3] virtio-mem: disallow mapping virtio-mem memory via /dev/mem

[David Hildenbrand]

I think this might be a good fit for the -mm tree, as the actual virtio-mem
changes are rather small.

--

Let's add the basic infrastructure to exclude some physical memory
regions marked as "IORESOURCE_SYSTEM_RAM" completely from /dev/mem access,
even though they are not marked IORESOURCE_BUSY and even though
"iomem=relaxed" is set. Resource IORESOURCE_EXCLUSIVE for that purpose
instead of adding new flags to express something similar to
"soft-busy" or "not busy yet, but already prepared by a driver and not
to be mapped by user space".

Use it for virtio-mem, to disallow mapping any virtio-mem memory via
/dev/mem to user space after the virtio-mem driver was loaded.

Details can be found in patch #2 and #3.

v2 -> v3:
- "kernel/resource: clean up and optimize iomem_is_exclusive()"
-- Reshuffled and moved for_each_resource() etc. into this patch
- "kernel/resource: disallow access to exclusive system RAM regions"
-- Leave CONFIG_STRICT_DEVMEM=n alone. Hoog into iomem_is_exclusive()
   instead.
-- Improve comments
- "virtio-mem: disallow mapping virtio-mem memory via /dev/mem"
-- Don't allow building virtio_mem without CONFIG_STRICT_DEVMEM when we
   have CONFIG_DEVMEM, where we don't have any guarantees.
- Rework all patch descriptions

v1 -> v2:
- "/dev/mem: disallow access to explicitly excluded system RAM regions"
-- Introduce and use for_each_resource() and next_resource_skip_children()
-- s/iomem_range_contains_excluded/iomem_range_contains_excluded_devmem/
- "kernel/resource: cleanup and optimize iomem_is_exclusive()"
-- Use for_each_resource()

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Hanjun Guo <guohanjun@huawei.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: virtualization@lists.linux-foundation.org
Cc: linux-mm@kvack.org

David Hildenbrand (3):
  kernel/resource: clean up and optimize iomem_is_exclusive()
  kernel/resource: disallow access to exclusive system RAM regions
  virtio-mem: disallow mapping virtio-mem memory via /dev/mem

 drivers/virtio/Kconfig      |  1 +
 drivers/virtio/virtio_mem.c |  4 ++-
 kernel/resource.c           | 54 ++++++++++++++++++++++++++-----------
 3 files changed, 43 insertions(+), 16 deletions(-)


base-commit: 7d2a07b769330c34b4deabeed939325c77a7ec2f
-- 
2.31.1

[PATCH v3 0/3] virtio-mem: disallow mapping virtio-mem memory via /dev/mem

[David Hildenbrand]

I think this might be a good fit for the -mm tree, as the actual virtio-mem
changes are rather small.

--

Let's add the basic infrastructure to exclude some physical memory
regions marked as "IORESOURCE_SYSTEM_RAM" completely from /dev/mem access,
even though they are not marked IORESOURCE_BUSY and even though
"iomem=relaxed" is set. Resource IORESOURCE_EXCLUSIVE for that purpose
instead of adding new flags to express something similar to
"soft-busy" or "not busy yet, but already prepared by a driver and not
to be mapped by user space".

Use it for virtio-mem, to disallow mapping any virtio-mem memory via
/dev/mem to user space after the virtio-mem driver was loaded.

Details can be found in patch #2 and #3.

v2 -> v3:
- "kernel/resource: clean up and optimize iomem_is_exclusive()"
-- Reshuffled and moved for_each_resource() etc. into this patch
- "kernel/resource: disallow access to exclusive system RAM regions"
-- Leave CONFIG_STRICT_DEVMEM=n alone. Hoog into iomem_is_exclusive()
   instead.
-- Improve comments
- "virtio-mem: disallow mapping virtio-mem memory via /dev/mem"
-- Don't allow building virtio_mem without CONFIG_STRICT_DEVMEM when we
   have CONFIG_DEVMEM, where we don't have any guarantees.
- Rework all patch descriptions

v1 -> v2:
- "/dev/mem: disallow access to explicitly excluded system RAM regions"
-- Introduce and use for_each_resource() and next_resource_skip_children()
-- s/iomem_range_contains_excluded/iomem_range_contains_excluded_devmem/
- "kernel/resource: cleanup and optimize iomem_is_exclusive()"
-- Use for_each_resource()

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Hanjun Guo <guohanjun@huawei.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: virtualization@lists.linux-foundation.org
Cc: linux-mm@kvack.org

David Hildenbrand (3):
  kernel/resource: clean up and optimize iomem_is_exclusive()
  kernel/resource: disallow access to exclusive system RAM regions
  virtio-mem: disallow mapping virtio-mem memory via /dev/mem

 drivers/virtio/Kconfig      |  1 +
 drivers/virtio/virtio_mem.c |  4 ++-
 kernel/resource.c           | 54 ++++++++++++++++++++++++++-----------
 3 files changed, 43 insertions(+), 16 deletions(-)


base-commit: 7d2a07b769330c34b4deabeed939325c77a7ec2f
-- 
2.31.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 1/3] kernel/resource: clean up and optimize iomem_is_exclusive()

[David Hildenbrand]

We end up traversing subtrees of ranges we are not interested in; let's
optimize this case, skipping such subtrees, cleaning up the function a bit.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 kernel/resource.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index ca9f5198a01f..2999f57da38c 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -73,6 +73,18 @@ static struct resource *next_resource(struct resource *p)
 	return p->sibling;
 }
 
+static struct resource *next_resource_skip_children(struct resource *p)
+{
+	while (!p->sibling && p->parent)
+		p = p->parent;
+	return p->sibling;
+}
+
+#define for_each_resource(_root, _p, _skip_children) \
+	for ((_p) = (_root)->child; (_p); \
+	     (_p) = (_skip_children) ? next_resource_skip_children(_p) : \
+				       next_resource(_p))
+
 static void *r_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	struct resource *p = v;
@@ -1712,10 +1724,9 @@ static int strict_iomem_checks;
  */
 bool iomem_is_exclusive(u64 addr)
 {
-	struct resource *p = &iomem_resource;
-	bool err = false;
-	loff_t l;
+	bool skip_children = false, err = false;
 	int size = PAGE_SIZE;
+	struct resource *p;
 
 	if (!strict_iomem_checks)
 		return false;
@@ -1723,15 +1734,19 @@ bool iomem_is_exclusive(u64 addr)
 	addr = addr & PAGE_MASK;
 
 	read_lock(&resource_lock);
-	for (p = p->child; p ; p = r_next(NULL, p, &l)) {
+	for_each_resource(&iomem_resource, p, skip_children) {
 		/*
 		 * We can probably skip the resources without
 		 * IORESOURCE_IO attribute?
 		 */
 		if (p->start >= addr + size)
 			break;
-		if (p->end < addr)
+		if (p->end < addr) {
+			skip_children = true;
 			continue;
+		}
+		skip_children = false;
+
 		/*
 		 * A resource is exclusive if IORESOURCE_EXCLUSIVE is set
 		 * or CONFIG_IO_STRICT_DEVMEM is enabled and the
-- 
2.31.1

[PATCH v3 2/3] kernel/resource: disallow access to exclusive system RAM regions

[David Hildenbrand]

virtio-mem dynamically exposes memory inside a device memory region as
system RAM to Linux, coordinating with the hypervisor which parts are
actually "plugged" and consequently usable/accessible. On the one hand, the
virtio-mem driver adds/removes whole memory blocks, creating/removing busy
IORESOURCE_SYSTEM_RAM resources, on the other hand, it logically (un)plugs
memory inside added memory blocks, dynamically either exposing them to
the buddy or hiding them from the buddy and marking them PG_offline.

In contrast to physical devices, like a DIMM, the virtio-mem driver
is required to actually make use of any of the device-provided memory,
because it performs the handshake with the hypervisor. virtio-mem memory
cannot simply be access via /dev/mem without a driver.

There is no safe way to:
a) Access plugged memory blocks via /dev/mem, as they might contain
   unplugged holes or might get silently unplugged by the virtio-mem
   driver and consequently turned inaccessible.
b) Access unplugged memory blocks via /dev/mem because the virtio-mem
   driver is required to make them actually accessible first.

The virtio-spec states that unplugged memory blocks MUST NOT be
written, and only selected unplugged memory blocks MAY be read. We want
to make sure, this is the case in sane environments -- where the
virtio-mem driver was loaded.

We want to make sure that in a sane environment, nobody "accidentially"
accesses unplugged memory inside the device managed region. For example,
a user might spot a memory region in /proc/iomem and try accessing it via
/dev/mem via gdb or dumping it via something else. By the time the mmap()
happens, the memory might already have been removed by the virtio-mem
driver silently: the mmap() would succeeed and user space might
accidentially access unplugged memory.

So once the driver was loaded and detected the device along the
device-managed region, we just want to disallow any access via
/dev/mem to it.

In an ideal world, we would mark the whole region as busy ("owned by a
driver") and exclude it; however, that would be wrong, as we don't
really have actual system RAM at these ranges added to Linux ("busy system
RAM"). Instead, we want to mark such ranges as "not actual busy system RAM
but still soft-reserved and prepared by a driver for future use."

Let's teach iomem_is_exclusive() to reject access to any range
with "IORESOURCE_SYSTEM_RAM | IORESOURCE_EXCLUSIVE", even if not busy
and even if "iomem=relaxed" is set.

For now, there are no applicable ranges and we'll modify virtio-mem next to
properly set IORESOURCE_EXCLUSIVE on the parent resource container it
creates to contain all actual busy system RAM added via
add_memory_driver_managed().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 kernel/resource.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 2999f57da38c..5ad3eba619ba 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1719,26 +1719,23 @@ static int strict_iomem_checks;
 #endif
 
 /*
- * check if an address is reserved in the iomem resource tree
- * returns true if reserved, false if not reserved.
+ * Check if an address is exclusive to the kernel and must not be mapped to
+ * user space, for example, via /dev/mem.
+ *
+ * Returns true if exclusive to the kernel, otherwise returns false.
  */
 bool iomem_is_exclusive(u64 addr)
 {
+	const unsigned int exclusive_system_ram = IORESOURCE_SYSTEM_RAM |
+						  IORESOURCE_EXCLUSIVE;
 	bool skip_children = false, err = false;
 	int size = PAGE_SIZE;
 	struct resource *p;
 
-	if (!strict_iomem_checks)
-		return false;
-
 	addr = addr & PAGE_MASK;
 
 	read_lock(&resource_lock);
 	for_each_resource(&iomem_resource, p, skip_children) {
-		/*
-		 * We can probably skip the resources without
-		 * IORESOURCE_IO attribute?
-		 */
 		if (p->start >= addr + size)
 			break;
 		if (p->end < addr) {
@@ -1747,12 +1744,24 @@ bool iomem_is_exclusive(u64 addr)
 		}
 		skip_children = false;
 
+		/*
+		 * IORESOURCE_SYSTEM_RAM resources are exclusive if
+		 * IORESOURCE_EXCLUSIVE is set, even if they
+		 * are not busy and even if "iomem=relaxed" is set. The
+		 * responsible driver dynamically adds/removes system RAM within
+		 * such an area and uncontrolled access is dangerous.
+		 */
+		if ((p->flags & exclusive_system_ram) == exclusive_system_ram) {
+			err = true;
+			break;
+		}
+
 		/*
 		 * A resource is exclusive if IORESOURCE_EXCLUSIVE is set
 		 * or CONFIG_IO_STRICT_DEVMEM is enabled and the
 		 * resource is busy.
 		 */
-		if ((p->flags & IORESOURCE_BUSY) == 0)
+		if (!strict_iomem_checks || !(p->flags & IORESOURCE_BUSY))
 			continue;
 		if (IS_ENABLED(CONFIG_IO_STRICT_DEVMEM)
 				|| p->flags & IORESOURCE_EXCLUSIVE) {
-- 
2.31.1

[PATCH v3 3/3] virtio-mem: disallow mapping virtio-mem memory via /dev/mem

[David Hildenbrand]

We don't want user space to be able to map virtio-mem device memory
directly (e.g., via /dev/mem) in order to have guarantees that in a sane
setup we'll never accidentially access unplugged memory within the
device-managed region of a virtio-mem device, just as required by the
virtio-spec.

As soon as the virtio-mem driver is loaded, the device region is visible
in /proc/iomem via the parent device region. From that point on user space
is aware of the device region and we want to disallow mapping anything
inside that region (where we will dynamically (un)plug memory) until
the driver has been unloaded cleanly and e.g., another driver might take
over.

By creating our parent IORESOURCE_SYSTEM_RAM resource with
IORESOURCE_EXCLUSIVE, we will disallow any /dev/mem access to our
device region until the driver was unloaded cleanly and removed the
parent region. This will work even though only some memory blocks are
actually currently added to Linux and appear as busy in the resource tree.

So access to the region from user space is only possible
a) if we don't load the virtio-mem driver.
b) after unloading the virtio-mem driver cleanly.

Don't build virtio-mem if access to /dev/mem cannot be restricticted --
if we have CONFIG_DEVMEM=y but CONFIG_STRICT_DEVMEM is not set.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 drivers/virtio/Kconfig      | 1 +
 drivers/virtio/virtio_mem.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index ce1b3f6ec325..ff80cd03f1d1 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -101,6 +101,7 @@ config VIRTIO_MEM
 	depends on MEMORY_HOTPLUG_SPARSE
 	depends on MEMORY_HOTREMOVE
 	depends on CONTIG_ALLOC
+	depends on !DEVMEM || STRICT_DEVMEM
 	help
 	 This driver provides access to virtio-mem paravirtualized memory
 	 devices, allowing to hotplug and hotunplug memory.
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index b91bc810a87e..c2d93492cf0f 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -2523,8 +2523,10 @@ static int virtio_mem_create_resource(struct virtio_mem *vm)
 	if (!name)
 		return -ENOMEM;
 
+	/* Disallow mapping device memory via /dev/mem completely. */
 	vm->parent_resource = __request_mem_region(vm->addr, vm->region_size,
-						   name, IORESOURCE_SYSTEM_RAM);
+						   name, IORESOURCE_SYSTEM_RAM |
+						   IORESOURCE_EXCLUSIVE);
 	if (!vm->parent_resource) {
 		kfree(name);
 		dev_warn(&vm->vdev->dev, "could not reserve device region\n");
-- 
2.31.1

Re: [PATCH v3 2/3] kernel/resource: disallow access to exclusive system RAM regions

[Williams, Dan J]

On Tue, 2021-08-31 at 22:21 +0200, David Hildenbrand wrote:
> virtio-mem dynamically exposes memory inside a device memory region as
> system RAM to Linux, coordinating with the hypervisor which parts are
> actually "plugged" and consequently usable/accessible. On the one hand, the
> virtio-mem driver adds/removes whole memory blocks, creating/removing busy
> IORESOURCE_SYSTEM_RAM resources, on the other hand, it logically (un)plugs
> memory inside added memory blocks, dynamically either exposing them to
> the buddy or hiding them from the buddy and marking them PG_offline.
> 
> In contrast to physical devices, like a DIMM, the virtio-mem driver
> is required to actually make use of any of the device-provided memory,
> because it performs the handshake with the hypervisor. virtio-mem memory
> cannot simply be access via /dev/mem without a driver.
> 
> There is no safe way to:
> a) Access plugged memory blocks via /dev/mem, as they might contain
>    unplugged holes or might get silently unplugged by the virtio-mem
>    driver and consequently turned inaccessible.
> b) Access unplugged memory blocks via /dev/mem because the virtio-mem
>    driver is required to make them actually accessible first.
> 
> The virtio-spec states that unplugged memory blocks MUST NOT be
> written, and only selected unplugged memory blocks MAY be read. We want
> to make sure, this is the case in sane environments -- where the
> virtio-mem driver was loaded.
> 
> We want to make sure that in a sane environment, nobody "accidentially"
> accesses unplugged memory inside the device managed region. For example,
> a user might spot a memory region in /proc/iomem and try accessing it via
> /dev/mem via gdb or dumping it via something else. By the time the mmap()
> happens, the memory might already have been removed by the virtio-mem
> driver silently: the mmap() would succeeed and user space might
> accidentially access unplugged memory.
> 
> So once the driver was loaded and detected the device along the
> device-managed region, we just want to disallow any access via
> /dev/mem to it.
> 
> In an ideal world, we would mark the whole region as busy ("owned by a
> driver") and exclude it; however, that would be wrong, as we don't
> really have actual system RAM at these ranges added to Linux ("busy system
> RAM"). Instead, we want to mark such ranges as "not actual busy system RAM
> but still soft-reserved and prepared by a driver for future use."
> 
> Let's teach iomem_is_exclusive() to reject access to any range
> with "IORESOURCE_SYSTEM_RAM | IORESOURCE_EXCLUSIVE", even if not busy
> and even if "iomem=relaxed" is set.
> 
> For now, there are no applicable ranges and we'll modify virtio-mem next to
> properly set IORESOURCE_EXCLUSIVE on the parent resource container it
> creates to contain all actual busy system RAM added via
> add_memory_driver_managed().

Looks good,

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH v3 3/3] virtio-mem: disallow mapping virtio-mem memory via /dev/mem

[Williams, Dan J]

On Tue, 2021-08-31 at 22:21 +0200, David Hildenbrand wrote:
> We don't want user space to be able to map virtio-mem device memory
> directly (e.g., via /dev/mem) in order to have guarantees that in a sane
> setup we'll never accidentially access unplugged memory within the
> device-managed region of a virtio-mem device, just as required by the
> virtio-spec.
> 
> As soon as the virtio-mem driver is loaded, the device region is visible
> in /proc/iomem via the parent device region. From that point on user space
> is aware of the device region and we want to disallow mapping anything
> inside that region (where we will dynamically (un)plug memory) until
> the driver has been unloaded cleanly and e.g., another driver might take
> over.
> 
> By creating our parent IORESOURCE_SYSTEM_RAM resource with
> IORESOURCE_EXCLUSIVE, we will disallow any /dev/mem access to our
> device region until the driver was unloaded cleanly and removed the
> parent region. This will work even though only some memory blocks are
> actually currently added to Linux and appear as busy in the resource tree.
> 
> So access to the region from user space is only possible
> a) if we don't load the virtio-mem driver.
> b) after unloading the virtio-mem driver cleanly.
> 
> Don't build virtio-mem if access to /dev/mem cannot be restricticted --
> if we have CONFIG_DEVMEM=y but CONFIG_STRICT_DEVMEM is not set.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  drivers/virtio/Kconfig      | 1 +
>  drivers/virtio/virtio_mem.c | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index ce1b3f6ec325..ff80cd03f1d1 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -101,6 +101,7 @@ config VIRTIO_MEM
>         depends on MEMORY_HOTPLUG_SPARSE
>         depends on MEMORY_HOTREMOVE
>         depends on CONTIG_ALLOC
> +       depends on !DEVMEM || STRICT_DEVMEM

Thanks for this. It keeps the special case out of the implementation
while not compromising integrity for VIRTIO_MEM.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

>         help
>          This driver provides access to virtio-mem paravirtualized memory
>          devices, allowing to hotplug and hotunplug memory.
> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index b91bc810a87e..c2d93492cf0f 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c
> @@ -2523,8 +2523,10 @@ static int virtio_mem_create_resource(struct virtio_mem *vm)
>         if (!name)
>                 return -ENOMEM;
>  
> +       /* Disallow mapping device memory via /dev/mem completely. */
>         vm->parent_resource = __request_mem_region(vm->addr, vm->region_size,
> -                                                  name, IORESOURCE_SYSTEM_RAM);
> +                                                  name, IORESOURCE_SYSTEM_RAM |
> +                                                  IORESOURCE_EXCLUSIVE);
>         if (!vm->parent_resource) {
>                 kfree(name);
>                 dev_warn(&vm->vdev->dev, "could not reserve device region\n");

Re: [PATCH v3 1/3] kernel/resource: clean up and optimize iomem_is_exclusive()

[Williams, Dan J]

On Tue, 2021-08-31 at 22:21 +0200, David Hildenbrand wrote:
> We end up traversing subtrees of ranges we are not interested in; let's
> optimize this case, skipping such subtrees, cleaning up the function a bit.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  kernel/resource.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)

That diffstat does not come across as "cleanup", and the skip_children
flag changing values mid-iteration feels tricky. Is there a win here,
the same number of entries still need to be accessed, right?

BTW, I had to pull this from lore to reply to it, looks like the
intended Cc's were missing?

Re: [PATCH v3 1/3] kernel/resource: clean up and optimize iomem_is_exclusive()

[David Hildenbrand]

On 01.09.21 21:43, Williams, Dan J wrote:
> On Tue, 2021-08-31 at 22:21 +0200, David Hildenbrand wrote:
>> We end up traversing subtrees of ranges we are not interested in; let's
>> optimize this case, skipping such subtrees, cleaning up the function a bit.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   kernel/resource.c | 25 ++++++++++++++++++++-----
>>   1 file changed, 20 insertions(+), 5 deletions(-)
> 
> That diffstat does not come across as "cleanup", and the skip_children
> flag changing values mid-iteration feels tricky. Is there a win here,
> the same number of entries still need to be accessed, right?

Right, most of the patch changes falls under "optimize". The cleanup is 
using for_each_resource() and not using r_next(NULL, p, &l). Sure, I 
could have split this up but then I'd just introduce for_each_resource() 
to modify it immediately again.


Let's take a look at /proc/iomem on my notebook:

00000000-00000fff : Reserved
00001000-00057fff : System RAM
00058000-00058fff : Reserved
00059000-0009cfff : System RAM
0009d000-000fffff : Reserved
   000a0000-000bffff : PCI Bus 0000:00
   000c0000-000c3fff : PCI Bus 0000:00
   000c4000-000c7fff : PCI Bus 0000:00
   000c8000-000cbfff : PCI Bus 0000:00
   000cc000-000cffff : PCI Bus 0000:00
   000d0000-000d3fff : PCI Bus 0000:00
   000d4000-000d7fff : PCI Bus 0000:00
   000d8000-000dbfff : PCI Bus 0000:00
   000dc000-000dffff : PCI Bus 0000:00
   000e0000-000e3fff : PCI Bus 0000:00
   000e4000-000e7fff : PCI Bus 0000:00
   000e8000-000ebfff : PCI Bus 0000:00
   000ec000-000effff : PCI Bus 0000:00
   000f0000-000fffff : PCI Bus 0000:00
     000f0000-000fffff : System ROM
00100000-3fffffff : System RAM
40000000-403fffff : Reserved
   40000000-403fffff : pnp 00:00
40400000-80a79fff : System RAM
...

Why should we take a look at any children of "0009d000-000fffff : 
Reserved" if we can just skip these 15 items directly because the parent 
range is not of interest?


It gets even worse for some PCI buses:

8f800000-f7ffffff : PCI Bus 0000:00
   8f800000-8f9fffff : PCI Bus 0000:01
   8fa00000-8fbfffff : PCI Bus 0000:01
   90000000-b1ffffff : PCI Bus 0000:04
     90000000-b1ffffff : PCI Bus 0000:05
       90000000-b1ffffff : PCI Bus 0000:07
         90000000-b1ffffff : PCI Bus 0000:08
           90000000-b1ffffff : PCI Bus 0000:0d
   c0000000-cfffffff : 0000:00:02.0
   d0000000-e60fffff : PCI Bus 0000:04
     d0000000-e60fffff : PCI Bus 0000:05
       d0000000-e5efffff : PCI Bus 0000:07
         d0000000-e5efffff : PCI Bus 0000:08
           d0000000-d00fffff : PCI Bus 0000:09
           d0000000-d000ffff : 0000:09:00.0
           d0000000-d000ffff : xhci-hcd
           d0010000-d0010fff : 0000:09:00.0
           d0011000-d0011fff : 0000:09:00.0
           d0100000-d01fffff : PCI Bus 0000:0b
           d0100000-d010ffff : 0000:0b:00.0
           d0100000-d010ffff : xhci-hcd
           d0110000-d0110fff : 0000:0b:00.0
           d0111000-d0111fff : 0000:0b:00.0
           d0200000-e5efffff : PCI Bus 0000:0d
       e5f00000-e5ffffff : PCI Bus 0000:3c
         e5f00000-e5f0ffff : 0000:3c:00.0
           e5f00000-e5f0ffff : xhci-hcd
       e6000000-e60fffff : PCI Bus 0000:06
         e6000000-e603ffff : 0000:06:00.0
           e6000000-e603ffff : thunderbolt
         e6040000-e6040fff : 0000:06:00.0
   e7000000-e7ffffff : 0000:00:02.0
   e8000000-e80fffff : PCI Bus 0000:3e
     e8000000-e8003fff : 0000:3e:00.0
       e8000000-e8003fff : nvme
   e8100000-e81fffff : PCI Bus 0000:3d
     e8100000-e8101fff : 0000:3d:00.0
       e8100000-e8101fff : iwlwifi
   e8200000-e821ffff : 0000:00:1f.6
     e8200000-e821ffff : e1000e
   e8220000-e822ffff : 0000:00:14.0
     e8220000-e822ffff : xhci-hcd
       e8228070-e822846f : intel_xhci_usb_sw
   e8230000-e823ffff : 0000:00:1f.3
     e8230000-e823ffff : ICH HD audio
   e8240000-e8247fff : 0000:00:04.0
     e8240000-e8247fff : proc_thermal
   e8248000-e824bfff : 0000:00:1f.3
     e8248000-e824bfff : ICH HD audio
   e824c000-e824ffff : 0000:00:1f.2
   e8250000-e8250fff : 0000:00:08.0
   e8251000-e8251fff : 0000:00:14.2
     e8251000-e8251fff : Intel PCH thermal driver
   e8252000-e8252fff : 0000:00:15.0
     e8252000-e82521ff : lpss_dev
       e8252000-e82521ff : i2c_designware.0 lpss_dev
     e8252200-e82522ff : lpss_priv
     e8252800-e8252fff : idma64.0
       e8252800-e8252fff : idma64.0 idma64.0
   e8253000-e8253fff : 0000:00:16.0
     e8253000-e8253fff : mei_me
   e8254000-e82540ff : 0000:00:1f.4
   f7fe0000-f7ffffff : pnp 00:08
     f7fe0000-f7ffffff : pnp 00:0a

I didn't count how many entries these are, but it's certainly more 
entries in that subtree than I have directly under the root, meaning in 
my setup we end up looking at at least 50% less entries (actually, much 
more).

> 
> BTW, I had to pull this from lore to reply to it, looks like the
> intended Cc's were missing?

Yes, I messed up this time, sorry -- I forgot "--cover-cc" w ... I will 
resend the patches so everybody has them without going trough extra trouble.


-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/3] kernel/resource: clean up and optimize iomem_is_exclusive()

[Dan Williams]

On Thu, Sep 2, 2021 at 12:52 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 01.09.21 21:43, Williams, Dan J wrote:
> > On Tue, 2021-08-31 at 22:21 +0200, David Hildenbrand wrote:
> >> We end up traversing subtrees of ranges we are not interested in; let's
> >> optimize this case, skipping such subtrees, cleaning up the function a bit.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>   kernel/resource.c | 25 ++++++++++++++++++++-----
> >>   1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > That diffstat does not come across as "cleanup", and the skip_children
> > flag changing values mid-iteration feels tricky. Is there a win here,
> > the same number of entries still need to be accessed, right?
>
> Right, most of the patch changes falls under "optimize". The cleanup is
> using for_each_resource() and not using r_next(NULL, p, &l). Sure, I
> could have split this up but then I'd just introduce for_each_resource()
> to modify it immediately again.
>
>
> Let's take a look at /proc/iomem on my notebook:
>
> 00000000-00000fff : Reserved
> 00001000-00057fff : System RAM
> 00058000-00058fff : Reserved
> 00059000-0009cfff : System RAM
> 0009d000-000fffff : Reserved
>    000a0000-000bffff : PCI Bus 0000:00
>    000c0000-000c3fff : PCI Bus 0000:00
>    000c4000-000c7fff : PCI Bus 0000:00
>    000c8000-000cbfff : PCI Bus 0000:00
>    000cc000-000cffff : PCI Bus 0000:00
>    000d0000-000d3fff : PCI Bus 0000:00
>    000d4000-000d7fff : PCI Bus 0000:00
>    000d8000-000dbfff : PCI Bus 0000:00
>    000dc000-000dffff : PCI Bus 0000:00
>    000e0000-000e3fff : PCI Bus 0000:00
>    000e4000-000e7fff : PCI Bus 0000:00
>    000e8000-000ebfff : PCI Bus 0000:00
>    000ec000-000effff : PCI Bus 0000:00
>    000f0000-000fffff : PCI Bus 0000:00
>      000f0000-000fffff : System ROM
> 00100000-3fffffff : System RAM
> 40000000-403fffff : Reserved
>    40000000-403fffff : pnp 00:00
> 40400000-80a79fff : System RAM
> ...
>
> Why should we take a look at any children of "0009d000-000fffff :
> Reserved" if we can just skip these 15 items directly because the parent
> range is not of interest?

Oh I misread, it never loads the child entries into cache, so it's a
true skip and not a continue.

You can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

...I was going to say it should be named for_each_top_resource(), but
we can cross that bridge when / if something needs an iterator that
includes children.