[PATCH 0/4] Update QEMU qnic driver to "new" XenDevice model

[PATCH 0/4] Update QEMU qnic driver to "new" XenDevice model

[David Woodhouse]

This has been on my TODO list for a while, and Paul's since 2019. Having 
converted the console driver just to get PV guests booting, I figured I 
should do this one while I still remember how.

The fact that net_cleanup() frees my NIC from underneath me confused
me for a while. Not entirely sure what's going on there. Other devices
seem to survive just because they aren't cleaned up at exit. But XenBus
devices really should be properly cleaned up on exit, because in some
cases they leave detritus in XenStore, which outlives QEMU. So "Don't
Do That Then" doesn't seem like it's the answer.

The default NIC handling is horrid (I mean, before I even looked at it)
but that isn't today's yak to shave...

David Woodhouse (4):
      hw/xen: only remove peers of PCI NICs on unplug
      hw/xen: update Xen PV NIC to XenDevice model
      [WTF] avoid qemu_del_nic() in xen_netdev_unrealize() on shutdown
      hw/i386/pc: support '-nic' for xen-net-device

 hw/i386/pc.c               |  11 ++-
 hw/i386/pc_piix.c          |   2 +-
 hw/i386/pc_q35.c           |   2 +-
 hw/i386/xen/xen_platform.c |   9 ++-
 hw/net/meson.build         |   2 +-
 hw/net/trace-events        |   9 +++
 hw/net/xen_nic.c           | 434 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------
 hw/xen/xen-bus.c           |   4 +-
 hw/xenpv/xen_machine_pv.c  |   1 -
 include/hw/i386/pc.h       |   4 +-
 include/hw/xen/xen-bus.h   |   2 +-
 11 files changed, 373 insertions(+), 107 deletions(-)

[PATCH 1/4] hw/xen: only remove peers of PCI NICs on unplug

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

When the Xen guest asks to unplug *emulated* NICs, it's kind of unhelpful
also to unplug the peer of the *Xen* PV NIC.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/i386/xen/xen_platform.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index 17457ff3de..e2dd1b536a 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -140,9 +140,14 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
 /* Remove the peer of the NIC device. Normally, this would be a tap device. */
 static void del_nic_peer(NICState *nic, void *opaque)
 {
-    NetClientState *nc;
+    NetClientState *nc = qemu_get_queue(nic);
+    ObjectClass *klass = module_object_class_by_name(nc->model);
+
+    /* Only delete peers of PCI NICs that we're about to delete */
+    if (!klass || !object_class_dynamic_cast(klass, TYPE_PCI_DEVICE)) {
+        return;
+    }
 
-    nc = qemu_get_queue(nic);
     if (nc->peer)
         qemu_del_net_client(nc->peer);
 }
-- 
2.40.1

[PATCH 2/4] hw/xen: update Xen PV NIC to XenDevice model

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/net/meson.build        |   2 +-
 hw/net/trace-events       |   9 +
 hw/net/xen_nic.c          | 426 +++++++++++++++++++++++++++++---------
 hw/xenpv/xen_machine_pv.c |   1 -
 4 files changed, 342 insertions(+), 96 deletions(-)

diff --git a/hw/net/meson.build b/hw/net/meson.build
index 2632634df3..f64651c467 100644
--- a/hw/net/meson.build
+++ b/hw/net/meson.build
@@ -1,5 +1,5 @@
 system_ss.add(when: 'CONFIG_DP8393X', if_true: files('dp8393x.c'))
-system_ss.add(when: 'CONFIG_XEN', if_true: files('xen_nic.c'))
+system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen_nic.c'))
 system_ss.add(when: 'CONFIG_NE2000_COMMON', if_true: files('ne2000.c'))
 
 # PCI network cards
diff --git a/hw/net/trace-events b/hw/net/trace-events
index 3abfd65e5b..ee56acfbce 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -482,3 +482,12 @@ dp8393x_receive_oversize(int size) "oversize packet, pkt_size is %d"
 dp8393x_receive_not_netcard(void) "packet not for netcard"
 dp8393x_receive_packet(int crba) "Receive packet at 0x%"PRIx32
 dp8393x_receive_write_status(int crba) "Write status at 0x%"PRIx32
+
+# xen_nic.c
+xen_netdev_realize(int idx, const char *info) "idx %u info '%s'"
+xen_netdev_unrealize(int idx) "idx %u"
+xen_netdev_create(int idx) "idx %u"
+xen_netdev_destroy(int idx) "idx %u"
+xen_netdev_disconnect(int idx) "idx %u"
+xen_netdev_connect(int idx, unsigned int tx, unsigned int rx, int port) "idx %u tx %u rx %u port %u"
+xen_netdev_frontend_changed(const char *idx, int state) "idx %s state %d"
diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 9bbf6599fc..84914c329c 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -20,6 +20,11 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
+#include "qemu/qemu-print.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/error.h"
+
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #include <sys/wait.h>
@@ -27,18 +32,26 @@
 #include "net/net.h"
 #include "net/checksum.h"
 #include "net/util.h"
-#include "hw/xen/xen-legacy-backend.h"
+
+#include "hw/xen/xen-backend.h"
+#include "hw/xen/xen-bus-helper.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 
 #include "hw/xen/interface/io/netif.h"
