Update to use byteswap to swap bytes.

No functional change.

Signed-off-by: Lin Liu <lin.liu@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Wei Liu <wl@xen.org>
Changes in v3:
- Update xen/common/device_tree.c to use be32_to_cpu
- Keep const in type cast in unaligned.h
---
 xen/common/device_tree.c           | 44 +++++++++++++++---------------
 xen/common/libelf/libelf-private.h |  6 ++--
 xen/common/xz/private.h            |  2 +-
 xen/include/xen/unaligned.h        | 24 ++++++++--------
 4 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 4aae281e89..70d3be3be6 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
     if ( !val || len < sizeof(*out_value) )
         return 0;
 
-    *out_value = be32_to_cpup(val);
+    *out_value = be32_to_cpu(*val);
 
     return 1;
 }
@@ -264,7 +264,7 @@ int dt_property_read_variable_u32_array(const struct dt_device_node *np,
 
     count = sz;
     while ( count-- )
-        *out_values++ = be32_to_cpup(val++);
+        *out_values++ = be32_to_cpu(*val++);
 
     return sz;
 }
@@ -490,7 +490,7 @@ static int __dt_n_addr_cells(const struct dt_device_node *np, bool_t parent)
 
         ip = dt_get_property(np, "#address-cells", NULL);
         if ( ip )
-            return be32_to_cpup(ip);
+            return be32_to_cpu(*ip);
     } while ( np->parent );
     /* No #address-cells property for the root node */
     return DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
@@ -507,7 +507,7 @@ int __dt_n_size_cells(const struct dt_device_node *np, bool_t parent)
 
         ip = dt_get_property(np, "#size-cells", NULL);
         if ( ip )
-            return be32_to_cpup(ip);
+            return be32_to_cpu(*ip);
     } while ( np->parent );
     /* No #address-cells property for the root node */
     return DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
