diff mbox

[v1,3/3] arm64: dts: add Hi6220 mailbox node

Message ID 1439977055-1747-4-git-send-email-leo.yan@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Leo Yan Aug. 19, 2015, 9:37 a.m. UTC
On Hi6220, below memory regions in DDR have specific purpose:

  0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
  0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
  0x06df,f000 - 0x06df,ffff: For mailbox message data.

This patch reserves these memory regions and add device node for
mailbox in dts.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

Comments

Mark Rutland Aug. 21, 2015, 6:40 p.m. UTC | #1
On Wed, Aug 19, 2015 at 10:37:35AM +0100, Leo Yan wrote:
> On Hi6220, below memory regions in DDR have specific purpose:
> 
>   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
>   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
>   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> 
> This patch reserves these memory regions and add device node for
> mailbox in dts.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> index e36a539..d5470d3 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> @@ -7,9 +7,6 @@
>  
>  /dts-v1/;
>  
> -/*Reserved 1MB memory for MCU*/
> -/memreserve/ 0x05e00000 0x00100000;
> -
>  #include "hi6220.dtsi"
>  
>  / {
> @@ -28,4 +25,21 @@
>  		device_type = "memory";
>  		reg = <0x0 0x0 0x0 0x40000000>;
>  	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		mcu-buf@05e00000 {
> +			no-map;
> +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> +		};
> +
> +		mbox-buf@06dff000 {
> +			no-map;
> +			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
> +		};
> +	};

As far as I can see, it would be simpler to simply carve these out of the
memory node.

I don't see why you need reserved-memory here, given you're not referring to
these regions by phandle anyway.

Thanks,
Mark.


>  };
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 3f03380..9ff25bc 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -167,5 +167,13 @@
>  			clocks = <&ao_ctrl 36>, <&ao_ctrl 36>;
>  			clock-names = "uartclk", "apb_pclk";
>  		};
> +
> +		mailbox: mailbox@f7510000 {
> +			#mbox-cells = <1>;
> +			compatible = "hisilicon,hi6220-mbox";
> +			reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
> +			      <0x0 0x06dff800 0x0 0x0800>; /* Mailbox buffer */
> +			interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> +		};
>  	};
>  };
> -- 
> 1.9.1
>
Leo Yan Aug. 22, 2015, 1:30 p.m. UTC | #2
Hi Mark,

On Fri, Aug 21, 2015 at 07:40:59PM +0100, Mark Rutland wrote:
> On Wed, Aug 19, 2015 at 10:37:35AM +0100, Leo Yan wrote:
> > On Hi6220, below memory regions in DDR have specific purpose:
> > 
> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> >   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> > 
> > This patch reserves these memory regions and add device node for
> > mailbox in dts.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
> >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
> >  2 files changed, 25 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > index e36a539..d5470d3 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > @@ -7,9 +7,6 @@
> >  
> >  /dts-v1/;
> >  
> > -/*Reserved 1MB memory for MCU*/
> > -/memreserve/ 0x05e00000 0x00100000;
> > -
> >  #include "hi6220.dtsi"
> >  
> >  / {
> > @@ -28,4 +25,21 @@
> >  		device_type = "memory";
> >  		reg = <0x0 0x0 0x0 0x40000000>;
> >  	};
> > +
> > +	reserved-memory {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +
> > +		mcu-buf@05e00000 {
> > +			no-map;
> > +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > +		};
> > +
> > +		mbox-buf@06dff000 {
> > +			no-map;
> > +			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
> > +		};
> > +	};
> 
> As far as I can see, it would be simpler to simply carve these out of the
> memory node.

Will modify for MCU firmware buffer and section.

> I don't see why you need reserved-memory here, given you're not referring to
> these regions by phandle anyway.

mbox-buf is used by below mailbox's node, but the start address has
been truncated with 4KB alignment; so should keep it, right?

Thanks,
Leo Yan

> >  };
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > index 3f03380..9ff25bc 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > @@ -167,5 +167,13 @@
> >  			clocks = <&ao_ctrl 36>, <&ao_ctrl 36>;
> >  			clock-names = "uartclk", "apb_pclk";
> >  		};
> > +
> > +		mailbox: mailbox@f7510000 {
> > +			#mbox-cells = <1>;
> > +			compatible = "hisilicon,hi6220-mbox";
> > +			reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
> > +			      <0x0 0x06dff800 0x0 0x0800>; /* Mailbox buffer */
> > +			interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> > +		};
> >  	};
> >  };
> > -- 
> > 1.9.1
> >
Leo Yan Aug. 24, 2015, 3:27 a.m. UTC | #3
Hi Mark,

On Sat, Aug 22, 2015 at 09:30:50PM +0800, Leo Yan wrote:
> On Fri, Aug 21, 2015 at 07:40:59PM +0100, Mark Rutland wrote:
> > On Wed, Aug 19, 2015 at 10:37:35AM +0100, Leo Yan wrote:
> > > On Hi6220, below memory regions in DDR have specific purpose:
> > > 
> > >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> > >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> > >   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> > > 
> > > This patch reserves these memory regions and add device node for
> > > mailbox in dts.
> > > 
> > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
> > >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
> > >  2 files changed, 25 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > index e36a539..d5470d3 100644
> > > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > @@ -7,9 +7,6 @@
> > >  
> > >  /dts-v1/;
> > >  
> > > -/*Reserved 1MB memory for MCU*/
> > > -/memreserve/ 0x05e00000 0x00100000;
> > > -
> > >  #include "hi6220.dtsi"
> > >  
> > >  / {
> > > @@ -28,4 +25,21 @@
> > >  		device_type = "memory";
> > >  		reg = <0x0 0x0 0x0 0x40000000>;
> > >  	};
> > > +
> > > +	reserved-memory {
> > > +		#address-cells = <2>;
> > > +		#size-cells = <2>;
> > > +		ranges;
> > > +
> > > +		mcu-buf@05e00000 {
> > > +			no-map;
> > > +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > > +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > > +		};
> > > +
> > > +		mbox-buf@06dff000 {
> > > +			no-map;
> > > +			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
> > > +		};
> > > +	};
> > 
> > As far as I can see, it would be simpler to simply carve these out of the
> > memory node.
> 
> Will modify for MCU firmware buffer and section.
> 
> > I don't see why you need reserved-memory here, given you're not referring to
> > these regions by phandle anyway.
> 
> mbox-buf is used by below mailbox's node, but the start address has
> been truncated with 4KB alignment; so should keep it, right?

I think i got your point, all these nodes can be removed and just use
memory node to carve them out; but currently i saw the memory node
cannot be passed correctly from UEFI to kernel, we will check for
this. So will follow your suggestion if without any unknown reason.

Thanks,
Leo Yan

> > >  };
> > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > > index 3f03380..9ff25bc 100644
> > > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > > @@ -167,5 +167,13 @@
> > >  			clocks = <&ao_ctrl 36>, <&ao_ctrl 36>;
> > >  			clock-names = "uartclk", "apb_pclk";
> > >  		};
> > > +
> > > +		mailbox: mailbox@f7510000 {
> > > +			#mbox-cells = <1>;
> > > +			compatible = "hisilicon,hi6220-mbox";
> > > +			reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
> > > +			      <0x0 0x06dff800 0x0 0x0800>; /* Mailbox buffer */
> > > +			interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> > > +		};
> > >  	};
> > >  };
> > > -- 
> > > 1.9.1
> > >
Leo Yan Aug. 24, 2015, 9:18 a.m. UTC | #4
Hi Mark,

On Fri, Aug 21, 2015 at 07:40:59PM +0100, Mark Rutland wrote:
> On Wed, Aug 19, 2015 at 10:37:35AM +0100, Leo Yan wrote:
> > On Hi6220, below memory regions in DDR have specific purpose:
> > 
> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> >   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> > 
> > This patch reserves these memory regions and add device node for
> > mailbox in dts.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
> >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
> >  2 files changed, 25 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > index e36a539..d5470d3 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > @@ -7,9 +7,6 @@
> >  
> >  /dts-v1/;
> >  
> > -/*Reserved 1MB memory for MCU*/
> > -/memreserve/ 0x05e00000 0x00100000;
> > -
> >  #include "hi6220.dtsi"
> >  
> >  / {
> > @@ -28,4 +25,21 @@
> >  		device_type = "memory";
> >  		reg = <0x0 0x0 0x0 0x40000000>;
> >  	};
> > +
> > +	reserved-memory {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +
> > +		mcu-buf@05e00000 {
> > +			no-map;
> > +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > +		};
> > +
> > +		mbox-buf@06dff000 {
> > +			no-map;
> > +			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
> > +		};
> > +	};
> 
> As far as I can see, it would be simpler to simply carve these out of the
> memory node.
> 
> I don't see why you need reserved-memory here, given you're not referring to
> these regions by phandle anyway.

- Now we have enabled EFI_STUB, so the memory node will be removed in
  kernel:
    efi_entry()
      \-> allocate_new_fdt_and_exit_boot()
            \-> update_fdt();

  Finally in kernel it cannot use memory node to carve out reseved
  memory regions.

- On the other hand, DTS's the memory node is to "describes the
  physical memory layout for the system"; so it's better to use it only
  to describe the hardware info for memory. We can use reserved-memory
  to help manage the memory regions which are reserved from software
  perspective.

According to upper info, we still need to use reserved-memory node to
depict the reserved memory regions. i have no knowledge about EFI_STUB,
so please confirm or correct as needed.

Thanks,
Leo Yan
Mark Rutland Aug. 24, 2015, 9:51 a.m. UTC | #5
On Mon, Aug 24, 2015 at 10:18:45AM +0100, Leo Yan wrote:
> Hi Mark,
> 
> On Fri, Aug 21, 2015 at 07:40:59PM +0100, Mark Rutland wrote:
> > On Wed, Aug 19, 2015 at 10:37:35AM +0100, Leo Yan wrote:
> > > On Hi6220, below memory regions in DDR have specific purpose:
> > > 
> > >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> > >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> > >   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> > > 
> > > This patch reserves these memory regions and add device node for
> > > mailbox in dts.
> > > 
> > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
> > >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
> > >  2 files changed, 25 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > index e36a539..d5470d3 100644
> > > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > @@ -7,9 +7,6 @@
> > >  
> > >  /dts-v1/;
> > >  
> > > -/*Reserved 1MB memory for MCU*/
> > > -/memreserve/ 0x05e00000 0x00100000;
> > > -
> > >  #include "hi6220.dtsi"
> > >  
> > >  / {
> > > @@ -28,4 +25,21 @@
> > >  		device_type = "memory";
> > >  		reg = <0x0 0x0 0x0 0x40000000>;
> > >  	};
> > > +
> > > +	reserved-memory {
> > > +		#address-cells = <2>;
> > > +		#size-cells = <2>;
> > > +		ranges;
> > > +
> > > +		mcu-buf@05e00000 {
> > > +			no-map;
> > > +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > > +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > > +		};
> > > +
> > > +		mbox-buf@06dff000 {
> > > +			no-map;
> > > +			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
> > > +		};
> > > +	};
> > 
> > As far as I can see, it would be simpler to simply carve these out of the
> > memory node.
> > 
> > I don't see why you need reserved-memory here, given you're not referring to
> > these regions by phandle anyway.
> 
> - Now we have enabled EFI_STUB, so the memory node will be removed in
>   kernel:
>     efi_entry()
>       \-> allocate_new_fdt_and_exit_boot()
>             \-> update_fdt();
> 
>   Finally in kernel it cannot use memory node to carve out reseved
>   memory regions.
> 
> - On the other hand, DTS's the memory node is to "describes the
>   physical memory layout for the system"; so it's better to use it only
>   to describe the hardware info for memory. We can use reserved-memory
>   to help manage the memory regions which are reserved from software
>   perspective.

The fact that you have no-map means that the memory should not be
described to the kernel as mappable in the first place. It's wrong to
place such memory in the memory node, even if listed in reserved-memory.

