[Qemu-devel] Re: [PATCH 1/4] Add 'set_link' monitor command

[Qemu-devel] Re: [PATCH 1/4] Add 'set_link' monitor command

[Anthony Liguori]

Mark McLoughlin wrote:
> Add a monitor command to setting a given network device's link status
> to 'up' or 'down'.
>
> Allows simulation of network cable disconnect.
>
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> ---
>  monitor.c |    1 +
>  net.c     |   41 ++++++++++++++++++++++++++++++++++++++++-
>  net.h     |    2 ++
>  3 files changed, 43 insertions(+), 1 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index f142a87..ac74711 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1497,6 +1497,7 @@ static const term_cmd_t term_cmds[] = {
>        "value", "set maximum speed (in bytes) for migrations" },
>      { "balloon", "i", do_balloon,
>        "target", "request VM to change it's memory allocation (in MB)" },
> +    { "set_link", "iis", do_set_link, "vlan_id dev_idx [up|down]" },
>      { NULL, NULL, },
>  };
>   

This command seems reasonable to me but the dev_idx thing is a bit too 
hacky.

> +    for(vc = vlan->first_client; vc != NULL; vc = vc->next)
> +        if (i++ == device_idx)
> +            break;
>   

I think a better way would be to allow a user to specify an id for the 
nic that gets saved early on.  info nics should print the id and a 
default id should be assigned.  I think you could argue either way as to 
whether the ids should be vlan local or global.

I sort of think that making them global has a number of advantages.  You 
could also use a string to identify the nics globally, it's up to you.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 1/4] Add 'set_link' monitor command

[Mark McLoughlin]

On Tue, 2008-12-09 at 09:01 -0600, Anthony Liguori wrote:
> > +    for(vc = vlan->first_client; vc != NULL; vc = vc->next)
> > +        if (i++ == device_idx)
> > +            break;
> >   
> 
> I think a better way would be to allow a user to specify an id for the 
> nic that gets saved early on.  info nics should print the id and a 
> default id should be assigned.  I think you could argue either way as to 
> whether the ids should be vlan local or global.
> 
> I sort of think that making them global has a number of advantages.  You 
> could also use a string to identify the nics globally, it's up to you.

Okay, here's a few patches to add a vlan client name.

I'll repost the set_link patches once we agree on this part.

Cheers,
Mark.

Re: [Qemu-devel] Re: [PATCH 1/4] Add 'set_link' monitor command

[Mark McLoughlin]

Hi Anthony,

On Fri, 2008-12-12 at 14:45 +0000, Mark McLoughlin wrote:
> On Tue, 2008-12-09 at 09:01 -0600, Anthony Liguori wrote:
> > > +    for(vc = vlan->first_client; vc != NULL; vc = vc->next)
> > > +        if (i++ == device_idx)
> > > +            break;
> > >   
> > 
> > I think a better way would be to allow a user to specify an id for the 
> > nic that gets saved early on.  info nics should print the id and a 
> > default id should be assigned.  I think you could argue either way as to 
> > whether the ids should be vlan local or global.
> > 
> > I sort of think that making them global has a number of advantages.  You 
> > could also use a string to identify the nics globally, it's up to you.
> 
> Okay, here's a few patches to add a vlan client name.
> 
> I'll repost the set_link patches once we agree on this part.

Re-posting a re-based version of these patches; also includes a fix for
the build error pointer out by Aurelien Jarno.

Cheers,
Mark.

[Qemu-devel] [PATCH 1/4] Add 'set_link' monitor command

[Mark McLoughlin]

Add a monitor command to setting a given network device's link status
to 'up' or 'down'.

Allows simulation of network cable disconnect.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 monitor.c |    1 +
 net.c     |   31 ++++++++++++++++++++++++++++++-
 net.h     |    2 ++
 3 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 745c335..7ff9890 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1516,6 +1516,7 @@ static const term_cmd_t term_cmds[] = {
       "value", "set maximum speed (in bytes) for migrations" },
     { "balloon", "i", do_balloon,
       "target", "request VM to change it's memory allocation (in MB)" },
+    { "set_link", "ss", do_set_link, "name [up|down]" },
     { NULL, NULL, },
 };
 
diff --git a/net.c b/net.c
index 4609be2..eef0564 100644
--- a/net.c
+++ b/net.c
@@ -386,12 +386,15 @@ void qemu_send_packet(VLANClientState *vc1, const uint8_t *buf, int size)
     VLANState *vlan = vc1->vlan;
     VLANClientState *vc;
 
