Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1474276 > unrolled thread
| Started by | zijun_hu <zijun_hu@zoho.com> |
|---|---|
| First post | 2016-09-01 13:00 +0200 |
| Last post | 2016-09-01 18:50 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH 2/2] arm64: regard FDT_SW_MAGIC as good fdt magic zijun_hu <zijun_hu@zoho.com> - 2016-09-01 13:00 +0200
Re: [PATCH 2/2] arm64: regard FDT_SW_MAGIC as good fdt magic Mark Rutland <mark.rutland@arm.com> - 2016-09-01 13:30 +0200
Re: [PATCH 2/2] arm64: regard FDT_SW_MAGIC as good fdt magic zijun_hu <zijun_hu@zoho.com> - 2016-09-01 18:10 +0200
Re: [PATCH 2/2] arm64: regard FDT_SW_MAGIC as good fdt magic Mark Rutland <mark.rutland@arm.com> - 2016-09-01 18:50 +0200
| From | zijun_hu <zijun_hu@zoho.com> |
|---|---|
| Date | 2016-09-01 13:00 +0200 |
| Subject | [PATCH 2/2] arm64: regard FDT_SW_MAGIC as good fdt magic |
| Message-ID | <scxSW-H2-17@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2016-09-01 13:30 +0200 |
| Message-ID | <scylZ-1an-51@gated-at.bofh.it> |
| In reply to | #1474276 |
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.
[toc] | [prev] | [next] | [standalone]
| From | zijun_hu <zijun_hu@zoho.com> |
|---|---|
| Date | 2016-09-01 18:10 +0200 |
| Message-ID | <scCIV-43H-1@gated-at.bofh.it> |
| In reply to | #1474318 |
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.
>
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2016-09-01 18:50 +0200 |
| Message-ID | <scDlE-4lf-55@gated-at.bofh.it> |
| In reply to | #1474514 |
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.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web