[patch 7/9] lguest: the net driver

[patch 7/9] lguest: the net driver

[akpm]

From: Rusty Russell <rusty@rustcorp.com.au>

Lguest net driver

A simple net driver for lguest.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: Andi Kleen <ak@suse.de>
Cc: Jeff Garzik <jeff@garzik.org>
Acked-by: James Morris <jmorris@namei.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/net/Makefile     |    1 
 drivers/net/lguest_net.c |  345 +++++++++++++++++++++++++++++++++++++
 2 files changed, 346 insertions(+)

diff -puN drivers/net/Makefile~lguest-the-net-driver drivers/net/Makefile
--- a/drivers/net/Makefile~lguest-the-net-driver
+++ a/drivers/net/Makefile
@@ -220,3 +220,4 @@ obj-$(CONFIG_NETCONSOLE) += netconsole.o
 obj-$(CONFIG_FS_ENET) += fs_enet/
 
 obj-$(CONFIG_NETXEN_NIC) += netxen/
+obj-$(CONFIG_LGUEST_GUEST) += lguest_net.o
diff -puN /dev/null drivers/net/lguest_net.c
--- /dev/null
+++ a/drivers/net/lguest_net.c
@@ -0,0 +1,345 @@
+/* A simple network driver for lguest.
+ *
+ * Copyright 2006 Rusty Russell <rusty@rustcorp.com.au> IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+//#define DEBUG
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/module.h>
+#include <linux/mm_types.h>
+#include <linux/lguest_bus.h>
+#include <asm/io.h>
+
+#define SHARED_SIZE		PAGE_SIZE
+#define MAX_LANS		4
+#define NUM_SKBS		8
+
+struct lguestnet_info
+{
+	/* The shared page(s). */
+	struct lguest_net *peer;
+	unsigned long peer_phys;
+	unsigned long mapsize;
+
+	/* My peerid. */
+	unsigned int me;
+
+	/* Receive queue. */
+	struct sk_buff *skb[NUM_SKBS];
+	struct lguest_dma dma[NUM_SKBS];
+};
+
+/* How many bytes left in this page. */
+static unsigned int rest_of_page(void *data)
+{
+	return PAGE_SIZE - ((unsigned long)data % PAGE_SIZE);
+}
+
+/* Simple convention: offset 4 * peernum. */
+static unsigned long peer_key(struct lguestnet_info *info, unsigned peernum)
+{
+	return info->peer_phys + 4 * peernum;
+}
+
+static void skb_to_dma(const struct sk_buff *skb, unsigned int headlen,
+		       struct lguest_dma *dma)
+{
+	unsigned int i, seg;
+
+	for (i = seg = 0; i < headlen; seg++, i += rest_of_page(skb->data+i)) {
+		dma->addr[seg] = virt_to_phys(skb->data + i);
+		dma->len[seg] = min((unsigned)(headlen - i),
+				    rest_of_page(skb->data + i));
+	}
+	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++, seg++) {
+		const skb_frag_t *f = &skb_shinfo(skb)->frags[i];
+		/* Should not happen with MTU less than 64k - 2 * PAGE_SIZE. */
+		if (seg == LGUEST_MAX_DMA_SECTIONS) {
+			printk("Woah dude!  Megapacket!\n");
+			break;
+		}
+		dma->addr[seg] = page_to_phys(f->page) + f->page_offset;
+		dma->len[seg] = f->size;
+	}
+	if (seg < LGUEST_MAX_DMA_SECTIONS)
+		dma->len[seg] = 0;
+}
+
+/* We overload multicast bit to show promiscuous mode. */
+#define PROMISC_BIT		0x01
+
+static void lguestnet_set_multicast(struct net_device *dev)
+{
+	struct lguestnet_info *info = dev->priv;
+
+	if ((dev->flags & (IFF_PROMISC|IFF_ALLMULTI)) || dev->mc_count)
+		info->peer[info->me].mac[0] |= PROMISC_BIT;
+	else
+		info->peer[info->me].mac[0] &= ~PROMISC_BIT;
+}
+
+static int promisc(struct lguestnet_info *info, unsigned int peer)
+{
+	return info->peer[peer].mac[0] & PROMISC_BIT;
+}
+
+static int mac_eq(const unsigned char mac[ETH_ALEN],
+		  struct lguestnet_info *info, unsigned int peer)
+{
+	/* Ignore multicast bit, which peer turns on to mean promisc. */
+	if ((info->peer[peer].mac[0] & (~PROMISC_BIT)) != mac[0])
+		return 0;
+	return memcmp(mac+1, info->peer[peer].mac+1, ETH_ALEN-1) == 0;
+}
+
+static void transfer_packet(struct net_device *dev,
+			    struct sk_buff *skb,
+			    unsigned int peernum)
+{
+	struct lguestnet_info *info = dev->priv;
+	struct lguest_dma dma;
+
+	skb_to_dma(skb, skb_headlen(skb), &dma);
+	pr_debug("xfer length %04x (%u)\n", htons(skb->len), skb->len);
+
+	hcall(LHCALL_SEND_DMA, peer_key(info,peernum), __pa(&dma), 0);
+	if (dma.used_len != skb->len) {
+		dev->stats.tx_carrier_errors++;
+		pr_debug("Bad xfer to peer %i: %i of %i (dma %p/%i)\n",
+			 peernum, dma.used_len, skb->len,
+			 (void *)dma.addr[0], dma.len[0]);
+	} else {
+		dev->stats.tx_bytes += skb->len;
+		dev->stats.tx_packets++;
+	}
+}
+
+static int unused_peer(const struct lguest_net peer[], unsigned int num)
+{
+	return peer[num].mac[0] == 0;
+}
+
+static int lguestnet_start_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	unsigned int i;
+	int broadcast;
+	struct lguestnet_info *info = dev->priv;
+	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
+
+	pr_debug("%s: xmit %02x:%02x:%02x:%02x:%02x:%02x\n",
+		 dev->name, dest[0],dest[1],dest[2],dest[3],dest[4],dest[5]);
+
+	broadcast = is_multicast_ether_addr(dest);
+	for (i = 0; i < info->mapsize/sizeof(struct lguest_net); i++) {
+		if (i == info->me || unused_peer(info->peer, i))
+			continue;
+
+		if (!broadcast && !promisc(info, i) && !mac_eq(dest, info, i))
+			continue;
+
+		pr_debug("lguestnet %s: sending from %i to %i\n",
+			 dev->name, info->me, i);
+		transfer_packet(dev, skb, i);
+	}
+	dev_kfree_skb(skb);
+	return 0;
+}
+
+/* Find a new skb to put in this slot in shared mem. */
+static int fill_slot(struct net_device *dev, unsigned int slot)
+{
+	struct lguestnet_info *info = dev->priv;
+	/* Try to create and register a new one. */
+	info->skb[slot] = netdev_alloc_skb(dev, ETH_HLEN + ETH_DATA_LEN);
+	if (!info->skb[slot]) {
+		printk("%s: could not fill slot %i\n", dev->name, slot);
+		return -ENOMEM;
+	}
+
+	skb_to_dma(info->skb[slot], ETH_HLEN + ETH_DATA_LEN, &info->dma[slot]);
+	wmb();
+	/* Now we tell hypervisor it can use the slot. */
+	info->dma[slot].used_len = 0;
+	return 0;
+}
+
+static irqreturn_t lguestnet_rcv(int irq, void *dev_id)
+{
+	struct net_device *dev = dev_id;
+	struct lguestnet_info *info = dev->priv;
+	unsigned int i, done = 0;
+
+	for (i = 0; i < ARRAY_SIZE(info->dma); i++) {
+		unsigned int length;
+		struct sk_buff *skb;
+
+		length = info->dma[i].used_len;
+		if (length == 0)
+			continue;
+
+		done++;
+		skb = info->skb[i];
+		fill_slot(dev, i);
+
+		if (length < ETH_HLEN || length > ETH_HLEN + ETH_DATA_LEN) {
+			pr_debug(KERN_WARNING "%s: unbelievable skb len: %i\n",
+				 dev->name, length);
+			dev_kfree_skb(skb);
+			continue;
+		}
+
+		skb_put(skb, length);
+		skb->protocol = eth_type_trans(skb, dev);
+		/* This is a reliable transport. */
+		if (dev->features & NETIF_F_NO_CSUM)
+			skb->ip_summed = CHECKSUM_UNNECESSARY;
+		pr_debug("Receiving skb proto 0x%04x len %i type %i\n",
+			 ntohs(skb->protocol), skb->len, skb->pkt_type);
+
+		dev->stats.rx_bytes += skb->len;
+		dev->stats.rx_packets++;
+		netif_rx(skb);
+	}
+	return done ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int lguestnet_open(struct net_device *dev)
+{
+	int i;
+	struct lguestnet_info *info = dev->priv;
+
+	/* Set up our MAC address */
+	memcpy(info->peer[info->me].mac, dev->dev_addr, ETH_ALEN);
+
+	/* Turn on promisc mode if needed */
+	lguestnet_set_multicast(dev);
+
+	for (i = 0; i < ARRAY_SIZE(info->dma); i++) {
+		if (fill_slot(dev, i) != 0)
+			goto cleanup;
+	}
+	if (!hcall(LHCALL_BIND_DMA, peer_key(info, info->me), __pa(info->dma),
+		   (NUM_SKBS << 8) | dev->irq))
+		goto cleanup;
+	return 0;
+
+cleanup:
+	while (--i >= 0)
+		dev_kfree_skb(info->skb[i]);
+	return -ENOMEM;
+}
+
+static int lguestnet_close(struct net_device *dev)
+{
+	unsigned int i;
+	struct lguestnet_info *info = dev->priv;
+
+	/* Clear all trace: others might deliver packets, we'll ignore it. */
+	memset(&info->peer[info->me], 0, sizeof(info->peer[info->me]));
+
+	/* Deregister sg lists. */
+	hcall(LHCALL_BIND_DMA, peer_key(info, info->me), __pa(info->dma), 0);
+	for (i = 0; i < ARRAY_SIZE(info->dma); i++)
+		dev_kfree_skb(info->skb[i]);
+	return 0;
+}
+
+static int lguestnet_probe(struct lguest_device *lgdev)
+{
+	int err, irqf = IRQF_SHARED;
+	struct net_device *dev;
+	struct lguestnet_info *info;
+	struct lguest_device_desc *desc = &lguest_devices[lgdev->index];
+
+	pr_debug("lguest_net: probing for device %i\n", lgdev->index);
+
+	dev = alloc_etherdev(sizeof(struct lguestnet_info));
+	if (!dev)
+		return -ENOMEM;
+
+	SET_MODULE_OWNER(dev);
+
+	/* Ethernet defaults with some changes */
+	ether_setup(dev);
+	dev->set_mac_address = NULL;
+
+	dev->dev_addr[0] = 0x02; /* set local assignment bit (IEEE802) */
+	dev->dev_addr[1] = 0x00;
+	memcpy(&dev->dev_addr[2], &lguest_data.guestid, 2);
+	dev->dev_addr[4] = 0x00;
+	dev->dev_addr[5] = 0x00;
+
+	dev->open = lguestnet_open;
+	dev->stop = lguestnet_close;
+	dev->hard_start_xmit = lguestnet_start_xmit;
+
+	/* Turning on/off promisc will call dev->set_multicast_list.
+	 * We don't actually support multicast yet */
+	dev->set_multicast_list = lguestnet_set_multicast;
+	dev->mem_start = ((unsigned long)desc->pfn << PAGE_SHIFT);
+	dev->mem_end = dev->mem_start + PAGE_SIZE * desc->num_pages;
+	dev->irq = lgdev->index+1;
+	dev->features = NETIF_F_SG;
+	if (desc->features & LGUEST_NET_F_NOCSUM)
+		dev->features |= NETIF_F_NO_CSUM;
+
+	info = dev->priv;
+	info->mapsize = PAGE_SIZE * desc->num_pages;
+	info->peer_phys = ((unsigned long)desc->pfn << PAGE_SHIFT);
+	info->peer = (void *)ioremap(info->peer_phys, info->mapsize);
+	/* This stores our peerid (upper bits reserved for future). */
+	info->me = (desc->features & (info->mapsize-1));
+
+	err = register_netdev(dev);
+	if (err) {
+		pr_debug("lguestnet: registering device failed\n");
+		goto free;
+	}
+
+	if (lguest_devices[lgdev->index].features & LGUEST_DEVICE_F_RANDOMNESS)
+		irqf |= IRQF_SAMPLE_RANDOM;
+	if (request_irq(dev->irq, lguestnet_rcv, irqf, "lguestnet", dev) != 0) {
+		pr_debug("lguestnet: could not get net irq %i\n", dev->irq);
+		goto unregister;
+	}
+
+	pr_debug("lguestnet: registered device %s\n", dev->name);
+	lgdev->private = dev;
+	return 0;
+
+unregister:
+	unregister_netdev(dev);
+free:
+	free_netdev(dev);
+	return err;
+}
+
+static struct lguest_driver lguestnet_drv = {
+	.name = "lguestnet",
+	.owner = THIS_MODULE,
+	.device_type = LGUEST_DEVICE_T_NET,
+	.probe = lguestnet_probe,
+};
+
+static __init int lguestnet_init(void)
+{
+	return register_lguest_driver(&lguestnet_drv);
+}
+module_init(lguestnet_init);
+
+MODULE_DESCRIPTION("Lguest network driver");
+MODULE_LICENSE("GPL");
_

Re: [patch 7/9] lguest: the net driver

[Herbert Xu]

akpm@linux-foundation.org wrote:
>
> +       if (desc->features & LGUEST_NET_F_NOCSUM)
> +               dev->features |= NETIF_F_NO_CSUM;

Any reason why you're using NO_CSUM here instead of HW_CSUM?
Practically there is no difference but NO_CSUM could be treated
differently in future and I'm not sure whether such changes would
be desirable in this driver (i.e., not even generating a partial
checksum).

Also, there doesn't seem to be any passing of metadata to the
backend which means that neither NO_CSUM/HW_CSUM can work if
somebody needs to look at the checksum field.  You could use
IP_CSUM if you do the same hack on the backend that Xen does.
Otherwise you'll have to make do with no checksum offload at all.

I think you'd also need a change_mtu function if the SG feature
is going to be of some use since the default Ethernet one limits
the MTU to 1500.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [patch 7/9] lguest: the net driver

[Rusty Russell]

On Wed, 2007-05-09 at 20:12 +1000, Herbert Xu wrote:
> akpm@linux-foundation.org wrote:
> >
> > +       if (desc->features & LGUEST_NET_F_NOCSUM)
> > +               dev->features |= NETIF_F_NO_CSUM;
> 
> Any reason why you're using NO_CSUM here instead of HW_CSUM?
> Practically there is no difference but NO_CSUM could be treated
> differently in future and I'm not sure whether such changes would
> be desirable in this driver (i.e., not even generating a partial
> checksum).

Hi Herbert,

	NO_CSUM because it really doesn't need a checksum.  The
LGUEST_NET_F_NOCSUM is only set for local inter-guest networking.  If
some guest were to route the packets outside the machine, this would be
an issue, though ("don't do that").

> Also, there doesn't seem to be any passing of metadata to the
> backend which means that neither NO_CSUM/HW_CSUM can work if
> somebody needs to look at the checksum field.  You could use
> IP_CSUM if you do the same hack on the backend that Xen does.
> Otherwise you'll have to make do with no checksum offload at all.

Yeah, definitely good future work.  This is far simpler: external-facing
networks don't get marked with the LGUEST_NET_F_NOCSUM but set.

KVM is going to have a paravirt network driver too: it'd be nice to
share code here.

> I think you'd also need a change_mtu function if the SG feature
> is going to be of some use since the default Ethernet one limits
> the MTU to 1500.

Indeed, that would be a new feature, and is certainly a consideration
for more efficient inter-guest networking.  However, I consider that
somewhat cheating: it's nice to know that 1500 doesn't suck too hard.

Remember, "Features kill puppies!" 8)

