Re: [PATCH v2] MIPS: replace add_memory_region with memblock

[Serge Semin <fancer.lancer@gmail.com>]

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