Re: [PATCH -v2] Resource: fix region_intersects() for CXL memory

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Aug 19, 2024 at 10:34:13AM +0800, Huang Ying wrote:
> On a system with CXL memory installed, the resource tree (/proc/iomem)
> related to CXL memory looks like something as follows.
> 
> 490000000-50fffffff : CXL Window 0
>   490000000-50fffffff : region0
>     490000000-50fffffff : dax0.0
>       490000000-50fffffff : System RAM (kmem)

I think the subject is too specific (the problem is something to do
with the tree topology, not the fact that it's "CXL memory") and at
the same time not specific enough ("fix" doesn't say anything about
what was wrong or how it is fixed).

IMO it could be improved by saying something about what is different
about CXL, e.g., maybe it could mention checking children in addition
to top-level resources.

> When the following command line is run to try writing some memory in
> CXL memory range,
> 
>  $ dd if=data of=/dev/mem bs=1k seek=19136512 count=1
>  dd: error writing '/dev/mem': Bad address
>  1+0 records in
>  0+0 records out
>  0 bytes copied, 0.0283507 s, 0.0 kB/s

Took me a minute, but I guess the connection is that
19136512 * 1k = 0x490000000, which is the beginning of the CXL Window.

> the command fails as expected.  However, the error code is wrong.  It
> should be "Operation not permitted" instead of "Bad address".  And,
> the following warning is reported in kernel log.

This intro makes it sound like the problem being solved is the error
code being wrong.  But it seems like a more serious problem than that.

>  ioremap on RAM at 0x0000000490000000 - 0x0000000490000fff

Incidental: it seems a little weird that this warning only exists on
x86 and mips (and powerpc32 has a similar warning with different
wording), but I assume we don't want to ioremap RAM on *any*
architecture?

>  WARNING: CPU: 2 PID: 416 at arch/x86/mm/ioremap.c:216 __ioremap_caller.constprop.0+0x131/0x35d
>  Modules linked in: cxl_pmem libnvdimm cbc encrypted_keys cxl_pmu
>  CPU: 2 UID: 0 PID: 416 Comm: dd Not tainted 6.11.0-rc3-kvm #40
>  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
>  RIP: 0010:__ioremap_caller.constprop.0+0x131/0x35d
> ...

> In the above resource tree, "System RAM" is a descendant of "CXL
> Window 0" instead of a top level resource.  So, region_intersects()
> will report no System RAM resources in the CXL memory region
> incorrectly, because it only checks the top level resources.
> Consequently, devmem_is_allowed() will return 1 (allow access via
> /dev/mem) for CXL memory region incorrectly.  Fortunately, ioremap()
> doesn't allow to map System RAM and reject the access.
> 
> However, region_intersects() needs to be fixed to work correctly with
> the resources tree with CXL Window as above.  To fix it, if we found a
> unmatched resource in the top level, we will continue to search
> matched resources in its descendant resources.  So, we will not miss
> any matched resources in resource tree anymore.  In the new
> implementation,
> 
> |------------- "CXL Window 0" ------------|
> |-- "System RAM" --|
> 
> will look as if
> 
> |-- "System RAM" --||-- "CXL Window 0a" --|

Where did "0a" come from?  The /proc/iomem above mentioned
"CXL Window 0"; is the "a" spurious?  Same question applies to the
code comment below.

> in effect.
> +		 * |------------- "CXL Window 0" ------------|
> +		 * |-- "System RAM" --|
> +		 *
> +		 * looks as if
> +		 *
> +		 * |-- "System RAM" --||-- "CXL Window 0a" --|
> +		 *
> +		 * in effect.