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

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

[Horatiu Vultur]

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.

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 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.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 arch/mips/kernel/setup.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 8d1dc6c..774ee00 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -264,6 +264,17 @@ static unsigned long __init init_initrd(void)
 		pr_err("initrd start must be page aligned\n");
 		goto disable;
 	}
+
+	/*
+	 * In case the initrd_start and initrd_end are read from DT,
+	 * then they are converted to virtual address, therefore convert
+	 * them back to physical address.
+	 */
+	if (!IS_ENABLED(CONFIG_EVA) && fw_arg0 == -2) {
+		initrd_start = initrd_start - PAGE_OFFSET + PHYS_OFFSET;
+		initrd_end = initrd_end - PAGE_OFFSET + PHYS_OFFSET;
+	}
+
 	if (initrd_start < PAGE_OFFSET) {
 		pr_err("initrd start < PAGE_OFFSET\n");
 		goto disable;
-- 
2.7.4

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

[Paul Burton]

Hi Horatiu,

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.

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.

> 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?

Even if we were to have this code try to sanitize the value with
something like __va(__pa(initrd_start)), it only covers systems using
the UHI boot protocol which isn't the only way we can obtain a DT. If a
system builds in its DTB for example it'll get different behaviour to if
it's passed via the UHI protocol by the bootloader.

Thanks,
    Paul

> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  arch/mips/kernel/setup.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 8d1dc6c..774ee00 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -264,6 +264,17 @@ static unsigned long __init init_initrd(void)
>  		pr_err("initrd start must be page aligned\n");
>  		goto disable;
>  	}
> +
> +	/*
> +	 * In case the initrd_start and initrd_end are read from DT,
> +	 * then they are converted to virtual address, therefore convert
> +	 * them back to physical address.
> +	 */
> +	if (!IS_ENABLED(CONFIG_EVA) && fw_arg0 == -2) {
> +		initrd_start = initrd_start - PAGE_OFFSET + PHYS_OFFSET;
> +		initrd_end = initrd_end - PAGE_OFFSET + PHYS_OFFSET;
> +	}
> +
>  	if (initrd_start < PAGE_OFFSET) {
>  		pr_err("initrd start < PAGE_OFFSET\n");
>  		goto disable;
> -- 
> 2.7.4
> 

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

[Horatiu Vultur]

Hi Paul,

Thank you for your detail explanation. There are few observations below.

The 04/19/2019 20:55, Paul Burton wrote:
> External E-Mail
> 
> 
> Hi Horatiu,
> 
> 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.

> 
> 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?

> 
> > 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.

> 
> Even if we were to have this code try to sanitize the value with
> something like __va(__pa(initrd_start)), it only covers systems using
> the UHI boot protocol which isn't the only way we can obtain a DT. If a
> system builds in its DTB for example it'll get different behaviour to if
> it's passed via the UHI protocol by the bootloader.
> 
> Thanks,
>     Paul
> 
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  arch/mips/kernel/setup.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> > index 8d1dc6c..774ee00 100644
> > --- a/arch/mips/kernel/setup.c
> > +++ b/arch/mips/kernel/setup.c
> > @@ -264,6 +264,17 @@ static unsigned long __init init_initrd(void)
> >  		pr_err("initrd start must be page aligned\n");
> >  		goto disable;
> >  	}
> > +
> > +	/*
> > +	 * In case the initrd_start and initrd_end are read from DT,
> > +	 * then they are converted to virtual address, therefore convert
> > +	 * them back to physical address.
> > +	 */
> > +	if (!IS_ENABLED(CONFIG_EVA) && fw_arg0 == -2) {
> > +		initrd_start = initrd_start - PAGE_OFFSET + PHYS_OFFSET;
> > +		initrd_end = initrd_end - PAGE_OFFSET + PHYS_OFFSET;
> > +	}
> > +
> >  	if (initrd_start < PAGE_OFFSET) {
> >  		pr_err("initrd start < PAGE_OFFSET\n");
> >  		goto disable;
> > -- 
> > 2.7.4
> > 
> 

-- 
/Horatiu

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