If your EFI memory map describes the memory as mappable, it is wrong.

> According to upper info, we still need to use reserved-memory node to
> depict the reserved memory regions. i have no knowledge about EFI_STUB,
> so please confirm or correct as needed.

If the memory shouldn't be mapped, it should neither be in the memory
node nor EFI memory map (with attributes allowing it to be mapped) to
begin with.

As far as I can see you do not need to use reserved-memory.

Thanks,
Mark.
Haojian Zhuang Aug. 24, 2015, 10:19 a.m. UTC | #6
On Mon, 2015-08-24 at 10:51 +0100, Mark Rutland wrote:
> On Mon, Aug 24, 2015 at 10:18:45AM +0100, Leo Yan wrote:
> > Hi Mark,
> > 
> > On Fri, Aug 21, 2015 at 07:40:59PM +0100, Mark Rutland wrote:
> > > On Wed, Aug 19, 2015 at 10:37:35AM +0100, Leo Yan wrote:
> > > > On Hi6220, below memory regions in DDR have specific purpose:
> > > > 
> > > >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> > > >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> > > >   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> > > > 
> > > > This patch reserves these memory regions and add device node for
> > > > mailbox in dts.
> > > > 
> > > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > > ---
> > > >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
> > > >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
> > > >  2 files changed, 25 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > index e36a539..d5470d3 100644
> > > > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > @@ -7,9 +7,6 @@
> > > >  
> > > >  /dts-v1/;
> > > >  
> > > > -/*Reserved 1MB memory for MCU*/
> > > > -/memreserve/ 0x05e00000 0x00100000;
> > > > -
> > > >  #include "hi6220.dtsi"
> > > >  
> > > >  / {
> > > > @@ -28,4 +25,21 @@
> > > >  		device_type = "memory";
> > > >  		reg = <0x0 0x0 0x0 0x40000000>;
> > > >  	};
> > > > +
> > > > +	reserved-memory {
> > > > +		#address-cells = <2>;
> > > > +		#size-cells = <2>;
> > > > +		ranges;
> > > > +
> > > > +		mcu-buf@05e00000 {
> > > > +			no-map;
> > > > +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > > > +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > > > +		};
> > > > +
> > > > +		mbox-buf@06dff000 {
> > > > +			no-map;
> > > > +			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
> > > > +		};
> > > > +	};
> > > 
> > > As far as I can see, it would be simpler to simply carve these out of the
> > > memory node.
> > > 
> > > I don't see why you need reserved-memory here, given you're not referring to
> > > these regions by phandle anyway.
> > 
> > - Now we have enabled EFI_STUB, so the memory node will be removed in
> >   kernel:
> >     efi_entry()
> >       \-> allocate_new_fdt_and_exit_boot()
> >             \-> update_fdt();
> > 
> >   Finally in kernel it cannot use memory node to carve out reseved
> >   memory regions.
> > 
> > - On the other hand, DTS's the memory node is to "describes the
> >   physical memory layout for the system"; so it's better to use it only
> >   to describe the hardware info for memory. We can use reserved-memory
> >   to help manage the memory regions which are reserved from software
> >   perspective.
> 
> The fact that you have no-map means that the memory should not be
> described to the kernel as mappable in the first place. It's wrong to
> place such memory in the memory node, even if listed in reserved-memory.
> 
> If your EFI memory map describes the memory as mappable, it is wrong.

When kernel is working, kernel will create its own page table based on
UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
be moved to reserved memblock. Why is it wrong?

In the second, UEFI is firmware. When it's stable, nobody should change
it without any reason. These reserved memory are used in mailbox driver.
Look. It's driver, so it could be changed at any time. Why do you want
to UEFI knowing this memory range? Do you hope UEFI to change when
mailbox driver is changed?

> 
> > According to upper info, we still need to use reserved-memory node to
> > depict the reserved memory regions. i have no knowledge about EFI_STUB,
> > so please confirm or correct as needed.
> 
> If the memory shouldn't be mapped, it should neither be in the memory
> node nor EFI memory map (with attributes allowing it to be mapped) to
> begin with.

As I said above, kernel will create its own page table. When kernel's
page table is working, UEFI's page table is destroying. So the memory
won't be mapped twice at the same time. What's wrong?
> 
> As far as I can see you do not need to use reserved-memory.

1. Are we talking on the same thing? Leo already mentioned that all
memory node in DTB will be destroyed by kernel when EFI_STUB is enabled
on arm. Did you read the source code after his reply?
And you suggested that Leo to use discrete memory region in DTB. It is
really wrong. Kernel only gets memory map information from UEFI, not
DTB.

2. The working flow is in below.
   a. Kernel gets memory map information from UEFI.
   b. Kernel loads the memory reserved information from DTB.

3. Do you mean the reserved-memory is totally wrong? If it's wrong,
please submit patches to remove all reserved-memory in linux kernel
first.

4. Again and again. Memory node should be only used to describe the
RAM information.
Leif Lindholm Aug. 24, 2015, 11:49 a.m. UTC | #7
On Mon, Aug 24, 2015 at 06:19:56PM +0800, Haojian Zhuang wrote:
> > If your EFI memory map describes the memory as mappable, it is wrong.
> 
> When kernel is working, kernel will create its own page table based on
> UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> be moved to reserved memblock. Why is it wrong?
> 
> In the second, UEFI is firmware. When it's stable, nobody should change
> it without any reason.

Much like the memory map.

> These reserved memory are used in mailbox driver.
> Look. It's driver, so it could be changed at any time.

No, it is a set of regions of memory set aside for use by a different
master in the system as well as communications with that master.

The fact that there is a driver somewhere that is aware of this is
entirely beside the point. All agents in the system must adher to this
protocol.

> Why do you want
> to UEFI knowing this memory range? Do you hope UEFI to change when
> mailbox driver is changed?

Yes.

UEFI is a runtime environment. Having random magic areas not to be
touched will cause random pieces of software running under it to break
horribly or break other things horribly.
Unless you mark them as reserved in the UEFI memory map.
At which point the Linux kernel will automatically ignore them, and
the proposed patch is redundant.

So, yes, if you want a system that can boot reliably, run testsuites
(like SCT or FWTS), run applications (like fastboot ... or the EFI
stub kernel itself), then any memory regions that is reserved for
mailbox communication (or other masters in the system) _must_ be
marked in the EFI memory map.

/
    Leif
Mark Rutland Aug. 24, 2015, 12:48 p.m. UTC | #8
> > > > I don't see why you need reserved-memory here, given you're not referring to
> > > > these regions by phandle anyway.
> > > 
> > > - Now we have enabled EFI_STUB, so the memory node will be removed in
> > >   kernel:
> > >     efi_entry()
> > >       \-> allocate_new_fdt_and_exit_boot()
> > >             \-> update_fdt();
> > > 
> > >   Finally in kernel it cannot use memory node to carve out reseved
> > >   memory regions.
> > > 
> > > - On the other hand, DTS's the memory node is to "describes the
> > >   physical memory layout for the system"; so it's better to use it only
> > >   to describe the hardware info for memory. We can use reserved-memory
> > >   to help manage the memory regions which are reserved from software
> > >   perspective.
> > 
> > The fact that you have no-map means that the memory should not be
> > described to the kernel as mappable in the first place. It's wrong to
> > place such memory in the memory node, even if listed in reserved-memory.
> > 
> > If your EFI memory map describes the memory as mappable, it is wrong.
> 
> When kernel is working, kernel will create its own page table based on
> UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> be moved to reserved memblock. Why is it wrong?

That is a _Linux_ detail, not a _UEFI_ detail.

Anything which only handles UEFI and knows nothing of reserved-memory
(e.g. GRUB) will be broken and/or break the Linux use of the region, as
it will happily try to allocate memory in the region (and could even
decide to reserve it permanently for its own usage).

If the memory is truly specific to the mailbox, then UEFI needs to know
that it is reserved as such. If it is not, then it need not be described
in the DT and can be allocated dynamically by the kernel.

> In the second, UEFI is firmware. When it's stable, nobody should change
> it without any reason. These reserved memory are used in mailbox driver.
> Look. It's driver, so it could be changed at any time. Why do you want
> to UEFI knowing this memory range? Do you hope UEFI to change when
> mailbox driver is changed?

It shouldn't need to change if that memory is truly reserved for the
sole use of the mailbox. If that's not the case then we have a different
issue.

If the memory range to use can be allocated by the driver, then I don't
see why it should be described in reserved-memory. It certainly should
not require a no-map attribute.

Additionally, the driver needs to ensure that the requisite cache
maintenance takes place prior to the use of the memory region given
prior agents may have ampped it as cacheable, leaving stale (perhaps
dirty) lines in the caches.

> > > According to upper info, we still need to use reserved-memory node to
> > > depict the reserved memory regions. i have no knowledge about EFI_STUB,
> > > so please confirm or correct as needed.
> > 
> > If the memory shouldn't be mapped, it should neither be in the memory
> > node nor EFI memory map (with attributes allowing it to be mapped) to
> > begin with.
> 
> As I said above, kernel will create its own page table. When kernel's
> page table is working, UEFI's page table is destroying. So the memory
> won't be mapped twice at the same time. What's wrong?
> > 
> > As far as I can see you do not need to use reserved-memory.
> 
> 1. Are we talking on the same thing? Leo already mentioned that all
> memory node in DTB will be destroyed by kernel when EFI_STUB is enabled
> on arm. Did you read the source code after his reply?
> And you suggested that Leo to use discrete memory region in DTB. It is
> really wrong. Kernel only gets memory map information from UEFI, not
> DTB.

I did _not_ suggest that Leo use discrete memory. I suggested that
reserved regions should not be described in the memory node (the same
premise applying to the UEFI memory map).

w.r.t. UEFI, please see my comments above. If you're using the UEFI
memory map, you have to use the UEFI memory map, not the UEFI memory map
with additional (non-UEFI) caveats applied atop.

> 2. The working flow is in below.
>    a. Kernel gets memory map information from UEFI.
>    b. Kernel loads the memory reserved information from DTB.

This relies on Linux, and ignores other UEFI clients.

> 3. Do you mean the reserved-memory is totally wrong? If it's wrong,
> please submit patches to remove all reserved-memory in linux kernel
> first.

I did not say that.

I said that describing some memory in a memory node, then also
describing that in reserved-memory with a no-map property was wrong. If
it's never meant to be mapped then there's no reason for it to be in the
memory node.

> 4. Again and again. Memory node should be only used to describe the
> RAM information.

The memory node describes the memory available to the OS. There are some
caveats w.r.t. /memreserve/, regions which may be mapped but remain
unused and so on, but the memory node does generally encode a policy
that the memory may be used.

Describing unusable memory in a memory node is pointless, and has an
adverse effect on clients which don't support reserved-memory. It's
doubly harmful when that memory should never be mapped.

Thanks,
Mark.
Haojian Zhuang Aug. 25, 2015, 8:04 a.m. UTC | #9
On Mon, 2015-08-24 at 13:48 +0100, Mark Rutland wrote:
> > > > > I don't see why you need reserved-memory here, given you're not referring to
> > > > > these regions by phandle anyway.
> > > > 
> > > > - Now we have enabled EFI_STUB, so the memory node will be removed in
> > > >   kernel:
> > > >     efi_entry()
> > > >       \-> allocate_new_fdt_and_exit_boot()
> > > >             \-> update_fdt();
> > > > 
> > > >   Finally in kernel it cannot use memory node to carve out reseved
> > > >   memory regions.
> > > > 
> > > > - On the other hand, DTS's the memory node is to "describes the
> > > >   physical memory layout for the system"; so it's better to use it only
> > > >   to describe the hardware info for memory. We can use reserved-memory
> > > >   to help manage the memory regions which are reserved from software
> > > >   perspective.
> > > 
> > > The fact that you have no-map means that the memory should not be
> > > described to the kernel as mappable in the first place. It's wrong to
> > > place such memory in the memory node, even if listed in reserved-memory.
> > > 
> > > If your EFI memory map describes the memory as mappable, it is wrong.
> > 
> > When kernel is working, kernel will create its own page table based on
> > UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> > be moved to reserved memblock. Why is it wrong?
> 
> That is a _Linux_ detail, not a _UEFI_ detail.
> 
> Anything which only handles UEFI and knows nothing of reserved-memory
> (e.g. GRUB) will be broken and/or break the Linux use of the region, as
> it will happily try to allocate memory in the region (and could even
> decide to reserve it permanently for its own usage).
> 
> If the memory is truly specific to the mailbox, then UEFI needs to know
> that it is reserved as such. If it is not, then it need not be described
> in the DT and can be allocated dynamically by the kernel.

