The DT functions (dt_read_number(), device_tree_get_reg(), fdt_get_mem_rsv())
currently accept or return 64-bit values.
In future when we support 32-bit physical address, these DT functions are
expected to accept/return 32-bit or 64-bit values (depending on the width of
physical address). Also, we wish to detect if any truncation has occurred
(i.e. while parsing 32-bit physical addresses from 64-bit values read from DT).
device_tree_get_reg() should now be able to return paddr_t. This is invoked by
various callers to get DT address and size.
For fdt_get_mem_rsv(), we have introduced a wrapper named
fdt_get_mem_rsv_paddr() which will invoke fdt_get_mem_rsv() and translate
uint64_t to paddr_t. The reason being we cannot modify fdt_get_mem_rsv() as it
has been imported from external source.
For dt_read_number(), we have also introduced a wrapper named dt_read_paddr()
dt_read_paddr() to read physical addresses. We chose not to modify the original
function as it is used in places where it needs to specifically read 64-bit
values from dt (For e.g. dt_property_read_u64()).
Xen prints warning when it detects truncation in cases where it is not able to
return error.
Also, replaced u32/u64 with uint32_t/uint64_t in the functions touched
by the code changes.
Also, initialized variables to fix the warning "-Werror=maybe-uninitialized".
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from
v1 - 1. Dropped "[XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t" and
"[XEN v1 4/9] xen/arm: Use translate_dt_address_size() to translate between device tree addr/size and paddr_t", instead
this approach achieves the same purpose.
2. No need to check for truncation while converting values from u64 to paddr_t.
v2 - 1. Use "( (dt_start >> (PADDR_SHIFT - 1)) > 1 )" to detect truncation.
2. Introduced libfdt_xen.h to implement fdt_get_mem_rsv_paddr
3. Logged error messages in case truncation is detected.
v3 - 1. Renamed libfdt_xen.h to libfdt-xen.h.
2. Replaced u32/u64 with uint32_t/uint64_t
3. Use "(paddr_t)val != val" to check for truncation.
4. Removed the alias "#define PADDR_SHIFT PADDR_BITS".
v4 - 1. Added a WARN() when truncation is detected.
2. Always check the return value of fdt_get_mem_rsv().
xen/arch/arm/bootfdt.c | 48 +++++++++++++++++++------
xen/arch/arm/domain_build.c | 2 +-
xen/arch/arm/include/asm/setup.h | 4 +--
xen/arch/arm/setup.c | 18 +++++-----
xen/arch/arm/smpboot.c | 2 +-
xen/include/xen/device_tree.h | 23 ++++++++++++
xen/include/xen/libfdt/libfdt-xen.h | 55 +++++++++++++++++++++++++++++
7 files changed, 129 insertions(+), 23 deletions(-)
create mode 100644 xen/include/xen/libfdt/libfdt-xen.h
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 0085c28d74..ac8148da55 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -11,7 +11,7 @@
#include <xen/efi.h>
#include <xen/device_tree.h>
#include <xen/lib.h>
-#include <xen/libfdt/libfdt.h>
+#include <xen/libfdt/libfdt-xen.h>
#include <xen/sort.h>
#include <xsm/xsm.h>
#include <asm/setup.h>
@@ -52,11 +52,37 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
return false;
}
-void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
- u32 size_cells, u64 *start, u64 *size)
+void __init device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
+ uint32_t size_cells, paddr_t *start,
+ paddr_t *size)
{
- *start = dt_next_cell(address_cells, cell);
- *size = dt_next_cell(size_cells, cell);
+ uint64_t dt_start, dt_size;
+
+ /*
+ * dt_next_cell will return uint64_t whereas paddr_t may not be 64-bit.
+ * Thus, there is an implicit cast from uint64_t to paddr_t.
+ */
+ dt_start = dt_next_cell(address_cells, cell);
+ dt_size = dt_next_cell(size_cells, cell);
+
+ if ( dt_start != (paddr_t)dt_start )
+ {
+ printk("Error: Physical address greater than max width supported\n");
+ WARN();
+ }
+
+ if ( dt_size != (paddr_t)dt_size )
+ {
+ printk("Error: Physical size greater than max width supported\n");
+ WARN();
+ }
+
+ /*
+ * Xen will truncate the address/size if it is greater than the maximum
+ * supported width and it will give an appropriate warning.
+ */
+ *start = dt_start;
+ *size = dt_size;
}
static int __init device_tree_get_meminfo(const void *fdt, int node,
@@ -326,7 +352,7 @@ static int __init process_chosen_node(const void *fdt, int node,
printk("linux,initrd-start property has invalid length %d\n", len);
return -EINVAL;
}
- start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
+ start = dt_read_paddr((void *)&prop->data, dt_size_to_cells(len));
prop = fdt_get_property(fdt, node, "linux,initrd-end", &len);
if ( !prop )
@@ -339,7 +365,7 @@ static int __init process_chosen_node(const void *fdt, int node,
printk("linux,initrd-end property has invalid length %d\n", len);
return -EINVAL;
}
- end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
+ end = dt_read_paddr((void *)&prop->data, dt_size_to_cells(len));
if ( start >= end )
{
@@ -593,10 +619,12 @@ static void __init early_print_info(void)
nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
for ( i = 0; i < nr_rsvd; i++ )
{
- paddr_t s, e;
- if ( fdt_get_mem_rsv(device_tree_flattened, i, &s, &e) < 0 )
+ paddr_t s = 0, e = 0;
+
+ if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &e) < 0 )
continue;
- /* fdt_get_mem_rsv returns length */
+
+ /* fdt_get_mem_rsv_paddr returns length */
e += s;
printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i, s, e);
}
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c8f08d8ee2..15c8bdd9e4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -949,7 +949,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
BUG_ON(!prop);
cells = (const __be32 *)prop->value;
device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase);
- psize = dt_read_number(cells, size_cells);
+ psize = dt_read_paddr(cells, size_cells);
if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) )
{
printk("%pd: physical address 0x%"PRIpaddr", or guest address 0x%"PRIpaddr" is not suitably aligned.\n",
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index a926f30a2b..7b697d879e 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -157,8 +157,8 @@ const char *boot_module_kind_as_string(bootmodule_kind kind);
extern uint32_t hyp_traps_vector[];
void init_traps(void);
-void device_tree_get_reg(const __be32 **cell, u32 address_cells,
- u32 size_cells, u64 *start, u64 *size);
+void device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
+ uint32_t size_cells, paddr_t *start, paddr_t *size);
u32 device_tree_get_u32(const void *fdt, int node,
const char *prop_name, u32 dflt);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 1f26f67b90..d2a3d8c340 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -29,7 +29,7 @@
#include <xen/virtual_region.h>
#include <xen/vmap.h>
#include <xen/trace.h>
-#include <xen/libfdt/libfdt.h>
+#include <xen/libfdt/libfdt-xen.h>
#include <xen/acpi.h>
#include <xen/warning.h>
#include <asm/alternative.h>
@@ -220,13 +220,13 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
for ( i = first; i < nr ; i++ )
{
- paddr_t r_s, r_e;
+ paddr_t r_s = 0, r_e = 0;
- if ( fdt_get_mem_rsv(device_tree_flattened, i, &r_s, &r_e ) < 0 )
+ if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &r_s, &r_e ) < 0 )
/* If we can't read it, pretend it doesn't exist... */
continue;
- r_e += r_s; /* fdt_get_mem_rsv returns length */
+ r_e += r_s; /* fdt_get_mem_rsv_paddr returns length */
if ( s < r_e && r_s < e )
{
@@ -500,15 +500,15 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
for ( ; i < mi->nr_mods + nr; i++ )
{
- paddr_t mod_s, mod_e;
+ paddr_t mod_s = 0, mod_e = 0;
- if ( fdt_get_mem_rsv(device_tree_flattened,
- i - mi->nr_mods,
- &mod_s, &mod_e ) < 0 )
+ if ( fdt_get_mem_rsv_paddr(device_tree_flattened,
+ i - mi->nr_mods,
+ &mod_s, &mod_e ) < 0 )
/* If we can't read it, pretend it doesn't exist... */
continue;
- /* fdt_get_mem_rsv returns length */
+ /* fdt_get_mem_rsv_paddr returns length */
mod_e += mod_s;
if ( s < mod_e && mod_s < e )
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 412ae22869..c15c177487 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -159,7 +159,7 @@ static void __init dt_smp_init_cpus(void)
continue;
}
- addr = dt_read_number(prop, dt_n_addr_cells(cpu));
+ addr = dt_read_paddr(prop, dt_n_addr_cells(cpu));
hwid = addr;
if ( hwid != addr )
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 19a74909ce..11bda2fd3d 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -241,6 +241,29 @@ static inline u64 dt_read_number(const __be32 *cell, int size)
return r;
}
+/* Wrapper for dt_read_number() to return paddr_t (instead of uint64_t) */
+static inline paddr_t dt_read_paddr(const __be32 *cell, int size)
+{
+ uint64_t dt_r = 0;
+ paddr_t r;
+
+ dt_r = dt_read_number(cell, size);
+
+ if ( dt_r != (paddr_t)dt_r )
+ {
+ printk("Error: Physical address greater than max width supported\n");
+ WARN();
+ }
+
+ /*
+ * Xen will truncate the address/size if it is greater than the maximum
+ * supported width and it will give an appropriate warning.
+ */
+ r = dt_r;
+
+ return r;
+}
+
/* Helper to convert a number of cells to bytes */
static inline int dt_cells_to_size(int size)
{
diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
new file mode 100644
index 0000000000..3296a368a6
--- /dev/null
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -0,0 +1,55 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-only
+ *
+ * xen/include/xen/libfdt/libfdt-xen.h
+ *
+ * Wrapper functions for device tree. This helps to convert dt values
+ * between uint64_t and paddr_t.
+ *
+ * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved.
+ */
+
+#ifndef LIBFDT_XEN_H
+#define LIBFDT_XEN_H
+
+#include <xen/libfdt/libfdt.h>
+
+static inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
+ paddr_t *address,
+ paddr_t *size)
+{
+ uint64_t dt_addr;
+ uint64_t dt_size;
+ int ret = 0;
+
+ ret = fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size);
+ if ( ret )
+ return ret;
+
+ if ( dt_addr != (paddr_t)dt_addr )
+ {
+ printk("Error: Physical address greater than max width supported\n");
+ return -FDT_ERR_MAX;
+ }
+
+ if ( dt_size != (paddr_t)dt_size )
+ {
+ printk("Error: Physical size greater than max width supported\n");
+ return -FDT_ERR_MAX;
+ }
+
+ *address = dt_addr;
+ *size = dt_size;
+
+ return ret;
+}
+
+#endif /* LIBFDT_XEN_H */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.17.1Hi Ayan,
On 13/04/2023 19:37, Ayan Kumar Halder wrote:
>
>
> The DT functions (dt_read_number(), device_tree_get_reg(), fdt_get_mem_rsv())
> currently accept or return 64-bit values.
>
> In future when we support 32-bit physical address, these DT functions are
> expected to accept/return 32-bit or 64-bit values (depending on the width of
> physical address). Also, we wish to detect if any truncation has occurred
> (i.e. while parsing 32-bit physical addresses from 64-bit values read from DT).
>
> device_tree_get_reg() should now be able to return paddr_t. This is invoked by
> various callers to get DT address and size.
>
> For fdt_get_mem_rsv(), we have introduced a wrapper named
> fdt_get_mem_rsv_paddr() which will invoke fdt_get_mem_rsv() and translate
> uint64_t to paddr_t. The reason being we cannot modify fdt_get_mem_rsv() as it
> has been imported from external source.
>
> For dt_read_number(), we have also introduced a wrapper named dt_read_paddr()
> dt_read_paddr() to read physical addresses. We chose not to modify the original
> function as it is used in places where it needs to specifically read 64-bit
> values from dt (For e.g. dt_property_read_u64()).
>
> Xen prints warning when it detects truncation in cases where it is not able to
> return error.
>
> Also, replaced u32/u64 with uint32_t/uint64_t in the functions touched
> by the code changes.
>
> Also, initialized variables to fix the warning "-Werror=maybe-uninitialized".
I can see that now you explicitly set to 0 variables passed to fdt_get_mem_rsv_paddr()
which haven't been initialized before being passed to fdt_get_mem_rsv(). Is this what
you are reffering to? I cannot reproduce it, hence my question.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>
> Changes from
>
> v1 - 1. Dropped "[XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t" and
> "[XEN v1 4/9] xen/arm: Use translate_dt_address_size() to translate between device tree addr/size and paddr_t", instead
> this approach achieves the same purpose.
>
> 2. No need to check for truncation while converting values from u64 to paddr_t.
>
> v2 - 1. Use "( (dt_start >> (PADDR_SHIFT - 1)) > 1 )" to detect truncation.
> 2. Introduced libfdt_xen.h to implement fdt_get_mem_rsv_paddr
> 3. Logged error messages in case truncation is detected.
>
> v3 - 1. Renamed libfdt_xen.h to libfdt-xen.h.
> 2. Replaced u32/u64 with uint32_t/uint64_t
> 3. Use "(paddr_t)val != val" to check for truncation.
> 4. Removed the alias "#define PADDR_SHIFT PADDR_BITS".
>
> v4 - 1. Added a WARN() when truncation is detected.
> 2. Always check the return value of fdt_get_mem_rsv().
>
> xen/arch/arm/bootfdt.c | 48 +++++++++++++++++++------
> xen/arch/arm/domain_build.c | 2 +-
> xen/arch/arm/include/asm/setup.h | 4 +--
> xen/arch/arm/setup.c | 18 +++++-----
> xen/arch/arm/smpboot.c | 2 +-
> xen/include/xen/device_tree.h | 23 ++++++++++++
> xen/include/xen/libfdt/libfdt-xen.h | 55 +++++++++++++++++++++++++++++
> 7 files changed, 129 insertions(+), 23 deletions(-)
> create mode 100644 xen/include/xen/libfdt/libfdt-xen.h
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 0085c28d74..ac8148da55 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -11,7 +11,7 @@
> #include <xen/efi.h>
> #include <xen/device_tree.h>
> #include <xen/lib.h>
> -#include <xen/libfdt/libfdt.h>
> +#include <xen/libfdt/libfdt-xen.h>
> #include <xen/sort.h>
> #include <xsm/xsm.h>
> #include <asm/setup.h>
> @@ -52,11 +52,37 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
> return false;
> }
>
> -void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
> - u32 size_cells, u64 *start, u64 *size)
> +void __init device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
> + uint32_t size_cells, paddr_t *start,
> + paddr_t *size)
> {
> - *start = dt_next_cell(address_cells, cell);
> - *size = dt_next_cell(size_cells, cell);
> + uint64_t dt_start, dt_size;
> +
> + /*
> + * dt_next_cell will return uint64_t whereas paddr_t may not be 64-bit.
> + * Thus, there is an implicit cast from uint64_t to paddr_t.
> + */
> + dt_start = dt_next_cell(address_cells, cell);
> + dt_size = dt_next_cell(size_cells, cell);
> +
> + if ( dt_start != (paddr_t)dt_start )
> + {
> + printk("Error: Physical address greater than max width supported\n");
I find it a bit odd to see Error and on the next line warning. I would drop "Error" here
but feel free not to as this is just my POV.
> + WARN();
> + }
> +
> + if ( dt_size != (paddr_t)dt_size )
> + {
> + printk("Error: Physical size greater than max width supported\n");
> + WARN();
> + }
> +
> + /*
> + * Xen will truncate the address/size if it is greater than the maximum
> + * supported width and it will give an appropriate warning.
> + */
> + *start = dt_start;
> + *size = dt_size;
> }
>
> static int __init device_tree_get_meminfo(const void *fdt, int node,
> @@ -326,7 +352,7 @@ static int __init process_chosen_node(const void *fdt, int node,
> printk("linux,initrd-start property has invalid length %d\n", len);
> return -EINVAL;
> }
> - start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> + start = dt_read_paddr((void *)&prop->data, dt_size_to_cells(len));
>
> prop = fdt_get_property(fdt, node, "linux,initrd-end", &len);
> if ( !prop )
> @@ -339,7 +365,7 @@ static int __init process_chosen_node(const void *fdt, int node,
> printk("linux,initrd-end property has invalid length %d\n", len);
> return -EINVAL;
> }
> - end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> + end = dt_read_paddr((void *)&prop->data, dt_size_to_cells(len));
>
> if ( start >= end )
> {
> @@ -593,10 +619,12 @@ static void __init early_print_info(void)
> nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
> for ( i = 0; i < nr_rsvd; i++ )
> {
> - paddr_t s, e;
> - if ( fdt_get_mem_rsv(device_tree_flattened, i, &s, &e) < 0 )
> + paddr_t s = 0, e = 0;
> +
> + if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &e) < 0 )
> continue;
> - /* fdt_get_mem_rsv returns length */
> +
> + /* fdt_get_mem_rsv_paddr returns length */
> e += s;
> printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i, s, e);
> }
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c8f08d8ee2..15c8bdd9e4 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -949,7 +949,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> BUG_ON(!prop);
> cells = (const __be32 *)prop->value;
> device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase);
> - psize = dt_read_number(cells, size_cells);
> + psize = dt_read_paddr(cells, size_cells);
> if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) )
> {
> printk("%pd: physical address 0x%"PRIpaddr", or guest address 0x%"PRIpaddr" is not suitably aligned.\n",
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index a926f30a2b..7b697d879e 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -157,8 +157,8 @@ const char *boot_module_kind_as_string(bootmodule_kind kind);
> extern uint32_t hyp_traps_vector[];
> void init_traps(void);
>
> -void device_tree_get_reg(const __be32 **cell, u32 address_cells,
> - u32 size_cells, u64 *start, u64 *size);
> +void device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
> + uint32_t size_cells, paddr_t *start, paddr_t *size);
>
> u32 device_tree_get_u32(const void *fdt, int node,
> const char *prop_name, u32 dflt);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 1f26f67b90..d2a3d8c340 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -29,7 +29,7 @@
> #include <xen/virtual_region.h>
> #include <xen/vmap.h>
> #include <xen/trace.h>
> -#include <xen/libfdt/libfdt.h>
> +#include <xen/libfdt/libfdt-xen.h>
> #include <xen/acpi.h>
> #include <xen/warning.h>
> #include <asm/alternative.h>
> @@ -220,13 +220,13 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
>
> for ( i = first; i < nr ; i++ )
> {
> - paddr_t r_s, r_e;
> + paddr_t r_s = 0, r_e = 0;
>
> - if ( fdt_get_mem_rsv(device_tree_flattened, i, &r_s, &r_e ) < 0 )
> + if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &r_s, &r_e ) < 0 )
> /* If we can't read it, pretend it doesn't exist... */
> continue;
>
> - r_e += r_s; /* fdt_get_mem_rsv returns length */
> + r_e += r_s; /* fdt_get_mem_rsv_paddr returns length */
>
> if ( s < r_e && r_s < e )
> {
> @@ -500,15 +500,15 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
>
> for ( ; i < mi->nr_mods + nr; i++ )
> {
> - paddr_t mod_s, mod_e;
> + paddr_t mod_s = 0, mod_e = 0;
>
> - if ( fdt_get_mem_rsv(device_tree_flattened,
> - i - mi->nr_mods,
> - &mod_s, &mod_e ) < 0 )
> + if ( fdt_get_mem_rsv_paddr(device_tree_flattened,
> + i - mi->nr_mods,
> + &mod_s, &mod_e ) < 0 )
> /* If we can't read it, pretend it doesn't exist... */
> continue;
>
> - /* fdt_get_mem_rsv returns length */
> + /* fdt_get_mem_rsv_paddr returns length */
> mod_e += mod_s;
>
> if ( s < mod_e && mod_s < e )
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 412ae22869..c15c177487 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -159,7 +159,7 @@ static void __init dt_smp_init_cpus(void)
> continue;
> }
>
> - addr = dt_read_number(prop, dt_n_addr_cells(cpu));
> + addr = dt_read_paddr(prop, dt_n_addr_cells(cpu));
>
> hwid = addr;
> if ( hwid != addr )
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 19a74909ce..11bda2fd3d 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -241,6 +241,29 @@ static inline u64 dt_read_number(const __be32 *cell, int size)
> return r;
> }
>
> +/* Wrapper for dt_read_number() to return paddr_t (instead of uint64_t) */
> +static inline paddr_t dt_read_paddr(const __be32 *cell, int size)
> +{
> + uint64_t dt_r = 0;
no need for this assignment
> + paddr_t r;
> +
> + dt_r = dt_read_number(cell, size);
In device_tree_get_reg() you added a note about implicit cast and here it is missing.
> +
> + if ( dt_r != (paddr_t)dt_r )
> + {
> + printk("Error: Physical address greater than max width supported\n");
> + WARN();
> + }
> +
> + /*
> + * Xen will truncate the address/size if it is greater than the maximum
> + * supported width and it will give an appropriate warning.
> + */
> + r = dt_r;
> +
> + return r;
> +}
> +
> /* Helper to convert a number of cells to bytes */
> static inline int dt_cells_to_size(int size)
> {
> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
> new file mode 100644
> index 0000000000..3296a368a6
> --- /dev/null
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -0,0 +1,55 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-only
Our CODING_STYLE says:
New files should start with a single-line SPDX comment, ..., e.g.
/* SPDX-License-Identifier: GPL-2.0 */
For me it would be perfectly fine to do as you did but it is not what our docs state
(i.e. single-line comment). It might be that we need to modify CODING_STYLE instead.
> + *
> + * xen/include/xen/libfdt/libfdt-xen.h
> + *
> + * Wrapper functions for device tree. This helps to convert dt values
> + * between uint64_t and paddr_t.
> + *
> + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved.
> + */
> +
> +#ifndef LIBFDT_XEN_H
> +#define LIBFDT_XEN_H
> +
> +#include <xen/libfdt/libfdt.h>
> +
> +static inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
> + paddr_t *address,
> + paddr_t *size)
> +{
> + uint64_t dt_addr;
> + uint64_t dt_size;
> + int ret = 0;
no need for this assignment
> +
> + ret = fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size);
> + if ( ret )
> + return ret;
> +
> + if ( dt_addr != (paddr_t)dt_addr )
> + {
> + printk("Error: Physical address greater than max width supported\n");
> + return -FDT_ERR_MAX;
> + }
> +
> + if ( dt_size != (paddr_t)dt_size )
> + {
> + printk("Error: Physical size greater than max width supported\n");
> + return -FDT_ERR_MAX;
> + }
> +
> + *address = dt_addr;
> + *size = dt_size;
> +
> + return ret;
> +}
> +
> +#endif /* LIBFDT_XEN_H */
please add an empty line here.
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.17.1
>
>
~Michal
On 19/04/2023 14:19, Michal Orzel wrote: > Hi Ayan, Hi Michal, ... > --- /dev/null > +++ b/xen/include/xen/libfdt/libfdt-xen.h > @@ -0,0 +1,55 @@ > +/* > + * SPDX-License-Identifier: GPL-2.0-only > Our CODING_STYLE says: > New files should start with a single-line SPDX comment, ..., e.g. > /* SPDX-License-Identifier: GPL-2.0 */ > > For me it would be perfectly fine to do as you did but it is not what our docs state > (i.e. single-line comment). It might be that we need to modify CODING_STYLE instead. Just to be clear, this is what we should have (as Julien had earlier suggested to use **GPL-2.0-only** ) diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h index 3296a368a6..cad7ad3bfb 100644 --- a/xen/include/xen/libfdt/libfdt-xen.h +++ b/xen/include/xen/libfdt/libfdt-xen.h @@ -1,6 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only /* - * SPDX-License-Identifier: GPL-2.0-only - * * xen/include/xen/libfdt/libfdt-xen.h * * Wrapper functions for device tree. This helps to convert dt values - Ayan
Hi Ayan, On 19/04/2023 17:12, Ayan Kumar Halder wrote: > > On 19/04/2023 14:19, Michal Orzel wrote: >> Hi Ayan, > > Hi Michal, > > ... > >> --- /dev/null >> +++ b/xen/include/xen/libfdt/libfdt-xen.h >> @@ -0,0 +1,55 @@ >> +/* >> + * SPDX-License-Identifier: GPL-2.0-only >> Our CODING_STYLE says: >> New files should start with a single-line SPDX comment, ..., e.g. >> /* SPDX-License-Identifier: GPL-2.0 */ >> >> For me it would be perfectly fine to do as you did but it is not what our docs state >> (i.e. single-line comment). It might be that we need to modify CODING_STYLE instead. > > Just to be clear, this is what we should have (as Julien had earlier > suggested to use **GPL-2.0-only** ) I used GPL-2.0 just for example. We want of course GPL-2.0-only. > > diff --git a/xen/include/xen/libfdt/libfdt-xen.h > b/xen/include/xen/libfdt/libfdt-xen.h > index 3296a368a6..cad7ad3bfb 100644 > --- a/xen/include/xen/libfdt/libfdt-xen.h > +++ b/xen/include/xen/libfdt/libfdt-xen.h > @@ -1,6 +1,5 @@ > +// SPDX-License-Identifier: GPL-2.0-only You should use /* */ style instead of // > /* > - * SPDX-License-Identifier: GPL-2.0-only > - * > * xen/include/xen/libfdt/libfdt-xen.h > * > * Wrapper functions for device tree. This helps to convert dt values > > - Ayan > ~Michal
On 19/04/2023 15:19, Michal Orzel wrote: > > > Hi Ayan, > > On 13/04/2023 19:37, Ayan Kumar Halder wrote: >> >> >> The DT functions (dt_read_number(), device_tree_get_reg(), fdt_get_mem_rsv()) >> currently accept or return 64-bit values. >> >> In future when we support 32-bit physical address, these DT functions are >> expected to accept/return 32-bit or 64-bit values (depending on the width of >> physical address). Also, we wish to detect if any truncation has occurred >> (i.e. while parsing 32-bit physical addresses from 64-bit values read from DT). >> >> device_tree_get_reg() should now be able to return paddr_t. This is invoked by >> various callers to get DT address and size. >> >> For fdt_get_mem_rsv(), we have introduced a wrapper named >> fdt_get_mem_rsv_paddr() which will invoke fdt_get_mem_rsv() and translate >> uint64_t to paddr_t. The reason being we cannot modify fdt_get_mem_rsv() as it >> has been imported from external source. >> >> For dt_read_number(), we have also introduced a wrapper named dt_read_paddr() >> dt_read_paddr() to read physical addresses. We chose not to modify the original >> function as it is used in places where it needs to specifically read 64-bit >> values from dt (For e.g. dt_property_read_u64()). >> >> Xen prints warning when it detects truncation in cases where it is not able to >> return error. >> >> Also, replaced u32/u64 with uint32_t/uint64_t in the functions touched >> by the code changes. >> >> Also, initialized variables to fix the warning "-Werror=maybe-uninitialized". > I can see that now you explicitly set to 0 variables passed to fdt_get_mem_rsv_paddr() > which haven't been initialized before being passed to fdt_get_mem_rsv(). Is this what > you are reffering to? I cannot reproduce it, hence my question. I can see why did you get this error. Before your change, we always checked for an error from fdt_get_mem_rsv() by checking if < 0. In your wrapper fdt_get_mem_rsv_paddr(), you switched (not sure why) to checking if not zero. Becasue of this, you got an error and tried to fix it by initializing the variables to 0. ~Michal
On 19/04/2023 14:54, Michal Orzel wrote:
> On 19/04/2023 15:19, Michal Orzel wrote:
>>
>> Hi Ayan,
>>
>> On 13/04/2023 19:37, Ayan Kumar Halder wrote:
>>>
>>> The DT functions (dt_read_number(), device_tree_get_reg(), fdt_get_mem_rsv())
>>> currently accept or return 64-bit values.
>>>
>>> In future when we support 32-bit physical address, these DT functions are
>>> expected to accept/return 32-bit or 64-bit values (depending on the width of
>>> physical address). Also, we wish to detect if any truncation has occurred
>>> (i.e. while parsing 32-bit physical addresses from 64-bit values read from DT).
>>>
>>> device_tree_get_reg() should now be able to return paddr_t. This is invoked by
>>> various callers to get DT address and size.
>>>
>>> For fdt_get_mem_rsv(), we have introduced a wrapper named
>>> fdt_get_mem_rsv_paddr() which will invoke fdt_get_mem_rsv() and translate
>>> uint64_t to paddr_t. The reason being we cannot modify fdt_get_mem_rsv() as it
>>> has been imported from external source.
>>>
>>> For dt_read_number(), we have also introduced a wrapper named dt_read_paddr()
>>> dt_read_paddr() to read physical addresses. We chose not to modify the original
>>> function as it is used in places where it needs to specifically read 64-bit
>>> values from dt (For e.g. dt_property_read_u64()).
>>>
>>> Xen prints warning when it detects truncation in cases where it is not able to
>>> return error.
>>>
>>> Also, replaced u32/u64 with uint32_t/uint64_t in the functions touched
>>> by the code changes.
>>>
>>> Also, initialized variables to fix the warning "-Werror=maybe-uninitialized".
>> I can see that now you explicitly set to 0 variables passed to fdt_get_mem_rsv_paddr()
>> which haven't been initialized before being passed to fdt_get_mem_rsv(). Is this what
>> you are reffering to? I cannot reproduce it, hence my question.
> I can see why did you get this error.
> Before your change, we always checked for an error from fdt_get_mem_rsv() by checking if < 0.
> In your wrapper fdt_get_mem_rsv_paddr(), you switched (not sure why) to checking if not zero.
> Becasue of this, you got an error and tried to fix it by initializing the variables to 0.
I actually wanted to return the error code obtained from
fdt_get_mem_rsv() to the caller.
In this case, it returns a single error code. So does this look sane to
you ?
diff --git a/xen/include/xen/libfdt/libfdt-xen.h
b/xen/include/xen/libfdt/libfdt-xen.h
index 3296a368a6..1da87d6668 100644
--- a/xen/include/xen/libfdt/libfdt-xen.h
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -22,9 +22,8 @@ static inline int fdt_get_mem_rsv_paddr(const void
*fdt, int n,
uint64_t dt_size;
int ret = 0;
- ret = fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size);
- if ( ret )
- return ret;
+ if ( fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size) < 0 )
+ return -FDT_ERR_BADOFFSET;
if ( dt_addr != (paddr_t)dt_addr )
{
- Ayan
>
> ~Michal
Hi Ayan, On 19/04/2023 15:58, Ayan Kumar Halder wrote: > > On 19/04/2023 14:54, Michal Orzel wrote: >> On 19/04/2023 15:19, Michal Orzel wrote: >>> >>> Hi Ayan, >>> >>> On 13/04/2023 19:37, Ayan Kumar Halder wrote: >>>> >>>> The DT functions (dt_read_number(), device_tree_get_reg(), >>>> fdt_get_mem_rsv()) >>>> currently accept or return 64-bit values. >>>> >>>> In future when we support 32-bit physical address, these DT >>>> functions are >>>> expected to accept/return 32-bit or 64-bit values (depending on the >>>> width of >>>> physical address). Also, we wish to detect if any truncation has >>>> occurred >>>> (i.e. while parsing 32-bit physical addresses from 64-bit values >>>> read from DT). >>>> >>>> device_tree_get_reg() should now be able to return paddr_t. This is >>>> invoked by >>>> various callers to get DT address and size. >>>> >>>> For fdt_get_mem_rsv(), we have introduced a wrapper named >>>> fdt_get_mem_rsv_paddr() which will invoke fdt_get_mem_rsv() and >>>> translate >>>> uint64_t to paddr_t. The reason being we cannot modify >>>> fdt_get_mem_rsv() as it >>>> has been imported from external source. >>>> >>>> For dt_read_number(), we have also introduced a wrapper named >>>> dt_read_paddr() >>>> dt_read_paddr() to read physical addresses. We chose not to modify >>>> the original >>>> function as it is used in places where it needs to specifically read >>>> 64-bit >>>> values from dt (For e.g. dt_property_read_u64()). >>>> >>>> Xen prints warning when it detects truncation in cases where it is >>>> not able to >>>> return error. >>>> >>>> Also, replaced u32/u64 with uint32_t/uint64_t in the functions touched >>>> by the code changes. >>>> >>>> Also, initialized variables to fix the warning >>>> "-Werror=maybe-uninitialized". >>> I can see that now you explicitly set to 0 variables passed to >>> fdt_get_mem_rsv_paddr() >>> which haven't been initialized before being passed to >>> fdt_get_mem_rsv(). Is this what >>> you are reffering to? I cannot reproduce it, hence my question. >> I can see why did you get this error. >> Before your change, we always checked for an error from >> fdt_get_mem_rsv() by checking if < 0. >> In your wrapper fdt_get_mem_rsv_paddr(), you switched (not sure why) >> to checking if not zero. >> Becasue of this, you got an error and tried to fix it by initializing >> the variables to 0. > > I actually wanted to return the error code obtained from > fdt_get_mem_rsv() to the caller. > > In this case, it returns a single error code. I would rather not rely on this. > So does this look sane to > you ? > > diff --git a/xen/include/xen/libfdt/libfdt-xen.h > b/xen/include/xen/libfdt/libfdt-xen.h > index 3296a368a6..1da87d6668 100644 > --- a/xen/include/xen/libfdt/libfdt-xen.h > +++ b/xen/include/xen/libfdt/libfdt-xen.h > @@ -22,9 +22,8 @@ static inline int fdt_get_mem_rsv_paddr(const void > *fdt, int n, > uint64_t dt_size; > int ret = 0; > > - ret = fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size); > - if ( ret ) > - return ret; > + if ( fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size) < 0 ) > + return -FDT_ERR_BADOFFSET; So the problem if you check for ret to be non-zero. But the caller of fdt_get_mem_rsv_paddr() check for < 0. Given that fdt_get_mem_rsv() is not inline, the compiler doesn't know that it will not return a positive value (other than 0). Hence why I think you get an unitialize value. The snippet below should work: if ( ret < 0 ) return ret; Cheers, -- Julien Grall
On 19/04/2023 16:18, Julien Grall wrote: > Hi Ayan, Hi Julien, > > On 19/04/2023 15:58, Ayan Kumar Halder wrote: >> >> On 19/04/2023 14:54, Michal Orzel wrote: >>> On 19/04/2023 15:19, Michal Orzel wrote: >>>> >>>> Hi Ayan, >>>> >>>> On 13/04/2023 19:37, Ayan Kumar Halder wrote: >>>>> >>>>> The DT functions (dt_read_number(), device_tree_get_reg(), >>>>> fdt_get_mem_rsv()) >>>>> currently accept or return 64-bit values. >>>>> >>>>> In future when we support 32-bit physical address, these DT >>>>> functions are >>>>> expected to accept/return 32-bit or 64-bit values (depending on >>>>> the width of >>>>> physical address). Also, we wish to detect if any truncation has >>>>> occurred >>>>> (i.e. while parsing 32-bit physical addresses from 64-bit values >>>>> read from DT). >>>>> >>>>> device_tree_get_reg() should now be able to return paddr_t. This >>>>> is invoked by >>>>> various callers to get DT address and size. >>>>> >>>>> For fdt_get_mem_rsv(), we have introduced a wrapper named >>>>> fdt_get_mem_rsv_paddr() which will invoke fdt_get_mem_rsv() and >>>>> translate >>>>> uint64_t to paddr_t. The reason being we cannot modify >>>>> fdt_get_mem_rsv() as it >>>>> has been imported from external source. >>>>> >>>>> For dt_read_number(), we have also introduced a wrapper named >>>>> dt_read_paddr() >>>>> dt_read_paddr() to read physical addresses. We chose not to modify >>>>> the original >>>>> function as it is used in places where it needs to specifically >>>>> read 64-bit >>>>> values from dt (For e.g. dt_property_read_u64()). >>>>> >>>>> Xen prints warning when it detects truncation in cases where it is >>>>> not able to >>>>> return error. >>>>> >>>>> Also, replaced u32/u64 with uint32_t/uint64_t in the functions >>>>> touched >>>>> by the code changes. >>>>> >>>>> Also, initialized variables to fix the warning >>>>> "-Werror=maybe-uninitialized". >>>> I can see that now you explicitly set to 0 variables passed to >>>> fdt_get_mem_rsv_paddr() >>>> which haven't been initialized before being passed to >>>> fdt_get_mem_rsv(). Is this what >>>> you are reffering to? I cannot reproduce it, hence my question. >>> I can see why did you get this error. >>> Before your change, we always checked for an error from >>> fdt_get_mem_rsv() by checking if < 0. >>> In your wrapper fdt_get_mem_rsv_paddr(), you switched (not sure why) >>> to checking if not zero. >>> Becasue of this, you got an error and tried to fix it by >>> initializing the variables to 0. >> >> I actually wanted to return the error code obtained from >> fdt_get_mem_rsv() to the caller. >> >> In this case, it returns a single error code. > > I would rather not rely on this. > >> So does this look sane to you ? >> >> diff --git a/xen/include/xen/libfdt/libfdt-xen.h >> b/xen/include/xen/libfdt/libfdt-xen.h >> index 3296a368a6..1da87d6668 100644 >> --- a/xen/include/xen/libfdt/libfdt-xen.h >> +++ b/xen/include/xen/libfdt/libfdt-xen.h >> @@ -22,9 +22,8 @@ static inline int fdt_get_mem_rsv_paddr(const void >> *fdt, int n, >> uint64_t dt_size; >> int ret = 0; >> >> - ret = fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size); >> - if ( ret ) >> - return ret; >> + if ( fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size) < 0 ) >> + return -FDT_ERR_BADOFFSET; > > So the problem if you check for ret to be non-zero. But the caller of > fdt_get_mem_rsv_paddr() check for < 0. > > Given that fdt_get_mem_rsv() is not inline, the compiler doesn't know > that it will not return a positive value (other than 0). Hence why I > think you get an unitialize value. > > The snippet below should work: > > if ( ret < 0 ) > return ret; Awesome, thanks for the explanation. This works. :) - Ayan > > Cheers, >
Hi, >> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h >> new file mode 100644 >> index 0000000000..3296a368a6 >> --- /dev/null >> +++ b/xen/include/xen/libfdt/libfdt-xen.h >> @@ -0,0 +1,55 @@ >> +/* >> + * SPDX-License-Identifier: GPL-2.0-only > Our CODING_STYLE says: > New files should start with a single-line SPDX comment, ..., e.g. > /* SPDX-License-Identifier: GPL-2.0 */ > > For me it would be perfectly fine to do as you did but it is not what our docs state > (i.e. single-line comment). It might be that we need to modify CODING_STYLE instead. From my reading of https://spdx.dev/ids/#how, what you suggest would not be a valid SPDX-License-Idenfier as everything in needs to be in *one* (including the begin/end comment). Cheers, -- Julien Grall
Hi Julien, On 19/04/2023 15:26, Julien Grall wrote: > > > Hi, > >>> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h >>> new file mode 100644 >>> index 0000000000..3296a368a6 >>> --- /dev/null >>> +++ b/xen/include/xen/libfdt/libfdt-xen.h >>> @@ -0,0 +1,55 @@ >>> +/* >>> + * SPDX-License-Identifier: GPL-2.0-only >> Our CODING_STYLE says: >> New files should start with a single-line SPDX comment, ..., e.g. >> /* SPDX-License-Identifier: GPL-2.0 */ >> >> For me it would be perfectly fine to do as you did but it is not what our docs state >> (i.e. single-line comment). It might be that we need to modify CODING_STYLE instead. > > From my reading of https://spdx.dev/ids/#how, what you suggest would > not be a valid SPDX-License-Idenfier as everything in needs to be in > *one* (including the begin/end comment). I said what is written in our CODING_STYLE and what we did already for lots of files, i.e. having 2 comments: /* SPDX-License-Identifier: GPL-2.0 */ /* * ... */ In the link you provided, the "*one line*" requirement refers only to SPDX short form identifier which does not contain any other information like author, description, etc.. ~Michal
On 19/04/2023 14:39, Michal Orzel wrote: > Hi Julien, > > On 19/04/2023 15:26, Julien Grall wrote: >> >> >> Hi, >> >>>> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h >>>> new file mode 100644 >>>> index 0000000000..3296a368a6 >>>> --- /dev/null >>>> +++ b/xen/include/xen/libfdt/libfdt-xen.h >>>> @@ -0,0 +1,55 @@ >>>> +/* >>>> + * SPDX-License-Identifier: GPL-2.0-only >>> Our CODING_STYLE says: >>> New files should start with a single-line SPDX comment, ..., e.g. >>> /* SPDX-License-Identifier: GPL-2.0 */ >>> >>> For me it would be perfectly fine to do as you did but it is not what our docs state >>> (i.e. single-line comment). It might be that we need to modify CODING_STYLE instead. >> >> From my reading of https://spdx.dev/ids/#how, what you suggest would >> not be a valid SPDX-License-Idenfier as everything in needs to be in >> *one* (including the begin/end comment). > I said what is written in our CODING_STYLE and what we did already for lots of files, i.e. having 2 comments: > /* SPDX-License-Identifier: GPL-2.0 */ > /* > * ... > */ My comment was about your suggestion to update the CODING_STYLE not what it is currently written. Sorry if this wasn't clear enough. > > In the link you provided, the "*one line*" requirement refers only to SPDX short form identifier > which does not contain any other information like author, description, etc.. Right because the SPDX block is intended to be in own block and there is a another block for the author, description, etc. I am not in favor of changing of the CODING_STYLE and I thought I would express it right now to spare the round-trip of writing a patch. Cheers, -- Julien Grall
On 19/04/2023 16:20, Julien Grall wrote: > > > On 19/04/2023 14:39, Michal Orzel wrote: >> Hi Julien, >> >> On 19/04/2023 15:26, Julien Grall wrote: >>> >>> >>> Hi, >>> >>>>> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h >>>>> new file mode 100644 >>>>> index 0000000000..3296a368a6 >>>>> --- /dev/null >>>>> +++ b/xen/include/xen/libfdt/libfdt-xen.h >>>>> @@ -0,0 +1,55 @@ >>>>> +/* >>>>> + * SPDX-License-Identifier: GPL-2.0-only >>>> Our CODING_STYLE says: >>>> New files should start with a single-line SPDX comment, ..., e.g. >>>> /* SPDX-License-Identifier: GPL-2.0 */ >>>> >>>> For me it would be perfectly fine to do as you did but it is not what our docs state >>>> (i.e. single-line comment). It might be that we need to modify CODING_STYLE instead. >>> >>> From my reading of https://spdx.dev/ids/#how, what you suggest would >>> not be a valid SPDX-License-Idenfier as everything in needs to be in >>> *one* (including the begin/end comment). >> I said what is written in our CODING_STYLE and what we did already for lots of files, i.e. having 2 comments: >> /* SPDX-License-Identifier: GPL-2.0 */ >> /* >> * ... >> */ > > My comment was about your suggestion to update the CODING_STYLE not what > it is currently written. Sorry if this wasn't clear enough. > >> > In the link you provided, the "*one line*" requirement refers only to > SPDX short form identifier >> which does not contain any other information like author, description, etc.. > Right because the SPDX block is intended to be in own block and there is > a another block for the author, description, etc. > > I am not in favor of changing of the CODING_STYLE and I thought I would > express it right now to spare the round-trip of writing a patch. Sure thing :) So, all in all, we agree that SPDX comment must be a single line comment on its own (which is also stated in our CODING_STYLE) and not like it was placed in this patch, right? If so, somehow the wrong placement sneaked into recently added RISCV files: arch/riscv/include/asm/csr.h arch/riscv/include/asm/riscv_encoding.h ~Michal
On 19/04/2023 15:29, Michal Orzel wrote: >> I am not in favor of changing of the CODING_STYLE and I thought I would >> express it right now to spare the round-trip of writing a patch. > Sure thing :) > So, all in all, we agree that SPDX comment must be a single line comment on its own > (which is also stated in our CODING_STYLE) and not like it was placed in this patch, right? Correct. Cheers, -- Julien Grall
© 2016 - 2026 Red Hat, Inc.