diff mbox

[2/2] arm64: regard FDT_SW_MAGIC as good fdt magic

Message ID a6a8826c-3006-ae43-ef4c-8049ef93cd9f@zoho.com (mailing list archive)
State New, archived
Headers show

Commit Message

zijun_hu Sept. 1, 2016, 10:58 a.m. UTC 
From: zijun_hu <zijun_hu@htc.com>

regard FDT_SW_MAGIC as good fdt magic during mapping fdt area
see fdt_check_header() for details

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
 arch/arm64/mm/mmu.c                  | 3 ++-
 scripts/dtc/libfdt/fdt.h             | 3 ++-
 scripts/dtc/libfdt/libfdt.h          | 2 ++
 scripts/dtc/libfdt/libfdt_internal.h | 2 --
 4 files changed, 6 insertions(+), 4 deletions(-)

Comments

Mark Rutland Sept. 1, 2016, 11:21 a.m. UTC | #1 
On Thu, Sep 01, 2016 at 06:58:29PM +0800, zijun_hu wrote:
> From: zijun_hu <zijun_hu@htc.com>
> 
> regard FDT_SW_MAGIC as good fdt magic during mapping fdt area
> see fdt_check_header() for details

It looks like we should only see FDT_SW_MAGIC for a FDT that was in the
process of being created, but was not finished. So I'm somewhat confused
as to why fdt_check_header() would allow this.

Neither ePAPR nor the new devicetree spec define FDT_SW_MAGIC. They both
only define 0xd00dfeed as a valid magic value. In libfdt, FDT_SW_MAGIC
is an internal constant, and it looks like fdt_check_header() simply
accepts this for convenience within libfdt.

Given all of that, it looks like the kernel should *not* accept
FDT_SW_MAGIC in any case.

Why do you think this is necessary? Have you seen a problem in practice?

> --- a/scripts/dtc/libfdt/fdt.h
> +++ b/scripts/dtc/libfdt/fdt.h
> @@ -92,7 +92,8 @@ struct fdt_property {
>  
>  #endif /* !__ASSEMBLY */
>  
> -#define FDT_MAGIC	0xd00dfeed	/* 4: version, 4: total size */
> +/* 4: version, 4: total size */
> +#define FDT_MAGIC	((fdt32_t)0xd00dfeed)
>  #define FDT_TAGSIZE	sizeof(fdt32_t)
>  
>  #define FDT_BEGIN_NODE	0x1		/* Start node: full name */
> diff --git a/scripts/dtc/libfdt/libfdt.h b/scripts/dtc/libfdt/libfdt.h
> index 59ca33976e56..6998f9249183 100644
> --- a/scripts/dtc/libfdt/libfdt.h
> +++ b/scripts/dtc/libfdt/libfdt.h
> @@ -54,6 +54,8 @@
>  #include "libfdt_env.h"
>  #include "fdt.h"
>  
> +#define FDT_SW_MAGIC           (~FDT_MAGIC)
> +
>  #define FDT_FIRST_SUPPORTED_VERSION	0x10
>  #define FDT_LAST_SUPPORTED_VERSION	0x11
>  
> diff --git a/scripts/dtc/libfdt/libfdt_internal.h b/scripts/dtc/libfdt/libfdt_internal.h
> index 02cfa6fb612d..f4efde0119f2 100644
> --- a/scripts/dtc/libfdt/libfdt_internal.h
> +++ b/scripts/dtc/libfdt/libfdt_internal.h
> @@ -90,6 +90,4 @@ static inline struct fdt_reserve_entry *_fdt_mem_rsv_w(void *fdt, int n)
>  	return (void *)(uintptr_t)_fdt_mem_rsv(fdt, n);
>  }
>  
> -#define FDT_SW_MAGIC		(~FDT_MAGIC)
> -
>  #endif /* _LIBFDT_INTERNAL_H */

Regardless of the above, changes to libfdt must happen in the upstream
libfdt codebase first.