No. We support both UEFI and uboot on hikey. We must reserve these
mailbox buffer in DT. Otherwise, it's a problem to support uboot.
We should only deliver one kernel and one DTB to support both UEFI and
uboot.

And mailbox driver is already upgraded from beginning. Nobody can say
that it won't be upgraded again in the future.

By the way, UEFI is loaded in the upper memory region of hikey. It won't
break anything in linux kernel.
> 
> > In the second, UEFI is firmware. When it's stable, nobody should change
> > it without any reason. These reserved memory are used in mailbox driver.
> > Look. It's driver, so it could be changed at any time. Why do you want
> > to UEFI knowing this memory range? Do you hope UEFI to change when
> > mailbox driver is changed?
> 
> It shouldn't need to change if that memory is truly reserved for the
> sole use of the mailbox. If that's not the case then we have a different
> issue.
> 
> If the memory range to use can be allocated by the driver, then I don't
> see why it should be described in reserved-memory. It certainly should
> not require a no-map attribute.
> 
> Additionally, the driver needs to ensure that the requisite cache
> maintenance takes place prior to the use of the memory region given
> prior agents may have ampped it as cacheable, leaving stale (perhaps
> dirty) lines in the caches.
> 

Since those mailbox buffer is declared as reserved with "no-map"
property, it means that linux kernel won't create the page table of
them.

The meaning of "no-map" is removing it from memory memblock.
setup_arch()
  |
  ---> efi_init() -- Get memory map information from UEFI
  |
  ---> arm64_memblock_init()
  |      |
  |      ---> early_init_fdt_scan_reserved_mem()
  |           Get reserved memory buffer from DT. Split memory
  |           memblock according to reserved buffer.
  ---> paging_init()
         |--> map_mem()
              _Only_ map the discrete memory memblock into kernel
              page table.

From this working flow, we could know that those mailbox buffers
won't be mapped into kernel page table. So there's no concern on
cache maintenance.
              
> > > > According to upper info, we still need to use reserved-memory node to
> > > > depict the reserved memory regions. i have no knowledge about EFI_STUB,
> > > > so please confirm or correct as needed.
> > > 
> > > If the memory shouldn't be mapped, it should neither be in the memory
> > > node nor EFI memory map (with attributes allowing it to be mapped) to
> > > begin with.
> > 
> > As I said above, kernel will create its own page table. When kernel's
> > page table is working, UEFI's page table is destroying. So the memory
> > won't be mapped twice at the same time. What's wrong?
> > > 
> > > As far as I can see you do not need to use reserved-memory.
> > 
> > 1. Are we talking on the same thing? Leo already mentioned that all
> > memory node in DTB will be destroyed by kernel when EFI_STUB is enabled
> > on arm. Did you read the source code after his reply?
> > And you suggested that Leo to use discrete memory region in DTB. It is
> > really wrong. Kernel only gets memory map information from UEFI, not
> > DTB.
> 
> I did _not_ suggest that Leo use discrete memory. I suggested that
> reserved regions should not be described in the memory node (the same
> premise applying to the UEFI memory map).

I agree that reserved region shouldn't be described in the memory node.
And Leo didn't describe reserved region in memory node too.

> 
> w.r.t. UEFI, please see my comments above. If you're using the UEFI
> memory map, you have to use the UEFI memory map, not the UEFI memory map
> with additional (non-UEFI) caveats applied atop.

The main concern is that we're supporting both UEFI and uboot.
Declaring these reserved buffer in DTB will be a better choice.

> 
> > 2. The working flow is in below.
> >    a. Kernel gets memory map information from UEFI.
> >    b. Kernel loads the memory reserved information from DTB.
> 
> This relies on Linux, and ignores other UEFI clients.

Yes, it's depend on CONFIG_EFI_STUB.

> 
> > 3. Do you mean the reserved-memory is totally wrong? If it's wrong,
> > please submit patches to remove all reserved-memory in linux kernel
> > first.
> 
> I did not say that.
> 
> I said that describing some memory in a memory node, then also
> describing that in reserved-memory with a no-map property was wrong. If
> it's never meant to be mapped then there's no reason for it to be in the
> memory node.

No, it's not never mapped. Leo just wants it to be mapped as
uncacheable in mailbox driver.

If we look at his mailbox node in DT, Leo used these memory regions
in reg property. He wants to use ioremap() in mailbox driver.

> 
> > 4. Again and again. Memory node should be only used to describe the
> > RAM information.
> 
> The memory node describes the memory available to the OS. There are some
> caveats w.r.t. /memreserve/, regions which may be mapped but remain
> unused and so on, but the memory node does generally encode a policy
> that the memory may be used.
> 
> Describing unusable memory in a memory node is pointless, and has an
> adverse effect on clients which don't support reserved-memory. It's
> doubly harmful when that memory should never be mapped.
> 

He didn't declare those buffer in memory node. He only declared it
in reserved-memory node. And it's not never be mapped. He use ioremap()
in the driver.

And I think that Leo could use phandle to reference the reserved buffer
in mailbox node. Then it could be more clear.
Haojian Zhuang Aug. 25, 2015, 8:13 a.m. UTC | #10
On Mon, 2015-08-24 at 12:49 +0100, Leif Lindholm wrote:
> On Mon, Aug 24, 2015 at 06:19:56PM +0800, Haojian Zhuang wrote:
> > > If your EFI memory map describes the memory as mappable, it is wrong.
> > 
> > When kernel is working, kernel will create its own page table based on
> > UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> > be moved to reserved memblock. Why is it wrong?
> > 
> > In the second, UEFI is firmware. When it's stable, nobody should change
> > it without any reason.
> 
> Much like the memory map.
> 
> > These reserved memory are used in mailbox driver.
> > Look. It's driver, so it could be changed at any time.
> 
> No, it is a set of regions of memory set aside for use by a different
> master in the system as well as communications with that master.
> 
> The fact that there is a driver somewhere that is aware of this is
> entirely beside the point. All agents in the system must adher to this
> protocol.
> 
> > Why do you want
> > to UEFI knowing this memory range? Do you hope UEFI to change when
> > mailbox driver is changed?
> 
> Yes.
> 
> UEFI is a runtime environment. Having random magic areas not to be
> touched will cause random pieces of software running under it to break
> horribly or break other things horribly.
> Unless you mark them as reserved in the UEFI memory map.
> At which point the Linux kernel will automatically ignore them, and
> the proposed patch is redundant.
> 
> So, yes, if you want a system that can boot reliably, run testsuites
> (like SCT or FWTS), run applications (like fastboot ... or the EFI
> stub kernel itself), then any memory regions that is reserved for
> mailbox communication (or other masters in the system) _must_ be
> marked in the EFI memory map.

1. We need support both UEFI and uboot. So the reserved buffer have to
be declared in DTB since they are used by kernel driver, not UEFI.

2. UEFI just loads grub. It's no time to run any other custom EFI
application.

Regards
Haojian
Leif Lindholm Aug. 25, 2015, 9:46 a.m. UTC | #11
On Tue, Aug 25, 2015 at 04:13:47PM +0800, Haojian Zhuang wrote:
> On Mon, 2015-08-24 at 12:49 +0100, Leif Lindholm wrote:
> > On Mon, Aug 24, 2015 at 06:19:56PM +0800, Haojian Zhuang wrote:
> > > > If your EFI memory map describes the memory as mappable, it is wrong.
> > > 
> > > When kernel is working, kernel will create its own page table based on
> > > UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> > > be moved to reserved memblock. Why is it wrong?
> > > 
> > > In the second, UEFI is firmware. When it's stable, nobody should change
> > > it without any reason.
> > 
> > Much like the memory map.
> > 
> > > These reserved memory are used in mailbox driver.
> > > Look. It's driver, so it could be changed at any time.
> > 
> > No, it is a set of regions of memory set aside for use by a different
> > master in the system as well as communications with that master.
> > 
> > The fact that there is a driver somewhere that is aware of this is
> > entirely beside the point. All agents in the system must adher to this
> > protocol.
> > 
> > > Why do you want
> > > to UEFI knowing this memory range? Do you hope UEFI to change when
> > > mailbox driver is changed?
> > 
> > Yes.
> > 
> > UEFI is a runtime environment. Having random magic areas not to be
> > touched will cause random pieces of software running under it to break
> > horribly or break other things horribly.
> > Unless you mark them as reserved in the UEFI memory map.
> > At which point the Linux kernel will automatically ignore them, and
> > the proposed patch is redundant.
> > 
> > So, yes, if you want a system that can boot reliably, run testsuites
> > (like SCT or FWTS), run applications (like fastboot ... or the EFI
> > stub kernel itself), then any memory regions that is reserved for
> > mailbox communication (or other masters in the system) _must_ be
> > marked in the EFI memory map.
> 
> 1. We need support both UEFI and uboot. So the reserved buffer have to
> be declared in DTB since they are used by kernel driver, not UEFI.

The buffer may need to be declared in DTB also, but it most certanily
needs to be declared in UEFI.

And for the U-Boot case, since it is not memory available to Linux, it
should not be declared as "memory".

> 2. UEFI just loads grub. It's no time to run any other custom EFI
> application.

Apart from being completely irrelevant, how are you intending to
validate that GRUB never touches these memory regions?

Build a version once, test it, and hope the results remain valid
forever? And then when you move the regions and the previously working
GRUB now tramples all over them? Or when something changes in upstream
GRUB and its memory allocations drifts into the secretly untouchable
regions?

Are you then going to hack GRUB, release a special HiKey version of
GRUB, not support any other versions, and still can your firmware
UEFI?

Repeat again and again for any other UEFI applications - including
fastboot, SCT, FWTS and the UEFI stub kernel.

/
    Leif
Haojian Zhuang Aug. 25, 2015, 10:15 a.m. UTC | #12
On Tue, 2015-08-25 at 10:46 +0100, Leif Lindholm wrote:
> On Tue, Aug 25, 2015 at 04:13:47PM +0800, Haojian Zhuang wrote:
> > On Mon, 2015-08-24 at 12:49 +0100, Leif Lindholm wrote:
> > > On Mon, Aug 24, 2015 at 06:19:56PM +0800, Haojian Zhuang wrote:
> > > > > If your EFI memory map describes the memory as mappable, it is wrong.
> > > > 
> > > > When kernel is working, kernel will create its own page table based on
> > > > UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> > > > be moved to reserved memblock. Why is it wrong?
> > > > 
> > > > In the second, UEFI is firmware. When it's stable, nobody should change
> > > > it without any reason.
> > > 
> > > Much like the memory map.
> > > 
> > > > These reserved memory are used in mailbox driver.
> > > > Look. It's driver, so it could be changed at any time.
> > > 
> > > No, it is a set of regions of memory set aside for use by a different
> > > master in the system as well as communications with that master.
> > > 
> > > The fact that there is a driver somewhere that is aware of this is
> > > entirely beside the point. All agents in the system must adher to this
> > > protocol.
> > > 
> > > > Why do you want
> > > > to UEFI knowing this memory range? Do you hope UEFI to change when
> > > > mailbox driver is changed?
> > > 
> > > Yes.
> > > 
> > > UEFI is a runtime environment. Having random magic areas not to be
> > > touched will cause random pieces of software running under it to break
> > > horribly or break other things horribly.
> > > Unless you mark them as reserved in the UEFI memory map.
> > > At which point the Linux kernel will automatically ignore them, and
> > > the proposed patch is redundant.
> > > 
> > > So, yes, if you want a system that can boot reliably, run testsuites
> > > (like SCT or FWTS), run applications (like fastboot ... or the EFI
> > > stub kernel itself), then any memory regions that is reserved for
> > > mailbox communication (or other masters in the system) _must_ be
> > > marked in the EFI memory map.
> > 
> > 1. We need support both UEFI and uboot. So the reserved buffer have to
> > be declared in DTB since they are used by kernel driver, not UEFI.
> 
> The buffer may need to be declared in DTB also, but it most certanily
> needs to be declared in UEFI.
> 
> And for the U-Boot case, since it is not memory available to Linux, it
> should not be declared as "memory".

