Re: [PATCH v3 19/28] hw/xen: update Xen PV NIC to XenDevice model

Durrant, Paul
2023-10-27T08:43:41Z

On 25/10/2023 15:50, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> This allows us to use Xen PV networking with emulated Xen guests, and to
> add them on the command line or hotplug.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> hw/net/meson.build | 2 +-
> hw/net/trace-events | 11 +
> hw/net/xen_nic.c | 484 +++++++++++++++++++++++++++++---------
> hw/xenpv/xen_machine_pv.c | 1 -
> 4 files changed, 381 insertions(+), 117 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..3097742cc0 100644
> --- a/hw/net/trace-events
> +++ b/hw/net/trace-events
> @@ -482,3 +482,14 @@ 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 dev, const char *info, const char *peer) "vif%u info '%s' peer '%s'"
> +xen_netdev_unrealize(int dev) "vif%u"
> +xen_netdev_create(int dev) "vif%u"
> +xen_netdev_destroy(int dev) "vif%u"
> +xen_netdev_disconnect(int dev) "vif%u"
> +xen_netdev_connect(int dev, unsigned int tx, unsigned int rx, int port) "vif%u tx %u rx %u port %u"
> +xen_netdev_frontend_changed(const char *dev, int state) "vif%s state %d"
> +xen_netdev_tx(int dev, int ref, int off, int len, unsigned int flags, const char *c, const char *d, const char *m, const char *e) "vif%u ref %u off %u len %u flags 0x%x%s%s%s%s"
> +xen_netdev_rx(int dev, int idx, int status, int flags) "vif%u idx %d status %d flags 0x%x"
> diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
> index 9bbf6599fc..af4ba3f1e6 100644
> --- a/hw/net/xen_nic.c
> +++ b/hw/net/xen_nic.c
> @@ -20,6 +20,13 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
> +#include "qemu/cutils.h"
> +#include "qemu/log.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 +34,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 +62,11 @@ struct XenNetDev {
> NICState *nic;
> };
>
> +typedef struct XenNetDev XenNetDev;
> +
> +#define TYPE_XEN_NET_DEVICE "xen-net-device"
> +OBJECT_DECLARE_SIMPLE_TYPE(XenNetDev, XEN_NET_DEVICE)
> +
> /* ------------------------------------------------------------- */
>
> 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,13 +125,16 @@ 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;
> void *tmpbuf = NULL;
>
> + assert(qemu_mutex_iothread_locked());
> +
> for (;;) {
> rc = netdev->tx_ring.req_cons;
> rp = netdev->tx_ring.sring->req_prod;
> @@ -122,49 +146,52 @@ 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 *
> * feature-{sg,gso,whatelse} flags in xenstore (yet?) */
> if (txreq.flags & NETTXF_extra_info) {
> - xen_pv_printf(&netdev->xendev, 0, "FIXME: extra info flag\n");
> + qemu_log_mask(LOG_UNIMP, "vif%u: FIXME: extra info flag\n",
> + netdev->dev);
> net_tx_error(netdev, &txreq, rc);
> continue;
> }
> if (txreq.flags & NETTXF_more_data) {
> - xen_pv_printf(&netdev->xendev, 0, "FIXME: more data flag\n");
> + qemu_log_mask(LOG_UNIMP, "vif%u: FIXME: more data flag\n",
> + netdev->dev);
> net_tx_error(netdev, &txreq, rc);
> continue;
> }
> #endif

I know that this is just translation but the fact the above code is
there indicates that you're likely to see problems here. Not supporting
extra_info is likely ok as long as RSS, TSO or multicast filtering is
not advertized (as the comment says). But lack of support for more_data
basically means your frontend needs to send all packets in a single frag
(e.g. no header split) so that might bite pretty quickly. I guess it
goes to show that no-one has used this code in many many years.

The translation looks ok to me though so...

Reviewed-by: Paul Durrant <paul@xen.org>