+#include "hw/xen/interface/io/xs_wire.h"
+
+#include "trace.h"
 
 /* ------------------------------------------------------------- */
 
 struct XenNetDev {
-    struct XenLegacyDevice      xendev;  /* must be first */
-    char                  *mac;
+    struct XenDevice      xendev;  /* must be first */
+    XenEventChannel       *event_channel;
+    int                   dev;
     int                   tx_work;
-    int                   tx_ring_ref;
-    int                   rx_ring_ref;
+    unsigned int          tx_ring_ref;
+    unsigned int          rx_ring_ref;
     struct netif_tx_sring *txs;
     struct netif_rx_sring *rxs;
     netif_tx_back_ring_t  tx_ring;
@@ -47,6 +60,13 @@ struct XenNetDev {
     NICState              *nic;
 };
 
+typedef struct XenNetDev XenNetDev;
+
+#define TYPE_XEN_NET_DEVICE "xen-net-device"
+OBJECT_DECLARE_SIMPLE_TYPE(XenNetDev, XEN_NET_DEVICE)
+
+#define xen_pv_printf(a, n, ...) qemu_printf(__VA_ARGS__)
+
 /* ------------------------------------------------------------- */
 
 static void net_tx_response(struct XenNetDev *netdev, netif_tx_request_t *txp, int8_t st)
@@ -68,7 +88,8 @@ static void net_tx_response(struct XenNetDev *netdev, netif_tx_request_t *txp, i
     netdev->tx_ring.rsp_prod_pvt = ++i;
     RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&netdev->tx_ring, notify);
     if (notify) {
-        xen_pv_send_notify(&netdev->xendev);
+        xen_device_notify_event_channel(XEN_DEVICE(netdev),
+                                        netdev->event_channel, NULL);
     }
 
     if (i == netdev->tx_ring.req_cons) {
@@ -104,8 +125,9 @@ static void net_tx_error(struct XenNetDev *netdev, netif_tx_request_t *txp, RING
 #endif
 }
 
-static void net_tx_packets(struct XenNetDev *netdev)
+static bool net_tx_packets(struct XenNetDev *netdev)
 {
+    bool done_something = false;
     netif_tx_request_t txreq;
     RING_IDX rc, rp;
     void *page;
@@ -122,6 +144,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
             }
             memcpy(&txreq, RING_GET_REQUEST(&netdev->tx_ring, rc), sizeof(txreq));
             netdev->tx_ring.req_cons = ++rc;
+            done_something = true;
 
 #if 1
             /* should not happen in theory, we don't announce the *
@@ -151,7 +174,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
                 continue;
             }
 
-            xen_pv_printf(&netdev->xendev, 3,
+            if (0) qemu_printf(//&netdev->xendev, 3,
                           "tx packet ref %d, off %d, len %d, flags 0x%x%s%s%s%s\n",
                           txreq.gref, txreq.offset, txreq.size, txreq.flags,
                           (txreq.flags & NETTXF_csum_blank)     ? " csum_blank"     : "",
@@ -159,8 +182,8 @@ static void net_tx_packets(struct XenNetDev *netdev)
                           (txreq.flags & NETTXF_more_data)      ? " more_data"      : "",
                           (txreq.flags & NETTXF_extra_info)     ? " extra_info"     : "");
 
-            page = xen_be_map_grant_ref(&netdev->xendev, txreq.gref,
-                                        PROT_READ);
+            page = xen_device_map_grant_refs(&netdev->xendev, &txreq.gref, 1,
+                                             PROT_READ, NULL);
             if (page == NULL) {
                 xen_pv_printf(&netdev->xendev, 0,
                               "error: tx gref dereference failed (%d)\n",
@@ -181,7 +204,8 @@ static void net_tx_packets(struct XenNetDev *netdev)
                 qemu_send_packet(qemu_get_queue(netdev->nic),
                                  page + txreq.offset, txreq.size);
             }
-            xen_be_unmap_grant_ref(&netdev->xendev, page, txreq.gref);
+            xen_device_unmap_grant_refs(&netdev->xendev, page, &txreq.gref, 1,
+                                        NULL);
             net_tx_response(netdev, &txreq, NETIF_RSP_OKAY);
         }
         if (!netdev->tx_work) {
@@ -190,6 +214,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
         netdev->tx_work = 0;
     }
     g_free(tmpbuf);
+    return done_something;
 }
 
 /* ------------------------------------------------------------- */
@@ -212,14 +237,15 @@ static void net_rx_response(struct XenNetDev *netdev,
         resp->status = (int16_t)st;
     }
 
-    xen_pv_printf(&netdev->xendev, 3,
+    if (0) qemu_printf(//&netdev->xendev, 3,
                   "rx response: idx %d, status %d, flags 0x%x\n",
                   i, resp->status, resp->flags);
 
     netdev->rx_ring.rsp_prod_pvt = ++i;
     RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&netdev->rx_ring, notify);
     if (notify) {
-        xen_pv_send_notify(&netdev->xendev);
+        xen_device_notify_event_channel(XEN_DEVICE(netdev),
+                                        netdev->event_channel, NULL);
     }
 }
 
@@ -232,7 +258,7 @@ static ssize_t net_rx_packet(NetClientState *nc, const uint8_t *buf, size_t size
     RING_IDX rc, rp;
     void *page;
 
-    if (netdev->xendev.be_state != XenbusStateConnected) {
+    if (netdev->rx_ring.sring == NULL) {
         return -1;
     }
 
@@ -252,7 +278,8 @@ static ssize_t net_rx_packet(NetClientState *nc, const uint8_t *buf, size_t size
     memcpy(&rxreq, RING_GET_REQUEST(&netdev->rx_ring, rc), sizeof(rxreq));
     netdev->rx_ring.req_cons = ++rc;
 
-    page = xen_be_map_grant_ref(&netdev->xendev, rxreq.gref, PROT_WRITE);
+    page = xen_device_map_grant_refs(&netdev->xendev, &rxreq.gref, 1,
+                                     PROT_WRITE, NULL);
     if (page == NULL) {
         xen_pv_printf(&netdev->xendev, 0,
                       "error: rx gref dereference failed (%d)\n",
@@ -261,7 +288,7 @@ static ssize_t net_rx_packet(NetClientState *nc, const uint8_t *buf, size_t size
         return -1;
     }
     memcpy(page + NET_IP_ALIGN, buf, size);
-    xen_be_unmap_grant_ref(&netdev->xendev, page, rxreq.gref);
+    xen_device_unmap_grant_refs(&netdev->xendev, page, &rxreq.gref, 1, NULL);
     net_rx_response(netdev, &rxreq, NETIF_RSP_OKAY, NET_IP_ALIGN, size, 0);
 
     return size;
@@ -275,139 +302,350 @@ static NetClientInfo net_xen_info = {
     .receive = net_rx_packet,
 };
 
-static int net_init(struct XenLegacyDevice *xendev)
+static void xen_netdev_realize(XenDevice *xendev, Error **errp)
 {
-    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, xendev);
+    ERRP_GUARD();
+    XenNetDev *netdev = XEN_NET_DEVICE(xendev);
 
-    /* read xenstore entries */
-    if (netdev->mac == NULL) {
-        netdev->mac = xenstore_read_be_str(&netdev->xendev, "mac");
-    }
+    qemu_macaddr_default_if_unset(&netdev->conf.macaddr);
 
-    /* do we have all we need? */
-    if (netdev->mac == NULL) {
-        return -1;
-    }
-
-    if (net_parse_macaddr(netdev->conf.macaddr.a, netdev->mac) < 0) {
-        return -1;
-    }
+    xen_device_frontend_printf(xendev, "mac", "%02x:%02x:%02x:%02x:%02x:%02x",
+                               netdev->conf.macaddr.a[0],
+                               netdev->conf.macaddr.a[1],
+                               netdev->conf.macaddr.a[2],
+                               netdev->conf.macaddr.a[3],
+                               netdev->conf.macaddr.a[4],
+                               netdev->conf.macaddr.a[5]);
 
     netdev->nic = qemu_new_nic(&net_xen_info, &netdev->conf,
-                               "xen", NULL, netdev);
+                               object_get_typename(OBJECT(xendev)),
+                               DEVICE(xendev)->id, netdev);
 
-    qemu_set_info_str(qemu_get_queue(netdev->nic),
-                      "nic: xenbus vif macaddr=%s", netdev->mac);
+    qemu_format_nic_info_str(qemu_get_queue(netdev->nic), netdev->conf.macaddr.a);
 
     /* fill info */
-    xenstore_write_be_int(&netdev->xendev, "feature-rx-copy", 1);
-    xenstore_write_be_int(&netdev->xendev, "feature-rx-flip", 0);
+    xen_device_backend_printf(xendev, "feature-rx-copy", "%u", 1);
+    xen_device_backend_printf(xendev, "feature-rx-flip", "%u", 0);
 
-    return 0;
+    trace_xen_netdev_realize(netdev->dev, qemu_get_queue(netdev->nic)->info_str);
 }
 
-static int net_connect(struct XenLegacyDevice *xendev)
+static bool net_event(void *_xendev)
 {
-    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, xendev);
-    int rx_copy;
+    XenNetDev *netdev = XEN_NET_DEVICE(_xendev);
+    bool done_something;
 
-    if (xenstore_read_fe_int(&netdev->xendev, "tx-ring-ref",
-                             &netdev->tx_ring_ref) == -1) {
-        return -1;
+    done_something = net_tx_packets(netdev);
+    qemu_flush_queued_packets(qemu_get_queue(netdev->nic));
+    return done_something;
+}
+
+static bool xen_netdev_connect(XenDevice *xendev, Error **errp)
+{
+    XenNetDev *netdev = XEN_NET_DEVICE(xendev);
+    unsigned int port, rx_copy;
+
+    if (xen_device_frontend_scanf(xendev, "tx-ring-ref", "%u",
+                                  &netdev->tx_ring_ref) != 1) {
+        error_setg(errp, "failed to read tx-ring-ref");
+        return false;
     }
-    if (xenstore_read_fe_int(&netdev->xendev, "rx-ring-ref",
-                             &netdev->rx_ring_ref) == -1) {
-        return 1;
+
+    if (xen_device_frontend_scanf(xendev, "rx-ring-ref", "%u",
+                                  &netdev->rx_ring_ref) != 1) {
+        error_setg(errp, "failed to read rx-ring-ref");
+        return false;
     }
-    if (xenstore_read_fe_int(&netdev->xendev, "event-channel",
-                             &netdev->xendev.remote_port) == -1) {
-        return -1;
+
+    if (xen_device_frontend_scanf(xendev, "event-channel", "%u",
+                                  &port) != 1) {
+        error_setg(errp, "failed to read event-channel");
+        return false;
     }
 
-    if (xenstore_read_fe_int(&netdev->xendev, "request-rx-copy", &rx_copy) == -1) {
+    if (xen_device_frontend_scanf(xendev, "request-rx-copy", "%u",
+                                  &rx_copy) != 1) {
         rx_copy = 0;
     }
     if (rx_copy == 0) {
-        xen_pv_printf(&netdev->xendev, 0,
-                      "frontend doesn't support rx-copy.\n");
-        return -1;
+        error_setg(errp, "frontend doesn't support rx-copy");
+        return false;
     }
 
-    netdev->txs = xen_be_map_grant_ref(&netdev->xendev,
-                                       netdev->tx_ring_ref,
-                                       PROT_READ | PROT_WRITE);
+    netdev->txs = xen_device_map_grant_refs(xendev,
+                                            &netdev->tx_ring_ref, 1,
+                                            PROT_READ | PROT_WRITE,
+                                            errp);
     if (!netdev->txs) {
-        return -1;
+        error_prepend(errp, "failed to map tx grant ref: ");
+        return false;
     }
-    netdev->rxs = xen_be_map_grant_ref(&netdev->xendev,
-                                       netdev->rx_ring_ref,
-                                       PROT_READ | PROT_WRITE);
+
+    netdev->rxs = xen_device_map_grant_refs(xendev,
+                                            &netdev->rx_ring_ref, 1,
+                                            PROT_READ | PROT_WRITE,
+                                            errp);
     if (!netdev->rxs) {
-        xen_be_unmap_grant_ref(&netdev->xendev, netdev->txs,
-                               netdev->tx_ring_ref);
-        netdev->txs = NULL;
-        return -1;
+        error_prepend(errp, "failed to map rx grant ref: ");
+        return false;
     }
+
     BACK_RING_INIT(&netdev->tx_ring, netdev->txs, XEN_PAGE_SIZE);
     BACK_RING_INIT(&netdev->rx_ring, netdev->rxs, XEN_PAGE_SIZE);
 
-    xen_be_bind_evtchn(&netdev->xendev);
+    netdev->event_channel = xen_device_bind_event_channel(xendev, port,
+                                                          net_event,
+                                                          netdev,
+                                                          errp);
+    if (!netdev->event_channel) {
+        return false;
+    }
 
-    xen_pv_printf(&netdev->xendev, 1, "ok: tx-ring-ref %d, rx-ring-ref %d, "
-                  "remote port %d, local port %d\n",
-                  netdev->tx_ring_ref, netdev->rx_ring_ref,
-                  netdev->xendev.remote_port, netdev->xendev.local_port);
+    trace_xen_netdev_connect(netdev->dev, netdev->tx_ring_ref,
+                             netdev->rx_ring_ref, port);
 
     net_tx_packets(netdev);
-    return 0;
+    return true;
 }
 
-static void net_disconnect(struct XenLegacyDevice *xendev)
+static void xen_netdev_disconnect(XenDevice *xendev, Error **errp)
 {
-    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, xendev);
+    XenNetDev *netdev = XEN_NET_DEVICE(xendev);
+
+    trace_xen_netdev_disconnect(netdev->dev);
 
-    xen_pv_unbind_evtchn(&netdev->xendev);
+    netdev->tx_ring.sring = NULL;
+    netdev->rx_ring.sring = NULL;
 
+    if (netdev->event_channel) {
+        xen_device_unbind_event_channel(xendev, netdev->event_channel,
+                                        errp);
+        netdev->event_channel = NULL;
+    }
     if (netdev->txs) {
-        xen_be_unmap_grant_ref(&netdev->xendev, netdev->txs,
-                               netdev->tx_ring_ref);
+        xen_device_unmap_grant_refs(xendev, netdev->txs,
+                                    &netdev->tx_ring_ref, 1, errp);
         netdev->txs = NULL;
     }
     if (netdev->rxs) {
-        xen_be_unmap_grant_ref(&netdev->xendev, netdev->rxs,
-                               netdev->rx_ring_ref);
+        xen_device_unmap_grant_refs(xendev, netdev->rxs,
+                                    &netdev->rx_ring_ref, 1, errp);
         netdev->rxs = NULL;
     }
 }
 
-static void net_event(struct XenLegacyDevice *xendev)
+/* -------------------------------------------------------------------- */
+
+
+static void xen_netdev_frontend_changed(XenDevice *xendev,
+                                       enum xenbus_state frontend_state,
+                                       Error **errp)
 {
-    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, xendev);
-    net_tx_packets(netdev);
-    qemu_flush_queued_packets(qemu_get_queue(netdev->nic));
+    ERRP_GUARD();
+    enum xenbus_state backend_state = xen_device_backend_get_state(xendev);
+
+    trace_xen_netdev_frontend_changed(xendev->name, frontend_state);
+
+    switch (frontend_state) {
+    case XenbusStateInitialised:
+    case XenbusStateConnected:
+        if (backend_state == XenbusStateConnected) {
+            break;
+        }
+
+        xen_netdev_disconnect(xendev, errp);
+        if (*errp) {
+            break;
+        }
+
+        if (!xen_netdev_connect(xendev, errp)) {
+            xen_netdev_disconnect(xendev, NULL);
+            xen_device_backend_set_state(xendev, XenbusStateClosing);
+            break;
+        }
+
+        xen_device_backend_set_state(xendev, XenbusStateConnected);
+        break;
+
+    case XenbusStateClosing:
+        xen_device_backend_set_state(xendev, XenbusStateClosing);
+        break;
+
+    case XenbusStateClosed:
+    case XenbusStateUnknown:
+        xen_netdev_disconnect(xendev, errp);
+        if (*errp) {
+            break;
+        }
+
+        xen_device_backend_set_state(xendev, XenbusStateClosed);
+        break;
+
+    default:
+        break;
+    }
 }
 
-static int net_free(struct XenLegacyDevice *xendev)
+static char *xen_netdev_get_name(XenDevice *xendev, Error **errp)
 {
-    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, xendev);
+    XenNetDev *netdev = XEN_NET_DEVICE(xendev);
+
+    if (netdev->dev == -1) {
+        XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+        char fe_path[XENSTORE_ABS_PATH_MAX + 1];
+        int idx = (xen_mode == XEN_EMULATE) ? 0 : 1;
+        char *value;
+
+        /* Theoretically we could go up to INT_MAX here but that's overkill */
+        while (idx < 100) {
+            snprintf(fe_path, sizeof(fe_path),
+                     "/local/domain/%u/device/vif/%u",
+                     xendev->frontend_id, idx);
+            value = qemu_xen_xs_read(xenbus->xsh, XBT_NULL, fe_path, NULL);
+            if (!value) {
+                if (errno == ENOENT) {
+                    netdev->dev = idx;
+                    goto found;
+                }
+                error_setg(errp, "cannot read %s: %s", fe_path,
+                           strerror(errno));
+                return NULL;
+            }
+            free(value);
+            idx++;
+        }
+        error_setg(errp, "cannot find device index for netdev device");
+        return NULL;
+    }
+ found:
+    return g_strdup_printf("%u", netdev->dev);
+}
+
+static void xen_netdev_unrealize(XenDevice *xendev)
+{
+    XenNetDev *netdev = XEN_NET_DEVICE(xendev);
+
+    trace_xen_netdev_unrealize(netdev->dev);
+
+    /* Disconnect from the frontend in case this has not already happened */
+    xen_netdev_disconnect(xendev, NULL);
 
     if (netdev->nic) {
         qemu_del_nic(netdev->nic);
-        netdev->nic = NULL;
     }
-    g_free(netdev->mac);
-    netdev->mac = NULL;
-    return 0;
 }
 
 /* ------------------------------------------------------------- */
 
-struct XenDevOps xen_netdev_ops = {
-    .size       = sizeof(struct XenNetDev),
-    .flags      = DEVOPS_FLAG_NEED_GNTDEV,
-    .init       = net_init,
-    .initialise    = net_connect,
-    .event      = net_event,
-    .disconnect = net_disconnect,
-    .free       = net_free,
+static Property xen_netdev_properties[] = {
+    DEFINE_NIC_PROPERTIES(XenNetDev, conf),
+    DEFINE_PROP_INT32("idx", XenNetDev, dev, -1),
+    DEFINE_PROP_END_OF_LIST(),
 };
+
+static void xen_netdev_class_init(ObjectClass *class, void *data)
+{
+    DeviceClass *dev_class = DEVICE_CLASS(class);
+    XenDeviceClass *xendev_class = XEN_DEVICE_CLASS(class);
+
+    xendev_class->backend = "qnet";
+    xendev_class->device = "vif";
+    xendev_class->get_name = xen_netdev_get_name;
+    xendev_class->realize = xen_netdev_realize;
+    xendev_class->frontend_changed = xen_netdev_frontend_changed;
+    xendev_class->unrealize = xen_netdev_unrealize;
+    set_bit(DEVICE_CATEGORY_NETWORK, dev_class->categories);
+    dev_class->user_creatable = true;
+
+    device_class_set_props(dev_class, xen_netdev_properties);
+}
+
+static const TypeInfo xen_net_type_info = {
+    .name = TYPE_XEN_NET_DEVICE,
+    .parent = TYPE_XEN_DEVICE,
+    .instance_size = sizeof(XenNetDev),
+    .class_init = xen_netdev_class_init,
+};
+
+static void xen_net_register_types(void)
+{
+    type_register_static(&xen_net_type_info);
+}
+
+type_init(xen_net_register_types)
+
+/* Called to instantiate a XenNetDev when the backend is detected. */
+static void xen_net_device_create(XenBackendInstance *backend,
+                                  QDict *opts, Error **errp)
+{
+    ERRP_GUARD();
+    XenBus *xenbus = xen_backend_get_bus(backend);
+    const char *name = xen_backend_get_name(backend);
+    XenDevice *xendev = NULL;
+    unsigned long number;
+    const char *macstr;
+    XenNetDev *net;
+    MACAddr mac;
+
+    if (qemu_strtoul(name, NULL, 10, &number) || number >= INT_MAX) {
+        error_setg(errp, "failed to parse name '%s'", name);
+        goto fail;
+    }
+
+    trace_xen_netdev_create(number);
+
+    macstr = qdict_get_try_str(opts, "mac");
+    if (macstr == NULL) {
+        error_setg(errp, "no MAC address found");
+        goto fail;
+    }
+
+    if (net_parse_macaddr(mac.a, macstr) < 0) {
+        error_setg(errp, "failed to parse MAC address");
+        goto fail;
+    }
+
+    xendev = XEN_DEVICE(qdev_new(TYPE_XEN_NET_DEVICE));
+    net = XEN_NET_DEVICE(xendev);
+
+    net->dev = number;
+    memcpy(&net->conf.macaddr, &mac, sizeof(mac));
+
+    if (qdev_realize_and_unref(DEVICE(xendev), BUS(xenbus), errp)) {
+        xen_backend_set_device(backend, xendev);
+        return;
+    }
+
+    error_prepend(errp, "realization of net device %lu failed: ",
+                  number);
+
+ fail:
+    if (xendev) {
+        object_unparent(OBJECT(xendev));
+    }
+}
+
+static void xen_net_device_destroy(XenBackendInstance *backend,
+                                       Error **errp)
+{
+    ERRP_GUARD();
+    XenDevice *xendev = xen_backend_get_device(backend);
+    XenNetDev *netdev = XEN_NET_DEVICE(xendev);
+
+    trace_xen_netdev_destroy(netdev->dev);
+
+    object_unparent(OBJECT(xendev));
+}
+
+static const XenBackendInfo xen_net_backend_info  = {
+    .type = "qnet",
+    .create = xen_net_device_create,
+    .destroy = xen_net_device_destroy,
+};
+
+static void xen_net_register_backend(void)
+{
+    xen_backend_register(&xen_net_backend_info);
+}
+
+xen_backend_init(xen_net_register_backend);
diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
index 17cda5ec13..764ca5c4fa 100644
--- a/hw/xenpv/xen_machine_pv.c
+++ b/hw/xenpv/xen_machine_pv.c
@@ -55,7 +55,6 @@ static void xen_init_pv(MachineState *machine)
     }
 
     xen_be_register("vfb", &xen_framebuffer_ops);
-    xen_be_register("qnic", &xen_netdev_ops);
 
     /* configure framebuffer */
     if (vga_interface_type == VGA_XENFB) {
-- 
2.40.1

[PATCH 3/4] [WTF] avoid qemu_del_nic() in xen_netdev_unrealize() on shutdown

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

When QEMU is exiting, qemu_cleanup() calls net_cleanup(), which deletes
the NIC from underneath the xen-net-device. When xen_netdev_unrealize()
is later called via the xenbus exit notifier, it crashes.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/net/xen_nic.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 84914c329c..8d25fb3101 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -25,6 +25,8 @@
 #include "qapi/qmp/qdict.h"
 #include "qapi/error.h"
 
+#include "sysemu/runstate.h"
+
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #include <sys/wait.h>
@@ -530,7 +532,11 @@ static void xen_netdev_unrealize(XenDevice *xendev)
     /* Disconnect from the frontend in case this has not already happened */
     xen_netdev_disconnect(xendev, NULL);
 
-    if (netdev->nic) {
+    /*
+     * WTF? In RUN_STATE_SHUTDOWN, qemu_cleanup()→net_cleanup() already deleted
+     * our NIC from underneath us!
+     */
+    if (netdev->nic && !runstate_check(RUN_STATE_SHUTDOWN)) {
         qemu_del_nic(netdev->nic);
     }
 }
-- 
2.40.1

[PATCH 4/4] hw/i386/pc: support '-nic' for xen-net-device

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

The default NIC creation seems a bit hackish to me. I don't understand
why each platform ha to call pci_nic_init_nofail() from a point in the
code where it actually has a pointer to the PCI bus, and then we have
the special cases for things like ne2k_isa.

If qmp_device_add() can *find* the appropriate bus and instantiate
the device on it, why can't we just do that from generic code for
creating the default NICs too?

But that isn't a yak I want to shave today. Add a xenbus field to the
PCMachineState so that it can make its way from pc_basic_device_init()
to pc_nic_init() and be handled as a special case like ne2k_isa is.

Now we can launch emulated Xen guests with '-nic user'.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/i386/pc.c             | 11 ++++++++---
 hw/i386/pc_piix.c        |  2 +-
 hw/i386/pc_q35.c         |  2 +-
 hw/xen/xen-bus.c         |  4 +++-
 include/hw/i386/pc.h     |  4 +++-
 include/hw/xen/xen-bus.h |  2 +-
 6 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bb3854d1d0..7413ca50c8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1265,7 +1265,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
         if (pcms->bus) {
             pci_create_simple(pcms->bus, -1, "xen-platform");
         }
-        xen_bus_init();
+        pcms->xenbus = xen_bus_init();
         xen_be_init();
     }
 #endif
@@ -1291,7 +1291,8 @@ void pc_basic_device_init(struct PCMachineState *pcms,
                     pcms->vmport != ON_OFF_AUTO_ON);
 }
 
-void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
+void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus,
+                 BusState *xen_bus)
 {
     MachineClass *mc = MACHINE_CLASS(pcmc);
     int i;
@@ -1301,7 +1302,11 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
         NICInfo *nd = &nd_table[i];
         const char *model = nd->model ? nd->model : mc->default_nic;
 
-        if (g_str_equal(model, "ne2k_isa")) {
+        if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-device"))) {
+            DeviceState *dev = qdev_new("xen-net-device");
+            qdev_set_nic_properties(dev, nd);
+            qdev_realize_and_unref(dev, xen_bus, &error_fatal);
+        } else if (g_str_equal(model, "ne2k_isa")) {
             pc_init_ne2k_isa(isa_bus, nd);
         } else {
             pci_nic_init_nofail(nd, pci_bus, model, NULL);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e36a3262b2..90b5ae7258 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -322,7 +322,7 @@ static void pc_init1(MachineState *machine,
     pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, true,
                          0x4);
 
-    pc_nic_init(pcmc, isa_bus, pci_bus);
+    pc_nic_init(pcmc, isa_bus, pci_bus, pcms->xenbus);
 
     if (pcmc->pci_enabled) {
         PCIDevice *dev;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a7386f2ca2..2ed0aab2a7 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -340,7 +340,7 @@ static void pc_q35_init(MachineState *machine)
 
     /* the rest devices to which pci devfn is automatically assigned */
     pc_vga_init(isa_bus, host_bus);
-    pc_nic_init(pcmc, isa_bus, host_bus);
+    pc_nic_init(pcmc, isa_bus, host_bus, pcms->xenbus);
 
     if (machine->nvdimms_state->is_enabled) {
         nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 0da2aa219a..d7823964f8 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -1127,11 +1127,13 @@ static void xen_register_types(void)
 
 type_init(xen_register_types)
 
-void xen_bus_init(void)
+BusState *xen_bus_init(void)
 {
     DeviceState *dev = qdev_new(TYPE_XEN_BRIDGE);
     BusState *bus = qbus_new(TYPE_XEN_BUS, dev, NULL);
 
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     qbus_set_bus_hotplug_handler(bus);
+
+    return bus;
 }
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index bec38cb92c..feabf3d195 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -33,6 +33,7 @@ typedef struct PCMachineState {
 
     /* Pointers to devices and objects: */
     PCIBus *bus;
+    BusState *xenbus;
     I2CBus *smbus;
     PFlashCFI01 *flash[2];
     ISADevice *pcspk;
@@ -182,7 +183,8 @@ void pc_basic_device_init(struct PCMachineState *pcms,
 void pc_cmos_init(PCMachineState *pcms,
                   BusState *ide0, BusState *ide1,
                   ISADevice *s);
-void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus);
+void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus,
+                 BusState *xen_bus);
 
 void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs);
 
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index eb440880b5..acad871b80 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -75,7 +75,7 @@ struct XenBusClass {
 OBJECT_DECLARE_TYPE(XenBus, XenBusClass,
                     XEN_BUS)
 
-void xen_bus_init(void);
+BusState *xen_bus_init(void);
 
 void xen_device_backend_set_state(XenDevice *xendev,
                                   enum xenbus_state state);
-- 
2.40.1

Re: [PATCH 3/4] [WTF] avoid qemu_del_nic() in xen_netdev_unrealize() on shutdown

[David Woodhouse]

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

On Tue, 2023-10-17 at 19:25 +0100, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> When QEMU is exiting, qemu_cleanup() calls net_cleanup(), which deletes
> the NIC from underneath the xen-net-device. When xen_netdev_unrealize()
> is later called via the xenbus exit notifier, it crashes.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  hw/net/xen_nic.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
> index 84914c329c..8d25fb3101 100644
> --- a/hw/net/xen_nic.c
> +++ b/hw/net/xen_nic.c
> @@ -25,6 +25,8 @@
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/error.h"
>  
> +#include "sysemu/runstate.h"
> +
>  #include <sys/socket.h>
>  #include <sys/ioctl.h>
>  #include <sys/wait.h>
> @@ -530,7 +532,11 @@ static void xen_netdev_unrealize(XenDevice *xendev)
>      /* Disconnect from the frontend in case this has not already happened */
>      xen_netdev_disconnect(xendev, NULL);
>  
> -    if (netdev->nic) {
> +    /*
> +     * WTF? In RUN_STATE_SHUTDOWN, qemu_cleanup()→net_cleanup() already deleted
> +     * our NIC from underneath us!
> +     */
> +    if (netdev->nic && !runstate_check(RUN_STATE_SHUTDOWN)) {
>          qemu_del_nic(netdev->nic);
>      }
>  }

I wonder if this is the better answer? There's no point deleting the
*NICs*, is there? It's the other net clients we really want to clean
up?

--- a/net/net.c
+++ b/net/net.c
@@ -1499,18 +1499,22 @@ static void net_vm_change_state_handler(void *opaque, bool running,
 
 void net_cleanup(void)
 {
-    NetClientState *nc;
+    NetClientState *nc, **p = &net_clients.tqh_first;
 
     /*cleanup colo compare module for COLO*/
     colo_compare_cleanup();
 
-    /* We may del multiple entries during qemu_del_net_client(),
-     * so QTAILQ_FOREACH_SAFE() is also not safe here.
+    /*
+     * We may del multiple entries during qemu_del_net_client(), so
+     * QTAILQ_FOREACH_SAFE() is not safe here. The only safe pointer
+     * to keep is a NET_CLIENT_DRIVER_NIC entry, as we don't want
+     * to delete those (we'd upset the devices which own them, if we
+     * did).
      */
-    while (!QTAILQ_EMPTY(&net_clients)) {
-        nc = QTAILQ_FIRST(&net_clients);
+    while (*p) {
+        nc = *p;
         if (nc->info->type == NET_CLIENT_DRIVER_NIC) {
-            qemu_del_nic(qemu_get_nic(nc));
+            p = &nc->next.tqe_next;
         } else {
             qemu_del_net_client(nc);
         }


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH 1/4] hw/xen: only remove peers of PCI NICs on unplug

[Paul Durrant]

On 17/10/2023 19:25, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> When the Xen guest asks to unplug *emulated* NICs, it's kind of unhelpful
> also to unplug the peer of the *Xen* PV NIC.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/i386/xen/xen_platform.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
> index 17457ff3de..e2dd1b536a 100644
> --- a/hw/i386/xen/xen_platform.c
> +++ b/hw/i386/xen/xen_platform.c
> @@ -140,9 +140,14 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
>   /* Remove the peer of the NIC device. Normally, this would be a tap device. */
>   static void del_nic_peer(NICState *nic, void *opaque)
>   {
> -    NetClientState *nc;
> +    NetClientState *nc = qemu_get_queue(nic);
> +    ObjectClass *klass = module_object_class_by_name(nc->model);
> +
> +    /* Only delete peers of PCI NICs that we're about to delete */
> +    if (!klass || !object_class_dynamic_cast(klass, TYPE_PCI_DEVICE)) {

Would it not be better to test for object_class_dynamic_cast(klass, 
TYPE_XEN_DEVICE)?

   Paul

> +        return;
> +    }
>   
> -    nc = qemu_get_queue(nic);
>       if (nc->peer)
>           qemu_del_net_client(nc->peer);
>   }

Re: [PATCH 2/4] hw/xen: update Xen PV NIC to XenDevice model

[Paul Durrant]

On 17/10/2023 19:25, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/net/meson.build        |   2 +-
>   hw/net/trace-events       |   9 +
>   hw/net/xen_nic.c          | 426 +++++++++++++++++++++++++++++---------
>   hw/xenpv/xen_machine_pv.c |   1 -
>   4 files changed, 342 insertions(+), 96 deletions(-)
> 
> diff --git a/hw/net/meson.build b/hw/net/meson.build
> index 2632634df3..f64651c467 100644
> --- a/hw/net/meson.build
> +++ b/hw/net/meson.build
> @@ -1,5 +1,5 @@
>   system_ss.add(when: 'CONFIG_DP8393X', if_true: files('dp8393x.c'))
> -system_ss.add(when: 'CONFIG_XEN', if_true: files('xen_nic.c'))
> +system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen_nic.c'))
>   system_ss.add(when: 'CONFIG_NE2000_COMMON', if_true: files('ne2000.c'))
>   
>   # PCI network cards
> diff --git a/hw/net/trace-events b/hw/net/trace-events
> index 3abfd65e5b..ee56acfbce 100644
> --- a/hw/net/trace-events
> +++ b/hw/net/trace-events
> @@ -482,3 +482,12 @@ dp8393x_receive_oversize(int size) "oversize packet, pkt_size is %d"
>   dp8393x_receive_not_netcard(void) "packet not for netcard"
>   dp8393x_receive_packet(int crba) "Receive packet at 0x%"PRIx32
>   dp8393x_receive_write_status(int crba) "Write status at 0x%"PRIx32
> +
> +# xen_nic.c
> +xen_netdev_realize(int idx, const char *info) "idx %u info '%s'"
> +xen_netdev_unrealize(int idx) "idx %u"
> +xen_netdev_create(int idx) "idx %u"
> +xen_netdev_destroy(int idx) "idx %u"
> +xen_netdev_disconnect(int idx) "idx %u"
> +xen_netdev_connect(int idx, unsigned int tx, unsigned int rx, int port) "idx %u tx %u rx %u port %u"
> +xen_netdev_frontend_changed(const char *idx, int state) "idx %s state %d"
> diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
> index 9bbf6599fc..84914c329c 100644
> --- a/hw/net/xen_nic.c
> +++ b/hw/net/xen_nic.c
> @@ -20,6 +20,11 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
> +#include "qemu/qemu-print.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/error.h"
> +
>   #include <sys/socket.h>
>   #include <sys/ioctl.h>
>   #include <sys/wait.h>
> @@ -27,18 +32,26 @@
>   #include "net/net.h"
>   #include "net/checksum.h"
>   #include "net/util.h"
> -#include "hw/xen/xen-legacy-backend.h"
> +
> +#include "hw/xen/xen-backend.h"
> +#include "hw/xen/xen-bus-helper.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
>   
>   #include "hw/xen/interface/io/netif.h"
> +#include "hw/xen/interface/io/xs_wire.h"
> +
> +#include "trace.h"
>   
>   /* ------------------------------------------------------------- */
>   
>   struct XenNetDev {
> -    struct XenLegacyDevice      xendev;  /* must be first */
> -    char                  *mac;
> +    struct XenDevice      xendev;  /* must be first */
> +    XenEventChannel       *event_channel;
> +    int                   dev;
>       int                   tx_work;
> -    int                   tx_ring_ref;
> -    int                   rx_ring_ref;
> +    unsigned int          tx_ring_ref;
> +    unsigned int          rx_ring_ref;
>       struct netif_tx_sring *txs;
>       struct netif_rx_sring *rxs;
>       netif_tx_back_ring_t  tx_ring;
> @@ -47,6 +60,13 @@ struct XenNetDev {
>       NICState              *nic;
>   };
>   
> +typedef struct XenNetDev XenNetDev;
> +
> +#define TYPE_XEN_NET_DEVICE "xen-net-device"
> +OBJECT_DECLARE_SIMPLE_TYPE(XenNetDev, XEN_NET_DEVICE)
> +
> +#define xen_pv_printf(a, n, ...) qemu_printf(__VA_ARGS__)

Why define this...

> +
>   /* ------------------------------------------------------------- */
>   
>   static void net_tx_response(struct XenNetDev *netdev, netif_tx_request_t *txp, int8_t st)
> @@ -68,7 +88,8 @@ static void net_tx_response(struct XenNetDev *netdev, netif_tx_request_t *txp, i
>       netdev->tx_ring.rsp_prod_pvt = ++i;
>       RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&netdev->tx_ring, notify);
>       if (notify) {
> -        xen_pv_send_notify(&netdev->xendev);
> +        xen_device_notify_event_channel(XEN_DEVICE(netdev),
> +                                        netdev->event_channel, NULL);
>       }
>   
>       if (i == netdev->tx_ring.req_cons) {
> @@ -104,8 +125,9 @@ static void net_tx_error(struct XenNetDev *netdev, netif_tx_request_t *txp, RING
>   #endif
>   }
>   
> -static void net_tx_packets(struct XenNetDev *netdev)
> +static bool net_tx_packets(struct XenNetDev *netdev)
>   {
> +    bool done_something = false;
>       netif_tx_request_t txreq;
>       RING_IDX rc, rp;
>       void *page;
> @@ -122,6 +144,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
>               }
>               memcpy(&txreq, RING_GET_REQUEST(&netdev->tx_ring, rc), sizeof(txreq));
>               netdev->tx_ring.req_cons = ++rc;
> +            done_something = true;
>   
>   #if 1
>               /* should not happen in theory, we don't announce the *
> @@ -151,7 +174,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
>                   continue;
>               }
>   
> -            xen_pv_printf(&netdev->xendev, 3,
> +            if (0) qemu_printf(//&netdev->xendev, 3,

... and then not use it here? Perhaps the 'if (0)' ugliness can go in 
the macro too.

>                             "tx packet ref %d, off %d, len %d, flags 0x%x%s%s%s%s\n",
>                             txreq.gref, txreq.offset, txreq.size, txreq.flags,
>                             (txreq.flags & NETTXF_csum_blank)     ? " csum_blank"     : "",
> @@ -159,8 +182,8 @@ static void net_tx_packets(struct XenNetDev *netdev)
>                             (txreq.flags & NETTXF_more_data)      ? " more_data"      : "",
>                             (txreq.flags & NETTXF_extra_info)     ? " extra_info"     : "");
>   
> -            page = xen_be_map_grant_ref(&netdev->xendev, txreq.gref,
> -                                        PROT_READ);
> +            page = xen_device_map_grant_refs(&netdev->xendev, &txreq.gref, 1,
> +                                             PROT_READ, NULL);
>               if (page == NULL) {
>                   xen_pv_printf(&netdev->xendev, 0,
>                                 "error: tx gref dereference failed (%d)\n",
> @@ -181,7 +204,8 @@ static void net_tx_packets(struct XenNetDev *netdev)
>                   qemu_send_packet(qemu_get_queue(netdev->nic),
>                                    page + txreq.offset, txreq.size);
>               }
> -            xen_be_unmap_grant_ref(&netdev->xendev, page, txreq.gref);
> +            xen_device_unmap_grant_refs(&netdev->xendev, page, &txreq.gref, 1,
> +                                        NULL);
>               net_tx_response(netdev, &txreq, NETIF_RSP_OKAY);
>           }
>           if (!netdev->tx_work) {
> @@ -190,6 +214,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
>           netdev->tx_work = 0;
>       }
>       g_free(tmpbuf);
> +    return done_something;
>   }
>   
>   /* ------------------------------------------------------------- */
> @@ -212,14 +237,15 @@ static void net_rx_response(struct XenNetDev *netdev,
>           resp->status = (int16_t)st;
>       }
>   
> -    xen_pv_printf(&netdev->xendev, 3,
> +    if (0) qemu_printf(//&netdev->xendev, 3,

Same here.

>                     "rx response: idx %d, status %d, flags 0x%x\n",
>                     i, resp->status, resp->flags);
>   
>       netdev->rx_ring.rsp_prod_pvt = ++i;
>       RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&netdev->rx_ring, notify);
>       if (notify) {
> -        xen_pv_send_notify(&netdev->xendev);
> +        xen_device_notify_event_channel(XEN_DEVICE(netdev),
> +                                        netdev->event_channel, NULL);
>       }
>   }
>   
> @@ -232,7 +258,7 @@ static ssize_t net_rx_packet(NetClientState *nc, const uint8_t *buf, size_t size
>       RING_IDX rc, rp;
>       void *page;
>   
> -    if (netdev->xendev.be_state != XenbusStateConnected) {
> +    if (netdev->rx_ring.sring == NULL) {

Why not a straight swap for xen_device_backend_get_state()? Hard to see 
whether there any hidden side effects of this change otherwise.

>           return -1;
>       }
>   
> @@ -252,7 +278,8 @@ static ssize_t net_rx_packet(NetClientState *nc, const uint8_t *buf, size_t size
>       memcpy(&rxreq, RING_GET_REQUEST(&netdev->rx_ring, rc), sizeof(rxreq));
>       netdev->rx_ring.req_cons = ++rc;
>   
> -    page = xen_be_map_grant_ref(&netdev->xendev, rxreq.gref, PROT_WRITE);
> +    page = xen_device_map_grant_refs(&netdev->xendev, &rxreq.gref, 1,
> +                                     PROT_WRITE, NULL);
>       if (page == NULL) {
>           xen_pv_printf(&netdev->xendev, 0,
>                         "error: rx gref dereference failed (%d)\n",
> @@ -261,7 +288,7 @@ static ssize_t net_rx_packet(NetClientState *nc, const uint8_t *buf, size_t size
>           return -1;
>       }
>       memcpy(page + NET_IP_ALIGN, buf, size);
> -    xen_be_unmap_grant_ref(&netdev->xendev, page, rxreq.gref);
> +    xen_device_unmap_grant_refs(&netdev->xendev, page, &rxreq.gref, 1, NULL);
>       net_rx_response(netdev, &rxreq, NETIF_RSP_OKAY, NET_IP_ALIGN, size, 0);
>   
>       return size;
> @@ -275,139 +302,350 @@ static NetClientInfo net_xen_info = {
>       .receive = net_rx_packet,
>   };
>   
> -static int net_init(struct XenLegacyDevice *xendev)
> +static void xen_netdev_realize(XenDevice *xendev, Error **errp)
>   {
> -    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, xendev);
> +    ERRP_GUARD();
> +    XenNetDev *netdev = XEN_NET_DEVICE(xendev);
>   
> -    /* read xenstore entries */
> -    if (netdev->mac == NULL) {
> -        netdev->mac = xenstore_read_be_str(&netdev->xendev, "mac");
> -    }
> +    qemu_macaddr_default_if_unset(&netdev->conf.macaddr);
>   
> -    /* do we have all we need? */
> -    if (netdev->mac == NULL) {
> -        return -1;
> -    }
> -
> -    if (net_parse_macaddr(netdev->conf.macaddr.a, netdev->mac) < 0) {
> -        return -1;
> -    }
> +    xen_device_frontend_printf(xendev, "mac", "%02x:%02x:%02x:%02x:%02x:%02x",
> +                               netdev->conf.macaddr.a[0],
> +                               netdev->conf.macaddr.a[1],
> +                               netdev->conf.macaddr.a[2],
> +                               netdev->conf.macaddr.a[3],
> +                               netdev->conf.macaddr.a[4],
> +                               netdev->conf.macaddr.a[5]);
>   
>       netdev->nic = qemu_new_nic(&net_xen_info, &netdev->conf,
> -                               "xen", NULL, netdev);
> +                               object_get_typename(OBJECT(xendev)),
> +                               DEVICE(xendev)->id, netdev);
>   
> -    qemu_set_info_str(qemu_get_queue(netdev->nic),
> -                      "nic: xenbus vif macaddr=%s", netdev->mac);
> +    qemu_format_nic_info_str(qemu_get_queue(netdev->nic), netdev->conf.macaddr.a);
>   
>       /* fill info */
> -    xenstore_write_be_int(&netdev->xendev, "feature-rx-copy", 1);
> -    xenstore_write_be_int(&netdev->xendev, "feature-rx-flip", 0);
> +    xen_device_backend_printf(xendev, "feature-rx-copy", "%u", 1);
> +    xen_device_backend_printf(xendev, "feature-rx-flip", "%u", 0);
>   
> -    return 0;
> +    trace_xen_netdev_realize(netdev->dev, qemu_get_queue(netdev->nic)->info_str);
>   }
>   
> -static int net_connect(struct XenLegacyDevice *xendev)
> +static bool net_event(void *_xendev)
>   {
> -    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, xendev);
> -    int rx_copy;
> +    XenNetDev *netdev = XEN_NET_DEVICE(_xendev);
> +    bool done_something;
>   
> -    if (xenstore_read_fe_int(&netdev->xendev, "tx-ring-ref",
> -                             &netdev->tx_ring_ref) == -1) {
> -        return -1;
> +    done_something = net_tx_packets(netdev);
> +    qemu_flush_queued_packets(qemu_get_queue(netdev->nic));
> +    return done_something;
> +}
> +
> +static bool xen_netdev_connect(XenDevice *xendev, Error **errp)
> +{
> +    XenNetDev *netdev = XEN_NET_DEVICE(xendev);
> +    unsigned int port, rx_copy;
> +
> +    if (xen_device_frontend_scanf(xendev, "tx-ring-ref", "%u",
> +                                  &netdev->tx_ring_ref) != 1) {
> +        error_setg(errp, "failed to read tx-ring-ref");
> +        return false;
>       }
> -    if (xenstore_read_fe_int(&netdev->xendev, "rx-ring-ref",
> -                             &netdev->rx_ring_ref) == -1) {
> -        return 1;
> +
> +    if (xen_device_frontend_scanf(xendev, "rx-ring-ref", "%u",
> +                                  &netdev->rx_ring_ref) != 1) {
> +        error_setg(errp, "failed to read rx-ring-ref");
> +        return false;
>       }
> -    if (xenstore_read_fe_int(&netdev->xendev, "event-channel",
> -                             &netdev->xendev.remote_port) == -1) {
> -        return -1;
> +
> +    if (xen_device_frontend_scanf(xendev, "event-channel", "%u",
> +                                  &port) != 1) {
> +        error_setg(errp, "failed to read event-channel");
> +        return false;
>       }
>   
> -    if (xenstore_read_fe_int(&netdev->xendev, "request-rx-copy", &rx_copy) == -1) {
> +    if (xen_device_frontend_scanf(xendev, "request-rx-copy", "%u",
> +                                  &rx_copy) != 1) {
>           rx_copy = 0;
>       }
>       if (rx_copy == 0) {
> -        xen_pv_printf(&netdev->xendev, 0,
> -                      "frontend doesn't support rx-copy.\n");
> -        return -1;
> +        error_setg(errp, "frontend doesn't support rx-copy");
> +        return false;
>       }
>   
> -    netdev->txs = xen_be_map_grant_ref(&netdev->xendev,
> -                                       netdev->tx_ring_ref,
> -                                       PROT_READ | PROT_WRITE);
> +    netdev->txs = xen_device_map_grant_refs(xendev,
> +                                            &netdev->tx_ring_ref, 1,
> +                                            PROT_READ | PROT_WRITE,
> +                                            errp);
>       if (!netdev->txs) {
> -        return -1;
> +        error_prepend(errp, "failed to map tx grant ref: ");
> +        return false;
>       }
> -    netdev->rxs = xen_be_map_grant_ref(&netdev->xendev,
> -                                       netdev->rx_ring_ref,
> -                                       PROT_READ | PROT_WRITE);
> +
> +    netdev->rxs = xen_device_map_grant_refs(xendev,
> +                                            &netdev->rx_ring_ref, 1,
> +                                            PROT_READ | PROT_WRITE,
> +                                            errp);
>       if (!netdev->rxs) {
> -        xen_be_unmap_grant_ref(&netdev->xendev, netdev->txs,
> -                               netdev->tx_ring_ref);
> -        netdev->txs = NULL;
> -        return -1;
> +        error_prepend(errp, "failed to map rx grant ref: ");
> +        return false;
>       }
> +
>       BACK_RING_INIT(&netdev->tx_ring, netdev->txs, XEN_PAGE_SIZE);
>       BACK_RING_INIT(&netdev->rx_ring, netdev->rxs, XEN_PAGE_SIZE);
>   
> -    xen_be_bind_evtchn(&netdev->xendev);
> +    netdev->event_channel = xen_device_bind_event_channel(xendev, port,
> +                                                          net_event,
> +                                                          netdev,
> +                                                          errp);
> +    if (!netdev->event_channel) {
> +        return false;
> +    }
>   
> -    xen_pv_printf(&netdev->xendev, 1, "ok: tx-ring-ref %d, rx-ring-ref %d, "
> -                  "remote port %d, local port %d\n",
> -                  netdev->tx_ring_ref, netdev->rx_ring_ref,
> -                  netdev->xendev.remote_port, netdev->xendev.local_port);
> +    trace_xen_netdev_connect(netdev->dev, netdev->tx_ring_ref,
> +                             netdev->rx_ring_ref, port);
>   
>       net_tx_packets(netdev);
> -    return 0;
> +    return true;
>   }
>   
> -static void net_disconnect(struct XenLegacyDevice *xendev)
> +static void xen_netdev_disconnect(XenDevice *xendev, Error **errp)
>   {
> -    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, xendev);
> +    XenNetDev *netdev = XEN_NET_DEVICE(xendev);
> +
> +    trace_xen_netdev_disconnect(netdev->dev);
>   
> -    xen_pv_unbind_evtchn(&netdev->xendev);
> +    netdev->tx_ring.sring = NULL;
> +    netdev->rx_ring.sring = NULL;
>   
> +    if (netdev->event_channel) {
> +        xen_device_unbind_event_channel(xendev, netdev->event_channel,
> +                                        errp);
> +        netdev->event_channel = NULL;
> +    }
>       if (netdev->txs) {
> -        xen_be_unmap_grant_ref(&netdev->xendev, netdev->txs,
> -                               netdev->tx_ring_ref);
> +        xen_device_unmap_grant_refs(xendev, netdev->txs,
> +                                    &netdev->tx_ring_ref, 1, errp);
>           netdev->txs = NULL;
>       }
>       if (netdev->rxs) {
> -        xen_be_unmap_grant_ref(&netdev->xendev, netdev->rxs,
> -                               netdev->rx_ring_ref);
> +        xen_device_unmap_grant_refs(xendev, netdev->rxs,
> +                                    &netdev->rx_ring_ref, 1, errp);
>           netdev->rxs = NULL;
>       }
>   }
>   
> -static void net_event(struct XenLegacyDevice *xendev)
> +/* -------------------------------------------------------------------- */
> +
> +
> +static void xen_netdev_frontend_changed(XenDevice *xendev,
> +                                       enum xenbus_state frontend_state,
> +                                       Error **errp)
>   {
> -    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, xendev);
> -    net_tx_packets(netdev);
> -    qemu_flush_queued_packets(qemu_get_queue(netdev->nic));
> +    ERRP_GUARD();
> +    enum xenbus_state backend_state = xen_device_backend_get_state(xendev);
> +
> +    trace_xen_netdev_frontend_changed(xendev->name, frontend_state);
> +
> +    switch (frontend_state) {
> +    case XenbusStateInitialised:

I don't think that's really a valid state for a network frontend. Linux 
netback just ignores it.

   Paul

> +    case XenbusStateConnected:
> +        if (backend_state == XenbusStateConnected) {
> +            break;
> +        }
> +
> +        xen_netdev_disconnect(xendev, errp);
> +        if (*errp) {
> +            break;
> +        }
> +
> +        if (!xen_netdev_connect(xendev, errp)) {
> +            xen_netdev_disconnect(xendev, NULL);
> +            xen_device_backend_set_state(xendev, XenbusStateClosing);
> +            break;
> +        }
> +
> +        xen_device_backend_set_state(xendev, XenbusStateConnected);
> +        break;
> +
> +    case XenbusStateClosing:
> +        xen_device_backend_set_state(xendev, XenbusStateClosing);
> +        break;
> +
> +    case XenbusStateClosed:
> +    case XenbusStateUnknown:
> +        xen_netdev_disconnect(xendev, errp);
> +        if (*errp) {
> +            break;
> +        }
> +
> +        xen_device_backend_set_state(xendev, XenbusStateClosed);
> +        break;
> +
> +    default:
> +        break;
> +    }
>   }
>   
> -static int net_free(struct XenLegacyDevice *xendev)
> +static char *xen_netdev_get_name(XenDevice *xendev, Error **errp)
>   {
> -    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, xendev);
> +    XenNetDev *netdev = XEN_NET_DEVICE(xendev);
> +
> +    if (netdev->dev == -1) {
> +        XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> +        char fe_path[XENSTORE_ABS_PATH_MAX + 1];
> +        int idx = (xen_mode == XEN_EMULATE) ? 0 : 1;
> +        char *value;
> +
> +        /* Theoretically we could go up to INT_MAX here but that's overkill */
> +        while (idx < 100) {
> +            snprintf(fe_path, sizeof(fe_path),
> +                     "/local/domain/%u/device/vif/%u",
> +                     xendev->frontend_id, idx);
> +            value = qemu_xen_xs_read(xenbus->xsh, XBT_NULL, fe_path, NULL);
> +            if (!value) {
> +                if (errno == ENOENT) {
> +                    netdev->dev = idx;
> +                    goto found;
> +                }
> +                error_setg(errp, "cannot read %s: %s", fe_path,
> +                           strerror(errno));
> +                return NULL;
> +            }
> +            free(value);
> +            idx++;
> +        }
> +        error_setg(errp, "cannot find device index for netdev device");
> +        return NULL;
> +    }
> + found:
> +    return g_strdup_printf("%u", netdev->dev);
> +}
> +
> +static void xen_netdev_unrealize(XenDevice *xendev)
> +{
> +    XenNetDev *netdev = XEN_NET_DEVICE(xendev);
> +
> +    trace_xen_netdev_unrealize(netdev->dev);
> +
> +    /* Disconnect from the frontend in case this has not already happened */
> +    xen_netdev_disconnect(xendev, NULL);
>   
>       if (netdev->nic) {
>           qemu_del_nic(netdev->nic);
> -        netdev->nic = NULL;
>       }
> -    g_free(netdev->mac);
> -    netdev->mac = NULL;
> -    return 0;
>   }
>   
>   /* ------------------------------------------------------------- */
>   
> -struct XenDevOps xen_netdev_ops = {
> -    .size       = sizeof(struct XenNetDev),
> -    .flags      = DEVOPS_FLAG_NEED_GNTDEV,
> -    .init       = net_init,
> -    .initialise    = net_connect,
> -    .event      = net_event,
> -    .disconnect = net_disconnect,
> -    .free       = net_free,
> +static Property xen_netdev_properties[] = {
> +    DEFINE_NIC_PROPERTIES(XenNetDev, conf),
> +    DEFINE_PROP_INT32("idx", XenNetDev, dev, -1),
> +    DEFINE_PROP_END_OF_LIST(),
>   };
> +
> +static void xen_netdev_class_init(ObjectClass *class, void *data)
> +{
> +    DeviceClass *dev_class = DEVICE_CLASS(class);
> +    XenDeviceClass *xendev_class = XEN_DEVICE_CLASS(class);
> +
> +    xendev_class->backend = "qnet";
> +    xendev_class->device = "vif";
> +    xendev_class->get_name = xen_netdev_get_name;
> +    xendev_class->realize = xen_netdev_realize;
> +    xendev_class->frontend_changed = xen_netdev_frontend_changed;
> +    xendev_class->unrealize = xen_netdev_unrealize;
> +    set_bit(DEVICE_CATEGORY_NETWORK, dev_class->categories);
> +    dev_class->user_creatable = true;
> +
> +    device_class_set_props(dev_class, xen_netdev_properties);
> +}
> +
> +static const TypeInfo xen_net_type_info = {
> +    .name = TYPE_XEN_NET_DEVICE,
> +    .parent = TYPE_XEN_DEVICE,
> +    .instance_size = sizeof(XenNetDev),
> +    .class_init = xen_netdev_class_init,
> +};
> +
> +static void xen_net_register_types(void)
> +{
> +    type_register_static(&xen_net_type_info);
> +}
> +
> +type_init(xen_net_register_types)
> +
> +/* Called to instantiate a XenNetDev when the backend is detected. */
> +static void xen_net_device_create(XenBackendInstance *backend,
> +                                  QDict *opts, Error **errp)
> +{
> +    ERRP_GUARD();
> +    XenBus *xenbus = xen_backend_get_bus(backend);
> +    const char *name = xen_backend_get_name(backend);
> +    XenDevice *xendev = NULL;
> +    unsigned long number;
> +    const char *macstr;
> +    XenNetDev *net;
> +    MACAddr mac;
> +
> +    if (qemu_strtoul(name, NULL, 10, &number) || number >= INT_MAX) {
> +        error_setg(errp, "failed to parse name '%s'", name);
> +        goto fail;
> +    }
> +
> +    trace_xen_netdev_create(number);
> +
> +    macstr = qdict_get_try_str(opts, "mac");
> +    if (macstr == NULL) {
> +        error_setg(errp, "no MAC address found");
> +        goto fail;
> +    }
> +
> +    if (net_parse_macaddr(mac.a, macstr) < 0) {
> +        error_setg(errp, "failed to parse MAC address");
> +        goto fail;
> +    }
> +
> +    xendev = XEN_DEVICE(qdev_new(TYPE_XEN_NET_DEVICE));
> +    net = XEN_NET_DEVICE(xendev);
> +
> +    net->dev = number;
> +    memcpy(&net->conf.macaddr, &mac, sizeof(mac));
> +
> +    if (qdev_realize_and_unref(DEVICE(xendev), BUS(xenbus), errp)) {
> +        xen_backend_set_device(backend, xendev);
> +        return;
> +    }
> +
> +    error_prepend(errp, "realization of net device %lu failed: ",
> +                  number);
> +
> + fail:
> +    if (xendev) {
> +        object_unparent(OBJECT(xendev));
> +    }
> +}
> +
> +static void xen_net_device_destroy(XenBackendInstance *backend,
> +                                       Error **errp)
> +{
> +    ERRP_GUARD();
> +    XenDevice *xendev = xen_backend_get_device(backend);
> +    XenNetDev *netdev = XEN_NET_DEVICE(xendev);
> +
> +    trace_xen_netdev_destroy(netdev->dev);
> +
> +    object_unparent(OBJECT(xendev));
> +}
> +
> +static const XenBackendInfo xen_net_backend_info  = {
> +    .type = "qnet",
> +    .create = xen_net_device_create,
> +    .destroy = xen_net_device_destroy,
> +};
> +
> +static void xen_net_register_backend(void)
> +{
> +    xen_backend_register(&xen_net_backend_info);
> +}
> +
> +xen_backend_init(xen_net_register_backend);
> diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
> index 17cda5ec13..764ca5c4fa 100644
> --- a/hw/xenpv/xen_machine_pv.c
> +++ b/hw/xenpv/xen_machine_pv.c
> @@ -55,7 +55,6 @@ static void xen_init_pv(MachineState *machine)
>       }
>   
>       xen_be_register("vfb", &xen_framebuffer_ops);
> -    xen_be_register("qnic", &xen_netdev_ops);
>   
>       /* configure framebuffer */
>       if (vga_interface_type == VGA_XENFB) {

Re: [PATCH 2/4] hw/xen: update Xen PV NIC to XenDevice model

[David Woodhouse]

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

On Tue, 2023-10-24 at 15:47 +0100, Paul Durrant wrote:
> On 17/10/2023 19:25, David Woodhouse wrote:
> > +
> > +#define xen_pv_printf(a, n, ...) qemu_printf(__VA_ARGS__)
> 
> Why define this...

In the first place, just to make it build in the short term. Then I
forgot to clean it up before posting. In my tree this is all tracing
now.

> 
> > @@ -232,7 +258,7 @@ static ssize_t net_rx_packet(NetClientState *nc, const uint8_t *buf, size_t size
> >       RING_IDX rc, rp;
> >       void *page;
> >   
> > -    if (netdev->xendev.be_state != XenbusStateConnected) {
> > +    if (netdev->rx_ring.sring == NULL) {
> 
> Why not a straight swap for xen_device_backend_get_state()? Hard to see 
> whether there any hidden side effects of this change otherwise.

Could do, but what I *actually* cared about when looking at that check,
was whether the ring pointer was NULL. So I checked that explicitly.

It should be identical.

> > +static void xen_netdev_frontend_changed(XenDevice *xendev,
> > +                                       enum xenbus_state frontend_state,
> > +                                       Error **errp)
> >   {
> > -    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, xendev);
> > -    net_tx_packets(netdev);
> > -    qemu_flush_queued_packets(qemu_get_queue(netdev->nic));
> > +    ERRP_GUARD();
> > +    enum xenbus_state backend_state = xen_device_backend_get_state(xendev);
> > +
> > +    trace_xen_netdev_frontend_changed(xendev->name, frontend_state);
> > +
> > +    switch (frontend_state) {
> > +    case XenbusStateInitialised:
> 
> I don't think that's really a valid state for a network frontend. Linux 
> netback just ignores it.

Must we? I was thinking of making the ->frontend_changed() methods
optional and allowing backends to just provide ->connect() and
->disconnect() methods instead if they wanted to. Because we have three
identical ->frontend_changed() methods now...



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH 1/4] hw/xen: only remove peers of PCI NICs on unplug

[David Woodhouse]

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

On Tue, 2023-10-24 at 15:32 +0100, Paul Durrant wrote:
> On 17/10/2023 19:25, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > When the Xen guest asks to unplug *emulated* NICs, it's kind of unhelpful
> > also to unplug the peer of the *Xen* PV NIC.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >    hw/i386/xen/xen_platform.c | 9 +++++++--
> >    1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
> > index 17457ff3de..e2dd1b536a 100644
> > --- a/hw/i386/xen/xen_platform.c
> > +++ b/hw/i386/xen/xen_platform.c
> > @@ -140,9 +140,14 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
> >    /* Remove the peer of the NIC device. Normally, this would be a tap device. */
> >    static void del_nic_peer(NICState *nic, void *opaque)
> >    {
> > -    NetClientState *nc;
> > +    NetClientState *nc = qemu_get_queue(nic);
> > +    ObjectClass *klass = module_object_class_by_name(nc->model);
> > +
> > +    /* Only delete peers of PCI NICs that we're about to delete */
> > +    if (!klass || !object_class_dynamic_cast(klass, TYPE_PCI_DEVICE)) {
> 
> Would it not be better to test for object_class_dynamic_cast(klass, 
> TYPE_XEN_DEVICE)?

Only if we also change the actual unplug to destroy non-PCI devices too.

The only non-PCI device you could have here is an ISA NE2000, I think.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH 2/4] hw/xen: update Xen PV NIC to XenDevice model

[Paul Durrant]

On 24/10/2023 16:17, David Woodhouse wrote:
[snip]
>>
>> I don't think that's really a valid state for a network frontend. Linux
>> netback just ignores it.
> 
> Must we? I was thinking of making the ->frontend_changed() methods
> optional and allowing backends to just provide ->connect() and
> ->disconnect() methods instead if they wanted to. Because we have three
> identical ->frontend_changed() methods now...
> 

Now maybe... Not sure things will look so common when other backends are 
converted. I'd prefer to maintain fidelity with Linux xen-netback as it 
is generally considered to be the canonical implementation.

   Paul

Re: [PATCH 2/4] hw/xen: update Xen PV NIC to XenDevice model

[David Woodhouse]

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

On Tue, 2023-10-24 at 15:47 +0100, Paul Durrant wrote:
> 
> >    
> > -    if (netdev->xendev.be_state != XenbusStateConnected) {
> > +    if (netdev->rx_ring.sring == NULL) {
> 
> Why not a straight swap for xen_device_backend_get_state()? Hard to see 
> whether there any hidden side effects of this change otherwise.

I suppose if I litter it with a few assertions that the iothread mutex
is locked, I can live with that. As long as it's atomic w.r.t.
disconnection, they *are* identical checks.

(And if it could be running simultaneously with disconnection, we'd be
hosed anyway).


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]