Thanks,
Rusty.

Re: [patch 7/9] lguest: the net driver

[Herbert Xu]

Hi Rusty:

On Wed, May 09, 2007 at 09:55:25PM +1000, Rusty Russell wrote:
> 
> 	NO_CSUM because it really doesn't need a checksum.  The
> LGUEST_NET_F_NOCSUM is only set for local inter-guest networking.  If
> some guest were to route the packets outside the machine, this would be
> an issue, though ("don't do that").

While I can see that this is good in keeping things simple, I think
it's something that you want to be able to support since the user
may wish to setup a guest as a firewall appliance which would involve
passing packets from another guest to the outside world.
 
> Indeed, that would be a new feature, and is certainly a consideration
> for more efficient inter-guest networking.  However, I consider that
> somewhat cheating: it's nice to know that 1500 doesn't suck too hard.
> 
> Remember, "Features kill puppies!" 8)

Heh :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [patch 7/9] lguest: the net driver

[Rusty Russell]

On Wed, 2007-05-09 at 22:00 +1000, Herbert Xu wrote:
> Hi Rusty:
> 
> On Wed, May 09, 2007 at 09:55:25PM +1000, Rusty Russell wrote:
> > 
> > 	NO_CSUM because it really doesn't need a checksum.  The
> > LGUEST_NET_F_NOCSUM is only set for local inter-guest networking.  If
> > some guest were to route the packets outside the machine, this would be
> > an issue, though ("don't do that").
> 
> While I can see that this is good in keeping things simple, I think
> it's something that you want to be able to support since the user
> may wish to setup a guest as a firewall appliance which would involve
> passing packets from another guest to the outside world.

