linux-kernel - Re: [PATCH v2] MIPS: replace add_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: <20201008212043.uolf4orh6ctr6ihz@mobilestation>
Date:   Fri, 9 Oct 2020 00:20:43 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     "Maciej W. Rozycki" <macro@...ux-mips.org>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Rafał Miłecki <zajec5@...il.com>,
        bcm-kernel-feedback-list@...adcom.com,
        Jiaxun Yang <jiaxun.yang@...goat.com>,
        Keguang Zhang <keguang.zhang@...il.com>,
        John Crispin <john@...ozen.org>, linux-mips@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2] MIPS: replace add_memory_region with memblock

On Thu, Oct 08, 2020 at 09:49:46AM -0700, Florian Fainelli wrote:
> 
> 
> On 10/8/2020 8:54 AM, Serge Semin wrote:
> > On Thu, Oct 08, 2020 at 04:30:35PM +0100, Maciej W. Rozycki wrote:
> > > On Thu, 8 Oct 2020, Serge Semin wrote:
> > > 
> > > > At least I don't see a decent reason to preserve them. The memory registration
> > > > method does nearly the same sanity checks. The memory reservation function
> > > > defers a bit in adding the being reserved memory first. That seems redundant,
> > > > since the reserved memory won't be available for the system anyway. Do I miss
> > > > something?
> > > 
> > 
> > >   At the very least it serves informational purposes as it shows up in
> > > /proc/iomem.
> > 
> > I thought about that, but /proc/iomem prints the System RAM up. Adding the reserved
> > memory regions to be just memory region first still seem redundant, since
> > reserving a non-reflected in memory region most likely indicates an erroneous
> > dts. I failed to find that, but do the kernel or DTC make sure that the reserved
> > memory regions has actual memory behind? (At least in the framework of the
> > memblock.memory vs memblock.reserved arrays or in the DT source file)
> 
> AFAICT DTC does not do any validation that regions you declare in
> /memreserve or /reserved-memory are within the 'reg' property defined for
> the /memory node.

> Not that it could not but that goes a little beyond is
> compiler job.

You are probably right, but it does the #{address,size}-cells validations
against the subnode' "reg" properties. It even does I2C controllers subnodes
"reg" validation so they wouldn't be greater than 7 or 10 bits wide. It also
does some magic checks of SPI controllers subnodes. See scripts/dtc/checks.c for
details. So I thought DTC might do some memory/reserved-memory validations too.
Apparently it doesn't. On the other hand it would probably be pointless, since a
system bootloader may change the "memory" node' "reg" value anyway if a platform
has a variable memory layout. So any validation being passed at the DTS compile
time wouldn't surely mean the memory/reserved-memory nodes coherency at the
system boot time.

> 
> The kernel ought to be able to do that validation through memblock but there
> could be valid use cases behind declaring a reserved memory region that is
> not backed by a corresponding DRAM region. For instance if you hotplugged
> memory through the sysfs probe interface, and that memory was not initially
> declared in the Device Tree, but there were reserved regions within that
> hot-plugged range that you would have to be aware of, then this would break.

Yeah, it seems to me all hot-pluggable regions are also marked as reserved. So
hot-plugging a memory device behind the manually reserved regions (reserved by
means of the DT reserved-memory/memreserve nodes) will unreserve them, which
most likely isn't what has been originally wanted.

Alas I don't have any hardware with hot-pluggable memory device to check out the
problem availability.

> 
> > 
> > I also don't see the other platforms doing that, since the MIPS arch only
> > redefines these methods. So if a problem of adding a reserved memory with
> > possible no real memory behind exist, it should be fixed in the cross-platform
> > basis, don't you think?
> 

> Would we be breaking any use case if we stopped allowing reserved region
> that are not part of DRAM being declared?

Hm, good question. I don't really know. But AFAICS from the reserved-memory node
DT bindings the reserved regions can be used to declare both normal RAM and some
vendor-specific regions. The later one theoretically can be some resource, bus or
memory-mapped device thing especially if it's marked with "no-map" boolean
property.

---

Getting back to the topic. In the MIPS-specific early_init_dt_reserve_memory_arch()
method (which is called for every region declared in reserved-memory/memreserve nodes)
we currently consider the reserved region as DRAM if 'no-map' property isn't specified.
The main question here is whether it is correct...

-Sergey

> -- 
> Florian