Something are messed at here. We have these buffer are used in mailbox.
They should be allocated as non-cacheable.

If these buffers are contained in memory memblock in kernel, it means
that they exist in kernel page table with cachable property. When it's
used in mailbox driver with non-cachable property, it'll only cause
cache maintenance issue. So Leo declared these buffers as reserved
in DT with "no-map" property. It's the key. It could avoid the cache
maintenance issue.

> 
> > 2. UEFI just loads grub. It's no time to run any other custom EFI
> > application.
> 
> Apart from being completely irrelevant, how are you intending to
> validate that GRUB never touches these memory regions?
> 

GRUB is just a part of bootloader. When linux kernel is running,
who cares GRUB? GRUB's lifetime is already finished.

By the way, UEFI code region is at [0x3Dxx_xxxx, 0x3DFF_FFFF]. Those
mailbox buffer is in [0x05e0_xxxx, 0x06f0_xxxx]. Then I can make sure
UEFI won't touch the reserved buffer. Even if UEFI touched the reserved
buffer, is it an issue? Definitely it's not. UEFI's lifetime is end
when linux kernel is running at hikey. Even if UEFI runtime service
is enabled, the runtime data area is at [0x38xx_xxxx, 0x38xx_xxxx].

> Build a version once, test it, and hope the results remain valid
> forever? And then when you move the regions and the previously working
> GRUB now tramples all over them? Or when something changes in upstream
> GRUB and its memory allocations drifts into the secretly untouchable
> regions?

As I said above, UEFI won't touch it. And even UEFI touch it, kernel
doesn't care since UEFI's lifetime is end.

> 
> Are you then going to hack GRUB, release a special HiKey version of
> GRUB, not support any other versions, and still can your firmware
> UEFI?

I don't need to hack GRUB at all.
Leif Lindholm Aug. 25, 2015, 10:40 a.m. UTC | #13
On Tue, Aug 25, 2015 at 06:15:10PM +0800, Haojian Zhuang wrote:
> > > 1. We need support both UEFI and uboot. So the reserved buffer have to
> > > be declared in DTB since they are used by kernel driver, not UEFI.
> > 
> > The buffer may need to be declared in DTB also, but it most certanily
> > needs to be declared in UEFI.
> > 
> > And for the U-Boot case, since it is not memory available to Linux, it
> > should not be declared as "memory".
> 
> Something are messed at here. We have these buffer are used in mailbox.
> They should be allocated as non-cacheable.

That is a completely different issue, and if that is not currently
possible, then we need to fix that. But it needs to be fixed in the
right place.

> If these buffers are contained in memory memblock in kernel, it means
> that they exist in kernel page table with cachable property. When it's
> used in mailbox driver with non-cachable property, it'll only cause
> cache maintenance issue. So Leo declared these buffers as reserved
> in DT with "no-map" property. It's the key. It could avoid the cache
> maintenance issue.

Yes, when not booting with UEFI.

> > > 2. UEFI just loads grub. It's no time to run any other custom EFI
> > > application.
> > 
> > Apart from being completely irrelevant, how are you intending to
> > validate that GRUB never touches these memory regions?
> 
> GRUB is just a part of bootloader. When linux kernel is running,
> who cares GRUB? GRUB's lifetime is already finished.

We don't care once Linux is running - we care between UEFI boot
services starting and Linux memblock being initialised.

> By the way, UEFI code region is at [0x3Dxx_xxxx, 0x3DFF_FFFF]. Those
> mailbox buffer is in [0x05e0_xxxx, 0x06f0_xxxx]. Then I can make sure
> UEFI won't touch the reserved buffer.

And if a UEFI application explicitly requests to map an area
elsewhere, will your UEFI reject that request? How will it do that
without having information in its memory map about areas it must not
access?

> Even if UEFI touched the reserved
> buffer, is it an issue? Definitely it's not. UEFI's lifetime is end
> when linux kernel is running at hikey. Even if UEFI runtime service
> is enabled, the runtime data area is at [0x38xx_xxxx, 0x38xx_xxxx].

The runtime data area is currently, in your current image, at
[0x38xx_xxxx, 0x38xx_xxxx].

What happens if a UEFI application registers a configuration table?
Or registers a protocol for use at runtime?

Areas of memory that are not available for UEFI _must_ be marked as
such in the UEFI memory map. Once they are, we can deal with them in
the kernel. If this is not currently being done, that is a bug that
needs fixing.

> > Build a version once, test it, and hope the results remain valid
> > forever? And then when you move the regions and the previously working
> > GRUB now tramples all over them? Or when something changes in upstream
> > GRUB and its memory allocations drifts into the secretly untouchable
> > regions?
> 
> As I said above, UEFI won't touch it. And even UEFI touch it, kernel
> doesn't care since UEFI's lifetime is end.

UEFI's lifetime doesn't end until reset.

> > Are you then going to hack GRUB, release a special HiKey version of
> > GRUB, not support any other versions, and still can your firmware
> > UEFI?
> 
> I don't need to hack GRUB at all.

You will if you're running it under a "UEFI" which has areas you can't
touch and aren't telling it about that.

/
    Leif
Mark Rutland Aug. 25, 2015, 10:42 a.m. UTC | #14
On Tue, Aug 25, 2015 at 11:15:10AM +0100, Haojian Zhuang wrote:
> On Tue, 2015-08-25 at 10:46 +0100, Leif Lindholm wrote:
> > On Tue, Aug 25, 2015 at 04:13:47PM +0800, Haojian Zhuang wrote:
> > > On Mon, 2015-08-24 at 12:49 +0100, Leif Lindholm wrote:
> > > > On Mon, Aug 24, 2015 at 06:19:56PM +0800, Haojian Zhuang wrote:
> > > > > > If your EFI memory map describes the memory as mappable, it is wrong.
> > > > > 
> > > > > When kernel is working, kernel will create its own page table based on
> > > > > UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> > > > > be moved to reserved memblock. Why is it wrong?
> > > > > 
> > > > > In the second, UEFI is firmware. When it's stable, nobody should change
> > > > > it without any reason.
> > > > 
> > > > Much like the memory map.
> > > > 
> > > > > These reserved memory are used in mailbox driver.
> > > > > Look. It's driver, so it could be changed at any time.
> > > > 
> > > > No, it is a set of regions of memory set aside for use by a different
> > > > master in the system as well as communications with that master.
> > > > 
> > > > The fact that there is a driver somewhere that is aware of this is
> > > > entirely beside the point. All agents in the system must adher to this
> > > > protocol.
> > > > 
> > > > > Why do you want
> > > > > to UEFI knowing this memory range? Do you hope UEFI to change when
> > > > > mailbox driver is changed?
> > > > 
> > > > Yes.
> > > > 
> > > > UEFI is a runtime environment. Having random magic areas not to be
> > > > touched will cause random pieces of software running under it to break
> > > > horribly or break other things horribly.
> > > > Unless you mark them as reserved in the UEFI memory map.
> > > > At which point the Linux kernel will automatically ignore them, and
> > > > the proposed patch is redundant.
> > > > 
> > > > So, yes, if you want a system that can boot reliably, run testsuites
> > > > (like SCT or FWTS), run applications (like fastboot ... or the EFI
> > > > stub kernel itself), then any memory regions that is reserved for
> > > > mailbox communication (or other masters in the system) _must_ be
> > > > marked in the EFI memory map.
> > > 
> > > 1. We need support both UEFI and uboot. So the reserved buffer have to
> > > be declared in DTB since they are used by kernel driver, not UEFI.
> > 
> > The buffer may need to be declared in DTB also, but it most certanily
> > needs to be declared in UEFI.
> > 
> > And for the U-Boot case, since it is not memory available to Linux, it
> > should not be declared as "memory".
> 
> Something are messed at here. We have these buffer are used in mailbox.
> They should be allocated as non-cacheable.
> 
> If these buffers are contained in memory memblock in kernel, it means
> that they exist in kernel page table with cachable property. When it's
> used in mailbox driver with non-cachable property, it'll only cause
> cache maintenance issue. So Leo declared these buffers as reserved
> in DT with "no-map" property. It's the key. It could avoid the cache
> maintenance issue.

The better solution is to never describe the memory to the kernel as
memory, by never placing it in a memory node, and ensuring that if it is
in the UEFI memory map, its attributes do not allow it to be mapped.

That way a driver can map it as non-cacheable if it wishes, but nothing
else can possibly touch that memory.

That is all you need to do.

> > > 2. UEFI just loads grub. It's no time to run any other custom EFI
> > > application.
> > 
> > Apart from being completely irrelevant, how are you intending to
> > validate that GRUB never touches these memory regions?
> > 
> 
> GRUB is just a part of bootloader. When linux kernel is running,
> who cares GRUB? GRUB's lifetime is already finished.

If GRUB temporarily maps memory as cacheable, or hands it to a device,
then your statements above about cache maintenance are broken.

An EFI application like GRUB might leave something resident in memory
after it's done (consider the UEFI shim), or it could even load the
kernel into the region that you care about having reserved, because as
far as it's concerned it's just memory. That could leave you with a
conflict for that region of memory.

You _must_ care about GRUB (and other EFI applications) doing the right
thing. To get them to avoid a region of memory, it must not be described
as being usable by them in the UEFI memory map.

> By the way, UEFI code region is at [0x3Dxx_xxxx, 0x3DFF_FFFF]. Those
> mailbox buffer is in [0x05e0_xxxx, 0x06f0_xxxx]. Then I can make sure
> UEFI won't touch the reserved buffer. Even if UEFI touched the reserved
> buffer, is it an issue? Definitely it's not.

It definitely is, due to the possibility of stale cache lines being left
in the region from when UEFI may have mapped it with cacheable
attributes.

> > Build a version once, test it, and hope the results remain valid
> > forever? And then when you move the regions and the previously working
> > GRUB now tramples all over them? Or when something changes in upstream
> > GRUB and its memory allocations drifts into the secretly untouchable
> > regions?
> 
> As I said above, UEFI won't touch it. And even UEFI touch it, kernel
> doesn't care since UEFI's lifetime is end.

If EFI touches it there may be stale cache lines left around, which you
don't seem to expect.

> > Are you then going to hack GRUB, release a special HiKey version of
> > GRUB, not support any other versions, and still can your firmware
> > UEFI?
> 
> I don't need to hack GRUB at all.

Then it is working for you by pure chance alone.

Please listen to the advice you are being given here; we're trying to
ensure that your platform functions (and continues to function) as best
it can.

Thanks,
Mark.
Mark Rutland Aug. 25, 2015, 11:09 a.m. UTC | #15
Hi,