Thanks,
Mark.
zijun_hu Sept. 1, 2016, 4:03 p.m. UTC | #2 
On 09/01/2016 07:21 PM, Mark Rutland wrote:
> On Thu, Sep 01, 2016 at 06:58:29PM +0800, zijun_hu wrote:
>> From: zijun_hu <zijun_hu@htc.com>
>>
>> regard FDT_SW_MAGIC as good fdt magic during mapping fdt area
>> see fdt_check_header() for details
> 
> It looks like we should only see FDT_SW_MAGIC for a FDT that was in the
> process of being created, but was not finished. So I'm somewhat confused
> as to why fdt_check_header() would allow this.
> 
> Neither ePAPR nor the new devicetree spec define FDT_SW_MAGIC. They both
> only define 0xd00dfeed as a valid magic value. In libfdt, FDT_SW_MAGIC
> is an internal constant, and it looks like fdt_check_header() simply
> accepts this for convenience within libfdt.
> 
> Given all of that, it looks like the kernel should *not* accept
> FDT_SW_MAGIC in any case.
> 
> Why do you think this is necessary? Have you seen a problem in practice?
>

i don't understand function modules involved with FDT_SW_MAGIC very well
i just think it isn't a bad thing to keep consistent with fdt_check_header()

no, i have no problem about fdt magic in practice

BTW
it seems FDT_SW_MAGIC is involved in fdt_create_empty_tree()@fdt_sw.c which
operate fdt in runtime
in kernel, this function is used in drivers/firmware/efi/libstub/fdt.c
in u-boot, in arch/sandbox/cpu/cpu.c an arch/sandbox/cpu/state.c
the sources mentioned above maybe help you for further decision

>> --- a/scripts/dtc/libfdt/fdt.h
>> +++ b/scripts/dtc/libfdt/fdt.h
>> @@ -92,7 +92,8 @@ struct fdt_property {
>>  
>>  #endif /* !__ASSEMBLY */
>>  
>> -#define FDT_MAGIC	0xd00dfeed	/* 4: version, 4: total size */
>> +/* 4: version, 4: total size */
>> +#define FDT_MAGIC	((fdt32_t)0xd00dfeed)
>>  #define FDT_TAGSIZE	sizeof(fdt32_t)
>>  
>>  #define FDT_BEGIN_NODE	0x1		/* Start node: full name */
>> diff --git a/scripts/dtc/libfdt/libfdt.h b/scripts/dtc/libfdt/libfdt.h
>> index 59ca33976e56..6998f9249183 100644
>> --- a/scripts/dtc/libfdt/libfdt.h
>> +++ b/scripts/dtc/libfdt/libfdt.h
>> @@ -54,6 +54,8 @@
>>  #include "libfdt_env.h"
>>  #include "fdt.h"
>>  
>> +#define FDT_SW_MAGIC           (~FDT_MAGIC)
>> +
>>  #define FDT_FIRST_SUPPORTED_VERSION	0x10
>>  #define FDT_LAST_SUPPORTED_VERSION	0x11
>>  
>> diff --git a/scripts/dtc/libfdt/libfdt_internal.h b/scripts/dtc/libfdt/libfdt_internal.h
>> index 02cfa6fb612d..f4efde0119f2 100644
>> --- a/scripts/dtc/libfdt/libfdt_internal.h
>> +++ b/scripts/dtc/libfdt/libfdt_internal.h
>> @@ -90,6 +90,4 @@ static inline struct fdt_reserve_entry *_fdt_mem_rsv_w(void *fdt, int n)
>>  	return (void *)(uintptr_t)_fdt_mem_rsv(fdt, n);
>>  }
>>  
>> -#define FDT_SW_MAGIC		(~FDT_MAGIC)
>> -
>>  #endif /* _LIBFDT_INTERNAL_H */
> 
> Regardless of the above, changes to libfdt must happen in the upstream
> libfdt codebase first.

okay, make sense
FDT team maybe help decide whether to expose FDT_SW_MAGIC to users
>
> Thanks,
> Mark.
>
Mark Rutland Sept. 1, 2016, 4:45 p.m. UTC | #3 
On Fri, Sep 02, 2016 at 12:03:58AM +0800, zijun_hu wrote:
> On 09/01/2016 07:21 PM, Mark Rutland wrote:
> > On Thu, Sep 01, 2016 at 06:58:29PM +0800, zijun_hu wrote:
> >> From: zijun_hu <zijun_hu@htc.com>
> >>
> >> regard FDT_SW_MAGIC as good fdt magic during mapping fdt area
> >> see fdt_check_header() for details
> > 
> > It looks like we should only see FDT_SW_MAGIC for a FDT that was in the
> > process of being created, but was not finished. So I'm somewhat confused
> > as to why fdt_check_header() would allow this.
> > 
> > Neither ePAPR nor the new devicetree spec define FDT_SW_MAGIC. They both
> > only define 0xd00dfeed as a valid magic value. In libfdt, FDT_SW_MAGIC
> > is an internal constant, and it looks like fdt_check_header() simply
> > accepts this for convenience within libfdt.

