* [PATCH 2/2] arm64: regard FDT_SW_MAGIC as good fdt magic
@ 2016-09-01 10:58 zijun_hu
2016-09-01 11:21 ` Mark Rutland
0 siblings, 1 reply; 4+ messages in thread
From: zijun_hu @ 2016-09-01 10:58 UTC (permalink / raw)
To: linux-arm-kernel
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(-)
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 */
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/2] arm64: regard FDT_SW_MAGIC as good fdt magic
2016-09-01 10:58 [PATCH 2/2] arm64: regard FDT_SW_MAGIC as good fdt magic zijun_hu
@ 2016-09-01 11:21 ` Mark Rutland
2016-09-01 16:03 ` zijun_hu
0 siblings, 1 reply; 4+ messages in thread
From: Mark Rutland @ 2016-09-01 11:21 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH 2/2] arm64: regard FDT_SW_MAGIC as good fdt magic
2016-09-01 11:21 ` Mark Rutland
@ 2016-09-01 16:03 ` zijun_hu
2016-09-01 16:45 ` Mark Rutland
0 siblings, 1 reply; 4+ messages in thread
From: zijun_hu @ 2016-09-01 16:03 UTC (permalink / raw)
To: linux-arm-kernel
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.
>
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH 2/2] arm64: regard FDT_SW_MAGIC as good fdt magic
2016-09-01 16:03 ` zijun_hu
@ 2016-09-01 16:45 ` Mark Rutland
0 siblings, 0 replies; 4+ messages in thread
From: Mark Rutland @ 2016-09-01 16:45 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-01 16:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-01 10:58 [PATCH 2/2] arm64: regard FDT_SW_MAGIC as good fdt magic zijun_hu
2016-09-01 11:21 ` Mark Rutland
2016-09-01 16:03 ` zijun_hu
2016-09-01 16:45 ` Mark Rutland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).