@@ -660,7 +660,7 @@ static void dt_bus_pci_count_cells(const struct dt_device_node *np,
 static unsigned int dt_bus_pci_get_flags(const __be32 *addr)
 {
     unsigned int flags = 0;
-    u32 w = be32_to_cpup(addr);
+    u32 w = be32_to_cpu(*addr);
 
     switch((w >> 24) & 0x03) {
     case 0x01:
@@ -1077,7 +1077,7 @@ dt_irq_find_parent(const struct dt_device_node *child)
         if ( parp == NULL )
             p = dt_get_parent(child);
         else
-            p = dt_find_node_by_phandle(be32_to_cpup(parp));
+            p = dt_find_node_by_phandle(be32_to_cpu(*parp));
         child = p;
     } while ( p && dt_get_property(p, "#interrupt-cells", NULL) == NULL );
 
@@ -1110,7 +1110,7 @@ unsigned int dt_number_of_irq(const struct dt_device_node *device)
     intlen /= sizeof(*intspec);
 
     dt_dprintk(" using 'interrupts' property\n");
-    dt_dprintk(" intspec=%d intlen=%d\n", be32_to_cpup(intspec), intlen);
+    dt_dprintk(" intspec=%d intlen=%d\n", be32_to_cpu(*intspec), intlen);
 
     /* Look for the interrupt parent. */
     p = dt_irq_find_parent(device);
@@ -1241,7 +1241,7 @@ int dt_for_each_irq_map(const struct dt_device_node *dev,
         imaplen -= addrsize + intsize;
 
         /* Get the interrupt parent */
-        ipar = dt_find_node_by_phandle(be32_to_cpup(imap));
+        ipar = dt_find_node_by_phandle(be32_to_cpu(*imap));
         imap++;
         --imaplen;
 
@@ -1358,8 +1358,8 @@ static int dt_irq_map_raw(const struct dt_device_node *parent,
     int match, i;
 
     dt_dprintk("dt_irq_map_raw: par=%s,intspec=[0x%08x 0x%08x...],ointsize=%d\n",
-               parent->full_name, be32_to_cpup(intspec),
-               be32_to_cpup(intspec + 1), ointsize);
+               parent->full_name, be32_to_cpu(*intspec),
+               be32_to_cpu(*(intspec+1)), ointsize);
 
     ipar = parent;
 
@@ -1471,7 +1471,7 @@ static int dt_irq_map_raw(const struct dt_device_node *parent,
             dt_dprintk(" -> match=%d (imaplen=%d)\n", match, imaplen);
 
             /* Get the interrupt parent */
-            newpar = dt_find_node_by_phandle(be32_to_cpup(imap));
+            newpar = dt_find_node_by_phandle(be32_to_cpu(*imap));
             imap++;
             --imaplen;
 
@@ -1565,7 +1565,7 @@ int dt_device_get_raw_irq(const struct dt_device_node *device,
     intlen /= sizeof(*intspec);
 
     dt_dprintk(" using 'interrupts' property\n");
-    dt_dprintk(" intspec=%d intlen=%d\n", be32_to_cpup(intspec), intlen);
+    dt_dprintk(" intspec=%d intlen=%d\n", be32_to_cpu(*intspec), intlen);
 
     /* Look for the interrupt parent. */
     p = dt_irq_find_parent(device);
@@ -1676,7 +1676,7 @@ static int __dt_parse_phandle_with_args(const struct dt_device_node *np,
          * If phandle is 0, then it is an empty entry with no
          * arguments.  Skip forward to the next entry.
          * */
-        phandle = be32_to_cpup(list++);
+        phandle = be32_to_cpu(*list++);
         if ( phandle )
         {
             /*
@@ -1745,7 +1745,7 @@ static int __dt_parse_phandle_with_args(const struct dt_device_node *np,
                 out_args->np = node;
                 out_args->args_count = count;
                 for ( i = 0; i < count; i++ )
-                    out_args->args[i] = be32_to_cpup(list++);
+                    out_args->args[i] = be32_to_cpu(*list++);
             }
 
             /* Found it! return success */
@@ -1826,7 +1826,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
     int has_name = 0;
     int new_format = 0;
 
-    tag = be32_to_cpup((__be32 *)(*p));
+    tag = be32_to_cpu(*(__be32 *)(*p));
     if ( tag != FDT_BEGIN_NODE )
     {
         printk(XENLOG_WARNING "Weird tag at start of node: %x\n", tag);
@@ -1919,7 +1919,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
         u32 sz, noff;
         const char *pname;
 
-        tag = be32_to_cpup((__be32 *)(*p));
+        tag = be32_to_cpu(*(__be32 *)(*p));
         if ( tag == FDT_NOP )
         {
             *p += 4;
@@ -1928,8 +1928,8 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
         if ( tag != FDT_PROP )
             break;
         *p += 4;
-        sz = be32_to_cpup((__be32 *)(*p));
-        noff = be32_to_cpup((__be32 *)((*p) + 4));
+        sz = be32_to_cpu(*(__be32 *)(*p));
+        noff = be32_to_cpu(*(__be32 *)((*p) + 4));
         *p += 8;
         if ( fdt_version(fdt) < 0x10 )
             *p = ROUNDUP(*p, sz >= 8 ? 8 : 4);
@@ -1956,13 +1956,13 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
                  (strcmp(pname, "linux,phandle") == 0) )
             {
                 if ( np->phandle == 0 )
-                    np->phandle = be32_to_cpup((__be32*)*p);
+                    np->phandle = be32_to_cpu(*(__be32*)*p);
             }
             /* And we process the "ibm,phandle" property
              * used in pSeries dynamic device tree
              * stuff */
             if ( strcmp(pname, "ibm,phandle") == 0 )
-                np->phandle = be32_to_cpup((__be32 *)*p);
+                np->phandle = be32_to_cpu(*(__be32 *)*p);
             pp->name = pname;
             pp->length = sz;
             pp->value = (void *)*p;
@@ -2034,7 +2034,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
             *p += 4;
         else
             mem = unflatten_dt_node(fdt, mem, p, np, allnextpp, fpsize);
-        tag = be32_to_cpup((__be32 *)(*p));
+        tag = be32_to_cpu(*(__be32 *)(*p));
     }
     if ( tag != FDT_END_NODE )
     {
@@ -2086,7 +2086,7 @@ static void __init __unflatten_device_tree(const void *fdt,
     /* Second pass, do actual unflattening */
     start = ((unsigned long)fdt) + fdt_off_dt_struct(fdt);
     unflatten_dt_node(fdt, mem, &start, NULL, &allnextp, 0);
-    if ( be32_to_cpup((__be32 *)start) != FDT_END )
+    if ( be32_to_cpu(*(__be32 *)start) != FDT_END )
         printk(XENLOG_WARNING "Weird tag at end of tree: %08x\n",
                   *((u32 *)start));
     if ( be32_to_cpu(((__be32 *)mem)[size / 4]) != 0xdeadbeef )
diff --git a/xen/common/libelf/libelf-private.h b/xen/common/libelf/libelf-private.h
index 47db679966..6062598fb8 100644
--- a/xen/common/libelf/libelf-private.h
+++ b/xen/common/libelf/libelf-private.h
@@ -31,9 +31,9 @@
    printk(fmt, ## args )
 
 #define strtoull(str, end, base) simple_strtoull(str, end, base)
-#define bswap_16(x) swab16(x)
-#define bswap_32(x) swab32(x)
-#define bswap_64(x) swab64(x)
+#define bswap_16(x) bswap16(x)
+#define bswap_32(x) bswap32(x)
+#define bswap_64(x) bswap64(x)
 
 #else /* !__XEN__ */
 
diff --git a/xen/common/xz/private.h b/xen/common/xz/private.h
index 511343fcc2..97131fa714 100644
--- a/xen/common/xz/private.h
+++ b/xen/common/xz/private.h
@@ -28,7 +28,7 @@ static inline void put_unaligned_le32(u32 val, void *p)
 
 #endif
 
-#define get_le32(p) le32_to_cpup((const uint32_t *)(p))
+#define get_le32(p) le32_to_cpu(*(const uint32_t *)(p))
 
 #define false 0
 #define true 1
diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
index 0a2b16d05d..16b2e6f5f0 100644
--- a/xen/include/xen/unaligned.h
+++ b/xen/include/xen/unaligned.h
@@ -20,62 +20,62 @@
 
 static inline uint16_t get_unaligned_be16(const void *p)
 {
-	return be16_to_cpup(p);
+	return be16_to_cpu(*(const uint16_t *)p);
 }
 
 static inline void put_unaligned_be16(uint16_t val, void *p)
 {
-	*(__force __be16*)p = cpu_to_be16(val);
+	*(__be16 *)p = cpu_to_be16(val);
 }
 
 static inline uint32_t get_unaligned_be32(const void *p)
 {
-	return be32_to_cpup(p);
+	return be32_to_cpu(*(const uint32_t *)p);
 }
 
 static inline void put_unaligned_be32(uint32_t val, void *p)
 {
-	*(__force __be32*)p = cpu_to_be32(val);
+	*(__be32 *)p = cpu_to_be32(val);
 }
 
 static inline uint64_t get_unaligned_be64(const void *p)
 {
-	return be64_to_cpup(p);
+	return be64_to_cpu(*(const uint64_t *)p);
 }
 
 static inline void put_unaligned_be64(uint64_t val, void *p)
 {
-	*(__force __be64*)p = cpu_to_be64(val);
+	*(__be64 *)p = cpu_to_be64(val);
 }
 
 static inline uint16_t get_unaligned_le16(const void *p)
 {
-	return le16_to_cpup(p);
+	return le16_to_cpu(*(const uint16_t *)p);
 }
 
 static inline void put_unaligned_le16(uint16_t val, void *p)
 {
-	*(__force __le16*)p = cpu_to_le16(val);
+	*(__le16 *)p = cpu_to_le16(val);
 }
 
 static inline uint32_t get_unaligned_le32(const void *p)
 {
-	return le32_to_cpup(p);
+	return le32_to_cpu(*(const uint32_t *)p);
 }
 
 static inline void put_unaligned_le32(uint32_t val, void *p)
 {
-	*(__force __le32*)p = cpu_to_le32(val);
+	*(__le32 *)p = cpu_to_le32(val);
 }
 
 static inline uint64_t get_unaligned_le64(const void *p)
 {
-	return le64_to_cpup(p);
+	return le64_to_cpu(*(const uint64_t *)p);
 }
 
 static inline void put_unaligned_le64(uint64_t val, void *p)
 {
-	*(__force __le64*)p = cpu_to_le64(val);
+	*(__le64 *)p = cpu_to_le64(val);
 }
 
 #endif /* __XEN_UNALIGNED_H__ */
-- 
2.27.0

On Tue, May 10, 2022 at 06:15:22AM -0400, Lin Liu wrote:
> Update to use byteswap to swap bytes.
> 
> No functional change.
> 
> Signed-off-by: Lin Liu <lin.liu@citrix.com>

FYI, this patch breaks build of stubdomain:

In file included from /var/tmp/git.xen.lU52/stubdom/include/../../xen/common/unxz.c:124,
                 from xg_dom_decompress_unsafe_xz.c:40:
/var/tmp/git.xen.lU52/stubdom/include/../../xen/common/xz/dec_stream.c: In function ‘dec_stream_header’:
/var/tmp/git.xen.lU52/stubdom/include/../../xen/common/xz/private.h:31:21: error: implicit declaration of function ‘le32_to_cpu’; did you mean ‘le32_to_cpup’? [-Werror=implicit-function-declaration]
   31 | #define get_le32(p) le32_to_cpu(*(const uint32_t *)(p))
      |                     ^~~~~~~~~~~
/var/tmp/git.xen.lU52/stubdom/include/../../xen/common/xz/dec_stream.c:393:7: note: in expansion of macro ‘get_le32’
  393 |    != get_le32(s->temp.buf + HEADER_MAGIC_SIZE + 2))
      |       ^~~~~~~~
cc1: all warnings being treated as errors
make[2]: *** [/var/tmp/git.xen.lU52/stubdom/libs-x86_64/guest/../../../tools/Rules.mk:150: xg_dom_decompress_unsafe_xz.o] Error 1
make[1]: *** [Makefile:367: libs-x86_64/guest/libxenguest.a] Error 2
make: *** [Makefile:73: build-stubdom] Error 2

Cheers,

-- 
Anthony PERARD

Hi,

On 10/05/2022 11:15, Lin Liu wrote:
> Update to use byteswap to swap bytes.
> 
> No functional change.
> 
> Signed-off-by: Lin Liu <lin.liu@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien@xen.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Wei Liu <wl@xen.org>
> Changes in v3:
> - Update xen/common/device_tree.c to use be32_to_cpu
> - Keep const in type cast in unaligned.h
> ---
>   xen/common/device_tree.c           | 44 +++++++++++++++---------------
>   xen/common/libelf/libelf-private.h |  6 ++--
>   xen/common/xz/private.h            |  2 +-
>   xen/include/xen/unaligned.h        | 24 ++++++++--------
>   4 files changed, 38 insertions(+), 38 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 4aae281e89..70d3be3be6 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
>       if ( !val || len < sizeof(*out_value) )
>           return 0;
>   
> -    *out_value = be32_to_cpup(val);
> +    *out_value = be32_to_cpu(*val);

This code has been taken from Linux and I would rather prefer to keep 
the *cpup* helpers to avoid any changes when backporting.

> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
> index 0a2b16d05d..16b2e6f5f0 100644
> --- a/xen/include/xen/unaligned.h
> +++ b/xen/include/xen/unaligned.h
> @@ -20,62 +20,62 @@
>   
>   static inline uint16_t get_unaligned_be16(const void *p)
>   {
> -	return be16_to_cpup(p);
> +	return be16_to_cpu(*(const uint16_t *)p)

I haven't checked the existing implementation of be16_to_cpup(). 
However, this new approach would allow the compiler to use a single load 
instruction to read the 16-bit value from memory. So this change may 
break on platform where unaligned access is forbidden (such as arm32).

>   }
>   
>   static inline void put_unaligned_be16(uint16_t val, void *p)
>   {
> -	*(__force __be16*)p = cpu_to_be16(val);
> +	*(__be16 *)p = cpu_to_be16(val);

Why did you drop the __force?

>   }
>

Cheers,

-- 
Julien Grall

Subject: Re: [PATCH v3 4/6] xen: Switch to byteswap
Hi,

On 10/05/2022 11:15, Lin Liu wrote:
> Update to use byteswap to swap bytes.
>
> No functional change.
>
> Signed-off-by: Lin Liu <lin.liu@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien@xen.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Wei Liu <wl@xen.org>
> Changes in v3:
> - Update xen/common/device_tree.c to use be32_to_cpu
> - Keep const in type cast in unaligned.h
> ---
>   xen/common/device_tree.c           | 44 +++++++++++++++---------------
>   xen/common/libelf/libelf-private.h |  6 ++--
>   xen/common/xz/private.h            |  2 +-
>   xen/include/xen/unaligned.h        | 24 ++++++++--------
>   4 files changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 4aae281e89..70d3be3be6 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
>       if ( !val || len < sizeof(*out_value) )
>           return 0;
>
> -    *out_value = be32_to_cpup(val);
> +    *out_value = be32_to_cpu(*val);

> This code has been taken from Linux and I would rather prefer to keep
>the *cpup* helpers to avoid any changes when backporting.

> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
> index 0a2b16d05d..16b2e6f5f0 100644
> --- a/xen/include/xen/unaligned.h
> +++ b/xen/include/xen/unaligned.h
> @@ -20,62 +20,62 @@
>
>   static inline uint16_t get_unaligned_be16(const void *p)
>   {
> -     return be16_to_cpup(p);
> +     return be16_to_cpu(*(const uint16_t *)p)

> I haven't checked the existing implementation of be16_to_cpup().
> However, this new approach would allow the compiler to use a single load
> instruction to read the 16-bit value from memory. So this change may
> break on platform where unaligned access is forbidden (such as arm32).

>   }
>
>   static inline void put_unaligned_be16(uint16_t val, void *p)
>   {
> -     *(__force __be16*)p = cpu_to_be16(val);
> +     *(__be16 *)p = cpu_to_be16(val);

>> Why did you drop the __force?

Google told me __force is used in linux kernel to suppress warning in sparse,
https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
Is sparse also used in xen?


>   }
>

Hi,

Please configure your e-mail client to send in plain text.

On 11/05/2022 07:30, Lin Liu （刘林） wrote:
> Subject: Re: [PATCH v3 4/6] xen: Switch to byteswap
> On 10/05/2022 11:15, Lin Liu wrote:
>> Update to use byteswap to swap bytes.
>>
>> No functional change.
>>
>> Signed-off-by: Lin Liu <lin.liu@citrix.com>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <george.dunlap@citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Wei Liu <wl@xen.org>
>> Changes in v3:
>> - Update xen/common/device_tree.c to use be32_to_cpu
>> - Keep const in type cast in unaligned.h
>> ---
>>    xen/common/device_tree.c           | 44 +++++++++++++++---------------
>>    xen/common/libelf/libelf-private.h |  6 ++--
>>    xen/common/xz/private.h            |  2 +-
>>    xen/include/xen/unaligned.h        | 24 ++++++++--------
>>    4 files changed, 38 insertions(+), 38 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 4aae281e89..70d3be3be6 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
>>        if ( !val || len < sizeof(*out_value) )
>>            return 0;
>>
>> -    *out_value = be32_to_cpup(val);
>> +    *out_value = be32_to_cpu(*val);
> 
>> This code has been taken from Linux and I would rather prefer to keep
>> the *cpup* helpers to avoid any changes when backporting.
> 
>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>> index 0a2b16d05d..16b2e6f5f0 100644
>> --- a/xen/include/xen/unaligned.h
>> +++ b/xen/include/xen/unaligned.h
>> @@ -20,62 +20,62 @@
>>
>>    static inline uint16_t get_unaligned_be16(const void *p)
>>    {
>> -     return be16_to_cpup(p);
>> +     return be16_to_cpu(*(const uint16_t *)p)
> 
>> I haven't checked the existing implementation of be16_to_cpup().
>> However, this new approach would allow the compiler to use a single load
>> instruction to read the 16-bit value from memory. So this change may
>> break on platform where unaligned access is forbidden (such as arm32).
> 
>>    }
>>
>>    static inline void put_unaligned_be16(uint16_t val, void *p)
>>    {
>> -     *(__force __be16*)p = cpu_to_be16(val);
>> +     *(__be16 *)p = cpu_to_be16(val);
> 
>>> Why did you drop the __force?
> 
> Google told me __force is used in linux kernel to suppress warning in sparse,
> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
> Is sparse also used in xen?

I am not aware of any use of Sparse in Xen, but it would technically be 
possible.

However, my point here is more that this change seems to be unrelated to 
what the patch is meant to do (i.e. switching to byteswap). So if it is 
unnecessary, then it should be dropped from this patch.

Cheers,

-- 
Julien Grall

> On May 11, 2022, at 9:34 AM, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> Please configure your e-mail client to send in plain text.
> 
> On 11/05/2022 07:30, Lin Liu （刘林） wrote:
>> Subject: Re: [PATCH v3 4/6] xen: Switch to byteswap
>> On 10/05/2022 11:15, Lin Liu wrote:
>>> Update to use byteswap to swap bytes.
>>> 
>>> No functional change.
>>> 
>>> Signed-off-by: Lin Liu <lin.liu@citrix.com>
>>> ---
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Julien Grall <julien@xen.org>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Cc: George Dunlap <george.dunlap@citrix.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Wei Liu <wl@xen.org>
>>> Changes in v3:
>>> - Update xen/common/device_tree.c to use be32_to_cpu
>>> - Keep const in type cast in unaligned.h
>>> ---
>>>   xen/common/device_tree.c           | 44 +++++++++++++++---------------
>>>   xen/common/libelf/libelf-private.h |  6 ++--
>>>   xen/common/xz/private.h            |  2 +-
>>>   xen/include/xen/unaligned.h        | 24 ++++++++--------
>>>   4 files changed, 38 insertions(+), 38 deletions(-)
>>> 
>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>> index 4aae281e89..70d3be3be6 100644
>>> --- a/xen/common/device_tree.c
>>> +++ b/xen/common/device_tree.c
>>> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
>>>       if ( !val || len < sizeof(*out_value) )
>>>           return 0;
>>> 
>>> -    *out_value = be32_to_cpup(val);
>>> +    *out_value = be32_to_cpu(*val);
>>> This code has been taken from Linux and I would rather prefer to keep
>>> the *cpup* helpers to avoid any changes when backporting.
>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>> index 0a2b16d05d..16b2e6f5f0 100644
>>> --- a/xen/include/xen/unaligned.h
>>> +++ b/xen/include/xen/unaligned.h
>>> @@ -20,62 +20,62 @@
>>> 
>>>   static inline uint16_t get_unaligned_be16(const void *p)
>>>   {
>>> -     return be16_to_cpup(p);
>>> +     return be16_to_cpu(*(const uint16_t *)p)
>>> I haven't checked the existing implementation of be16_to_cpup().
>>> However, this new approach would allow the compiler to use a single load
>>> instruction to read the 16-bit value from memory. So this change may
>>> break on platform where unaligned access is forbidden (such as arm32).
>>>   }
>>> 
>>>   static inline void put_unaligned_be16(uint16_t val, void *p)
>>>   {
>>> -     *(__force __be16*)p = cpu_to_be16(val);
>>> +     *(__be16 *)p = cpu_to_be16(val);
>>>> Why did you drop the __force?
>> Google told me __force is used in linux kernel to suppress warning in sparse,
>> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
>> Is sparse also used in xen?
> 
> I am not aware of any use of Sparse in Xen, but it would technically be possible.
> 
> However, my point here is more that this change seems to be unrelated to what the patch is meant to do (i.e. switching to byteswap). So if it is unnecessary, then it should be dropped from this patch.

I think making people pull little changes like this out into separate patches is asking too much.  It’s a lot of extra effort on the part of the submitter for basically no value.  We commonly do little clean-ups like this in patches, and just require a comment at the bottom, like this:

8<—

While here:
- Drop ‘_force’ keyword, which is only needed when running the Sparse analysis tool

—>8

I do agree that minor changes like this need to be described, so that people 5 years from now have some hope of figuring out what’s going on.

 -George

Hi George,

> On 11 May 2022, at 13:11, George Dunlap <george.dunlap@citrix.com> wrote:
> 
> 
> 
>> On May 11, 2022, at 9:34 AM, Julien Grall <julien@xen.org> wrote:
>> 
>> Hi,
>> 
>> Please configure your e-mail client to send in plain text.
>> 
>> On 11/05/2022 07:30, Lin Liu （刘林） wrote:
>>> Subject: Re: [PATCH v3 4/6] xen: Switch to byteswap
>>> On 10/05/2022 11:15, Lin Liu wrote:
>>>> Update to use byteswap to swap bytes.
>>>> 
>>>> No functional change.
>>>> 
>>>> Signed-off-by: Lin Liu <lin.liu@citrix.com>
>>>> ---
>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>> Cc: Julien Grall <julien@xen.org>
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Cc: George Dunlap <george.dunlap@citrix.com>
>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>> Cc: Wei Liu <wl@xen.org>
>>>> Changes in v3:
>>>> - Update xen/common/device_tree.c to use be32_to_cpu
>>>> - Keep const in type cast in unaligned.h
>>>> ---
>>>> xen/common/device_tree.c | 44 +++++++++++++++---------------
>>>> xen/common/libelf/libelf-private.h | 6 ++--
>>>> xen/common/xz/private.h | 2 +-
>>>> xen/include/xen/unaligned.h | 24 ++++++++--------
>>>> 4 files changed, 38 insertions(+), 38 deletions(-)
>>>> 
>>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>>> index 4aae281e89..70d3be3be6 100644
>>>> --- a/xen/common/device_tree.c
>>>> +++ b/xen/common/device_tree.c
>>>> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
>>>> if ( !val || len < sizeof(*out_value) )
>>>> return 0;
>>>> 
>>>> - *out_value = be32_to_cpup(val);
>>>> + *out_value = be32_to_cpu(*val);
>>>> This code has been taken from Linux and I would rather prefer to keep
>>>> the *cpup* helpers to avoid any changes when backporting.
>>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>>> index 0a2b16d05d..16b2e6f5f0 100644
>>>> --- a/xen/include/xen/unaligned.h
>>>> +++ b/xen/include/xen/unaligned.h
>>>> @@ -20,62 +20,62 @@
>>>> 
>>>> static inline uint16_t get_unaligned_be16(const void *p)
>>>> {
>>>> - return be16_to_cpup(p);
>>>> + return be16_to_cpu(*(const uint16_t *)p)
>>>> I haven't checked the existing implementation of be16_to_cpup().
>>>> However, this new approach would allow the compiler to use a single load
>>>> instruction to read the 16-bit value from memory. So this change may
>>>> break on platform where unaligned access is forbidden (such as arm32).
>>>> }
>>>> 
>>>> static inline void put_unaligned_be16(uint16_t val, void *p)
>>>> {
>>>> - *(__force __be16*)p = cpu_to_be16(val);
>>>> + *(__be16 *)p = cpu_to_be16(val);
>>>>> Why did you drop the __force?
>>> Google told me __force is used in linux kernel to suppress warning in sparse,
>>> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
>>> Is sparse also used in xen?
>> 
>> I am not aware of any use of Sparse in Xen, but it would technically be possible.
>> 
>> However, my point here is more that this change seems to be unrelated to what the patch is meant to do (i.e. switching to byteswap). So if it is unnecessary, then it should be dropped from this patch.
> 
> I think making people pull little changes like this out into separate patches is asking too much. It’s a lot of extra effort on the part of the submitter for basically no value. We commonly do little clean-ups like this in patches, and just require a comment at the bottom, like this:
> 
> 8<—
> 
> While here:
> - Drop ‘_force’ keyword, which is only needed when running the Sparse analysis tool
> 
> —>8
> 
> I do agree that minor changes like this need to be described, so that people 5 years from now have some hope of figuring out what’s going on.

I fully agree here. The effort involved by splitting a patch in several ones (both for the
contributor and the maintainers) means it should be prevented unless the original patch
could not be reviewed as is (patch to long or to complex to test for example but there
might be other valid cases).
Modifying the commit message to describe all changes is definitely mandatory
 though (but could be done without a v+1).

Bertrand

> 
> -George

On 11.05.2022 16:21, Bertrand Marquis wrote:
>> On 11 May 2022, at 13:11, George Dunlap <george.dunlap@citrix.com> wrote:
>>> On May 11, 2022, at 9:34 AM, Julien Grall <julien@xen.org> wrote:
>>> On 11/05/2022 07:30, Lin Liu （刘林） wrote:
>>>> On 10/05/2022 11:15, Lin Liu wrote:
>>>>> static inline void put_unaligned_be16(uint16_t val, void *p)
>>>>> {
>>>>> - *(__force __be16*)p = cpu_to_be16(val);
>>>>> + *(__be16 *)p = cpu_to_be16(val);
>>>>>> Why did you drop the __force?
>>>> Google told me __force is used in linux kernel to suppress warning in sparse,
>>>> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
>>>> Is sparse also used in xen?
>>>
>>> I am not aware of any use of Sparse in Xen, but it would technically be possible.
>>>
>>> However, my point here is more that this change seems to be unrelated to what the patch is meant to do (i.e. switching to byteswap). So if it is unnecessary, then it should be dropped from this patch.
>>
>> I think making people pull little changes like this out into separate patches is asking too much. It’s a lot of extra effort on the part of the submitter for basically no value. We commonly do little clean-ups like this in patches, and just require a comment at the bottom, like this:
>>
>> 8<—
>>
>> While here:
>> - Drop ‘_force’ keyword, which is only needed when running the Sparse analysis tool
>>
>> —>8
>>
>> I do agree that minor changes like this need to be described, so that people 5 years from now have some hope of figuring out what’s going on.
> 
> I fully agree here. The effort involved by splitting a patch in several ones (both for the
> contributor and the maintainers) means it should be prevented unless the original patch
> could not be reviewed as is (patch to long or to complex to test for example but there
> might be other valid cases).

This is fine for uncontroversial changes, but not in cases like this
one. Like Julien, I think we should rather strive towards completing
the Linux-inherited annotations like __force. Even without the actual
use of sparse they serve a documentation purpose.

Jan

Hi George,

On 11/05/2022 13:11, George Dunlap wrote:
>>> Google told me __force is used in linux kernel to suppress warning in sparse,
>>> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
>>> Is sparse also used in xen?
>>
>> I am not aware of any use of Sparse in Xen, but it would technically be possible.
>>
>> However, my point here is more that this change seems to be unrelated to what the patch is meant to do (i.e. switching to byteswap). So if it is unnecessary, then it should be dropped from this patch.
> 
> I think making people pull little changes like this out into separate patches is asking too much.  It’s a lot of extra effort on the part of the submitter for basically no value.  We commonly do little clean-ups like this in patches, and just require a comment at the bottom, like this:

I suggested to drop from the patch because I don't think we should 
remove the __force. In fact, I have contemplated a few times in the past 
to use sparse in Xen.

Sorry I should have been clearer.

Cheers,

-- 
Julien Grall

On 10/05/2022 11:51, Julien Grall wrote:
> On 10/05/2022 11:15, Lin Liu wrote:
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 4aae281e89..70d3be3be6 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct
>> dt_device_node *np,
>>       if ( !val || len < sizeof(*out_value) )
>>           return 0;
>>   -    *out_value = be32_to_cpup(val);
>> +    *out_value = be32_to_cpu(*val);
>
> This code has been taken from Linux and I would rather prefer to keep
> the *cpup* helpers to avoid any changes when backporting.

I specifically requested that this be de-obfuscated.  Hiding indirection
is a fantastic way to introduce bugs, and we've had XSAs in the past
because of it (admittedly in libxl, but still...).

This file is already Xen style, not Linux, so won't be taking backports
directly, and the resulting compiler diagnostic will make it obvious
what is going on. be32_to_cpu(*val) works fine on older versions of Xen too.

In this case, the cost of changing is well worth the improvements and
simplifications gained.  See the 0/6 diffstat and see that the compiler
can make better optimisations when it can see the builtin.

>
>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>> index 0a2b16d05d..16b2e6f5f0 100644
>> --- a/xen/include/xen/unaligned.h
>> +++ b/xen/include/xen/unaligned.h
>> @@ -20,62 +20,62 @@
>>     static inline uint16_t get_unaligned_be16(const void *p)
>>   {
>> -    return be16_to_cpup(p);
>> +    return be16_to_cpu(*(const uint16_t *)p)
>
> I haven't checked the existing implementation of be16_to_cpup().

It's a plain dereference, just like this.  AFAICT, it wasn't unaligned
safe before, either.

It should be reasonably easy to fix in a followup patch.  Just memcpy()
to/from the void pointer to a stack variable of the appropriate type.

~Andrew

Hi,

On 10/05/2022 12:09, Andrew Cooper wrote:
> On 10/05/2022 11:51, Julien Grall wrote:
>> On 10/05/2022 11:15, Lin Liu wrote:
>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>> index 4aae281e89..70d3be3be6 100644
>>> --- a/xen/common/device_tree.c
>>> +++ b/xen/common/device_tree.c
>>> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct
>>> dt_device_node *np,
>>>        if ( !val || len < sizeof(*out_value) )
>>>            return 0;
>>>    -    *out_value = be32_to_cpup(val);
>>> +    *out_value = be32_to_cpu(*val);
>>
>> This code has been taken from Linux and I would rather prefer to keep
>> the *cpup* helpers to avoid any changes when backporting.
> 
> I specifically requested that this be de-obfuscated.  Hiding indirection
> is a fantastic way to introduce bugs, and we've had XSAs in the past
> because of it (admittedly in libxl, but still...).

Care providing a link to those XSAs? But I don't really see what's the 
problem here, this is no better no worth than passing pointer to other 
functions...

> 
> This file is already Xen style, not Linux, so won't be taking backports
> directly, and the resulting compiler diagnostic will make it obvious
> what is going on. be32_to_cpu(*val) works fine on older versions of Xen too.
> 
> In this case, the cost of changing is well worth the improvements and
> simplifications gained.  See the 0/6 diffstat and see that the compiler
> can make better optimisations when it can see the builtin.

I take your point... However, the commit message provides virtually zero 
justification into why we should switch to be32_to_cpup(). So to me, the 
changes so far looks unwanted.

> 
>>
>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>> index 0a2b16d05d..16b2e6f5f0 100644
>>> --- a/xen/include/xen/unaligned.h
>>> +++ b/xen/include/xen/unaligned.h
>>> @@ -20,62 +20,62 @@
>>>      static inline uint16_t get_unaligned_be16(const void *p)
>>>    {
>>> -    return be16_to_cpup(p);
>>> +    return be16_to_cpu(*(const uint16_t *)p)
>>
>> I haven't checked the existing implementation of be16_to_cpup().
> 
> It's a plain dereference, just like this.  AFAICT, it wasn't unaligned
> safe before, either.

Well, technically an architecture could provide an override for the 
copy. I agree that arm32 is already bogus but...

> 
> It should be reasonably easy to fix in a followup patch.  Just memcpy()
> to/from the void pointer to a stack variable of the appropriate type.
... I disagree that it should be fixed in a follow-up patch. It should 
be fixed now as this is where the badness is spread to any architecture.

Cheers,

-- 
Julien Grall

On 10/05/2022 12:17, Julien Grall wrote:
>>
>>>
>>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>>> index 0a2b16d05d..16b2e6f5f0 100644
>>>> --- a/xen/include/xen/unaligned.h
>>>> +++ b/xen/include/xen/unaligned.h
>>>> @@ -20,62 +20,62 @@
>>>>      static inline uint16_t get_unaligned_be16(const void *p)
>>>>    {
>>>> -    return be16_to_cpup(p);
>>>> +    return be16_to_cpu(*(const uint16_t *)p)
>>>
>>> I haven't checked the existing implementation of be16_to_cpup().
>>
>> It's a plain dereference, just like this.  AFAICT, it wasn't unaligned
>> safe before, either.
>
> Well, technically an architecture could provide an override for the
> copy. I agree that arm32 is already bogus but...
>
>>
>> It should be reasonably easy to fix in a followup patch.  Just memcpy()
>> to/from the void pointer to a stack variable of the appropriate type.
> ... I disagree that it should be fixed in a follow-up patch. It should
> be fixed now as this is where the badness is spread to any architecture.

No.  That is an inappropriate request to make.

Lin's patch does not alter the broken-ness of unaligned on arm32, and
does improve the aspect of the hypervisor that it pertains to.  It
therefore stands on its own merit.

Your choices are to either fix it yourself (after all, you are the
maintainer who cares about this unrelated bug), or you ask Lin kindly if
he has time to look into fixing the unrelated bug after this series is
complete.

It is not reasonable to say "this unrelated thing is broken, and you
need to fix it first to get your series in".  Requests like that are,
I'm sure, part of what Bertrand raised in the community call as
unnecessary fiction getting work submitted.

~Andrew

Hi,

On 10/05/2022 12:34, Andrew Cooper wrote:
> On 10/05/2022 12:17, Julien Grall wrote:
>>>
>>>>
>>>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>>>> index 0a2b16d05d..16b2e6f5f0 100644
>>>>> --- a/xen/include/xen/unaligned.h
>>>>> +++ b/xen/include/xen/unaligned.h
>>>>> @@ -20,62 +20,62 @@
>>>>>       static inline uint16_t get_unaligned_be16(const void *p)
>>>>>     {
>>>>> -    return be16_to_cpup(p);
>>>>> +    return be16_to_cpu(*(const uint16_t *)p)
>>>>
>>>> I haven't checked the existing implementation of be16_to_cpup().
>>>
>>> It's a plain dereference, just like this.  AFAICT, it wasn't unaligned
>>> safe before, either.
>>
>> Well, technically an architecture could provide an override for the
>> copy. I agree that arm32 is already bogus but...
>>
>>>
>>> It should be reasonably easy to fix in a followup patch.  Just memcpy()
>>> to/from the void pointer to a stack variable of the appropriate type.
>> ... I disagree that it should be fixed in a follow-up patch. It should
>> be fixed now as this is where the badness is spread to any architecture.
> 
> No.  That is an inappropriate request to make.
> 
> Lin's patch does not alter the broken-ness of unaligned on arm32, and
> does improve the aspect of the hypervisor that it pertains to.  It
> therefore stands on its own merit.
I am not sure sure why switching from *cpup* improves things... and as 
usual you haven't answered to the clarification questions.

> 
> Your choices are to either fix it yourself (after all, you are the
> maintainer who cares about this unrelated bug), or you ask Lin kindly if
> he has time to look into fixing the unrelated bug after this series is
> complete.

Or 3) keep *cpup* so there is only one place to fix it.

> 
> It is not reasonable to say "this unrelated thing is broken, and you
> need to fix it first to get your series in".  Requests like that are,
> I'm sure, part of what Bertrand raised in the community call as
> unnecessary fiction getting work submitted.

To be honest, you put the contributor in this situation. I would have 
been perfectly happy if we keep *cpup* around as there would be only a 
place to fix.

With this approach, you are effectively going to increase the work later 
one because now we would have to chase all the open-coded version of 
*cpup* and check which one is not safe.

Cheers,

-- 
Julien Grall

On 10/05/2022 12:47, Julien Grall wrote:
> Hi,
>
> On 10/05/2022 12:34, Andrew Cooper wrote:
>> On 10/05/2022 12:17, Julien Grall wrote:
>>>>
>>>>>
>>>>>> diff --git a/xen/include/xen/unaligned.h
>>>>>> b/xen/include/xen/unaligned.h
>>>>>> index 0a2b16d05d..16b2e6f5f0 100644
>>>>>> --- a/xen/include/xen/unaligned.h
>>>>>> +++ b/xen/include/xen/unaligned.h
>>>>>> @@ -20,62 +20,62 @@
>>>>>>       static inline uint16_t get_unaligned_be16(const void *p)
>>>>>>     {
>>>>>> -    return be16_to_cpup(p);
>>>>>> +    return be16_to_cpu(*(const uint16_t *)p)
>>>>>
>>>>> I haven't checked the existing implementation of be16_to_cpup().
>>>>
>>>> It's a plain dereference, just like this.  AFAICT, it wasn't unaligned
>>>> safe before, either.
>>>
>>> Well, technically an architecture could provide an override for the
>>> copy. I agree that arm32 is already bogus but...
>>>
>>>>
>>>> It should be reasonably easy to fix in a followup patch.  Just
>>>> memcpy()
>>>> to/from the void pointer to a stack variable of the appropriate type.
>>> ... I disagree that it should be fixed in a follow-up patch. It should
>>> be fixed now as this is where the badness is spread to any
>>> architecture.
>>
>> No.  That is an inappropriate request to make.
>>
>> Lin's patch does not alter the broken-ness of unaligned on arm32, and
>> does improve the aspect of the hypervisor that it pertains to.  It
>> therefore stands on its own merit.
> I am not sure sure why switching from *cpup* improves things... and as
> usual you haven't answered to the clarification questions.

Adjust your tone back to something appropriate to the conversation.

The p helpers hide a unsafe typecast&dereference which will erroneously
compile both of these:

void foo(void *ptr)
{
    blah_p(ptr);
}

void bar(baz *ptr)
{
    blah_p(ptr);
}

and potentially malfunction as a consequence, not to mention that it's
sufficient obfuscation to trick a maintainer into believe an unrelated
area of code was safe when it wasn't.

Deleting the p helpers is a specific objective of the work, because it
forces the author to resolve to an integral type, and have the deference
chain visible in a single location which improves code clarity.

On a hunch, I checked the MISRA spec, and it turns out there is a rule
against the p helpers (specifically the type safety aspect), so this
series is removing a whole load of DIR 4.9 violations from the codebase.

~Andrew

On 11/05/2022 10:56, Andrew Cooper wrote:
> On 10/05/2022 12:47, Julien Grall wrote:
>> Hi,
>>
>> On 10/05/2022 12:34, Andrew Cooper wrote:
>>> On 10/05/2022 12:17, Julien Grall wrote:
>>>>>
>>>>>>
>>>>>>> diff --git a/xen/include/xen/unaligned.h
>>>>>>> b/xen/include/xen/unaligned.h
>>>>>>> index 0a2b16d05d..16b2e6f5f0 100644
>>>>>>> --- a/xen/include/xen/unaligned.h
>>>>>>> +++ b/xen/include/xen/unaligned.h
>>>>>>> @@ -20,62 +20,62 @@
>>>>>>>        static inline uint16_t get_unaligned_be16(const void *p)
>>>>>>>      {
>>>>>>> -    return be16_to_cpup(p);
>>>>>>> +    return be16_to_cpu(*(const uint16_t *)p)
>>>>>>
>>>>>> I haven't checked the existing implementation of be16_to_cpup().
>>>>>
>>>>> It's a plain dereference, just like this.  AFAICT, it wasn't unaligned
>>>>> safe before, either.
>>>>
>>>> Well, technically an architecture could provide an override for the
>>>> copy. I agree that arm32 is already bogus but...
>>>>
>>>>>
>>>>> It should be reasonably easy to fix in a followup patch.  Just
>>>>> memcpy()
>>>>> to/from the void pointer to a stack variable of the appropriate type.
>>>> ... I disagree that it should be fixed in a follow-up patch. It should
>>>> be fixed now as this is where the badness is spread to any
>>>> architecture.
>>>
>>> No.  That is an inappropriate request to make.
>>>
>>> Lin's patch does not alter the broken-ness of unaligned on arm32, and
>>> does improve the aspect of the hypervisor that it pertains to.  It
>>> therefore stands on its own merit.
>> I am not sure sure why switching from *cpup* improves things... and as
>> usual you haven't answered to the clarification questions.
> 
> Adjust your tone back to something appropriate to the conversation.

It was indeed harsh. Sorry for that.

> 
> The p helpers hide a unsafe typecast&dereference which will erroneously
> compile both of these:
> 
> void foo(void *ptr)
> {
>      blah_p(ptr);
> }
> 
> void bar(baz *ptr)
> {
>      blah_p(ptr);
> }

I am assuming that blah would expect a (blah *).

> 
> and potentially malfunction as a consequence, not to mention that it's
> sufficient obfuscation to trick a maintainer into believe an unrelated
> area of code was safe when it wasn't.

I looked at the helpers, they are static inline and use a proper type. 
Therefore, I am not sure why bar would compile in this situation.

In fact, to me it seems this is an inherent issue to C: any void pointer 
can be casted to anything. You are removing one here, but there are 
hundreds of other potential "unsafe" place in Xen.

> 
> Deleting the p helpers is a specific objective of the work, because it
> forces the author to resolve to an integral type, and have the deference
> chain visible in a single location which improves code clarity.

See above, I think dropping p helpers is not solving the underlying 
problem (we are not going to be able to remove pointers in Xen).

What would solve the problem is forbidding cast from void pointer to any 
pointer. At which point, keeping *cpup* is not going to be problem.

> 
> On a hunch, I checked the MISRA spec, and it turns out there is a rule
> against the p helpers (specifically the type safety aspect), so this
> series is removing a whole load of DIR 4.9 violations from the codebase.

I read through DIR 4.9., AFAIU it is about using function rather than 
macro. The current implementation of *cpup* are using function so I 
don't understand how removing *cpup* would help.

So I am afraid, I still see no justifications to drop *cpup* here and I 
actually prefer them the open-code version. I will not Nack it, but I 
will not support.

In any case, the commit message should contain some information why they 
are dropped.

Cheers,

-- 
Julien Grall

On Tue, 10 May 2022, Julien Grall wrote:
> > It is not reasonable to say "this unrelated thing is broken, and you
> > need to fix it first to get your series in".  Requests like that are,
> > I'm sure, part of what Bertrand raised in the community call as
> > unnecessary fiction getting work submitted.
> 
> To be honest, you put the contributor in this situation. I would have been
> perfectly happy if we keep *cpup* around as there would be only a place to
> fix.
> 
> With this approach, you are effectively going to increase the work later one
> because now we would have to chase all the open-coded version of *cpup* and
> check which one is not safe.


Without disagreeing with Julien or Andrew, I am actually happy to see an
effort to make the review process faster. We have lot of room for
improvement and spotting opportunities to do so is the first step toward
improving the process. I have actually been thinking about how to make
things faster in cases like this and I have a suggesion below.

In this case all of the options are OK. Whether we fix the alignment
problem as part of this patch or soon after it doesn't make much of a
difference. It is more important that we don't get bogged down in a long
discussion. Coding is faster and more fun.

It would take less time for Julien (or Andrew) to write the code than to
explain to the contributor how to do it. English is a good language for
an architectural discussion, but simply replying with the example code
in C would be much faster in cases like this one.

So my suggestion is that it would be best if the reviewer (Julien in
this case) replied directly with the code snipper he wants added. Just
an example without looking too closely:

---
Please do this instead so that alignment also gets fixed:

return be16_to_cpu(*(const __packed uint16_t *)p);
---


Alternatively, I also think that taking this patch as is without
alignment fix (either using be16_to_cpu or be16_to_cpup) is fine. The
alignment could be fixed afterwards. The key is that I think it should
be one of the maintainers to write the follow-up fix. Both of you are
very fast coders and would certainly finish the patch before finishing
the explanation on what needs to be done, which then would need to be
understood and implemented by the contributor (opportunity for
misunderstandings), and verified again by the reviewer on v2. That would
take an order of magnitude more of collective time and effort.

Of course this doesn't apply to all cases, but it should apply to quite
a few.

In short, less English, more C  ;-)

Hi,

> On 11 May 2022, at 04:12, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 10 May 2022, Julien Grall wrote:
>>> It is not reasonable to say "this unrelated thing is broken, and you
>>> need to fix it first to get your series in".  Requests like that are,
>>> I'm sure, part of what Bertrand raised in the community call as
>>> unnecessary fiction getting work submitted.
>> 
>> To be honest, you put the contributor in this situation. I would have been
>> perfectly happy if we keep *cpup* around as there would be only a place to
>> fix.
>> 
>> With this approach, you are effectively going to increase the work later one
>> because now we would have to chase all the open-coded version of *cpup* and
>> check which one is not safe.
> 
> 
> Without disagreeing with Julien or Andrew, I am actually happy to see an
> effort to make the review process faster. We have lot of room for
> improvement and spotting opportunities to do so is the first step toward
> improving the process. I have actually been thinking about how to make
> things faster in cases like this and I have a suggesion below.

Definitely with you here, it is good to see that my message on review process
and effort is actually in people’s minds :-)

> 
> In this case all of the options are OK. Whether we fix the alignment
> problem as part of this patch or soon after it doesn't make much of a
> difference. It is more important that we don't get bogged down in a long
> discussion. Coding is faster and more fun.

I would just make a small exception here (which corresponds to something
Julien kind of suggested during the discussion): unless we introduce a
temporary bug between the patches.
But this could actually be solved here by making a patch upfront and merging
it before the one in discussion (which might require some rebasing).

> 
> It would take less time for Julien (or Andrew) to write the code than to
> explain to the contributor how to do it. English is a good language for
> an architectural discussion, but simply replying with the example code
> in C would be much faster in cases like this one.
> 
> So my suggestion is that it would be best if the reviewer (Julien in
> this case) replied directly with the code snipper he wants added. Just
> an example without looking too closely:
> 
> ---
> Please do this instead so that alignment also gets fixed:
> 
> return be16_to_cpu(*(const __packed uint16_t *)p);
> ---
> 
> 
> Alternatively, I also think that taking this patch as is without
> alignment fix (either using be16_to_cpu or be16_to_cpup) is fine. The
> alignment could be fixed afterwards. The key is that I think it should
> be one of the maintainers to write the follow-up fix. Both of you are
> very fast coders and would certainly finish the patch before finishing
> the explanation on what needs to be done, which then would need to be
> understood and implemented by the contributor (opportunity for
> misunderstandings), and verified again by the reviewer on v2. That would
> take an order of magnitude more of collective time and effort.

Agree with the exception I mentioned.

> 
> Of course this doesn't apply to all cases, but it should apply to quite
> a few.
> 
> In short, less English, more C  ;-)

Same goes for things like “please add a comment” or “please say
something in the commit message”, it would be most of the time easier
for everyone to do: Could you add the comment “xxx” on top of this or
the sentence “yyy” in your commit (or even better ask the contributor if
he is ok with it and do it on commit when it not modifying the code).

Cheers
Bertrand

Hi,

On 11/05/2022 04:12, Stefano Stabellini wrote:
> Alternatively, I also think that taking this patch as is without
> alignment fix (either using be16_to_cpu or be16_to_cpup) is fine. The
> alignment could be fixed afterwards. The key is that I think it should
> be one of the maintainers to write the follow-up fix. Both of you are
> very fast coders and would certainly finish the patch before finishing
> the explanation on what needs to be done, which then would need to be
> understood and implemented by the contributor (opportunity for
> misunderstandings), and verified again by the reviewer on v2. That would
> take an order of magnitude more of collective time and effort.

I am happy to write a follow-up patch so long be16_to_cpup() (& co) are 
kept (because this is where I want to fix this issue so it is done for 
everyone).

Cheers,

-- 
Julien Grall