> > Why do you think this is necessary? Have you seen a problem in practice?

> i don't understand function modules involved with FDT_SW_MAGIC very well
> i just think it isn't a bad thing to keep consistent with fdt_check_header()

I agree that the inconsistency is not great.

However, I think that we do not want the kernel to accept FDT_SW_MAGIC
in any case, given this implies a DTB mid-creation.

Which is to say that either fdt_check_header() is doing the wrong thing,
or that we're using it in places where it's inappropriate.

> BTW
> it seems FDT_SW_MAGIC is involved in fdt_create_empty_tree()@fdt_sw.c which
> operate fdt in runtime
> in kernel, this function is used in drivers/firmware/efi/libstub/fdt.c
> in u-boot, in arch/sandbox/cpu/cpu.c an arch/sandbox/cpu/state.c
> the sources mentioned above maybe help you for further decision

Note that fdt_create_empty_tree() calls fdt_finish(), which fixes some
details up, then sets the magic to the real FDT_MAGIC.

So that should be fine.

Thanks,
Mark.
diff mbox

Patch

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 4989948d1feb..83df20509eae 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -716,7 +716,8 @@  void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot)
 	create_mapping_noalloc(round_down(dt_phys, SWAPPER_BLOCK_SIZE),
 			dt_virt_base, SWAPPER_BLOCK_SIZE, prot);
 
-	if (fdt_magic(dt_virt) != FDT_MAGIC)
+	if (fdt_magic(dt_virt) != FDT_MAGIC &&
+			fdt_magic(dt_virt) != FDT_SW_MAGIC)
 		return NULL;
 
 	*size = fdt_totalsize(dt_virt);
diff --git a/scripts/dtc/libfdt/fdt.h b/scripts/dtc/libfdt/fdt.h
index 526aedb51556..0e8806327b53 100644
--- a/scripts/dtc/libfdt/fdt.h
+++ b/scripts/dtc/libfdt/fdt.h
@@ -92,7 +92,8 @@  struct fdt_property {
 
 #endif /* !__ASSEMBLY */
 
-#define FDT_MAGIC	0xd00dfeed	/* 4: version, 4: total size */
+/* 4: version, 4: total size */
+#define FDT_MAGIC	((fdt32_t)0xd00dfeed)
 #define FDT_TAGSIZE	sizeof(fdt32_t)
 
 #define FDT_BEGIN_NODE	0x1		/* Start node: full name */
diff --git a/scripts/dtc/libfdt/libfdt.h b/scripts/dtc/libfdt/libfdt.h
index 59ca33976e56..6998f9249183 100644
--- a/scripts/dtc/libfdt/libfdt.h
+++ b/scripts/dtc/libfdt/libfdt.h
@@ -54,6 +54,8 @@ 
 #include "libfdt_env.h"
 #include "fdt.h"
 
+#define FDT_SW_MAGIC           (~FDT_MAGIC)
+
 #define FDT_FIRST_SUPPORTED_VERSION	0x10
 #define FDT_LAST_SUPPORTED_VERSION	0x11
 
diff --git a/scripts/dtc/libfdt/libfdt_internal.h b/scripts/dtc/libfdt/libfdt_internal.h
index 02cfa6fb612d..f4efde0119f2 100644
--- a/scripts/dtc/libfdt/libfdt_internal.h
+++ b/scripts/dtc/libfdt/libfdt_internal.h
@@ -90,6 +90,4 @@  static inline struct fdt_reserve_entry *_fdt_mem_rsv_w(void *fdt, int n)
 	return (void *)(uintptr_t)_fdt_mem_rsv(fdt, n);
 }
 
-#define FDT_SW_MAGIC		(~FDT_MAGIC)
-
 #endif /* _LIBFDT_INTERNAL_H */