Re: [PATCH] ibmveth: Fix more little endian issues

2013-12-25 Thread Ben Hutchings 

On Tue, 2013-12-24 at 12:55 +1100, Anton Blanchard wrote:
 The hypervisor expects MAC addresses passed in registers to be big
 endian u64. Create a helper function called ibmveth_encode_mac_addr
 which does the right thing in both big and little endian.
 
 We were storing the MAC address in a long in struct ibmveth_adapter.
 It's never used so remove it - we don't need another place in the
 driver where we create endian issues with MAC addresses.
[...]
 @@ -523,10 +523,20 @@ retry:
   return rc;
  }
  
 +/* The hypervisor expects MAC addresses passed in registers to be
 + * big endian u64.
 + */
 +static __be64 ibmveth_encode_mac_addr(char *mac)
 +{
 + unsigned long encoded = 0;

u64

 + memcpy(((char *)encoded) + 2, mac, ETH_ALEN);
 + return cpu_to_be64(encoded);
 +}
[...]

So on big-endian systems the byte order of the result will be:

0 0 mac0 mac1 mac2 mac3 mac4 mac5

and on little-endian systems it's:

mac5 mac4 mac3 mac2 mac1 mac0 0 0

It seems to me that 'encoded' is actually in big-endian order and this
function returns the address in CPU order.

So are you sure your explanation isn't backwards, because it looks to me
like the driver was already holding the MAC address in big-endian order
and perhaps the hypercall mechanism does a byte-swap when the guest is
little-endian.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ibmveth: Fix more little endian issues

2013-12-23 Thread Alexander Graf 

On 23.12.2013, at 07:38, Anton Blanchard an...@samba.org wrote:

 
 Hi Alex,
 
 The ibmveth driver is memcpy()'ing the mac address between a variable
 (register) and memory. This assumes a certain endianness of the
 system, so let's make that implicit assumption work again.
 
 Nice catch! I don't like how the driver has two different methods
 for creating these MAC addresses, both without comments. How does
 this look?

Heh - I didn't even realize those two places were doing the same thing.

Obviously your patch is by far nicer.

Reviewed-by: Alexander Graf ag...@suse.de


