add byteordered types

Hi,

This patch adds byteordered types and accessors to qemu.

That gave a compile error in usb-net.c due to le32 being defined there wrongly this way ...

typedef uint32_t le32

... so I took the opportunity to kill that and switch usb-net.c over to the new accessors.

cheers,
Gerd

From: Gerd Hoffmann <[email protected]>
Date: Wed, 27 Aug 2008 13:38:39 +0200
Subject: [PATCH] add byteordered types and accessor functions to qemu.

---
 bswap.h      |   29 +++++++++++
 hw/usb-net.c |  158 +++++++++++++++++++++++++++++----------------------------
 2 files changed, 109 insertions(+), 78 deletions(-)

index 523d805..e174e4e 100644
@@ -206,4 +206,33 @@ static inline void cpu_to_be32wu(uint32_t *p, uint32_t v)
 #undef le_bswaps
 #undef be_bswaps
 
+/* byteordered types and accessors */
+
+typedef struct { uint16_t le; } le16;
+typedef struct { uint32_t le; } le32;
+typedef struct { uint64_t le; } le64;
+typedef struct { uint16_t be; } be16;
+typedef struct { uint32_t be; } be32;
+typedef struct { uint64_t be; } be64;
+
+static inline uint16_t read_le16(le16 le) { return le16_to_cpu(le.le); }
+static inline uint32_t read_le32(le32 le) { return le32_to_cpu(le.le); }
+static inline uint64_t read_le64(le64 le) { return le64_to_cpu(le.le); }
+static inline uint16_t read_be16(be16 be) { return be16_to_cpu(be.be); }
+static inline uint32_t read_be32(be32 be) { return be32_to_cpu(be.be); }
+static inline uint64_t read_be64(be64 be) { return be64_to_cpu(be.be); }
+
+static inline le16 write_le16(uint16_t cpu) \
+	{ le16 ret = { .le = cpu_to_le16(cpu) }; return ret; }
+static inline le32 write_le32(uint32_t cpu) \
+	{ le32 ret = { .le = cpu_to_le32(cpu) }; return ret; }
+static inline le64 write_le64(uint64_t cpu) \
+	{ le64 ret = { .le = cpu_to_le64(cpu) }; return ret; }
+static inline be16 write_be16(uint16_t cpu) \
+	{ be16 ret = { .be = cpu_to_be16(cpu) }; return ret; }
+static inline be32 write_be32(uint32_t cpu) \
+	{ be32 ret = { .be = cpu_to_be32(cpu) }; return ret; }
+static inline be64 write_be64(uint64_t cpu) \
+	{ be64 ret = { .be = cpu_to_be64(cpu) }; return ret; }
+
 #endif /* BSWAP_H */

1.5.5.1

This won't do what you expect on some targets. In particular older ARM targets align all structs to a word (4-byte) boundary.

Paul

Is this a default which can be changed by adding alignment atttributes? Or is it done unconditionally?

cheers,
Gerd

... like this:

-typedef struct { uint16_t le; } le16;
+typedef struct { uint16_t le; } le16 __attribute__((__aligned__(2)));

Is the size if the struct padded to 4 bytes too (i.e. do I need packed too)?

cheers,
Gerd

RTFM:

"When used on a struct, or struct member, the `aligned' attribute
can only increase the alignment; in order to decrease it, the `packed' attribute must be specified as well."

Paul

This is IMHO a bad name for this function. It doesn't write anything.

Paul

In message: <[email protected]>
Gerd Hoffmann <[email protected]> writes:
: Paul Brook wrote:
: > On Wednesday 27 August 2008, Gerd Hoffmann wrote:
: >> +typedef struct { uint16_t le; } le16;
: >
: > This won't do what you expect on some targets. In particular older ARM targets : > align all structs to a word (4-byte) boundary.
:
: Is this a default which can be changed by adding alignment atttributes? : Or is it done unconditionally?

packed and alignment attributes.

Warner

I'm not that happy the name too and certainly open for better suggestions I could use instead of read/write.

The alternatives I can think of are not very nice either:

(1) get/set -- same problem as read/write.
(2) make_foo() instead of write_foo() -- "make" is too generic IMHO.
(3) create_foo() instead of write_foo() -- no nice symmetric replacement for read_foo().

better ideas anyone?

cheers,
Gerd

In message: <[email protected]>
Gerd Hoffmann <[email protected]> writes:
: Paul Brook wrote:
: > On Wednesday 27 August 2008, Gerd Hoffmann wrote:
: >> +static inline le16 write_le16(uint16_t cpu) \
: >
: > This is IMHO a bad name for this function. It doesn't write anything. :
: I'm not that happy the name too and certainly open for better : suggestions I could use instead of read/write.
:
: The alternatives I can think of are not very nice either:
:

:  (1) get/set -- same problem as read/write.
:  (2) make_foo() instead of write_foo() -- "make" is too generic IMHO.
:  (3) create_foo() instead of write_foo() -- no nice symmetric

: replacement for read_foo().
:
: better ideas anyone?

Using the existing names that linux/FreeBSD/NetBSD/OpenBSD are using?

Warner

cpu_to_* is taken already by the not-typechecked macros. And converting the whole qemu tree in one go so we could reuse the names is a bit unrealistic IMHO.

cheers,
Gerd

Personally, I dislike the whole struct thing. I even further dislike having multiple sets of conversion functions that are used in different places in the code.

Are we sure that this is something that we want to do?

Regards,

Anthony Liguori

As far I know this struct trick is the only way to have gcc check access to variables with an specific byteorder is done the correct way. Avi sguuested that and I also think this checking would be useful. I could live without that though if there is an agreement that we'll just stick with the current, unchecked cpu_to_<order><size> functions.

cheers,
Gerd

Yeah, AFAIK, it's the only way to enforce this sort of thing but it's also ugly. If we were starting from scratch, I could see the value in it but there's already a ton of code that's not going to be using this mechanism that noone is going to convert. That makes me think there isn't going to be a lot of value in it and will lead to a lot of overall confusion.

Regards,

Anthony Liguori

Provided you always use the type, and want access only via the macros, the fact its a struct is pretty much invisible, and modern compilers optimise them quite well like integers.

I'm not sure if a "long enum le32 { max = LONG_MAX, min = LONG_MIN }" can be used instead of a struct (same for other "short enum" etc.), to get some warnings instead of errors, and allow bitwise operations.

Changing in steps would work:

1. Change all QEMU source to use the new type names (as integers), while continuing to use the cpu_to_XY and XY_to_cpu functions.

Doesn't have to be all at once.

It's probably not that much work to change all the mainline source, as you can use the type-safe version to catch all places that need changing :-)

Let it settle for a while, until contributors start using the new type names in patches.

2. _Then_ change those types to structs and make the functions type-safe. A small patch.

3. Fix the few that got missed.

That's less invasive than changing the function names everywhere.

-- Jamie

Ok, I'll drop the whole struct and accessors stuff and resend with simple typedefs instead. So you can use le* and be* types, but it basically is a documentation thing without gcc actually checking stuff.
And the existing cpu_to_* macros will do just fine ;)

cheers,
Gerd