Indeed, you understand the tradeoff.  The example launcher could have an
option not to set the LGUEST_NET_F_NOCSUM in this case.

That said, one significant purpose of lguest is to serve as an example
of how to do things.  So if you feel really strongly that there's a
Right Way, we could look at the patch...

Thanks,
Rusty.

Re: [patch 7/9] lguest: the net driver

[Jeff Garzik]

akpm@linux-foundation.org wrote:
> +static void transfer_packet(struct net_device *dev,
> +			    struct sk_buff *skb,
> +			    unsigned int peernum)
> +{
> +	struct lguestnet_info *info = dev->priv;
> +	struct lguest_dma dma;
> +
> +	skb_to_dma(skb, skb_headlen(skb), &dma);
> +	pr_debug("xfer length %04x (%u)\n", htons(skb->len), skb->len);
> +
> +	hcall(LHCALL_SEND_DMA, peer_key(info,peernum), __pa(&dma), 0);

__pa() should not be used in any driver.

At the very least, lguest helper code should wrap this.



> +static irqreturn_t lguestnet_rcv(int irq, void *dev_id)
> +{
> +	struct net_device *dev = dev_id;
> +	struct lguestnet_info *info = dev->priv;
> +	unsigned int i, done = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(info->dma); i++) {
> +		unsigned int length;
> +		struct sk_buff *skb;
> +
> +		length = info->dma[i].used_len;
> +		if (length == 0)
> +			continue;
> +
> +		done++;
> +		skb = info->skb[i];
> +		fill_slot(dev, i);
> +
> +		if (length < ETH_HLEN || length > ETH_HLEN + ETH_DATA_LEN) {
> +			pr_debug(KERN_WARNING "%s: unbelievable skb len: %i\n",
> +				 dev->name, length);
> +			dev_kfree_skb(skb);
> +			continue;
> +		}
> +
> +		skb_put(skb, length);
> +		skb->protocol = eth_type_trans(skb, dev);
> +		/* This is a reliable transport. */
> +		if (dev->features & NETIF_F_NO_CSUM)
> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
> +		pr_debug("Receiving skb proto 0x%04x len %i type %i\n",
> +			 ntohs(skb->protocol), skb->len, skb->pkt_type);
> +
> +		dev->stats.rx_bytes += skb->len;
> +		dev->stats.rx_packets++;
> +		netif_rx(skb);
> +	}
> +	return done ? IRQ_HANDLED : IRQ_NONE;

Using NAPI would be preferable...



> +static int lguestnet_probe(struct lguest_device *lgdev)
> +{
> +	int err, irqf = IRQF_SHARED;
> +	struct net_device *dev;
> +	struct lguestnet_info *info;
> +	struct lguest_device_desc *desc = &lguest_devices[lgdev->index];
> +
> +	pr_debug("lguest_net: probing for device %i\n", lgdev->index);
> +
> +	dev = alloc_etherdev(sizeof(struct lguestnet_info));
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	SET_MODULE_OWNER(dev);
> +
> +	/* Ethernet defaults with some changes */
> +	ether_setup(dev);
> +	dev->set_mac_address = NULL;

why NULL?


> +	dev->dev_addr[0] = 0x02; /* set local assignment bit (IEEE802) */
> +	dev->dev_addr[1] = 0x00;
> +	memcpy(&dev->dev_addr[2], &lguest_data.guestid, 2);
> +	dev->dev_addr[4] = 0x00;
> +	dev->dev_addr[5] = 0x00;
> +
> +	dev->open = lguestnet_open;
> +	dev->stop = lguestnet_close;
> +	dev->hard_start_xmit = lguestnet_start_xmit;
> +
> +	/* Turning on/off promisc will call dev->set_multicast_list.
> +	 * We don't actually support multicast yet */
> +	dev->set_multicast_list = lguestnet_set_multicast;
> +	dev->mem_start = ((unsigned long)desc->pfn << PAGE_SHIFT);
> +	dev->mem_end = dev->mem_start + PAGE_SIZE * desc->num_pages;
> +	dev->irq = lgdev->index+1;

don't fill in mem_start, mem_end and irq.  they are useless, and for 
lguest, misleading.


> +	dev->features = NETIF_F_SG;
> +	if (desc->features & LGUEST_NET_F_NOCSUM)
> +		dev->features |= NETIF_F_NO_CSUM;

do not set SG without an accompanying csum bitflag


> +	info = dev->priv;

use netdev_priv()


> +	info->mapsize = PAGE_SIZE * desc->num_pages;
> +	info->peer_phys = ((unsigned long)desc->pfn << PAGE_SHIFT);
> +	info->peer = (void *)ioremap(info->peer_phys, info->mapsize);

check for NULL


> +	/* This stores our peerid (upper bits reserved for future). */
> +	info->me = (desc->features & (info->mapsize-1));
> +
> +	err = register_netdev(dev);
> +	if (err) {
> +		pr_debug("lguestnet: registering device failed\n");
> +		goto free;
> +	}
> +
> +	if (lguest_devices[lgdev->index].features & LGUEST_DEVICE_F_RANDOMNESS)
> +		irqf |= IRQF_SAMPLE_RANDOM;
> +	if (request_irq(dev->irq, lguestnet_rcv, irqf, "lguestnet", dev) != 0) {
> +		pr_debug("lguestnet: could not get net irq %i\n", dev->irq);
> +		goto unregister;
> +	}
> +
> +	pr_debug("lguestnet: registered device %s\n", dev->name);
> +	lgdev->private = dev;
> +	return 0;
> +
> +unregister:
> +	unregister_netdev(dev);
> +free:
> +	free_netdev(dev);

missing iounmap() on error


> +static struct lguest_driver lguestnet_drv = {
> +	.name = "lguestnet",
> +	.owner = THIS_MODULE,
> +	.device_type = LGUEST_DEVICE_T_NET,
> +	.probe = lguestnet_probe,
> +};

You are distinctly missing module remove support

	Jeff

Re: [patch 7/9] lguest: the net driver

[Andi Kleen]

> __pa() should not be used in any driver.

Besides that was always not supposed to work. RELOC_HIDE or __pa_symbol.

-Andi

Re: [patch 7/9] lguest: the net driver

[Rusty Russell]

Hi Jeff,

	Thanks for your review.  Questions below.

On Wed, 2007-05-09 at 08:28 -0400, Jeff Garzik wrote:
> akpm@linux-foundation.org wrote:
> > +static void transfer_packet(struct net_device *dev,
...
> > +	hcall(LHCALL_SEND_DMA, peer_key(info,peernum), __pa(&dma), 0);
> 
> __pa() should not be used in any driver.
> 
> At the very least, lguest helper code should wrap this.

	I realize your continual battle with this, but adding a layer of
indirection doesn't seem like it will add clarity.  The issues with
__pa() are reasonably known (don't hand it a vmalloc address, for
example).  Any wrapper I create would be another hurdle to jump 8(

> > +static irqreturn_t lguestnet_rcv(int irq, void *dev_id)
...
> > +	return done ? IRQ_HANDLED : IRQ_NONE;
> 
> Using NAPI would be preferable...

I'm not so convinced: scheduling tends to give us pretty good interrupt
mitigation.  However, if you wish to send a patch, I'd be happy to
benchmark the two 8)

> > +	/* Ethernet defaults with some changes */
> > +	ether_setup(dev);
> > +	dev->set_mac_address = NULL;
> 
> why NULL?

Because it's not implemented: our MAC is advertised in the device page
and we'd have to change it there too.

Trivial to do, but is there a compelling reason to implement it?

> > +	dev->mem_start = ((unsigned long)desc->pfn << PAGE_SHIFT);
> > +	dev->mem_end = dev->mem_start + PAGE_SIZE * desc->num_pages;
> > +	dev->irq = lgdev->index+1;
> 
> don't fill in mem_start, mem_end and irq.  they are useless, and for 
> lguest, misleading.

You meant to type "useful and accurate", I think?  They show up in
ifconfig, so you can see what the underlying devices are using.  They're
as useful for virtual hardware as they are for physical hardware.

> > +	dev->features = NETIF_F_SG;
> > +	if (desc->features & LGUEST_NET_F_NOCSUM)
> > +		dev->features |= NETIF_F_NO_CSUM;
> 
> do not set SG without an accompanying csum bitflag

That seems... odd.  My driver can do SG, and may or may not need csums.
The current Linux code turns SG off if I need csums and that's fine, but
it hardly seems like my device should be making that decision.

> > +	info = dev->priv;
> 
> use netdev_priv()

OK, thanks.

> > +	info->mapsize = PAGE_SIZE * desc->num_pages;
> > +	info->peer_phys = ((unsigned long)desc->pfn << PAGE_SHIFT);
> > +	info->peer = (void *)ioremap(info->peer_phys, info->mapsize);
> 
> check for NULL

Erk, good catch!

> > +unregister:
> > +	unregister_netdev(dev);
> > +free:
> > +	free_netdev(dev);
> 
> missing iounmap() on error

What is it about me and iomap... will fix that too.

> > +static struct lguest_driver lguestnet_drv = {
> > +	.name = "lguestnet",
> > +	.owner = THIS_MODULE,
> > +	.device_type = LGUEST_DEVICE_T_NET,
> > +	.probe = lguestnet_probe,
> > +};
> 
> You are distinctly missing module remove support

Yes.  It is never built as a module currently (though it should work).

Thanks!
Rusty.

Re: [patch 7/9] lguest: the net driver

[Christoph Hellwig]

On Thu, May 10, 2007 at 01:14:55AM +1000, Rusty Russell wrote:
> > > +	info->peer = (void *)ioremap(info->peer_phys, info->mapsize);
> > 
> > check for NULL
> 
> Erk, good catch!

Also the cast is bogus.  ioremap already returns void already.  Even
more importantly the lack of the __iomem annotations shows that either this
code hasn't been run through sparse or someone decided to ignore it's
errors.

Re: [patch 7/9] lguest: the net driver

[Jeff Garzik]

Rusty Russell wrote:
> Hi Jeff,
> 
> 	Thanks for your review.  Questions below.
> 
> On Wed, 2007-05-09 at 08:28 -0400, Jeff Garzik wrote:
>> akpm@linux-foundation.org wrote:
>>> +static void transfer_packet(struct net_device *dev,
> ...
>>> +	hcall(LHCALL_SEND_DMA, peer_key(info,peernum), __pa(&dma), 0);
>> __pa() should not be used in any driver.
>>
>> At the very least, lguest helper code should wrap this.
> 
> 	I realize your continual battle with this, but adding a layer of
> indirection doesn't seem like it will add clarity.  The issues with
> __pa() are reasonably known (don't hand it a vmalloc address, for
> example).  Any wrapper I create would be another hurdle to jump 8(

You don't want this low level stuff in drivers.

All such details should be hidden away in the arch code, which in your 
case is the lguest support code.

lguest should present a nice, friendly driver API that any Computer 
Science freshman at university will understand.  Because that's the 
level at which we driver writers exist... :)


>>> +static irqreturn_t lguestnet_rcv(int irq, void *dev_id)
> ...
>>> +	return done ? IRQ_HANDLED : IRQ_NONE;
>> Using NAPI would be preferable...
> 
> I'm not so convinced: scheduling tends to give us pretty good interrupt
> mitigation.  However, if you wish to send a patch, I'd be happy to
> benchmark the two 8)

NAPI means system-wide load leveling, across multiple network 
interfaces.  Lack of NAPI can mean competition at higher loads.  Though 
maybe that's less important with lguest.


>>> +	/* Ethernet defaults with some changes */
>>> +	ether_setup(dev);
>>> +	dev->set_mac_address = NULL;
>> why NULL?
> 
> Because it's not implemented: our MAC is advertised in the device page
> and we'd have to change it there too.
> 
> Trivial to do, but is there a compelling reason to implement it?

Bonding, and situations where you /do/ want the MAC address to "leak" 
out of the host onto the wider net.


>>> +	dev->mem_start = ((unsigned long)desc->pfn << PAGE_SHIFT);
>>> +	dev->mem_end = dev->mem_start + PAGE_SIZE * desc->num_pages;
>>> +	dev->irq = lgdev->index+1;
>> don't fill in mem_start, mem_end and irq.  they are useless, and for 
>> lguest, misleading.
> 
> You meant to type "useful and accurate", I think?  They show up in
> ifconfig, so you can see what the underlying devices are using.  They're
> as useful for virtual hardware as they are for physical hardware.

Indeed -- they are useless for physical hardware, as well.

Those items have not been used since the ISA days.  Hardware is far more 
complex than those three variables can provide, so the wise choice is to 
SET_NETDEV_DEV() to your device, which reveals all manner of bus 
information by reference.

Storing information in those variables is needless duplication, which is 
why they have been relegated only to ancient ISA drivers -- or in the 
case of ->mem_start, repurposed as a method of passing options.



>>> +	dev->features = NETIF_F_SG;
>>> +	if (desc->features & LGUEST_NET_F_NOCSUM)
>>> +		dev->features |= NETIF_F_NO_CSUM;
>> do not set SG without an accompanying csum bitflag
> 
> That seems... odd.  My driver can do SG, and may or may not need csums.
> The current Linux code turns SG off if I need csums and that's fine, but
> it hardly seems like my device should be making that decision.

The net stack does not have a safety cushion.  Set the wrong flags, and 
things can and will go wrong.  SG without CSUM support is illogical, and 
can and will break things.  Remember what SG is for.  If you cannot 
offload the csum, you have to build the csum yourself, at which point 
you might as well copy it too.  grep around for skb_copy_and_csum_dev() 
to see if that helps you at all.


>>> +static struct lguest_driver lguestnet_drv = {
>>> +	.name = "lguestnet",
>>> +	.owner = THIS_MODULE,
>>> +	.device_type = LGUEST_DEVICE_T_NET,
>>> +	.probe = lguestnet_probe,
>>> +};
>> You are distinctly missing module remove support
> 
> Yes.  It is never built as a module currently (though it should work).

That's my point.  It won't work as a module, because it lacks remove 
support.  It is not unrealistic to think of [un|re|]loading the net 
support module in an lguest guest.  And, adding module support makes the 
programmer more responsible, because they now have to learn to clean up 
after themselves.  Any driver that cannot clean up after itself is an 
incomplete driver in my book.

	Jeff

Re: [patch 7/9] lguest: the net driver

[Rusty Russell]

On Thu, 2007-05-10 at 01:33 -0400, Jeff Garzik wrote:
> Rusty Russell wrote:
> > 	I realize your continual battle with this, but adding a layer of
> > indirection doesn't seem like it will add clarity.  The issues with
> > __pa() are reasonably known (don't hand it a vmalloc address, for
> > example).  Any wrapper I create would be another hurdle to jump 8(
> 
> You don't want this low level stuff in drivers.
[snip answers]

Hi Jeff,

	This email was illuminating, thanks.  I've put NAPI, set_mac_address
and module unload in the documentation patch for future enthusiasts.
The rest is implemented here.

Here's the extract from my current "feedback" patch:

...
2) Jeff Garzik
    - Use netdev_priv instead of dev->priv.
    - Check for ioremap failure
    - iounmap on failure.
    - Wrap SEND_DMA and BIND_DMA calls
    - Don't set NETIF_F_SG unless we set NETIF_F_NO_CSUM
    - Use SET_NETDEV_DEV()
    - Don't set dev->irq, mem_start & mem_end (deprecated)

diff -r 35c8b37f8d3c drivers/lguest/lguest.c
--- a/drivers/lguest/lguest.c	Wed May 09 23:23:56 2007 +1000
+++ b/drivers/lguest/lguest.c	Thu May 10 20:06:25 2007 +1000
@@ -27,6 +27,7 @@
 #include <linux/interrupt.h>
 #include <linux/lguest.h>
 #include <linux/lguest_launcher.h>
+#include <linux/lguest_bus.h>
 #include <asm/paravirt.h>
 #include <asm/param.h>
 #include <asm/page.h>
@@ -99,6 +100,25 @@ void async_hcall(unsigned long call,
 			next_call = 0;
 	}
 	local_irq_restore(flags);
+}
+
+void lguest_send_dma(unsigned long key, struct lguest_dma *dma)
+{
+	dma->used_len = 0;
+	hcall(LHCALL_SEND_DMA, key, __pa(dma), 0);
+}
+
+int lguest_bind_dma(unsigned long key, struct lguest_dma *dmas,
+		    unsigned int num, u8 irq)
+{
+	if (!hcall(LHCALL_BIND_DMA, key, __pa(dmas), (num << 8) | irq))
+		return -ENOMEM;
+	return 0;
+}
+
+void lguest_unbind_dma(unsigned long key, struct lguest_dma *dmas)
+{
+	hcall(LHCALL_BIND_DMA, key, __pa(dmas), 0);
 }
 
 static unsigned long save_fl(void)
diff -r 35c8b37f8d3c drivers/net/lguest_net.c
--- a/drivers/net/lguest_net.c	Wed May 09 23:23:56 2007 +1000
+++ b/drivers/net/lguest_net.c	Thu May 10 19:30:21 2007 +1000
@@ -35,6 +35,9 @@ struct lguestnet_info
 	unsigned long peer_phys;
 	unsigned long mapsize;
 
+	/* The lguest_device I come from */
+	struct lguest_device *lgdev;
+
 	/* My peerid. */
 	unsigned int me;
 
@@ -84,7 +87,7 @@ static void skb_to_dma(const struct sk_b
 
 static void lguestnet_set_multicast(struct net_device *dev)
 {
-	struct lguestnet_info *info = dev->priv;
+	struct lguestnet_info *info = netdev_priv(dev);
 
 	if ((dev->flags & (IFF_PROMISC|IFF_ALLMULTI)) || dev->mc_count)
 		info->peer[info->me].mac[0] |= PROMISC_BIT;
@@ -110,13 +113,13 @@ static void transfer_packet(struct net_d
 			    struct sk_buff *skb,
 			    unsigned int peernum)
 {
-	struct lguestnet_info *info = dev->priv;
+	struct lguestnet_info *info = netdev_priv(dev);
 	struct lguest_dma dma;
 
 	skb_to_dma(skb, skb_headlen(skb), &dma);
 	pr_debug("xfer length %04x (%u)\n", htons(skb->len), skb->len);
 
-	hcall(LHCALL_SEND_DMA, peer_key(info,peernum), __pa(&dma), 0);
+	lguest_send_dma(peer_key(info, peernum), &dma);
 	if (dma.used_len != skb->len) {
 		dev->stats.tx_carrier_errors++;
 		pr_debug("Bad xfer to peer %i: %i of %i (dma %p/%i)\n",
@@ -137,7 +140,7 @@ static int lguestnet_start_xmit(struct s
 {
 	unsigned int i;
 	int broadcast;
-	struct lguestnet_info *info = dev->priv;
+	struct lguestnet_info *info = netdev_priv(dev);
 	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
 
 	pr_debug("%s: xmit %02x:%02x:%02x:%02x:%02x:%02x\n",
@@ -162,7 +165,7 @@ static int lguestnet_start_xmit(struct s
 /* Find a new skb to put in this slot in shared mem. */
 static int fill_slot(struct net_device *dev, unsigned int slot)
 {
-	struct lguestnet_info *info = dev->priv;
+	struct lguestnet_info *info = netdev_priv(dev);
 	/* Try to create and register a new one. */
 	info->skb[slot] = netdev_alloc_skb(dev, ETH_HLEN + ETH_DATA_LEN);
 	if (!info->skb[slot]) {
@@ -180,7 +183,7 @@ static irqreturn_t lguestnet_rcv(int irq
 static irqreturn_t lguestnet_rcv(int irq, void *dev_id)
 {
 	struct net_device *dev = dev_id;
-	struct lguestnet_info *info = dev->priv;
+	struct lguestnet_info *info = netdev_priv(dev);
 	unsigned int i, done = 0;
 
 	for (i = 0; i < ARRAY_SIZE(info->dma); i++) {
@@ -220,7 +223,7 @@ static int lguestnet_open(struct net_dev
 static int lguestnet_open(struct net_device *dev)
 {
 	int i;
-	struct lguestnet_info *info = dev->priv;
+	struct lguestnet_info *info = netdev_priv(dev);
 
 	/* Set up our MAC address */
 	memcpy(info->peer[info->me].mac, dev->dev_addr, ETH_ALEN);
@@ -232,8 +235,8 @@ static int lguestnet_open(struct net_dev
 		if (fill_slot(dev, i) != 0)
 			goto cleanup;
 	}
-	if (!hcall(LHCALL_BIND_DMA, peer_key(info, info->me), __pa(info->dma),
-		   (NUM_SKBS << 8) | dev->irq))
+	if (lguest_bind_dma(peer_key(info,info->me), info->dma,
+			    NUM_SKBS, lgdev_irq(info->lgdev)) != 0)
 		goto cleanup;
 	return 0;
 
@@ -246,13 +249,13 @@ static int lguestnet_close(struct net_de
 static int lguestnet_close(struct net_device *dev)
 {
 	unsigned int i;
-	struct lguestnet_info *info = dev->priv;
+	struct lguestnet_info *info = netdev_priv(dev);
 
 	/* Clear all trace: others might deliver packets, we'll ignore it. */
 	memset(&info->peer[info->me], 0, sizeof(info->peer[info->me]));
 
 	/* Deregister sg lists. */
-	hcall(LHCALL_BIND_DMA, peer_key(info, info->me), __pa(info->dma), 0);
+	lguest_unbind_dma(peer_key(info, info->me), info->dma);
 	for (i = 0; i < ARRAY_SIZE(info->dma); i++)
 		dev_kfree_skb(info->skb[i]);
 	return 0;
@@ -290,30 +293,34 @@ static int lguestnet_probe(struct lguest
 	/* Turning on/off promisc will call dev->set_multicast_list.
 	 * We don't actually support multicast yet */
 	dev->set_multicast_list = lguestnet_set_multicast;
-	dev->mem_start = ((unsigned long)desc->pfn << PAGE_SHIFT);
-	dev->mem_end = dev->mem_start + PAGE_SIZE * desc->num_pages;
-	dev->irq = lgdev->index+1;
-	dev->features = NETIF_F_SG;
+	SET_NETDEV_DEV(dev, &lgdev->dev);
 	if (desc->features & LGUEST_NET_F_NOCSUM)
-		dev->features |= NETIF_F_NO_CSUM;
-
-	info = dev->priv;
+		dev->features = NETIF_F_SG|NETIF_F_NO_CSUM;
+
+	info = netdev_priv(dev);
 	info->mapsize = PAGE_SIZE * desc->num_pages;
 	info->peer_phys = ((unsigned long)desc->pfn << PAGE_SHIFT);
-	info->peer = (void *)ioremap(info->peer_phys, info->mapsize);
+	info->lgdev = lgdev;
+	info->peer = (__force void *)ioremap(info->peer_phys, info->mapsize);
+	if (!info->peer) {
+		err = -ENOMEM;
+		goto free;
+	}
+
 	/* This stores our peerid (upper bits reserved for future). */
 	info->me = (desc->features & (info->mapsize-1));
 
 	err = register_netdev(dev);
 	if (err) {
 		pr_debug("lguestnet: registering device failed\n");
-		goto free;
+		goto unmap;
 	}
 
 	if (lguest_devices[lgdev->index].features & LGUEST_DEVICE_F_RANDOMNESS)
 		irqf |= IRQF_SAMPLE_RANDOM;
-	if (request_irq(dev->irq, lguestnet_rcv, irqf, "lguestnet", dev) != 0) {
-		pr_debug("lguestnet: could not get net irq %i\n", dev->irq);
+	if (request_irq(lgdev_irq(lgdev), lguestnet_rcv, irqf, "lguestnet",
+			dev) != 0) {
+		pr_debug("lguestnet: cannot get irq %i\n", lgdev_irq(lgdev));
 		goto unregister;
 	}
 
@@ -323,6 +330,8 @@ static int lguestnet_probe(struct lguest
 
 unregister:
 	unregister_netdev(dev);
+unmap:
+	iounmap((__force void __iomem *)info->peer);
 free:
 	free_netdev(dev);
 	return err;
diff -r 35c8b37f8d3c include/linux/lguest_bus.h
--- a/include/linux/lguest_bus.h	Wed May 09 23:23:56 2007 +1000
+++ b/include/linux/lguest_bus.h	Thu May 10 19:29:14 2007 +1000
@@ -7,7 +7,6 @@
 
 struct lguest_device {
 	/* Unique busid, and index into lguest_page->devices[] */
-	/* By convention, each device can use irq index+1 if it wants to. */
 	unsigned int index;
 
 	struct device dev;
@@ -15,6 +14,18 @@ struct lguest_device {
 	/* Driver can hang data off here. */
 	void *private;
 };
+
+/* By convention, each device can use irq index+1 if it wants to. */
+static inline int lgdev_irq(const struct lguest_device *dev)
+{
+	return dev->index + 1;
+}
+
+/* dma args must not be vmalloced! */
+void lguest_send_dma(unsigned long key, struct lguest_dma *dma);
+int lguest_bind_dma(unsigned long key, struct lguest_dma *dmas,
+		    unsigned int num, u8 irq);
+void lguest_unbind_dma(unsigned long key, struct lguest_dma *dmas);
 
 struct lguest_driver {
 	const char *name;