On Tue, Aug 25, 2015 at 09:04:45AM +0100, Haojian Zhuang wrote:
> On Mon, 2015-08-24 at 13:48 +0100, Mark Rutland wrote:
> > > > > > I don't see why you need reserved-memory here, given you're not referring to
> > > > > > these regions by phandle anyway.
> > > > > 
> > > > > - Now we have enabled EFI_STUB, so the memory node will be removed in
> > > > >   kernel:
> > > > >     efi_entry()
> > > > >       \-> allocate_new_fdt_and_exit_boot()
> > > > >             \-> update_fdt();
> > > > > 
> > > > >   Finally in kernel it cannot use memory node to carve out reseved
> > > > >   memory regions.
> > > > > 
> > > > > - On the other hand, DTS's the memory node is to "describes the
> > > > >   physical memory layout for the system"; so it's better to use it only
> > > > >   to describe the hardware info for memory. We can use reserved-memory
> > > > >   to help manage the memory regions which are reserved from software
> > > > >   perspective.
> > > > 
> > > > The fact that you have no-map means that the memory should not be
> > > > described to the kernel as mappable in the first place. It's wrong to
> > > > place such memory in the memory node, even if listed in reserved-memory.
> > > > 
> > > > If your EFI memory map describes the memory as mappable, it is wrong.
> > > 
> > > When kernel is working, kernel will create its own page table based on
> > > UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> > > be moved to reserved memblock. Why is it wrong?
> > 
> > That is a _Linux_ detail, not a _UEFI_ detail.
> > 
> > Anything which only handles UEFI and knows nothing of reserved-memory
> > (e.g. GRUB) will be broken and/or break the Linux use of the region, as
> > it will happily try to allocate memory in the region (and could even
> > decide to reserve it permanently for its own usage).
> > 
> > If the memory is truly specific to the mailbox, then UEFI needs to know
> > that it is reserved as such. If it is not, then it need not be described
> > in the DT and can be allocated dynamically by the kernel.
> 
> No. We support both UEFI and uboot on hikey. We must reserve these
> mailbox buffer in DT. Otherwise, it's a problem to support uboot.

The same solution applies to both: don't describe the memory in the
first place. Don't place it in a memory node, and don't give it
attributes allowing it to be mapped and used in the UEFI memory map.

> We should only deliver one kernel and one DTB to support both UEFI and
> uboot.

I do not disagree with this point generally, but it's irrelevant to the
point at hand.

> And mailbox driver is already upgraded from beginning. Nobody can say
> that it won't be upgraded again in the future.

This doesn't follow. If you want to pointlessly change things, you will
encounter pain. That's not an argument for hacking a partial solution
into the DT and ignoring the problems this causes.

As I tried to ask earlier, how is the mailbox memory used? Does the
kernel configure the address in the hardware, or is this pre-configured?
Could the kernel choose a region of memory to use dynamically?

> By the way, UEFI is loaded in the upper memory region of hikey. It won't
> break anything in linux kernel.

The code might be there, but UEFI can make use of any memory it knows
about for stacks, pool allocations and so on. It needs to be prevented
from using the mailbox memory.

> > > In the second, UEFI is firmware. When it's stable, nobody should change
> > > it without any reason. These reserved memory are used in mailbox driver.
> > > Look. It's driver, so it could be changed at any time. Why do you want
> > > to UEFI knowing this memory range? Do you hope UEFI to change when
> > > mailbox driver is changed?
> > 
> > It shouldn't need to change if that memory is truly reserved for the
> > sole use of the mailbox. If that's not the case then we have a different
> > issue.
> > 
> > If the memory range to use can be allocated by the driver, then I don't
> > see why it should be described in reserved-memory. It certainly should
> > not require a no-map attribute.
> > 
> > Additionally, the driver needs to ensure that the requisite cache
> > maintenance takes place prior to the use of the memory region given
> > prior agents may have ampped it as cacheable, leaving stale (perhaps
> > dirty) lines in the caches.
> > 
> 
> Since those mailbox buffer is declared as reserved with "no-map"
> property, it means that linux kernel won't create the page table of
> them.

I am well aware of how this works. Please re-read my replies, this is
not the issue I describe.

> The meaning of "no-map" is removing it from memory memblock.
> setup_arch()
>   |
>   ---> efi_init() -- Get memory map information from UEFI
>   |
>   ---> arm64_memblock_init()
>   |      |
>   |      ---> early_init_fdt_scan_reserved_mem()
>   |           Get reserved memory buffer from DT. Split memory
>   |           memblock according to reserved buffer.
>   ---> paging_init()
>          |--> map_mem()
>               _Only_ map the discrete memory memblock into kernel
>               page table.
> 
> From this working flow, we could know that those mailbox buffers
> won't be mapped into kernel page table. So there's no concern on
> cache maintenance.

This is simply not true. If UEFI (or any UEFI application) mapped the
memory as cacheable in the past, you need to perform cache maintenance
to get rid of any stale (dirty) cachelines.

This is one of the reasons that the UEFI memory map needs to not
describe the memory as being available to be mapped and used, and why
having a reserved-memory node is insufficient.

> > > > > According to upper info, we still need to use reserved-memory node to
> > > > > depict the reserved memory regions. i have no knowledge about EFI_STUB,
> > > > > so please confirm or correct as needed.
> > > > 
> > > > If the memory shouldn't be mapped, it should neither be in the memory
> > > > node nor EFI memory map (with attributes allowing it to be mapped) to
> > > > begin with.
> > > 
> > > As I said above, kernel will create its own page table. When kernel's
> > > page table is working, UEFI's page table is destroying. So the memory
> > > won't be mapped twice at the same time. What's wrong?
> > > > 
> > > > As far as I can see you do not need to use reserved-memory.
> > > 
> > > 1. Are we talking on the same thing? Leo already mentioned that all
> > > memory node in DTB will be destroyed by kernel when EFI_STUB is enabled
> > > on arm. Did you read the source code after his reply?
> > > And you suggested that Leo to use discrete memory region in DTB. It is
> > > really wrong. Kernel only gets memory map information from UEFI, not
> > > DTB.
> > 
> > I did _not_ suggest that Leo use discrete memory. I suggested that
> > reserved regions should not be described in the memory node (the same
> > premise applying to the UEFI memory map).
> 
> I agree that reserved region shouldn't be described in the memory node.
> And Leo didn't describe reserved region in memory node too.

The issue is that the region you describe in reserved-memory also falls
within an existing region in a memory node (or the UEFI memory map).

The no-map forces a memblock_remove after the memory was added to
memblock, but before it was mapped. This ensures Linux won't map the
memory. This does not ensure that UEFI or UEFI application will not map,
reserve, or use the memory for their own purposes.

> > w.r.t. UEFI, please see my comments above. If you're using the UEFI
> > memory map, you have to use the UEFI memory map, not the UEFI memory map
> > with additional (non-UEFI) caveats applied atop.
> 
> The main concern is that we're supporting both UEFI and uboot.
> Declaring these reserved buffer in DTB will be a better choice.

Unfortunately this is insufficient. This leaves a slew of problems when
using UEFI, and given that, it's not a common solution.

Using reserved-memory in this case only gives a semblance of
correctness, but doesn't mean you are doing the right thing.

> > > 4. Again and again. Memory node should be only used to describe the
> > > RAM information.
> > 
> > The memory node describes the memory available to the OS. There are some
> > caveats w.r.t. /memreserve/, regions which may be mapped but remain
> > unused and so on, but the memory node does generally encode a policy
> > that the memory may be used.
> > 
> > Describing unusable memory in a memory node is pointless, and has an
> > adverse effect on clients which don't support reserved-memory. It's
> > doubly harmful when that memory should never be mapped.
> > 
> 
> He didn't declare those buffer in memory node. He only declared it
> in reserved-memory node. And it's not never be mapped. He use ioremap()
> in the driver.

If the memory is not in the memory nodes nor the UEFI memory map, there
is no need for a reserved-memory entry. If it exists in the UEFI memory
map, the reserved-memory entry is insufficient for the reasons I have
stated previously.

Likewise for the MCU firmware memory.

Thanks,
Mark.
Sudeep Holla Aug. 25, 2015, 11:36 a.m. UTC | #16
On 19/08/15 10:37, Leo Yan wrote:
> On Hi6220, below memory regions in DDR have specific purpose:
>
>    0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
>    0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
>    0x06df,f000 - 0x06df,ffff: For mailbox message data.
>

Unless I am reading the DTS file completely wrong, I don't think the 
above memory regions are in DDR as per the memory node.

> This patch reserves these memory regions and add device node for
> mailbox in dts.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>   arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
>   arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
>   2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> index e36a539..d5470d3 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> @@ -7,9 +7,6 @@
>
>   /dts-v1/;
>
> -/*Reserved 1MB memory for MCU*/
> -/memreserve/ 0x05e00000 0x00100000;
> -
>   #include "hi6220.dtsi"
>
>   / {
> @@ -28,4 +25,21 @@
>   		device_type = "memory";
>   		reg = <0x0 0x0 0x0 0x40000000>;
>   	};

I have no access to the spec, but I read this as 1GB RAM @0x0
Unless this entry is completely wrong, what your commit log claims is
incorrect. If this entry is wrong I wonder how is it booting with this
DT then.

> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		mcu-buf@05e00000 {
> +			no-map;
> +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */

So I don't see how can this be part of DDR ? Or at-least part of DDR
that's mapped by kernel ?

Regards,
Sudeep
Haojian Zhuang Aug. 25, 2015, 1:43 p.m. UTC | #17
On Tue, 2015-08-25 at 11:42 +0100, Mark Rutland wrote:
> > > Are you then going to hack GRUB, release a special HiKey version of
> > > GRUB, not support any other versions, and still can your firmware
> > > UEFI?
> > 
> > I don't need to hack GRUB at all.
> 
> Then it is working for you by pure chance alone.
> 
> Please listen to the advice you are being given here; we're trying to
> ensure that your platform functions (and continues to function) as best
> it can.

Since we discussed a lot on this, let's make a conclusion on it.

1. UEFI could append the reserved buffer in it's memory mapping.
2. These reserved buffer must be declared in DT, since we also need to
   support non-UEFI (uboot) at the same time.
3. Mailbox node should reference reserved buffer by phandle in DT. Then
   map the buffer as non-cacheable in driver.
4. These reserved buffer must use "no-map" property since it should be
   non-cacheable in driver.
5. A patch is necessary in kernel. If efi stub feature is enabled,
   arm kernel should not parse memory node or reserved memory buffer in
   DT any more. Arm kernel should either fetch memory information from 
   efi or DT. Currently arm kernel fetch both efi memory information and
   reserved buffer from DTB at the same time.

Do you agree on these points?

Regards
Haojian
Leo Yan Aug. 25, 2015, 2:04 p.m. UTC | #18
Hi Sudeep,

On Tue, Aug 25, 2015 at 12:36:12PM +0100, Sudeep Holla wrote:
> 
> 
> On 19/08/15 10:37, Leo Yan wrote:
> >On Hi6220, below memory regions in DDR have specific purpose:
> >
> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> >   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> >
> 
> Unless I am reading the DTS file completely wrong, I don't think the
> above memory regions are in DDR as per the memory node.

i'm not sure if understand correctly for your question; Hikey board
has DDR 1GB@0x0, but there have some memory regions are used for MCU.

So this patch is to reserve these memory regions so that make sure
kernel will not map and allocate them.

Will remove these memory regions from memory node in next version.

> >This patch reserves these memory regions and add device node for
> >mailbox in dts.
> >
> >Signed-off-by: Leo Yan <leo.yan@linaro.org>
> >---
> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
> >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
> >  2 files changed, 25 insertions(+), 3 deletions(-)
> >
> >diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> >index e36a539..d5470d3 100644
> >--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> >+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> >@@ -7,9 +7,6 @@
> >
> >  /dts-v1/;
> >
> >-/*Reserved 1MB memory for MCU*/
> >-/memreserve/ 0x05e00000 0x00100000;
> >-
> >  #include "hi6220.dtsi"
> >
> >  / {
> >@@ -28,4 +25,21 @@
> >  		device_type = "memory";
> >  		reg = <0x0 0x0 0x0 0x40000000>;
> >  	};
> 
> I have no access to the spec, but I read this as 1GB RAM @0x0
> Unless this entry is completely wrong, what your commit log claims is
> incorrect. If this entry is wrong I wonder how is it booting with this
> DT then.

Do you mean should remove all reserved memory regions from memory
node? Will submit next version's patch for this.

Kernel can boot successfully on Hikey with this patch [1].

[1] https://github.com/96boards/linux

> >+
> >+	reserved-memory {
> >+		#address-cells = <2>;
> >+		#size-cells = <2>;
> >+		ranges;
> >+
> >+		mcu-buf@05e00000 {
> >+			no-map;
> >+			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> >+			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> 
> So I don't see how can this be part of DDR ? Or at-least part of DDR
> that's mapped by kernel ?

Here use reserved-memory node to remove these regions from memblock
during kernel's boot up; kernel also will not map for them with
property "no-map".

I think this is the same question which have been brought up by Mark
in his early mail and suggested to use UEFI to do this.

Thanks,
Leo Yan
Sudeep Holla Aug. 25, 2015, 2:13 p.m. UTC | #19
On 25/08/15 15:04, Leo Yan wrote:
> Hi Sudeep,
>
> On Tue, Aug 25, 2015 at 12:36:12PM +0100, Sudeep Holla wrote:
>>
>>
>> On 19/08/15 10:37, Leo Yan wrote:
>>> On Hi6220, below memory regions in DDR have specific purpose:
>>>
>>>    0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
>>>    0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
>>>    0x06df,f000 - 0x06df,ffff: For mailbox message data.
>>>
>>
>> Unless I am reading the DTS file completely wrong, I don't think the
>> above memory regions are in DDR as per the memory node.
>
> i'm not sure if understand correctly for your question; Hikey board
> has DDR 1GB@0x0, but there have some memory regions are used for MCU.
>

Ah, I misread the address range, left the leading zero and assumed they
are not in DDR range. Sorry for the noise.

Regards,
Sudeep
Leif Lindholm Aug. 25, 2015, 2:24 p.m. UTC | #20
On Tue, Aug 25, 2015 at 09:43:14PM +0800, Haojian Zhuang wrote:
> Since we discussed a lot on this, let's make a conclusion on it.
> 
> 1. UEFI could append the reserved buffer in it's memory mapping.

Yes. It needs to.

(I will let Mark comment on points 2-4.)

> 5. A patch is necessary in kernel. If efi stub feature is enabled,
>    arm kernel should not parse memory node or reserved memory buffer in
>    DT any more.

This is already the case. The stub deletes any present memory nodes and
reserved entries in drivers/firmware/efi/libstub/fdt.c:update_fdt().

Then, during setup_arch(), arch/arm64/kernel/efi.c:efi_init() calls
reserve_regions(), which adds only those memory regions available for
use by Linux as RAM to memblock.

>    Arm kernel should either fetch memory information from 
>    efi or DT.

Absolutely.

>    Currently arm kernel fetch both efi memory information and
>    reserved buffer from DTB at the same time.

No, it does not.

Regards,

Leif
Ard Biesheuvel Aug. 25, 2015, 2:51 p.m. UTC | #21
On 25 August 2015 at 16:24, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Aug 25, 2015 at 09:43:14PM +0800, Haojian Zhuang wrote:
>> Since we discussed a lot on this, let's make a conclusion on it.
>>
>> 1. UEFI could append the reserved buffer in it's memory mapping.
>
> Yes. It needs to.
>
> (I will let Mark comment on points 2-4.)
>
>> 5. A patch is necessary in kernel. If efi stub feature is enabled,
>>    arm kernel should not parse memory node or reserved memory buffer in
>>    DT any more.
>
> This is already the case. The stub deletes any present memory nodes and
> reserved entries in drivers/firmware/efi/libstub/fdt.c:update_fdt().
>
> Then, during setup_arch(), arch/arm64/kernel/efi.c:efi_init() calls
> reserve_regions(), which adds only those memory regions available for
> use by Linux as RAM to memblock.
>
>>    Arm kernel should either fetch memory information from
>>    efi or DT.
>
> Absolutely.
>
>>    Currently arm kernel fetch both efi memory information and
>>    reserved buffer from DTB at the same time.
>
> No, it does not.
>

It should not, but it does. Due to an oversight, the stub removes
/memreserve/ entries but ignores the reserved-memory node completely.

This was reported here in fact

http://thread.gmane.org/gmane.linux.kernel.efi/5736/focus=5742

but there has not been a followup to this series.

I think it is fine to keep those memory reservations in the DT, but
you should simply understand that UEFI does not parse the DT, so you
need to tell it which memory it cannot touch. Otherwise, the firmware
itself or anything that executes under it (UEFI drivers, the UEFI
Shell, GRUB, the UEFI stub in the kernel) will think it is available
and may allocate it for its own use. This may include runtime services
regions that will remain reserved even after exiting boot services.
Leif Lindholm Aug. 25, 2015, 3:37 p.m. UTC | #22
On Tue, Aug 25, 2015 at 04:51:22PM +0200, Ard Biesheuvel wrote:
> >>    Arm kernel should either fetch memory information from
> >>    efi or DT.
> >
> > Absolutely.
> >
> >>    Currently arm kernel fetch both efi memory information and
> >>    reserved buffer from DTB at the same time.
> >
> > No, it does not.
> 
> It should not, but it does. Due to an oversight, the stub removes
> /memreserve/ entries but ignores the reserved-memory node completely.

Urgh.

> This was reported here in fact
> 
> http://thread.gmane.org/gmane.linux.kernel.efi/5736/focus=5742
> 
> but there has not been a followup to this series.

Are all of those patches still relevant, or did some of them go in
already?

Haojian: can you give that patch a spin and see if it does what you
need, combined with adding the reserved areas to the UEFI memory map?

/
    Leif
Ard Biesheuvel Aug. 25, 2015, 3:45 p.m. UTC | #23
On 25 August 2015 at 17:37, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Aug 25, 2015 at 04:51:22PM +0200, Ard Biesheuvel wrote:
>> >>    Arm kernel should either fetch memory information from
>> >>    efi or DT.
>> >
>> > Absolutely.
>> >
>> >>    Currently arm kernel fetch both efi memory information and
>> >>    reserved buffer from DTB at the same time.
>> >
>> > No, it does not.
>>
>> It should not, but it does. Due to an oversight, the stub removes
>> /memreserve/ entries but ignores the reserved-memory node completely.
>
> Urgh.
>
>> This was reported here in fact
>>
>> http://thread.gmane.org/gmane.linux.kernel.efi/5736/focus=5742
>>
>> but there has not been a followup to this series.
>
> Are all of those patches still relevant, or did some of them go in
> already?
>

The first two patches are in v4.2-rc1 and up, the others should still
apply on top of that.
Leo Yan Aug. 25, 2015, 4 p.m. UTC | #24
On Tue, Aug 25, 2015 at 09:43:14PM +0800, Haojian Zhuang wrote:
> On Tue, 2015-08-25 at 11:42 +0100, Mark Rutland wrote:
> > > > Are you then going to hack GRUB, release a special HiKey version of
> > > > GRUB, not support any other versions, and still can your firmware
> > > > UEFI?
> > > 
> > > I don't need to hack GRUB at all.
> > 
> > Then it is working for you by pure chance alone.
> > 
> > Please listen to the advice you are being given here; we're trying to
> > ensure that your platform functions (and continues to function) as best
> > it can.
> 
> Since we discussed a lot on this, let's make a conclusion on it.
> 
> 1. UEFI could append the reserved buffer in it's memory mapping.
> 2. These reserved buffer must be declared in DT, since we also need to
>    support non-UEFI (uboot) at the same time.
> 3. Mailbox node should reference reserved buffer by phandle in DT. Then
>    map the buffer as non-cacheable in driver.
> 4. These reserved buffer must use "no-map" property since it should be
>    non-cacheable in driver.

For more specific discussion for DTS, i list two options at here;

- Option 1: just simply reserve memory regions through memory node,
  and mailbox node will directly use the buffer through reg ranges;

- Option 2: use reserved-memory and mailbox node will refer phandle
  of reserved-memory;

These two options both can work well with UEFI and Uboot, but option 1
is more simple and straightforward; so i personally prefer it. But
look forwarding your guys' suggestion.

Option 1:

	memory@0 {
		device_type = "memory";
		reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
		      <0x00000000 0x05f00000 0x00000000 0x00eff000>,
		      <0x00000000 0x06e00000 0x00000000 0x0060f000>,
		      <0x00000000 0x07410000 0x00000000 0x38bf0000>;
	};

        [...]

	mailbox: mailbox@f7510000 {
		#mbox-cells = <1>;
		compatible = "hisilicon,hi6220-mbox";
		reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
		      <0x0 0x06dff800 0x0 0x0800>; /* Mailbox buffer */
		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
	};

Option 2:

	memory@0 {
		device_type = "memory";
		reg = <0x0 0x0 0x0 0x40000000>;
	};

	reserved-memory {
		#address-cells = <2>;
		#size-cells = <2>;
		ranges;

		mcu_reserved: mcu_reserved@06dff000 {
			no-map;
			reg = <0x0 0x06dff000 0x0 0x00001000>,	/* MCU mailbox buffer */
			      <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
		};
	};

        [...]

	mailbox: mailbox@f7510000 {
		#mbox-cells = <1>;
		compatible = "hisilicon,hi6220-mbox";
		reg = <0x0 0xf7510000 0x0 0x1000>; /* IPC_S */
		memory-region = <&mcu_reserved>;   /* Mailbox buffer */
		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
	};


> 5. A patch is necessary in kernel. If efi stub feature is enabled,
>    arm kernel should not parse memory node or reserved memory buffer in
>    DT any more. Arm kernel should either fetch memory information from 
>    efi or DT. Currently arm kernel fetch both efi memory information and
>    reserved buffer from DTB at the same time.
> 
> Do you agree on these points?
> 
> Regards
> Haojian
>
Haojian Zhuang Aug. 26, 2015, 1:25 a.m. UTC | #25
On Wed, 2015-08-26 at 00:00 +0800, Leo Yan wrote:
> On Tue, Aug 25, 2015 at 09:43:14PM +0800, Haojian Zhuang wrote:
> > On Tue, 2015-08-25 at 11:42 +0100, Mark Rutland wrote:
> > > > > Are you then going to hack GRUB, release a special HiKey version of
> > > > > GRUB, not support any other versions, and still can your firmware
> > > > > UEFI?
> > > > 
> > > > I don't need to hack GRUB at all.
> > > 
> > > Then it is working for you by pure chance alone.
> > > 
> > > Please listen to the advice you are being given here; we're trying to
> > > ensure that your platform functions (and continues to function) as best
> > > it can.
> > 
> > Since we discussed a lot on this, let's make a conclusion on it.
> > 
> > 1. UEFI could append the reserved buffer in it's memory mapping.
> > 2. These reserved buffer must be declared in DT, since we also need to
> >    support non-UEFI (uboot) at the same time.
> > 3. Mailbox node should reference reserved buffer by phandle in DT. Then
> >    map the buffer as non-cacheable in driver.
> > 4. These reserved buffer must use "no-map" property since it should be
> >    non-cacheable in driver.
> 
> For more specific discussion for DTS, i list two options at here;
> 
> - Option 1: just simply reserve memory regions through memory node,
>   and mailbox node will directly use the buffer through reg ranges;
> 
> - Option 2: use reserved-memory and mailbox node will refer phandle
>   of reserved-memory;
> 
> These two options both can work well with UEFI and Uboot, but option 1
> is more simple and straightforward; so i personally prefer it. But
> look forwarding your guys' suggestion.
> 
> Option 1:
> 
> 	memory@0 {
> 		device_type = "memory";
> 		reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> 		      <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> 		      <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> 		      <0x00000000 0x07410000 0x00000000 0x38bf0000>;
> 	};
> 
>         [...]
> 
> 	mailbox: mailbox@f7510000 {
> 		#mbox-cells = <1>;
> 		compatible = "hisilicon,hi6220-mbox";
> 		reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
> 		      <0x0 0x06dff800 0x0 0x0800>; /* Mailbox buffer */
> 		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 
> Option 2:
> 
> 	memory@0 {
> 		device_type = "memory";
> 		reg = <0x0 0x0 0x0 0x40000000>;
> 	};
> 
> 	reserved-memory {
> 		#address-cells = <2>;
> 		#size-cells = <2>;
> 		ranges;
> 
> 		mcu_reserved: mcu_reserved@06dff000 {
> 			no-map;
> 			reg = <0x0 0x06dff000 0x0 0x00001000>,	/* MCU mailbox buffer */
> 			      <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> 			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> 		};
> 	};
> 
>         [...]
> 
> 	mailbox: mailbox@f7510000 {
> 		#mbox-cells = <1>;
> 		compatible = "hisilicon,hi6220-mbox";
> 		reg = <0x0 0xf7510000 0x0 0x1000>; /* IPC_S */
> 		memory-region = <&mcu_reserved>;   /* Mailbox buffer */
> 		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> 	};

I prefer the second one. From my view, memory node should only describe
the hardware information of memory.

Regards
Haojian
Haojian Zhuang Aug. 26, 2015, 2:41 a.m. UTC | #26
On Tue, 2015-08-25 at 16:37 +0100, Leif Lindholm wrote:
> On Tue, Aug 25, 2015 at 04:51:22PM +0200, Ard Biesheuvel wrote:
> > >>    Arm kernel should either fetch memory information from
> > >>    efi or DT.
> > >
> > > Absolutely.
> > >
> > >>    Currently arm kernel fetch both efi memory information and
> > >>    reserved buffer from DTB at the same time.
> > >
> > > No, it does not.
> > 
> > It should not, but it does. Due to an oversight, the stub removes
> > /memreserve/ entries but ignores the reserved-memory node completely.
> 
> Urgh.
> 
> > This was reported here in fact
> > 
> > http://thread.gmane.org/gmane.linux.kernel.efi/5736/focus=5742
> > 
> > but there has not been a followup to this series.
> 
> Are all of those patches still relevant, or did some of them go in
> already?
> 
> Haojian: can you give that patch a spin and see if it does what you
> need, combined with adding the reserved areas to the UEFI memory map?
> 
> /
>     Leif

It's so nice. This patch is what I need.

Thanks
Haojian
Leo Yan Aug. 26, 2015, 6:59 a.m. UTC | #27
Hi Haojian,

On Wed, Aug 26, 2015 at 09:25:41AM +0800, Haojian Zhuang wrote:
> On Wed, 2015-08-26 at 00:00 +0800, Leo Yan wrote:
> > On Tue, Aug 25, 2015 at 09:43:14PM +0800, Haojian Zhuang wrote:
> > > On Tue, 2015-08-25 at 11:42 +0100, Mark Rutland wrote:
> > > > > > Are you then going to hack GRUB, release a special HiKey version of
> > > > > > GRUB, not support any other versions, and still can your firmware
> > > > > > UEFI?
> > > > > 
> > > > > I don't need to hack GRUB at all.
> > > > 
> > > > Then it is working for you by pure chance alone.
> > > > 
> > > > Please listen to the advice you are being given here; we're trying to
> > > > ensure that your platform functions (and continues to function) as best
> > > > it can.
> > > 
> > > Since we discussed a lot on this, let's make a conclusion on it.
> > > 
> > > 1. UEFI could append the reserved buffer in it's memory mapping.
> > > 2. These reserved buffer must be declared in DT, since we also need to
> > >    support non-UEFI (uboot) at the same time.
> > > 3. Mailbox node should reference reserved buffer by phandle in DT. Then
> > >    map the buffer as non-cacheable in driver.
> > > 4. These reserved buffer must use "no-map" property since it should be
> > >    non-cacheable in driver.
> > 
> > For more specific discussion for DTS, i list two options at here;
> > 
> > - Option 1: just simply reserve memory regions through memory node,
> >   and mailbox node will directly use the buffer through reg ranges;
> > 
> > - Option 2: use reserved-memory and mailbox node will refer phandle
> >   of reserved-memory;
> > 
> > These two options both can work well with UEFI and Uboot, but option 1
> > is more simple and straightforward; so i personally prefer it. But
> > look forwarding your guys' suggestion.
> > 
> > Option 1:
> > 
> > 	memory@0 {
> > 		device_type = "memory";
> > 		reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> > 		      <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> > 		      <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> > 		      <0x00000000 0x07410000 0x00000000 0x38bf0000>;
> > 	};
> > 
> >         [...]
> > 
> > 	mailbox: mailbox@f7510000 {
> > 		#mbox-cells = <1>;
> > 		compatible = "hisilicon,hi6220-mbox";
> > 		reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
> > 		      <0x0 0x06dff800 0x0 0x0800>; /* Mailbox buffer */
> > 		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> > 
> > Option 2:
> > 
> > 	memory@0 {
> > 		device_type = "memory";
> > 		reg = <0x0 0x0 0x0 0x40000000>;
> > 	};
> > 
> > 	reserved-memory {
> > 		#address-cells = <2>;
> > 		#size-cells = <2>;
> > 		ranges;
> > 
> > 		mcu_reserved: mcu_reserved@06dff000 {
> > 			no-map;
> > 			reg = <0x0 0x06dff000 0x0 0x00001000>,	/* MCU mailbox buffer */
> > 			      <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > 			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > 		};
> > 	};
> > 
> >         [...]
> > 
> > 	mailbox: mailbox@f7510000 {
> > 		#mbox-cells = <1>;
> > 		compatible = "hisilicon,hi6220-mbox";
> > 		reg = <0x0 0xf7510000 0x0 0x1000>; /* IPC_S */
> > 		memory-region = <&mcu_reserved>;   /* Mailbox buffer */
> > 		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> 
> I prefer the second one. From my view, memory node should only describe
> the hardware information of memory.

Yes, option 2 will be more simple for memory node.

But option 2 also will introduce complexity for mailbox node, due mailbox
driver need use property "reg" and "memory-region" to sepeately depict
the regions for mailbox's ipc and slots. If later mailbox is designed to
use SRAM for both ipc and slots, then it will no matter with DDR anymore,
in this case option 1 will easily switch to support it.

Another question is a general question: for Linux kernel, which is the
best method to reserve memory regions? According to previous discussion,
we can use /memory/ node or /reseved-memory/ node to reserve memory
regions.

when review Juno's dts, i also see there have reserved 16MB DDR for secure
world. If so, looks like /reserved-memory/ node is unnecessary. if have some
specific scenarios will we use reserved-memory node to help reserve memory
regions?

Thanks,
Leo Yan
Daniel Thompson Aug. 27, 2015, 3:54 p.m. UTC | #28
On 26/08/15 02:25, Haojian Zhuang wrote:
>> Option 1:
>>
>> 	memory@0 {
>> 		device_type = "memory";
>> 		reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
>> 		      <0x00000000 0x05f00000 0x00000000 0x00eff000>,
>> 		      <0x00000000 0x06e00000 0x00000000 0x0060f000>,
>> 		      <0x00000000 0x07410000 0x00000000 0x38bf0000>;
>> 	};
>>
>> [snip]
 >>
>> Option 2:
>>
>> 	memory@0 {
>> 		device_type = "memory";
>> 		reg = <0x0 0x0 0x0 0x40000000>;
>> 	};
>>
 >> [snip]
 >>
>
> I prefer the second one. From my view, memory node should only describe
> the hardware information of memory.

Haven't we already established that, to avoid the risk of UEFI 
applications accessing inappropriate memory locations, a (correct) UEFI 
implementation must use, and pass to the kernel, a memory map that looks 
like option 1?

That being the case why would we want u-boot (or any other similar 
bootloader) to pass a memory map that is gratuitously different to the 
one supplied by UEFI?


Daniel.
Mark Rutland Aug. 27, 2015, 4:31 p.m. UTC | #29
On Wed, Aug 26, 2015 at 07:59:50AM +0100, Leo Yan wrote:
> Hi Haojian,
> 
> On Wed, Aug 26, 2015 at 09:25:41AM +0800, Haojian Zhuang wrote:
> > On Wed, 2015-08-26 at 00:00 +0800, Leo Yan wrote:
> > > On Tue, Aug 25, 2015 at 09:43:14PM +0800, Haojian Zhuang wrote:
> > > > On Tue, 2015-08-25 at 11:42 +0100, Mark Rutland wrote:
> > > > > > > Are you then going to hack GRUB, release a special HiKey version of
> > > > > > > GRUB, not support any other versions, and still can your firmware
> > > > > > > UEFI?
> > > > > > 
> > > > > > I don't need to hack GRUB at all.
> > > > > 
> > > > > Then it is working for you by pure chance alone.
> > > > > 
> > > > > Please listen to the advice you are being given here; we're trying to
> > > > > ensure that your platform functions (and continues to function) as best
> > > > > it can.
> > > > 
> > > > Since we discussed a lot on this, let's make a conclusion on it.
> > > > 
> > > > 1. UEFI could append the reserved buffer in it's memory mapping.
> > > > 2. These reserved buffer must be declared in DT, since we also need to
> > > >    support non-UEFI (uboot) at the same time.
> > > > 3. Mailbox node should reference reserved buffer by phandle in DT. Then
> > > >    map the buffer as non-cacheable in driver.
> > > > 4. These reserved buffer must use "no-map" property since it should be
> > > >    non-cacheable in driver.
> > > 
> > > For more specific discussion for DTS, i list two options at here;
> > > 
> > > - Option 1: just simply reserve memory regions through memory node,
> > >   and mailbox node will directly use the buffer through reg ranges;
> > > 
> > > - Option 2: use reserved-memory and mailbox node will refer phandle
> > >   of reserved-memory;
> > > 
> > > These two options both can work well with UEFI and Uboot, but option 1
> > > is more simple and straightforward; so i personally prefer it. But
> > > look forwarding your guys' suggestion.
> > > 
> > > Option 1:
> > > 
> > > 	memory@0 {
> > > 		device_type = "memory";
> > > 		reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> > > 		      <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> > > 		      <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> > > 		      <0x00000000 0x07410000 0x00000000 0x38bf0000>;
> > > 	};
> > > 
> > >         [...]
> > > 
> > > 	mailbox: mailbox@f7510000 {
> > > 		#mbox-cells = <1>;
> > > 		compatible = "hisilicon,hi6220-mbox";
> > > 		reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
> > > 		      <0x0 0x06dff800 0x0 0x0800>; /* Mailbox buffer */
> > > 		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> > > 	};
> > > 
> > > Option 2:
> > > 
> > > 	memory@0 {
> > > 		device_type = "memory";
> > > 		reg = <0x0 0x0 0x0 0x40000000>;
> > > 	};
> > > 
> > > 	reserved-memory {
> > > 		#address-cells = <2>;
> > > 		#size-cells = <2>;
> > > 		ranges;
> > > 
> > > 		mcu_reserved: mcu_reserved@06dff000 {
> > > 			no-map;
> > > 			reg = <0x0 0x06dff000 0x0 0x00001000>,	/* MCU mailbox buffer */
> > > 			      <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > > 			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > > 		};
> > > 	};
> > > 
> > >         [...]
> > > 
> > > 	mailbox: mailbox@f7510000 {
> > > 		#mbox-cells = <1>;
> > > 		compatible = "hisilicon,hi6220-mbox";
> > > 		reg = <0x0 0xf7510000 0x0 0x1000>; /* IPC_S */
> > > 		memory-region = <&mcu_reserved>;   /* Mailbox buffer */
> > > 		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> > > 	};
> > 
> > I prefer the second one. From my view, memory node should only describe
> > the hardware information of memory.
> 
> Yes, option 2 will be more simple for memory node.
> 
> But option 2 also will introduce complexity for mailbox node, due mailbox
> driver need use property "reg" and "memory-region" to sepeately depict
> the regions for mailbox's ipc and slots. If later mailbox is designed to
> use SRAM for both ipc and slots, then it will no matter with DDR anymore,
> in this case option 1 will easily switch to support it.
> 
> Another question is a general question: for Linux kernel, which is the
> best method to reserve memory regions? According to previous discussion,
> we can use /memory/ node or /reseved-memory/ node to reserve memory
> regions.

If the memory is truly reserved for a purpose and cannot be used for
anything else, I don't think it should be in the memory node at all, and
should be carved out. That aligns with what you'd do in UEFI (either not
listing the region in the memory map, or listing it with attributes such
that it may not be mapped and/or used).

I don't see much of a reason for /memreserve/, as it can cause issues
(by allowing the OS to map the region with cacheable attributes), and is
not as rigorously specified for ARM as it is for Power in ePAPR.

I understand that reserved-memory is for carving out (potentially
reusable) memory pools for devices or other special uses (perhaps a
panic log). Usually such memory may also be used by the kernel for its
own purposes if not presently required by the device.

Having an entry in reserved-memory does not necessitate the region also
appears in memory nodes, and if a region cannot be used by an OS (or
other software) for other purposes, I would not expect it to be describe
in any memory node. That will prevent other software (e.g. bootloaders)
from erroneously using the memory.

If you have a region described with no-map, I would expect that this
doesn't exist in any memory node or the UEFI memory map, and is only
under reserved-memory so it may be referred to by phandle in a
consistent manner.

> when review Juno's dts, i also see there have reserved 16MB DDR for secure
> world. If so, looks like /reserved-memory/ node is unnecessary. if have some
> specific scenarios will we use reserved-memory node to help reserve memory
> regions?

I'd expect shared DMA pools to appear in reserved-memory. The OS can
choose to use these or ignore them if it chooses (or is otherwise forced
to, e.g. were it loaded over one).

Thanks,
Mark.
Mark Rutland Aug. 27, 2015, 4:46 p.m. UTC | #30
> > Option 2:
> > 
> > 	memory@0 {
> > 		device_type = "memory";
> > 		reg = <0x0 0x0 0x0 0x40000000>;
> > 	};
> > 
> > 	reserved-memory {
> > 		#address-cells = <2>;
> > 		#size-cells = <2>;
> > 		ranges;
> > 
> > 		mcu_reserved: mcu_reserved@06dff000 {
> > 			no-map;
> > 			reg = <0x0 0x06dff000 0x0 0x00001000>,	/* MCU mailbox buffer */
> > 			      <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > 			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > 		};
> > 	};
> > 
> >         [...]
> > 
> > 	mailbox: mailbox@f7510000 {
> > 		#mbox-cells = <1>;
> > 		compatible = "hisilicon,hi6220-mbox";
> > 		reg = <0x0 0xf7510000 0x0 0x1000>; /* IPC_S */
> > 		memory-region = <&mcu_reserved>;   /* Mailbox buffer */
> > 		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> 
> I prefer the second one. From my view, memory node should only describe
> the hardware information of memory.

That doesn't align with the spec. Per ePAPR, in the description of a
memory node:

	The client program may access memory not covered by any memory
	reservations (see section 8.3) using any storage attributes it
	chooses. However, before changing the storage attributes used to
	access a real page, the client program is responsible for
	performing actions required by the architecture and
	implementation, possibly including flushing the real page from
	the caches.

Note that in this context, memory reservation applies to /memreserve/.
We can only expect other software to handle /memreserve/, and not
reserved-memory, as the latter was introduced by Linux and has not
existed for anywhere near as long.

Additionally, the OS is permitted to map reserved memory with cacheable
attributes.

So the memory nodes have never been about the raw hardware layout, but
rather the regions that the OS may map. If (outside of the driver
responsible for the region) the OS should not map a region of memory,
that region should not appear in any memory node.

As mentioned in my other reply, for a region that the OS could map but
cannot use, I don't see much point in listing that memory in any memory
node.

Thanks,
Mark
Leo Yan Aug. 28, 2015, 6:37 a.m. UTC | #31
Hi Mark,

On Thu, Aug 27, 2015 at 05:31:09PM +0100, Mark Rutland wrote:
> On Wed, Aug 26, 2015 at 07:59:50AM +0100, Leo Yan wrote:
> > Hi Haojian,
> > 
> > On Wed, Aug 26, 2015 at 09:25:41AM +0800, Haojian Zhuang wrote:
> > > On Wed, 2015-08-26 at 00:00 +0800, Leo Yan wrote:
> > > > On Tue, Aug 25, 2015 at 09:43:14PM +0800, Haojian Zhuang wrote:
> > > > > On Tue, 2015-08-25 at 11:42 +0100, Mark Rutland wrote:
> > > > > > > > Are you then going to hack GRUB, release a special HiKey version of
> > > > > > > > GRUB, not support any other versions, and still can your firmware
> > > > > > > > UEFI?
> > > > > > > 
> > > > > > > I don't need to hack GRUB at all.
> > > > > > 
> > > > > > Then it is working for you by pure chance alone.
> > > > > > 
> > > > > > Please listen to the advice you are being given here; we're trying to
> > > > > > ensure that your platform functions (and continues to function) as best
> > > > > > it can.
> > > > > 
> > > > > Since we discussed a lot on this, let's make a conclusion on it.
> > > > > 
> > > > > 1. UEFI could append the reserved buffer in it's memory mapping.
> > > > > 2. These reserved buffer must be declared in DT, since we also need to
> > > > >    support non-UEFI (uboot) at the same time.
> > > > > 3. Mailbox node should reference reserved buffer by phandle in DT. Then
> > > > >    map the buffer as non-cacheable in driver.
> > > > > 4. These reserved buffer must use "no-map" property since it should be
> > > > >    non-cacheable in driver.
> > > > 
> > > > For more specific discussion for DTS, i list two options at here;
> > > > 
> > > > - Option 1: just simply reserve memory regions through memory node,
> > > >   and mailbox node will directly use the buffer through reg ranges;
> > > > 
> > > > - Option 2: use reserved-memory and mailbox node will refer phandle
> > > >   of reserved-memory;
> > > > 
> > > > These two options both can work well with UEFI and Uboot, but option 1
> > > > is more simple and straightforward; so i personally prefer it. But
> > > > look forwarding your guys' suggestion.
> > > > 
> > > > Option 1:
> > > > 
> > > > 	memory@0 {
> > > > 		device_type = "memory";
> > > > 		reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> > > > 		      <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> > > > 		      <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> > > > 		      <0x00000000 0x07410000 0x00000000 0x38bf0000>;
> > > > 	};
> > > > 
> > > >         [...]
> > > > 
> > > > 	mailbox: mailbox@f7510000 {
> > > > 		#mbox-cells = <1>;
> > > > 		compatible = "hisilicon,hi6220-mbox";
> > > > 		reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
> > > > 		      <0x0 0x06dff800 0x0 0x0800>; /* Mailbox buffer */
> > > > 		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> > > > 	};
> > > > 
> > > > Option 2:
> > > > 
> > > > 	memory@0 {
> > > > 		device_type = "memory";
> > > > 		reg = <0x0 0x0 0x0 0x40000000>;
> > > > 	};
> > > > 
> > > > 	reserved-memory {
> > > > 		#address-cells = <2>;
> > > > 		#size-cells = <2>;
> > > > 		ranges;
> > > > 
> > > > 		mcu_reserved: mcu_reserved@06dff000 {
> > > > 			no-map;
> > > > 			reg = <0x0 0x06dff000 0x0 0x00001000>,	/* MCU mailbox buffer */
> > > > 			      <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > > > 			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > > > 		};
> > > > 	};
> > > > 
> > > >         [...]
> > > > 
> > > > 	mailbox: mailbox@f7510000 {
> > > > 		#mbox-cells = <1>;
> > > > 		compatible = "hisilicon,hi6220-mbox";
> > > > 		reg = <0x0 0xf7510000 0x0 0x1000>; /* IPC_S */
> > > > 		memory-region = <&mcu_reserved>;   /* Mailbox buffer */
> > > > 		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> > > > 	};
> > > 
> > > I prefer the second one. From my view, memory node should only describe
> > > the hardware information of memory.
> > 
> > Yes, option 2 will be more simple for memory node.
> > 
> > But option 2 also will introduce complexity for mailbox node, due mailbox
> > driver need use property "reg" and "memory-region" to sepeately depict
> > the regions for mailbox's ipc and slots. If later mailbox is designed to
> > use SRAM for both ipc and slots, then it will no matter with DDR anymore,
> > in this case option 1 will easily switch to support it.
> > 
> > Another question is a general question: for Linux kernel, which is the
> > best method to reserve memory regions? According to previous discussion,
> > we can use /memory/ node or /reseved-memory/ node to reserve memory
> > regions.
> 
> If the memory is truly reserved for a purpose and cannot be used for
> anything else, I don't think it should be in the memory node at all, and
> should be carved out. That aligns with what you'd do in UEFI (either not
> listing the region in the memory map, or listing it with attributes such
> that it may not be mapped and/or used).
> 
> I don't see much of a reason for /memreserve/, as it can cause issues
> (by allowing the OS to map the region with cacheable attributes), and is
> not as rigorously specified for ARM as it is for Power in ePAPR.
> 
> I understand that reserved-memory is for carving out (potentially
> reusable) memory pools for devices or other special uses (perhaps a
> panic log). Usually such memory may also be used by the kernel for its
> own purposes if not presently required by the device.
> 
> Having an entry in reserved-memory does not necessitate the region also
> appears in memory nodes, and if a region cannot be used by an OS (or
> other software) for other purposes, I would not expect it to be describe
> in any memory node. That will prevent other software (e.g. bootloaders)
> from erroneously using the memory.
> 
> If you have a region described with no-map, I would expect that this
> doesn't exist in any memory node or the UEFI memory map, and is only
> under reserved-memory so it may be referred to by phandle in a
> consistent manner.
> 
> > when review Juno's dts, i also see there have reserved 16MB DDR for secure
> > world. If so, looks like /reserved-memory/ node is unnecessary. if have some
> > specific scenarios will we use reserved-memory node to help reserve memory
> > regions?
> 
> I'd expect shared DMA pools to appear in reserved-memory. The OS can
> choose to use these or ignore them if it chooses (or is otherwise forced
> to, e.g. were it loaded over one).

Thanks a lot for detailed explain; it's quite clear now.

Thanks,
Leo Yan
diff mbox

Patch

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index e36a539..d5470d3 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -7,9 +7,6 @@ 
 
 /dts-v1/;
 
-/*Reserved 1MB memory for MCU*/
-/memreserve/ 0x05e00000 0x00100000;
-
 #include "hi6220.dtsi"
 
 / {
@@ -28,4 +25,21 @@ 
 		device_type = "memory";
 		reg = <0x0 0x0 0x0 0x40000000>;
 	};
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		mcu-buf@05e00000 {
+			no-map;
+			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
+			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
+		};
+
+		mbox-buf@06dff000 {
+			no-map;
+			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
+		};
+	};
 };
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 3f03380..9ff25bc 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -167,5 +167,13 @@ 
 			clocks = <&ao_ctrl 36>, <&ao_ctrl 36>;
 			clock-names = "uartclk", "apb_pclk";
 		};
+
+		mailbox: mailbox@f7510000 {
+			#mbox-cells = <1>;
+			compatible = "hisilicon,hi6220-mbox";
+			reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
+			      <0x0 0x06dff800 0x0 0x0800>; /* Mailbox buffer */
+			interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
+		};
 	};
 };