Re: [Resend] arch: mips: Fix initrd_start and initrd_end when read from DT

[Paul Burton]

Hi Horatiu,

On Wed, Apr 24, 2019 at 02:12:38PM +0200, Horatiu Vultur wrote:
> The 04/19/2019 20:55, Paul Burton wrote:
> > On Tue, Apr 16, 2019 at 12:18:20PM +0200, Horatiu Vultur wrote:
> > > When the bootloader passes arguments to linux kernel through device tree,
> > > it passes the address of initrd_start and initrd_stop, which are in kseg0.
> > > But when linux kernel reads these addresses from device tree, it converts
> > > them to virtual addresses inside the function
> > > __early_init_dt_declare_initrd.
> > 
> > I'm not sure I follow - if the bootloader provides an address in kseg0
> > then it's already a virtual address.
> 
> So I am just a novice in this, but in my case the bootloader(Uboot) passes
> the address in kseg0(e.g 0x9f8a6000), but if I understand correctly
> this is just cached access to location 0x1f8a6000.

That's right.

In this case the virtual address is 0x9f8a6000, which is in kseg0. That
means the cache-coherency attribute (CCA) is taken from the cop0 config
register's K0 field & is typically some form of cached access.

The physical address is 0x1f8a6000.

> > It looks like __early_init_dt_declare_initrd expects the DT to provide
> > physical addresses, which fits in well with the fact that DTs generally
> > use physical addresses for everything else.
> > 
> > __early_init_dt_declare_initrd calling __va on a virtual address will
> > give you something bogus, and it looks like you're just cancelling this
> > out below. In practice for a typical system where PAGE_OFFSET is the
> > start of kseg0 (0x80000000) the bogus address you get will happen to be
> > the same as the physical address, but that's not guaranteed.
> > 
> > > At a later point then in the function init_initrd, it is checking for
> > > initrd_start to be lower than PAGE_OFFSET, which for a 32 CPU it is not,
> > > therefore it would disable the initrd by setting 0 to initrd_start and
> > > initrd_stop.
> > 
> > The check you mention here is to make sure initrd_start looks like a
> > virtual address - if it's lower than PAGE_OFFSET (typically 0x80000000)
> > then it looks bad & initrd is disabled. I think your comment is
> > backwards - what you have is a physical address, entirely by accident,
> > and you're converting it back to a virtual address again by accident
> > which keeps the check happy.
> 
> I am a little bit confused here. so the initrd_start has to have a
> virtual address(in kseg0) inside the function init_initrd. Meaning that
> when the bootloader passes the arguments to linux through a command line,
> then initrd_start has to be already a virtual address? Because I
> couldn't see a place where it converts the initrd_start. But when the
> bootloader pass the arguments through DT it has to be physical address?

Hmm, that's a good point - it does look like we expect virtual addresses
when passed on the command line. That inconsistency with DT is
unfortunate, but I still think keeping the DT itself consistent &
keeping MIPS consistent with other architectures as far as DT goes makes
it worthwhile to use physical addresses in the DT.

> > > The fix consists of checking if linux kernel received a device tree and not
> > > having enable extended virtual address and in that case convert them back
> > > to physical addresses that point in kseg0 as expected.
> > 
> > Can you instead just have your bootloader provide physical addresses in
> > the DT?
> 
> Yes, I have done few tests and it seems to work fine, but I need to
> understand it better.

I hope the above helps makes sense of that. I think overall that using
the physical address of the initrd in the DT makes more sense than using
the virtual address. It is afterall what's specified in the DT binding
documentation too, see Documentation/devicetree/bindings/chosen.txt:

> linux,initrd-start and linux,initrd-end
> ---------------------------------------
> 
> These properties hold the physical start and end address of an initrd
> that's loaded by the bootloader.
>%

Thanks,
    Paul