+    if (vc1->link_down)
+        return;
+
 #ifdef DEBUG_NET
     printf("vlan %d send:\n", vlan->id);
     hex_dump(stdout, buf, size);
 #endif
     for(vc = vlan->first_client; vc != NULL; vc = vc->next) {
-        if (vc != vc1) {
+        if (vc != vc1 && !vc->link_down) {
             vc->fd_read(vc->opaque, buf, size);
         }
     }
@@ -1717,6 +1720,32 @@ void do_info_network(void)
     }
 }
 
+int do_set_link(const char *name, const char *up_or_down)
+{
+    VLANState *vlan;
+    VLANClientState *vc = NULL;
+
+    for (vlan = first_vlan; vlan != NULL; vlan = vlan->next)
+        for (vc = vlan->first_client; vc != NULL; vc = vc->next)
+            if (strcmp(vc->name, name) == 0)
+                break;
+
+    if (!vc) {
+        term_printf("could not find network device '%s'", name);
+        return 0;
+    }
+
+    if (strcmp(up_or_down, "up") == 0)
+        vc->link_down = 0;
+    else if (strcmp(up_or_down, "down") == 0)
+        vc->link_down = 1;
+    else
+        term_printf("invalid link status '%s'; only 'up' or 'down' valid\n",
+                    up_or_down);
+
+    return 1;
+}
+
 void net_cleanup(void)
 {
     VLANState *vlan;
diff --git a/net.h b/net.h
index 19ab649..3c0dab9 100644
--- a/net.h
+++ b/net.h
@@ -15,6 +15,7 @@ struct VLANClientState {
     /* Packets may still be sent if this returns zero.  It's used to
        rate-limit the slirp code.  */
     IOCanRWHandler *fd_can_read;
+    int link_down;
     void *opaque;
     struct VLANClientState *next;
     struct VLANState *vlan;
@@ -49,6 +50,7 @@ void qemu_check_nic_model_list(NICInfo *nd, const char * const *models,
 void qemu_handler_true(void *opaque);
 
 void do_info_network(void);
+int do_set_link(const char *name, const char *up_or_down);
 
 /* NIC info */
 
-- 
1.6.0.6

[Qemu-devel] [PATCH 2/4] Allow devices be notified of link status change

[Mark McLoughlin]

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |    3 +++
 net.h |    3 +++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net.c b/net.c
index eef0564..0a182b2 100644
--- a/net.c
+++ b/net.c
@@ -1743,6 +1743,9 @@ int do_set_link(const char *name, const char *up_or_down)
         term_printf("invalid link status '%s'; only 'up' or 'down' valid\n",
                     up_or_down);
 
+    if (vc->link_status_changed)
+        vc->link_status_changed(vc);
+
     return 1;
 }
 
diff --git a/net.h b/net.h
index 3c0dab9..291807a 100644
--- a/net.h
+++ b/net.h
@@ -9,12 +9,15 @@ typedef ssize_t (IOReadvHandler)(void *, const struct iovec *, int);
 
 typedef struct VLANClientState VLANClientState;
 
+typedef void (LinkStatusChanged)(VLANClientState *);
+
 struct VLANClientState {
     IOReadHandler *fd_read;
     IOReadvHandler *fd_readv;
     /* Packets may still be sent if this returns zero.  It's used to
        rate-limit the slirp code.  */
     IOCanRWHandler *fd_can_read;
+    LinkStatusChanged *link_status_changed;
     int link_down;
     void *opaque;
     struct VLANClientState *next;
-- 
1.6.0.6

[Qemu-devel] [PATCH 3/4] Implement e1000 link status

[Mark McLoughlin]

On link up or down we set the E1000_STATUS_LU ("link up") bit
in the status register and set the E1000_ICR_LSC ("link
status changed") bit in the interrupt cause register before
interrupting the guest.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 hw/e1000.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 09cb96a..ccf9bc0 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -570,6 +570,21 @@ receive_filter(E1000State *s, const uint8_t *buf, int size)
     return 0;
 }
 
+static void
+e1000_set_link_status(VLANClientState *vc)
+{
+    E1000State *s = vc->opaque;
+    uint32_t old_status = s->mac_reg[STATUS];
+
+    if (vc->link_down)
+        s->mac_reg[STATUS] &= ~E1000_STATUS_LU;
+    else
+        s->mac_reg[STATUS] |= E1000_STATUS_LU;
+
+    if (s->mac_reg[STATUS] != old_status)
+        set_ics(s, 0, E1000_ICR_LSC);
+}
+
 static int
 e1000_can_receive(void *opaque)
 {
@@ -1073,6 +1088,7 @@ pci_e1000_init(PCIBus *bus, NICInfo *nd, int devfn)
 
     d->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  e1000_receive, e1000_can_receive, d);
+    d->vc->link_status_changed = e1000_set_link_status;
 
     qemu_format_nic_info_str(d->vc, d->nd->macaddr);
 
-- 
1.6.0.6

[Qemu-devel] [PATCH 4/4] Implement virtio_net link status

[Mark McLoughlin]

Implement the VIRTIO_NET_F_STATUS feature by exposing the link status
through virtio_net_config::status.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 hw/virtio-net.c |   23 +++++++++++++++++++++--
 hw/virtio-net.h |    7 ++++++-
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 43fde20..54c0030 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -20,6 +20,7 @@ typedef struct VirtIONet
 {
     VirtIODevice vdev;
     uint8_t mac[6];
+    uint16_t status;
     VirtQueue *rx_vq;
     VirtQueue *tx_vq;
     VLANClientState *vc;
@@ -42,13 +43,28 @@ static void virtio_net_update_config(VirtIODevice *vdev, uint8_t *config)
     VirtIONet *n = to_virtio_net(vdev);
     struct virtio_net_config netcfg;
 
+    netcfg.status = n->status;
     memcpy(netcfg.mac, n->mac, 6);
     memcpy(config, &netcfg, sizeof(netcfg));
 }
 
+static void virtio_net_set_link_status(VLANClientState *vc)
+{
+    VirtIONet *n = vc->opaque;
+    uint16_t old_status = n->status;
+
+    if (vc->link_down)
+        n->status &= ~VIRTIO_NET_S_LINK_UP;
+    else
+        n->status |= VIRTIO_NET_S_LINK_UP;
+
+    if (n->status != old_status)
+        virtio_notify_config(&n->vdev);
+}
+
 static uint32_t virtio_net_get_features(VirtIODevice *vdev)
 {
-    uint32_t features = (1 << VIRTIO_NET_F_MAC);
+    uint32_t features = (1 << VIRTIO_NET_F_MAC) | (1 << VIRTIO_NET_F_STATUS);
 
     return features;
 }
@@ -307,7 +323,8 @@ void virtio_net_init(PCIBus *bus, NICInfo *nd, int devfn)
     n = (VirtIONet *)virtio_init_pci(bus, "virtio-net", 6900, 0x1000,
                                      0, VIRTIO_ID_NET,
                                      0x02, 0x00, 0x00,
-                                     6, sizeof(VirtIONet));
+                                     sizeof(struct virtio_net_config),
+                                     sizeof(VirtIONet));
     if (!n)
         return;
 
@@ -317,8 +334,10 @@ void virtio_net_init(PCIBus *bus, NICInfo *nd, int devfn)
     n->rx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_rx);
     n->tx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_tx);
     memcpy(n->mac, nd->macaddr, 6);
