[Xen-devel] Re: [patch 26/26] Xen-paravirt_ops: Add the Xen virtual network device driver.

[Stephen Hemminger <shemminger@xxxxxxxxxxxxxxxxxxxx>]

On Thu, 01 Mar 2007 15:25:09 -0800
Jeremy Fitzhardinge <jeremy@xxxxxxxx> wrote:

> The network device frontend driver allows the kernel to access network
> devices exported exported by a virtual machine containing a physical
> network device driver.
> 
> Signed-off-by: Ian Pratt <ian.pratt@xxxxxxxxxxxxx>
> Signed-off-by: Christian Limpach <Christian.Limpach@xxxxxxxxxxxx>
> Signed-off-by: Chris Wright <chrisw@xxxxxxxxxxxx>
> Signed-off-by: Jeremy Fitzhardinge <jeremy@xxxxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx
> Cc: Jeff Garzik <jeff@xxxxxxxxxx>
> 
> ---
>  drivers/net/Kconfig        |   12 
>  drivers/net/Makefile       |    2 
>  drivers/net/xen-netfront.c | 2066 
> ++++++++++++++++++++++++++++++++++++++++++++
>  include/xen/events.h       |    2 
>  4 files changed, 2082 insertions(+)
> 
> ===================================================================
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -2525,6 +2525,18 @@ source "drivers/atm/Kconfig"
>  
>  source "drivers/s390/net/Kconfig"
>  
> +config XEN_NETDEV_FRONTEND
> +     tristate "Xen network device frontend driver"
> +     depends on XEN
> +     default y
> +     help
> +       The network device frontend driver allows the kernel to
> +       access network devices exported exported by a virtual
> +       machine containing a physical network device driver. The
> +       frontend driver is intended for unprivileged guest domains;
> +       if you are compiling a kernel for a Xen guest, you almost
> +       certainly want to enable this.
> +
>  config ISERIES_VETH
>       tristate "iSeries Virtual Ethernet driver support"
>       depends on PPC_ISERIES

Might make more sense earlier in list (near other virtual devices).
===================================================================
>
> +/*
> + * Mutually-exclusive module options to select receive data path:
> + *  rx_copy : Packets are copied by network backend into local memory
> + *  rx_flip : Page containing packet data is transferred to our ownership
> + * For fully-virtualised guests there is no option - copying must be used.
> + * For paravirtualised guests, flipping is the default.
> + */
> +#ifdef CONFIG_XEN


Hey I thought this driver depended on CONFIG_XEN already?

> +static int MODPARM_rx_copy = 0;
> +module_param_named(rx_copy, MODPARM_rx_copy, bool, 0);
> +MODULE_PARM_DESC(rx_copy, "Copy packets from network card (rather than 
> flip)");
> +static int MODPARM_rx_flip = 0;
> +module_param_named(rx_flip, MODPARM_rx_flip, bool, 0);
> +MODULE_PARM_DESC(rx_flip, "Flip packets from network card (rather than 
> copy)");
> +#else
> +static const int MODPARM_rx_copy = 1;
> +static const int MODPARM_rx_flip = 0;
> +#endif


No MIXED case variable names please.


Why have two mutually exclusive values instead of just one value
with three states: 0 = normal, 1 = copy, 2 = flip?


> +#define DPRINTK(fmt, args...)                                \
> +     pr_debug("netfront (%s:%d) " fmt,               \
> +              __FUNCTION__, __LINE__, ##args)
> +#define IPRINTK(fmt, args...)                                \
> +     printk(KERN_INFO "netfront: " fmt, ##args)
> +#define WPRINTK(fmt, args...)                                \
> +     printk(KERN_WARNING "netfront: " fmt, ##args)


Could you use dev_dbg, dev_info, dev_warn instead of these macros?

> +
> +/** Send a packet on a net device to encourage switches to learn the
> + * MAC. We send a fake ARP request.
> + *
> + * @param dev device
> + * @return 0 on success, error code otherwise
> + */
Why the sudden urge to use docbook format on one internal function.


> +static int send_fake_arp(struct net_device *dev)
> +{
> +     struct sk_buff *skb;
> +     u32             src_ip, dst_ip;
> +
> +     dst_ip = INADDR_BROADCAST;
> +     src_ip = inet_select_addr(dev, dst_ip, RT_SCOPE_LINK);
> +
> +     /* No IP? Then nothing to do. */
> +     if (src_ip == 0)
> +             return 0;
> +
> +     skb = arp_create(ARPOP_REPLY, ETH_P_ARP,
> +                      dst_ip, dev, src_ip,
> +                      /*dst_hw*/ NULL, /*src_hw*/ NULL,
> +                      /*target_hw*/ dev->dev_addr);
> +     if (skb == NULL)
> +             return -ENOMEM;
> +
> +     return dev_queue_xmit(skb);
> +}

This should probably be done in generic (non driver code).
It creates lots of dependencies here.

> +/*
> + * We use this notifier to send out a fake ARP reply to reset switches and
> + * router ARP caches when an IP interface is brought up on a VIF.
> + */
> +static int
> +inetdev_notify(struct notifier_block *this, unsigned long event, void *ptr)
> +{
> +     struct in_ifaddr  *ifa = (struct in_ifaddr *)ptr;
> +     struct net_device *dev = ifa->ifa_dev->dev;
> +
> +     /* UP event and is it one of our devices? */
> +     if (event == NETDEV_UP && dev->open == network_open)
> +             (void)send_fake_arp(dev);
> +
> +     return NOTIFY_DONE;
> +}

Shouldn't just be a global kernel option for gratuitous ARP.
Doesn't seem to be unique to this driver.
With sysctl to enable it.

-- 
Stephen Hemminger <shemminger@xxxxxxxxxxxxxxxxxxxx>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel