[Qemu-devel] [patch] add byteordered types

[Qemu-devel] [patch] add byteordered types

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 290 bytes --]

  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

[-- Attachment #2: 0021-add-byteordered-types-and-accessor-functions-to-qemu.patch --]
[-- Type: text/plain, Size: 17055 bytes --]

>From 762d2de349de3820c9d1bdd8218d44ff997b2879 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed, 27 Aug 2008 13:38:39 +0200
Subject: [PATCH] add byteordered types and accessor functions to qemu.


Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 bswap.h      |   29 +++++++++++
 hw/usb-net.c |  158 +++++++++++++++++++++++++++++----------------------------
 2 files changed, 109 insertions(+), 78 deletions(-)

diff --git a/bswap.h b/bswap.h
index 523d805..e174e4e 100644
--- a/bswap.h
+++ b/bswap.h
@@ -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 */
diff --git a/hw/usb-net.c b/hw/usb-net.c
index 27dea10..dbeb54a 100644
--- a/hw/usb-net.c
+++ b/hw/usb-net.c
@@ -326,8 +326,6 @@ enum {
 #define OID_PNP_REMOVE_WAKE_UP_PATTERN	0xfd010104
 #define OID_PNP_ENABLE_WAKE_UP		0xfd010106
 
-typedef uint32_t le32;
-
 typedef struct rndis_init_msg_type {
     le32 MessageType;
     le32 MessageLength;
@@ -629,6 +627,7 @@ static int ndis_query(USBNetState *s, uint32_t oid,
                       size_t outlen)
 {
     unsigned int i, count;
+    le32 *outbuf32 = (void*)outbuf;
 
     switch (oid) {
     /* general oids (table 4-1) */
@@ -636,47 +635,47 @@ static int ndis_query(USBNetState *s, uint32_t oid,
     case OID_GEN_SUPPORTED_LIST:
         count = sizeof(oid_supported_list) / sizeof(uint32_t);
         for (i = 0; i < count; i++)
-            ((le32 *) outbuf)[i] = cpu_to_le32(oid_supported_list[i]);
+            outbuf32[i] = write_le32(oid_supported_list[i]);
         return sizeof(oid_supported_list);
 
     /* mandatory */
     case OID_GEN_HARDWARE_STATUS:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_MEDIA_SUPPORTED:
-        *((le32 *) outbuf) = cpu_to_le32(s->medium);
+        outbuf32[0] = write_le32(s->medium);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_MEDIA_IN_USE:
-        *((le32 *) outbuf) = cpu_to_le32(s->medium);
+        outbuf32[0] = write_le32(s->medium);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_MAXIMUM_FRAME_SIZE:
-        *((le32 *) outbuf) = cpu_to_le32(ETH_FRAME_LEN);
+        outbuf32[0] = write_le32(ETH_FRAME_LEN);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_LINK_SPEED:
-        *((le32 *) outbuf) = cpu_to_le32(s->speed);
+        outbuf32[0] = write_le32(s->speed);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_TRANSMIT_BLOCK_SIZE:
-        *((le32 *) outbuf) = cpu_to_le32(ETH_FRAME_LEN);
+        outbuf32[0] = write_le32(ETH_FRAME_LEN);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_RECEIVE_BLOCK_SIZE:
-        *((le32 *) outbuf) = cpu_to_le32(ETH_FRAME_LEN);
+        outbuf32[0] = write_le32(ETH_FRAME_LEN);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_VENDOR_ID:
-        *((le32 *) outbuf) = cpu_to_le32(s->vendorid);
+        outbuf32[0] = write_le32(s->vendorid);
         return sizeof(le32);
 
     /* mandatory */
@@ -685,30 +684,30 @@ static int ndis_query(USBNetState *s, uint32_t oid,
         return strlen(outbuf) + 1;
 
     case OID_GEN_VENDOR_DRIVER_VERSION:
-        *((le32 *) outbuf) = cpu_to_le32(1);
+        outbuf32[0] = write_le32(1);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_CURRENT_PACKET_FILTER:
-        *((le32 *) outbuf) = cpu_to_le32(s->filter);
+        outbuf32[0] = write_le32(s->filter);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_MAXIMUM_TOTAL_SIZE:
-        *((le32 *) outbuf) = cpu_to_le32(RNDIS_MAX_TOTAL_SIZE);
+        outbuf32[0] = write_le32(RNDIS_MAX_TOTAL_SIZE);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_MEDIA_CONNECT_STATUS:
-        *((le32 *) outbuf) = cpu_to_le32(s->media_state);
+        outbuf32[0] = write_le32(s->media_state);
         return sizeof(le32);
 
     case OID_GEN_PHYSICAL_MEDIUM:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     case OID_GEN_MAC_OPTIONS:
-        *((le32 *) outbuf) = cpu_to_le32(
+        outbuf32[0] = write_le32(
                         NDIS_MAC_OPTION_RECEIVE_SERIALIZED |
                         NDIS_MAC_OPTION_FULL_DUPLEX);
         return sizeof(le32);
@@ -716,27 +715,27 @@ static int ndis_query(USBNetState *s, uint32_t oid,
     /* statistics OIDs (table 4-2) */
     /* mandatory */
     case OID_GEN_XMIT_OK:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_RCV_OK:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_XMIT_ERROR:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_RCV_ERROR:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_RCV_NO_BUFFER:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* ieee802.3 OIDs (table 4-3) */
@@ -752,12 +751,12 @@ static int ndis_query(USBNetState *s, uint32_t oid,
 
     /* mandatory */
     case OID_802_3_MULTICAST_LIST:
-        *((le32 *) outbuf) = cpu_to_le32(0xe0000000);
+        outbuf32[0] = write_le32(0xe0000000);
         return sizeof(le32);
 
     /* mandatory */
     case OID_802_3_MAXIMUM_LIST_SIZE:
-        *((le32 *) outbuf) = cpu_to_le32(1);
+        outbuf32[0] = write_le32(1);
         return sizeof(le32);
 
     case OID_802_3_MAC_OPTIONS:
@@ -766,17 +765,17 @@ static int ndis_query(USBNetState *s, uint32_t oid,
     /* ieee802.3 statistics OIDs (table 4-4) */
     /* mandatory */
     case OID_802_3_RCV_ERROR_ALIGNMENT:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_802_3_XMIT_ONE_COLLISION:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_802_3_XMIT_MORE_COLLISIONS:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     default:
@@ -789,9 +788,11 @@ static int ndis_query(USBNetState *s, uint32_t oid,
 static int ndis_set(USBNetState *s, uint32_t oid,
                 uint8_t *inbuf, unsigned int inlen)
 {
+    le32 *inbuf32 = (void*)inbuf;
+
     switch (oid) {
     case OID_GEN_CURRENT_PACKET_FILTER:
-        s->filter = le32_to_cpup((le32 *) inbuf);
+        s->filter = read_le32(inbuf32[0]);
         if (s->filter) {
             s->rndis_state = RNDIS_DATA_INITIALIZED;
         } else {
@@ -850,20 +851,20 @@ static int rndis_init_response(USBNetState *s, rndis_init_msg_type *buf)
     if (!resp)
         return USB_RET_STALL;
 
-    resp->MessageType = cpu_to_le32(RNDIS_INITIALIZE_CMPLT);
-    resp->MessageLength = cpu_to_le32(sizeof(rndis_init_cmplt_type));
+    resp->MessageType = write_le32(RNDIS_INITIALIZE_CMPLT);
+    resp->MessageLength = write_le32(sizeof(rndis_init_cmplt_type));
     resp->RequestID = buf->RequestID; /* Still LE in msg buffer */
-    resp->Status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
-    resp->MajorVersion = cpu_to_le32(RNDIS_MAJOR_VERSION);
-    resp->MinorVersion = cpu_to_le32(RNDIS_MINOR_VERSION);
-    resp->DeviceFlags = cpu_to_le32(RNDIS_DF_CONNECTIONLESS);
-    resp->Medium = cpu_to_le32(RNDIS_MEDIUM_802_3);
-    resp->MaxPacketsPerTransfer = cpu_to_le32(1);
-    resp->MaxTransferSize = cpu_to_le32(ETH_FRAME_LEN +
+    resp->Status = write_le32(RNDIS_STATUS_SUCCESS);
+    resp->MajorVersion = write_le32(RNDIS_MAJOR_VERSION);
+    resp->MinorVersion = write_le32(RNDIS_MINOR_VERSION);
+    resp->DeviceFlags = write_le32(RNDIS_DF_CONNECTIONLESS);
+    resp->Medium = write_le32(RNDIS_MEDIUM_802_3);
+    resp->MaxPacketsPerTransfer = write_le32(1);
+    resp->MaxTransferSize = write_le32(ETH_FRAME_LEN +
                     sizeof(struct rndis_packet_msg_type) + 22);
-    resp->PacketAlignmentFactor = cpu_to_le32(0);
-    resp->AFListOffset = cpu_to_le32(0);
-    resp->AFListSize = cpu_to_le32(0);
+    resp->PacketAlignmentFactor = write_le32(0);
+    resp->AFListOffset = write_le32(0);
+    resp->AFListSize = write_le32(0);
     return 0;
 }
 
@@ -877,12 +878,12 @@ static int rndis_query_response(USBNetState *s,
     int infobuflen;
     unsigned int resplen;
 
-    bufoffs = le32_to_cpu(buf->InformationBufferOffset) + 8;
-    buflen = le32_to_cpu(buf->InformationBufferLength);
+    bufoffs = read_le32(buf->InformationBufferOffset) + 8;
+    buflen = read_le32(buf->InformationBufferLength);
     if (bufoffs + buflen > length)
         return USB_RET_STALL;
 
-    infobuflen = ndis_query(s, le32_to_cpu(buf->OID),
+    infobuflen = ndis_query(s, read_le32(buf->OID),
                             bufoffs + (uint8_t *) buf, buflen, infobuf,
                             sizeof(infobuf));
     resplen = sizeof(rndis_query_cmplt_type) +
@@ -891,22 +892,22 @@ static int rndis_query_response(USBNetState *s,
     if (!resp)
         return USB_RET_STALL;
 
-    resp->MessageType = cpu_to_le32(RNDIS_QUERY_CMPLT);
+    resp->MessageType = write_le32(RNDIS_QUERY_CMPLT);
     resp->RequestID = buf->RequestID; /* Still LE in msg buffer */
-    resp->MessageLength = cpu_to_le32(resplen);
+    resp->MessageLength = write_le32(resplen);
 
     if (infobuflen < 0) {
         /* OID not supported */
-        resp->Status = cpu_to_le32(RNDIS_STATUS_NOT_SUPPORTED);
-        resp->InformationBufferLength = cpu_to_le32(0);
-        resp->InformationBufferOffset = cpu_to_le32(0);
+        resp->Status = write_le32(RNDIS_STATUS_NOT_SUPPORTED);
+        resp->InformationBufferLength = write_le32(0);
+        resp->InformationBufferOffset = write_le32(0);
         return 0;
     }
 
-    resp->Status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
+    resp->Status = write_le32(RNDIS_STATUS_SUCCESS);
     resp->InformationBufferOffset =
-            cpu_to_le32(infobuflen ? sizeof(rndis_query_cmplt_type) - 8 : 0);
-    resp->InformationBufferLength = cpu_to_le32(infobuflen);
+            write_le32(infobuflen ? sizeof(rndis_query_cmplt_type) - 8 : 0);
+    resp->InformationBufferLength = write_le32(infobuflen);
     memcpy(resp + 1, infobuf, infobuflen);
 
     return 0;
@@ -923,22 +924,22 @@ static int rndis_set_response(USBNetState *s,
     if (!resp)
         return USB_RET_STALL;
 
-    bufoffs = le32_to_cpu(buf->InformationBufferOffset) + 8;
-    buflen = le32_to_cpu(buf->InformationBufferLength);
+    bufoffs = read_le32(buf->InformationBufferOffset) + 8;
+    buflen = read_le32(buf->InformationBufferLength);
     if (bufoffs + buflen > length)
         return USB_RET_STALL;
 
-    ret = ndis_set(s, le32_to_cpu(buf->OID),
+    ret = ndis_set(s, read_le32(buf->OID),
                     bufoffs + (uint8_t *) buf, buflen);
-    resp->MessageType = cpu_to_le32(RNDIS_SET_CMPLT);
+    resp->MessageType = write_le32(RNDIS_SET_CMPLT);
     resp->RequestID = buf->RequestID; /* Still LE in msg buffer */
-    resp->MessageLength = cpu_to_le32(sizeof(rndis_set_cmplt_type));
+    resp->MessageLength = write_le32(sizeof(rndis_set_cmplt_type));
     if (ret < 0) {
         /* OID not supported */
-        resp->Status = cpu_to_le32(RNDIS_STATUS_NOT_SUPPORTED);
+        resp->Status = write_le32(RNDIS_STATUS_NOT_SUPPORTED);
         return 0;
     }
-    resp->Status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
+    resp->Status = write_le32(RNDIS_STATUS_SUCCESS);
 
     return 0;
 }
@@ -951,10 +952,10 @@ static int rndis_reset_response(USBNetState *s, rndis_reset_msg_type *buf)
     if (!resp)
         return USB_RET_STALL;
 
-    resp->MessageType = cpu_to_le32(RNDIS_RESET_CMPLT);
-    resp->MessageLength = cpu_to_le32(sizeof(rndis_reset_cmplt_type));
-    resp->Status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
-    resp->AddressingReset = cpu_to_le32(1); /* reset information */
+    resp->MessageType = write_le32(RNDIS_RESET_CMPLT);
+    resp->MessageLength = write_le32(sizeof(rndis_reset_cmplt_type));
+    resp->Status = write_le32(RNDIS_STATUS_SUCCESS);
+    resp->AddressingReset = write_le32(1); /* reset information */
 
     return 0;
 }
@@ -968,10 +969,10 @@ static int rndis_keepalive_response(USBNetState *s,
     if (!resp)
         return USB_RET_STALL;
 
-    resp->MessageType = cpu_to_le32(RNDIS_KEEPALIVE_CMPLT);
-    resp->MessageLength = cpu_to_le32(sizeof(rndis_keepalive_cmplt_type));
+    resp->MessageType = write_le32(RNDIS_KEEPALIVE_CMPLT);
+    resp->MessageLength = write_le32(sizeof(rndis_keepalive_cmplt_type));
     resp->RequestID = buf->RequestID; /* Still LE in msg buffer */
-    resp->Status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
+    resp->Status = write_le32(RNDIS_STATUS_SUCCESS);
 
     return 0;
 }
@@ -979,10 +980,10 @@ static int rndis_keepalive_response(USBNetState *s,
 static int rndis_parse(USBNetState *s, uint8_t *data, int length)
 {
     uint32_t msg_type, msg_length;
-    le32 *tmp = (le32 *) data;
+    le32 *data32 = (le32 *) data;
 
-    msg_type = le32_to_cpup(tmp++);
-    msg_length = le32_to_cpup(tmp++);
+    msg_type = read_le32(data32[0]);
+    msg_length = read_le32(data32[1]);
 
     switch (msg_type) {
     case RNDIS_INITIALIZE_MSG:
@@ -1210,12 +1211,13 @@ static int usb_net_handle_control(USBDevice *dev, int request, int value,
 static int usb_net_handle_statusin(USBNetState *s, USBPacket *p)
 {
     int ret = 8;
+    le32 *data32 = (void*)p->data;
 
     if (p->len < 8)
         return USB_RET_STALL;
 
-    ((le32 *) p->data)[0] = cpu_to_le32(1);
-    ((le32 *) p->data)[1] = cpu_to_le32(0);
+    data32[0] = write_le32(1);
+    data32[1] = write_le32(0);
     if (!s->rndis_resp.tqh_first)
         ret = USB_RET_NAK;
 
@@ -1311,12 +1313,12 @@ static int usb_net_handle_dataout(USBNetState *s, USBPacket *p)
         }
         return ret;
     }
-    len = le32_to_cpu(msg->MessageLength);
+    len = read_le32(msg->MessageLength);
     if (s->out_ptr < 8 || s->out_ptr < len)
         return ret;
-    if (le32_to_cpu(msg->MessageType) == RNDIS_PACKET_MSG) {
-        uint32_t offs = 8 + le32_to_cpu(msg->DataOffset);
-        uint32_t size = le32_to_cpu(msg->DataLength);
+    if (read_le32(msg->MessageType) == RNDIS_PACKET_MSG) {
+        uint32_t offs = 8 + read_le32(msg->DataOffset);
+        uint32_t size = read_le32(msg->DataLength);
         if (offs + size <= len)
             qemu_send_packet(s->vc, s->out_buf + offs, size);
     }
@@ -1383,10 +1385,10 @@ static void usbnet_receive(void *opaque, const uint8_t *buf, int size)
             return;
 
         memset(msg, 0, sizeof(struct rndis_packet_msg_type));
-        msg->MessageType = cpu_to_le32(RNDIS_PACKET_MSG);
-        msg->MessageLength = cpu_to_le32(size + sizeof(struct rndis_packet_msg_type));
-        msg->DataOffset = cpu_to_le32(sizeof(struct rndis_packet_msg_type) - 8);
-        msg->DataLength = cpu_to_le32(size);
+        msg->MessageType = write_le32(RNDIS_PACKET_MSG);
+        msg->MessageLength = write_le32(size + sizeof(struct rndis_packet_msg_type));
+        msg->DataOffset = write_le32(sizeof(struct rndis_packet_msg_type) - 8);
+        msg->DataLength = write_le32(size);
         /* msg->OOBDataOffset;
          * msg->OOBDataLength;
          * msg->NumOOBDataElements;
-- 
1.5.5.1

Re: [Qemu-devel] [patch] add byteordered types

[Paul Brook]

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.

Paul

Re: [Qemu-devel] [patch] add byteordered types

[Gerd Hoffmann]

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?

cheers,
  Gerd

Re: [Qemu-devel] [patch] add byteordered types

[Gerd Hoffmann]

Gerd Hoffmann wrote:
> 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?

... 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

Re: [Qemu-devel] [patch] add byteordered types

[Paul Brook]

On Wednesday 27 August 2008, Gerd Hoffmann wrote:
> Gerd Hoffmann wrote:
> > 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?
>
> ... 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)?

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

Re: [Qemu-devel] [patch] add byteordered types

[Paul Brook]

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.

Paul

Re: [Qemu-devel] [patch] add byteordered types

[M. Warner Losh]

In message: <48B550F3.709@redhat.com>
            Gerd Hoffmann <kraxel@redhat.com> 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

Re: [Qemu-devel] [patch] add byteordered types

[Gerd Hoffmann]

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?

cheers,
  Gerd

Re: [Qemu-devel] [patch] add byteordered types

[M. Warner Losh]

In message: <48B56645.60206@redhat.com>
            Gerd Hoffmann <kraxel@redhat.com> 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

Re: [Qemu-devel] [patch] add byteordered types

[Gerd Hoffmann]

M. Warner Losh wrote:
> In message: <48B56645.60206@redhat.com>
>             Gerd Hoffmann <kraxel@redhat.com> 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?

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

Re: [Qemu-devel] [patch] add byteordered types

[Anthony Liguori]

Gerd Hoffmann wrote:
> M. Warner Losh wrote:
>   
>> In message: <48B56645.60206@redhat.com>
>>             Gerd Hoffmann <kraxel@redhat.com> 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?
>>     
>
> 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.
>   

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

> cheers,
>   Gerd
>
>
>   

Re: [Qemu-devel] [patch] add byteordered types

[Gerd Hoffmann]

Anthony Liguori wrote:
> 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?

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

Re: [Qemu-devel] [patch] add byteordered types

[Anthony Liguori]

Gerd Hoffmann wrote:
> Anthony Liguori wrote:
>   
>> 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?
>>     
>
> 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.
>   

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

> cheers,
>   Gerd
>
>
>   

Re: [Qemu-devel] [patch] add byteordered types

[Jamie Lokier]

Gerd Hoffmann wrote:
> Anthony Liguori wrote:
> > 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?
> 
> 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.

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

Re: [Qemu-devel] [patch] add byteordered types

[Gerd Hoffmann]

Anthony Liguori wrote:
> 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.

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