+    n->status = VIRTIO_NET_S_LINK_UP;
     n->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
                                  virtio_net_receive, virtio_net_can_receive, n);
+    n->vc->link_status_changed = virtio_net_set_link_status;
 
     qemu_format_nic_info_str(n->vc, n->mac);
 
diff --git a/hw/virtio-net.h b/hw/virtio-net.h
index 7446b11..148ec47 100644
--- a/hw/virtio-net.h
+++ b/hw/virtio-net.h
@@ -37,16 +37,21 @@
 #define VIRTIO_NET_F_HOST_ECN   13      /* Host can handle TSO[6] w/ ECN in. */
 #define VIRTIO_NET_F_HOST_UFO   14      /* Host can handle UFO in. */
 #define VIRTIO_NET_F_MRG_RXBUF  15      /* Host can merge receive buffers. */
+#define VIRTIO_NET_F_STATUS     16      /* virtio_net_config.status available */
+
+#define VIRTIO_NET_S_LINK_UP    1       /* Link is up */
 
 #define TX_TIMER_INTERVAL 150000 /* 150 us */
 
 /* Maximum packet size we can receive from tap device: header + 64k */
 #define VIRTIO_NET_MAX_BUFSIZE (sizeof(struct virtio_net_hdr) + (64 << 10))
 
-/* The config defining mac address (6 bytes) */
 struct virtio_net_config
 {
+    /* The config defining mac address (6 bytes) */
     uint8_t mac[6];
+    /* See VIRTIO_NET_F_STATUS and VIRTIO_NET_S_* above */
+    uint16_t status;
 } __attribute__((packed));
 
 /* This is the first element of the scatter-gather list.  If you don't
-- 
1.6.0.6

[Qemu-devel] Re: [PATCH 1/4] Add 'set_link' monitor command

[Anthony Liguori]

Mark McLoughlin wrote:
> Add a monitor command to setting a given network device's link status
> to 'up' or 'down'.
>
> Allows simulation of network cable disconnect.
>
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> ---
>
>   
Applied all four.  Thanks.

Regards,

Anthony Liguori