diff mbox series

[v5,4/6] xen: Switch to byteswap

Message ID dcabb541d0b5ab7858ccf1c925afc334f3123ad5.1653314499.git.lin.liu@citrix.com (mailing list archive)
State New, archived
Headers show
Series Implement byteswap and update references | expand

Commit Message

Lin Liu （刘林） May 23, 2022, 2:50 p.m. UTC 
Update to use byteswap to swap bytes
be*_to_cpup(p) is short for be*to_cpu(*p), update to use latter
one explictly

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: Bertrand Marquis <bertrand.marquis@arm.com>
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 v5:
- Add git message to explain be*to_cpu helper

Changes in v4:
- Revert the __force in type casting

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        | 12 ++++----
 4 files changed, 32 insertions(+), 32 deletions(-)

Comments

Julien Grall May 23, 2022, 2:56 p.m. UTC | #1 
Hi,

On 23/05/2022 15:50, Lin Liu wrote:
> Update to use byteswap to swap bytes
> be*_to_cpup(p) is short for be*to_cpu(*p), update to use latter
> one explictly

But why? I really don't have a suggestion on the comment because I 
disagree (and AFAICT Jan as well) with the approach.

In any case, I think it would be helpful if you participate in the 
discussion rather than sending a new version quickly. This would make 
sure you don't spend time on resending with unfinished discussion.

Cheers,
Andrew Cooper May 23, 2022, 3:38 p.m. UTC | #2 
On 23/05/2022 15:56, Julien Grall wrote:
> Hi,
>
> On 23/05/2022 15:50, Lin Liu wrote:
>> Update to use byteswap to swap bytes
>> be*_to_cpup(p) is short for be*to_cpu(*p), update to use latter
>> one explictly
>
> But why?

Because deleting code obfuscation constructs *is* the point of the cleanup.

> I really don't have a suggestion on the comment because I disagree
> (and AFAICT Jan as well) with the approach.

Dropping the obfuscation has uncovered pre-existing bugs in the
hypervisor.  The series stands on its own merit.

While I can't help if you like it or not, it really does bring an
improvement to code quality and legibility.

If you have no technical objections, and no suggestions for how to do it
differently while retaining the quality and legibility improvements,
then "I don't like it" doesn't block it going in.

I specifically do like this change, because it does improve the codebase.

~Andrew
Julien Grall May 23, 2022, 4:05 p.m. UTC | #3 
Hi Andrew,

On 23/05/2022 16:38, Andrew Cooper wrote:
> On 23/05/2022 15:56, Julien Grall wrote:
>> Hi,
>>
>> On 23/05/2022 15:50, Lin Liu wrote:
>>> Update to use byteswap to swap bytes
>>> be*_to_cpup(p) is short for be*to_cpu(*p), update to use latter
>>> one explictly
>>
>> But why?
> 
> Because deleting code obfuscation constructs *is* the point of the cleanup.
> 
>> I really don't have a suggestion on the comment because I disagree
>> (and AFAICT Jan as well) with the approach.
> 
> Dropping the obfuscation has uncovered pre-existing bugs in the
> hypervisor.  The series stands on its own merit.

I am guessing you mean that we don't handle unaligned access? If so, yes 
I agree this helped with that.

> 
> While I can't help if you like it or not, it really does bring an
> improvement to code quality and legibility.
> 
> If you have no technical objections, and no suggestions for how to do it
> differently while retaining the quality and legibility improvements,
> then "I don't like it" doesn't block it going in.

And you don't like the existing code :). I am willing to compromise, but 
for that I need to understand why the existing code is technically not 
correct.

So far, all the arguments you provided in v3 was either a matter of 
taste or IMHO bogus.

Your taste is nor better nor worse than mine. At which, we need someone 
else to break the tie.

If I am not mistaken, Jan is also objecting on the proposal. At which 
point, we are 2 vs 1.

So there are three choices here:
   1) You find two others maintainers (including on Arm maintainer) to 
agree with you
   2) You provide arguments that will sway one of us in your side
   3) We keep be32_cpu*() (they are simple wrapper and I am willing to 
write the code).

Cheers,
Jan Beulich May 23, 2022, 4:14 p.m. UTC | #4 
On 23.05.2022 17:38, Andrew Cooper wrote:
> On 23/05/2022 15:56, Julien Grall wrote:
>> On 23/05/2022 15:50, Lin Liu wrote:
>>> Update to use byteswap to swap bytes
>>> be*_to_cpup(p) is short for be*to_cpu(*p), update to use latter
>>> one explictly
>>
>> But why?
> 
> Because deleting code obfuscation constructs *is* the point of the cleanup.

It's obfuscation only as long as not implemented correctly, i.e. dealing
with unaligned data. Then "be*_to_cpup(p) is short for be*to_cpu(*p)" no
longer applies.

Jan
Lin Liu （刘林） May 24, 2022, 2:42 a.m. UTC | #5 
>Hi Andrew,
>
>On 23/05/2022 16:38, Andrew Cooper wrote:
>> On 23/05/2022 15:56, Julien Grall wrote:
>>> Hi,
>>>
>>> On 23/05/2022 15:50, Lin Liu wrote:
>>>> Update to use byteswap to swap bytes
>>>> be*_to_cpup(p) is short for be*to_cpu(*p), update to use latter
>>>> one explictly
>>>
>>> But why?
>>
>> Because deleting code obfuscation constructs *is* the point of the cleanup.
>>
>>> I really don't have a suggestion on the comment because I disagree
>>> (and AFAICT Jan as well) with the approach.
>>
>> Dropping the obfuscation has uncovered pre-existing bugs in the
>> hypervisor.  The series stands on its own merit.
>
>I am guessing you mean that we don't handle unaligned access? If so, yes
>I agree this helped with that.
>
>>
>> While I can't help if you like it or not, it really does bring an
>> improvement to code quality and legibility.
>>
>> If you have no technical objections, and no suggestions for how to do it
>> differently while retaining the quality and legibility improvements,
>> then "I don't like it" doesn't block it going in.
>
>And you don't like the existing code :). I am willing to compromise, but
>for that I need to understand why the existing code is technically not
>correct.
>
>So far, all the arguments you provided in v3 was either a matter of
>taste or IMHO bogus.
>
>Your taste is nor better nor worse than mine. At which, we need someone
>else to break the tie.
>
>If I am not mistaken, Jan is also objecting on the proposal. At which
>point, we are 2 vs 1.
>
>So there are three choices here:
>   1) You find two others maintainers (including on Arm maintainer) to
>agree with you
>   2) You provide arguments that will sway one of us in your side
>   3) We keep be32_cpu*() (they are simple wrapper and I am willing to
>write the code).

Personaly, I agree with Andrew Copper to remove the be*_to_cpup helpers as current
implemetation is just a wrapper, like

#ifndef __arch__swab16p
#  define __arch__swab16p(x) __arch__swab16(*(x))
#endif

With be*_to_cpup been removed, the interface keeps simple and clear, and callers
are dereference the pointer explictly.

I am very happy to see the three choices, hope we can reach an agreement about this soon.

Cherrs,
---
Lin
diff mbox series

Patch

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..56807bd157 100644
--- a/xen/include/xen/unaligned.h
+++ b/xen/include/xen/unaligned.h
@@ -20,7 +20,7 @@ 
 
 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)
@@ -30,7 +30,7 @@  static inline void put_unaligned_be16(uint16_t val, void *p)
 
 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)
@@ -40,7 +40,7 @@  static inline void put_unaligned_be32(uint32_t val, void *p)
 
 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)
@@ -50,7 +50,7 @@  static inline void put_unaligned_be64(uint64_t val, void *p)
 
 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)
@@ -60,7 +60,7 @@  static inline void put_unaligned_le16(uint16_t val, void *p)
 
 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)
@@ -70,7 +70,7 @@  static inline void put_unaligned_le32(uint32_t val, void *p)
 
 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)