Alex

 
 Anton
 --
 
 The hypervisor expects MAC addresses passed in registers to be big
 endian u64. Create a helper function called ibmveth_encode_mac_addr
 which does the right thing in both big and little endian.
 
 We were storing the MAC address in a long in struct ibmveth_adapter.
 It's never used so remove it - we don't need another place in the
 driver where we create endian issues with MAC addresses.
 
 Reported-by: Alexander Graf ag...@suse.de
 Signed-off-by: Anton Blanchard an...@samba.org
 ---
 
 diff --git a/drivers/net/ethernet/ibm/ibmveth.c 
 b/drivers/net/ethernet/ibm/ibmveth.c
 index 952d795..044178b 100644
 --- a/drivers/net/ethernet/ibm/ibmveth.c
 +++ b/drivers/net/ethernet/ibm/ibmveth.c
 @@ -523,6 +523,17 @@ retry:
   return rc;
 }
 
 +/*
 + * The hypervisor expects MAC addresses passed in registers to be
 + * big endian u64.
 + */
 +static unsigned long ibmveth_encode_mac_addr(char *mac)
 +{
 + unsigned long encoded = 0;
 + memcpy(((char *)encoded) + 2, mac, ETH_ALEN);
 + return cpu_to_be64(encoded);
 +}
 +
 static int ibmveth_open(struct net_device *netdev)
 {
   struct ibmveth_adapter *adapter = netdev_priv(netdev);
 @@ -580,8 +591,7 @@ static int ibmveth_open(struct net_device *netdev)
   adapter-rx_queue.num_slots = rxq_entries;
   adapter-rx_queue.toggle = 1;
 
 - memcpy(mac_address, netdev-dev_addr, netdev-addr_len);
 - mac_address = mac_address  16;
 + mac_address = ibmveth_encode_mac_addr(netdev-dev_addr);
 
   rxq_desc.fields.flags_len = IBMVETH_BUF_VALID |
   adapter-rx_queue.queue_len;
 @@ -1184,8 +1194,8 @@ static void ibmveth_set_multicast_list(struct 
 net_device *netdev)
   /* add the addresses to the filter table */
   netdev_for_each_mc_addr(ha, netdev) {
   /* add the multicast address to the filter table */
 - unsigned long mcast_addr = 0;
 - memcpy(((char *)mcast_addr)+2, ha-addr, ETH_ALEN);
 + unsigned long mcast_addr;
 + mcast_addr = ibmveth_encode_mac_addr(ha-addr);
   lpar_rc = h_multicast_ctrl(adapter-vdev-unit_address,
  IbmVethMcastAddFilter,
  mcast_addr);
 @@ -1369,9 +1379,6 @@ static int ibmveth_probe(struct vio_dev *dev, const 
 struct vio_device_id *id)
 
   netif_napi_add(netdev, adapter-napi, ibmveth_poll, 16);
 
 - adapter-mac_addr = 0;
 - memcpy(adapter-mac_addr, mac_addr_p, ETH_ALEN);
 -
   netdev-irq = dev-irq;
   netdev-netdev_ops = ibmveth_netdev_ops;
   netdev-ethtool_ops = netdev_ethtool_ops;
 @@ -1380,7 +1387,7 @@ static int ibmveth_probe(struct vio_dev *dev, const 
 struct vio_device_id *id)
   NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
   netdev-features |= netdev-hw_features;
 
 - memcpy(netdev-dev_addr, adapter-mac_addr, netdev-addr_len);
 + memcpy(netdev-dev_addr, mac_addr_p, ETH_ALEN);
 
   for (i = 0; i  IBMVETH_NUM_BUFF_POOLS; i++) {
   struct kobject *kobj = adapter-rx_buff_pool[i].kobj;
 diff --git a/drivers/net/ethernet/ibm/ibmveth.h 
 b/drivers/net/ethernet/ibm/ibmveth.h
 index 84066ba..2c636cb 100644
 --- a/drivers/net/ethernet/ibm/ibmveth.h
 +++ b/drivers/net/ethernet/ibm/ibmveth.h
 @@ -139,7 +139,6 @@ struct ibmveth_adapter {
 struct napi_struct napi;
 struct net_device_stats stats;
 unsigned int mcastFilterSize;
 -unsigned long mac_addr;
 void * buffer_list_addr;
 void * filter_list_addr;
 dma_addr_t buffer_list_dma;

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ibmveth: Fix more little endian issues

2013-12-23 Thread Joe Perches 
On Mon, 2013-12-23 at 17:38 +1100, Anton Blanchard wrote:
 The hypervisor expects MAC addresses passed in registers to be big
 endian u64.

So maybe use __be64 declarations?

 +static unsigned long ibmveth_encode_mac_addr(char *mac)

static __be64 ibmveth_encode_mac_addr(const char *mac)

?

etc...


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH] ibmveth: Fix more little endian issues

2013-12-23 Thread Anton Blanchard 

The hypervisor expects MAC addresses passed in registers to be big
endian u64. Create a helper function called ibmveth_encode_mac_addr
which does the right thing in both big and little endian.

We were storing the MAC address in a long in struct ibmveth_adapter.
It's never used so remove it - we don't need another place in the
driver where we create endian issues with MAC addresses.

Signed-off-by: Anton Blanchard an...@samba.org
Reviewed-by: Alexander Graf ag...@suse.de
---

v2: annotate with __be64 as suggested by Joe

diff --git a/drivers/net/ethernet/ibm/ibmveth.c 
b/drivers/net/ethernet/ibm/ibmveth.c
index 952d795..bb9a631 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -497,7 +497,7 @@ static void ibmveth_cleanup(struct ibmveth_adapter *adapter)
 }
 
 static int ibmveth_register_logical_lan(struct ibmveth_adapter *adapter,
-union ibmveth_buf_desc rxq_desc, u64 mac_address)
+   union ibmveth_buf_desc rxq_desc, __be64 mac_address)
 {
int rc, try_again = 1;
 
@@ -523,10 +523,20 @@ retry:
return rc;
 }
 
+/* The hypervisor expects MAC addresses passed in registers to be
+ * big endian u64.
+ */
+static __be64 ibmveth_encode_mac_addr(char *mac)
+{
+   unsigned long encoded = 0;
+   memcpy(((char *)encoded) + 2, mac, ETH_ALEN);
+   return cpu_to_be64(encoded);
+}
+
 static int ibmveth_open(struct net_device *netdev)
 {
struct ibmveth_adapter *adapter = netdev_priv(netdev);
-   u64 mac_address = 0;
+   __be64 mac_address = 0;
int rxq_entries = 1;
unsigned long lpar_rc;
int rc;
@@ -580,8 +590,7 @@ static int ibmveth_open(struct net_device *netdev)
adapter-rx_queue.num_slots = rxq_entries;
adapter-rx_queue.toggle = 1;
 
-   memcpy(mac_address, netdev-dev_addr, netdev-addr_len);
-   mac_address = mac_address  16;
+   mac_address = ibmveth_encode_mac_addr(netdev-dev_addr);
 
rxq_desc.fields.flags_len = IBMVETH_BUF_VALID |
adapter-rx_queue.queue_len;
@@ -1184,8 +1193,8 @@ static void ibmveth_set_multicast_list(struct net_device 
*netdev)
/* add the addresses to the filter table */
netdev_for_each_mc_addr(ha, netdev) {
/* add the multicast address to the filter table */
-   unsigned long mcast_addr = 0;
-   memcpy(((char *)mcast_addr)+2, ha-addr, ETH_ALEN);
+   __be64 mcast_addr;
+   mcast_addr = ibmveth_encode_mac_addr(ha-addr);
lpar_rc = h_multicast_ctrl(adapter-vdev-unit_address,
   IbmVethMcastAddFilter,
   mcast_addr);
@@ -1369,9 +1378,6 @@ static int ibmveth_probe(struct vio_dev *dev, const 
struct vio_device_id *id)
 
netif_napi_add(netdev, adapter-napi, ibmveth_poll, 16);
 
-   adapter-mac_addr = 0;
-   memcpy(adapter-mac_addr, mac_addr_p, ETH_ALEN);
-
netdev-irq = dev-irq;
netdev-netdev_ops = ibmveth_netdev_ops;
netdev-ethtool_ops = netdev_ethtool_ops;
@@ -1380,7 +1386,7 @@ static int ibmveth_probe(struct vio_dev *dev, const 
struct vio_device_id *id)
NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
netdev-features |= netdev-hw_features;
 
-   memcpy(netdev-dev_addr, adapter-mac_addr, netdev-addr_len);
+   memcpy(netdev-dev_addr, mac_addr_p, ETH_ALEN);
 
for (i = 0; i  IBMVETH_NUM_BUFF_POOLS; i++) {
struct kobject *kobj = adapter-rx_buff_pool[i].kobj;
diff --git a/drivers/net/ethernet/ibm/ibmveth.h 
b/drivers/net/ethernet/ibm/ibmveth.h
index 84066ba..2c636cb 100644
--- a/drivers/net/ethernet/ibm/ibmveth.h
+++ b/drivers/net/ethernet/ibm/ibmveth.h
@@ -139,7 +139,6 @@ struct ibmveth_adapter {
 struct napi_struct napi;
 struct net_device_stats stats;
 unsigned int mcastFilterSize;
-unsigned long mac_addr;
 void * buffer_list_addr;
 void * filter_list_addr;
 dma_addr_t buffer_list_dma;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ibmveth: Fix more little endian issues

2013-12-23 Thread Benjamin Herrenschmidt 
On Mon, 2013-12-23 at 06:52 -0800, Joe Perches wrote:
 On Mon, 2013-12-23 at 17:38 +1100, Anton Blanchard wrote:
  The hypervisor expects MAC addresses passed in registers to be big
  endian u64.
 
 So maybe use __be64 declarations?
 
  +static unsigned long ibmveth_encode_mac_addr(char *mac)
 
 static __be64 ibmveth_encode_mac_addr(const char *mac)

A register value has no endianness. Only memory content does. Especially
talking of a MAC address which is really a byte stream (Yes, our
__beXX types used without a * are borderline, but we've got used to it).

In fact I find the use of 

memcpy(((char *)encoded) + 2, mac, ETH_ALEN);

Really gross :-) Yes it works with the added cpu_to_be64() but in that
specific case, I think it would be nicer to simply load  shift into
position the 6 bytes and avoid the endianness issue completely.

Cheers,
Ben.




___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH] ibmveth: Fix more little endian issues

2013-12-22 Thread Alexander Graf 
The ibmveth driver is memcpy()'ing the mac address between a variable
(register) and memory. This assumes a certain endianness of the
system, so let's make that implicit assumption work again.

This patch adds be64_to_cpu() calls to all places where the mac address
gets memcpy()'ed into a local variable.

Signed-off-by: Alexander Graf ag...@suse.de
---
 drivers/net/ethernet/ibm/ibmveth.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c 
b/drivers/net/ethernet/ibm/ibmveth.c
index 952d795..97f4ee96 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -581,7 +581,7 @@ static int ibmveth_open(struct net_device *netdev)
adapter-rx_queue.toggle = 1;
 
memcpy(mac_address, netdev-dev_addr, netdev-addr_len);
-   mac_address = mac_address  16;
+   mac_address = be64_to_cpu(mac_address)  16;
 
rxq_desc.fields.flags_len = IBMVETH_BUF_VALID |
adapter-rx_queue.queue_len;
@@ -1186,6 +1186,7 @@ static void ibmveth_set_multicast_list(struct net_device 
*netdev)
/* add the multicast address to the filter table */
unsigned long mcast_addr = 0;
memcpy(((char *)mcast_addr)+2, ha-addr, ETH_ALEN);
+   mcast_addr = cpu_to_be64(mcast_addr);
lpar_rc = h_multicast_ctrl(adapter-vdev-unit_address,
   IbmVethMcastAddFilter,
   mcast_addr);
-- 
1.8.1.4

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ibmveth: Fix more little endian issues

2013-12-22 Thread Anton Blanchard 

Hi Alex,

 The ibmveth driver is memcpy()'ing the mac address between a variable
 (register) and memory. This assumes a certain endianness of the
 system, so let's make that implicit assumption work again.

Nice catch! I don't like how the driver has two different methods
for creating these MAC addresses, both without comments. How does
this look?

Anton
--

The hypervisor expects MAC addresses passed in registers to be big
endian u64. Create a helper function called ibmveth_encode_mac_addr
which does the right thing in both big and little endian.

We were storing the MAC address in a long in struct ibmveth_adapter.
It's never used so remove it - we don't need another place in the
driver where we create endian issues with MAC addresses.

Reported-by: Alexander Graf ag...@suse.de
Signed-off-by: Anton Blanchard an...@samba.org
---

diff --git a/drivers/net/ethernet/ibm/ibmveth.c 
b/drivers/net/ethernet/ibm/ibmveth.c
index 952d795..044178b 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -523,6 +523,17 @@ retry:
return rc;
 }
 
+/*
+ * The hypervisor expects MAC addresses passed in registers to be
+ * big endian u64.
+ */
+static unsigned long ibmveth_encode_mac_addr(char *mac)
+{
+   unsigned long encoded = 0;
+   memcpy(((char *)encoded) + 2, mac, ETH_ALEN);
+   return cpu_to_be64(encoded);
+}
+
 static int ibmveth_open(struct net_device *netdev)
 {
struct ibmveth_adapter *adapter = netdev_priv(netdev);
@@ -580,8 +591,7 @@ static int ibmveth_open(struct net_device *netdev)
adapter-rx_queue.num_slots = rxq_entries;
adapter-rx_queue.toggle = 1;
 
-   memcpy(mac_address, netdev-dev_addr, netdev-addr_len);
-   mac_address = mac_address  16;
+   mac_address = ibmveth_encode_mac_addr(netdev-dev_addr);
 
rxq_desc.fields.flags_len = IBMVETH_BUF_VALID |
adapter-rx_queue.queue_len;
@@ -1184,8 +1194,8 @@ static void ibmveth_set_multicast_list(struct net_device 
*netdev)
/* add the addresses to the filter table */
netdev_for_each_mc_addr(ha, netdev) {
/* add the multicast address to the filter table */
-   unsigned long mcast_addr = 0;
-   memcpy(((char *)mcast_addr)+2, ha-addr, ETH_ALEN);
+   unsigned long mcast_addr;
+   mcast_addr = ibmveth_encode_mac_addr(ha-addr);
lpar_rc = h_multicast_ctrl(adapter-vdev-unit_address,
   IbmVethMcastAddFilter,
   mcast_addr);
@@ -1369,9 +1379,6 @@ static int ibmveth_probe(struct vio_dev *dev, const 
struct vio_device_id *id)
 
netif_napi_add(netdev, adapter-napi, ibmveth_poll, 16);
 
-   adapter-mac_addr = 0;
-   memcpy(adapter-mac_addr, mac_addr_p, ETH_ALEN);
-
netdev-irq = dev-irq;
netdev-netdev_ops = ibmveth_netdev_ops;
netdev-ethtool_ops = netdev_ethtool_ops;
@@ -1380,7 +1387,7 @@ static int ibmveth_probe(struct vio_dev *dev, const 
struct vio_device_id *id)
NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
netdev-features |= netdev-hw_features;
 
-   memcpy(netdev-dev_addr, adapter-mac_addr, netdev-addr_len);
+   memcpy(netdev-dev_addr, mac_addr_p, ETH_ALEN);
 
for (i = 0; i  IBMVETH_NUM_BUFF_POOLS; i++) {
struct kobject *kobj = adapter-rx_buff_pool[i].kobj;
diff --git a/drivers/net/ethernet/ibm/ibmveth.h 
b/drivers/net/ethernet/ibm/ibmveth.h
index 84066ba..2c636cb 100644
--- a/drivers/net/ethernet/ibm/ibmveth.h
+++ b/drivers/net/ethernet/ibm/ibmveth.h
@@ -139,7 +139,6 @@ struct ibmveth_adapter {
 struct napi_struct napi;
 struct net_device_stats stats;
 unsigned int mcastFilterSize;
-unsigned long mac_addr;
 void * buffer_list_addr;
 void * filter_list_addr;
 dma_addr_t buffer_list_dma;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev