On Thu, Oct 15, 2020 at 01:03:10AM +0100, Maciej W. Rozycki wrote:
> On Thu, 15 Oct 2020, Serge Semin wrote:
>
> > > Table 2-2 REX Memory Regions
> > > -------------------------------------------------------------------------
> > > Starting Ending
> > > Region Address Address Use
> > > -------------------------------------------------------------------------
> > > 0 0xa0000000 0xa000ffff Restart block, exception vectors,
> > > REX stack and bss
> > > 1 0xa0010000 0xa0017fff Keyboard or tty drivers
> > >
> > > 2 0xa0018000 0xa001f3ff 1) CRT driver
> > >
> > > 3 0xa0020000 0xa002ffff boot, cnfg, init and t objects
> > >
> > > 4 0xa0020000 0xa002ffff 64KB scratch space
> > > -------------------------------------------------------------------------
> > > 1) Note that the last 3 Kbytes of region 2 are reserved for backward
> > > compatibility with previous system software.
> > > -------------------------------------------------------------------------
> > >
> >
> > ...
> >
> > > @@ -146,6 +150,9 @@ void __init plat_mem_setup(void)
> > >
> > > ioport_resource.start = ~0UL;
> > > ioport_resource.end = 0UL;
> >
> > > +
> > > + /* Stay away from the firmware working memory area for now. */
> > > + memblock_reserve(PHYS_OFFSET, __pa_symbol(&_text) - PHYS_OFFSET);
> >
> > I am just wondering...
> > Here we reserve a region within [PHYS_OFFSET, __pa_symbol(&_text)]. But then in
> > the prom_free_prom_memory() method we'll get to free a region as either
> > [PAGE_SIZE, __pa_symbol(&_text)] or [PAGE_SIZE, __pa_symbol(&_text) - 0x00020000].
> >
> > First of all the start address of the being freed region is PAGE_SIZE, which doesn't
> > take the PHYS_OFFSET into account. I assume it won't cause problems because
> > PHYS_OFFSET is set to 0 for DEC. If so then we either shouldn't use PHYS_OFFSET
> > here or should take PHYS_OFFSET into account there on freeing or should just forget
> > about that since other than confusion it won't cause any problem.)
> > (I should have posted this question to v1 of this patch instead of pointing out
> > on the confusing size argument of the memblock_reserve() method. Sorry about
> > that.)
>
> Technically, yes, we could use PHYS_OFFSET here, though as a separate
> change, as it's not related to the regression addressed.
>
> Please mind that this is very old code, which long predates the existence
> of PHYS_OFFSET, introduced with commit 6f284a2ce7b8 ("[MIPS] FLATMEM:
> introduce PHYS_OFFSET.") back in 2007 only. While `prom_free_prom_memory'
> code dates back to commit b5766e7e6177 ("o bootmem fixes for DECstations")
> (no proper change heading here; this is from CVS repo days) from 2000, and
> I fiddled with it myself not so long afterwards with commit e25ac8bd2505
> ("DECstation fixes from Maciej") in 2001 (both in the old LMO GIT repo).
> So if anytime it should have been updated in the course of a code audit
> that was surely due across all platforms with the introduction of
> PHYS_OFFSET.
>
> Of course it doesn't mean it must not or cannot be fixed now, but it has
> been functionally correct even if semantically broken, so no one saw the
> urge to change it (let alone notice the problem in the first place -- you
> are the first one).
>
> > Secondly why is PAGE_SIZE selected as the start address? It belongs to the
> > Region 1 in accordance with "Table 2-2 REX Memory Regions" cited above. Thus we
> > get to keep reserved just a part of the Region 1. Most likely it's the restart
> > block and the exception vectors. Even assuming that the DEC developers knew what
> > they were doing, I am wondering can we be sure that a single page is enough for
> > all that data?..
>
> Again this is so old as to predate the existence of synthesised exception
> handlers we currently use (which has been a blessing BTW), which I reckon
> take a little less space than the preassembled handlers we previously had
> used to, and stay well within even the smallest supported page size of
> 4KiB. Anything else we can just overwrite as we do not mean to call into
> the firmware at this point anymore; we couldn't trust it to do the right
> thing anyway once we have booted (e.g. not to keep interrupts masked for
> too long, etc.).
>
> I've been occasionally thinking about a better solution in place of the
> LANCE hack here, needed because the chip has only its low 16 out of 24
> address lines wired, due to a limitation of the IOASIC DMA controller (it
> also drives one half of the IOASIC's 16-bit data bus only, communicating
> with every other byte of system memory only and hence the requirement for
> a 128KiB allocation, with every other byte wasted, rather than a 64KiB
> one).
>
> Had all 24 lines been used, we could use dynamic ZONE_DMA buffers as with
> PC ISA DMA, as the LANCE implements proper DMA rings, but with low 16 only
> it would not really play. I have not come up with any solution however
> that would be significantly better than what we have, and the current one
> works, so I have left it as it is.
>
> Do these explanations address your concerns?
Yep, completely. Thanks for the very detailed response.
-Sergey
>
> Maciej
