* [PATCH v16 0/7] Add vmnet.framework based network backend
@ 2022-03-14 19:15 Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 1/7] net/vmnet: add vmnet dependency and customizable option Vladislav Yaroshchuk
` (6 more replies)
0 siblings, 7 replies; 16+ messages in thread
From: Vladislav Yaroshchuk @ 2022-03-14 19:15 UTC (permalink / raw)
To: qemu-devel
Cc: jasowang, r.bolshakov, eblake, phillip.ennen, phillip,
akihiko.odaki, armbru, hsp.cat7, hello, roman, peter.maydell,
dirty, f4bug, agraf, kraxel, alex.bennee, qemu_oss,
Vladislav Yaroshchuk
macOS provides networking API for VMs called 'vmnet.framework':
https://developer.apple.com/documentation/vmnet
We can provide its support as the new QEMU network backends which
represent three different vmnet.framework interface usage modes:
* `vmnet-shared`:
allows the guest to communicate with other guests in shared mode and
also with external network (Internet) via NAT. Has (macOS-provided)
DHCP server; subnet mask and IP range can be configured;
* `vmnet-host`:
allows the guest to communicate with other guests in host mode.
By default has enabled DHCP as `vmnet-shared`, but providing
network unique id (uuid) can make `vmnet-host` interfaces isolated
from each other and also disables DHCP.
* `vmnet-bridged`:
bridges the guest with a physical network interface.
This backends cannot work on macOS Catalina 10.15 cause we use
vmnet.framework API provided only with macOS 11 and newer. Seems
that it is not a problem, because QEMU guarantees to work on two most
recent versions of macOS which now are Big Sur (11) and Monterey (12).
Also, we have one inconvenient restriction: vmnet.framework interfaces
can create only privileged user:
`$ sudo qemu-system-x86_64 -nic vmnet-shared`
Attempt of `vmnet-*` netdev creation being unprivileged user fails with
vmnet's 'general failure'.
This happens because vmnet.framework requires `com.apple.vm.networking`
entitlement which is: "restricted to developers of virtualization software.
To request this entitlement, contact your Apple representative." as Apple
documentation says:
https://developer.apple.com/documentation/bundleresources/entitlements/com_apple_vm_networking
One more note: we still have quite useful but not supported
'vmnet.framework' features as creating port forwarding rules, IPv6
NAT prefix specifying and so on.
Nevertheless, new backends work fine and tested within `qemu-system-x86-64`
on macOS Bir Sur 11.5.2 host with such nic models:
* e1000-82545em
* virtio-net-pci
* vmxnet3
The guests were:
* macOS 10.15.7
* Ubuntu Bionic (server cloudimg)
This series partially reuses patches by Phillip Tennen:
https://patchew.org/QEMU/20210218134947.1860-1-phillip.ennen@gmail.com/
So I included them signed-off line into one of the commit messages and
also here.
v1 -> v2:
Since v1 minor typos were fixed, patches rebased onto latest master,
redundant changes removed (small commits squashed)
v2 -> v3:
- QAPI style fixes
- Typos fixes in comments
- `#include`'s updated to be in sync with recent master
v3 -> v4:
- Support vmnet interfaces isolation feature
- Support vmnet-host network uuid setting feature
- Refactored sources a bit
v4 -> v5:
- Missed 6.2 boat, now 7.0 candidate
- Fix qapi netdev descriptions and styles
(@subnetmask -> @subnet-mask)
- Support vmnet-shared IPv6 prefix setting feature
v5 -> v6
- provide detailed commit messages for commits of
many changes
- rename properties @dhcpstart and @dhcpend to
@start-address and @end-address
- improve qapi documentation about isolation
features (@isolated, @net-uuid)
v6 -> v7:
- update MAINTAINERS list
v7 -> v8
- QAPI code style fixes
v8 -> v9
- Fix building on Linux: add missing qapi
`'if': 'CONFIG_VMNET'` statement to Netdev union
v9 -> v10
- Disable vmnet feature for macOS < 11.0: add
vmnet.framework API probe into meson.build.
This fixes QEMU building on macOS < 11.0:
https://patchew.org/QEMU/20220110034000.20221-1-jasowang@redhat.com/
v10 -> v11
- Enable vmnet for macOS 10.15 with subset of available
features. Disable vmnet for macOS < 10.15.
- Fix typos
v11 -> v12
- use more general macOS version check with
MAC_OS_VERSION_11_0 instead of manual
definition creating.
v12 -> v13
- fix incorrect macOS version bound while
'feature available since 11.0' check.
Use MAC_OS_X_VERSION_MIN_REQUIRED instead of
MAC_OS_X_VERSION_MAX_ALLOWED.
v13 -> v14
- fix memory leaks
- get rid of direct global mutex taking while resending
packets from vmnet to QEMU, schedule a bottom half
instead (it can be a thing to discuss, maybe exists a
better way to perform the packets transfer)
- update hmp commands
- a bit refactor everything
- change the email from which patches are being
submitted, same to email in MAINTAINERS list
- P.S. sorry for so late reply
v14 -> v15
- restore --enable-vdi and --disable-vdi
mistakenly dropped in previous series
v15 -> v16
- common: complete sending pending packets when
QEMU is ready, refactor, fix memory leaks
- QAPI: change version to 7.1 (cause 7.0 feature freeze
happened). This is the only change in QAPI, Markus Armbruster,
please confirm if you can (decided to drop your Acked-by due
to this change)
- vmnet-bridged: extend "supported ifnames" message buffer len
- fix behaviour dependence on debug (add "return -1" after
assert_not_reached)
- use PRIu64 for proper printing
- NOTE: This version of patch series may be one the last
I submit - JetBrains has suspended operations in
Russia indefinitely due to all the awful things happened
the last weeks. I may leave this company and loose the
ability to work on vmnet support :(
It will be perfect if someone can handle my unfinished work,
if something required to fix/improve is found.
Because of this, MAINTAINERS list update is dropped
Vladislav Yaroshchuk (7):
net/vmnet: add vmnet dependency and customizable option
net/vmnet: add vmnet backends to qapi/net
net/vmnet: implement shared mode (vmnet-shared)
net/vmnet: implement host mode (vmnet-host)
net/vmnet: implement bridged mode (vmnet-bridged)
net/vmnet: update qemu-options.hx
net/vmnet: update hmp-commands.hx
hmp-commands.hx | 6 +-
meson.build | 16 +-
meson_options.txt | 2 +
net/clients.h | 11 ++
net/meson.build | 7 +
net/net.c | 10 ++
net/vmnet-bridged.m | 149 ++++++++++++++++
net/vmnet-common.m | 318 ++++++++++++++++++++++++++++++++++
net/vmnet-host.c | 128 ++++++++++++++
net/vmnet-shared.c | 112 ++++++++++++
net/vmnet_int.h | 64 +++++++
qapi/net.json | 133 +++++++++++++-
qemu-options.hx | 25 +++
scripts/meson-buildoptions.sh | 1 +
14 files changed, 978 insertions(+), 4 deletions(-)
create mode 100644 net/vmnet-bridged.m
create mode 100644 net/vmnet-common.m
create mode 100644 net/vmnet-host.c
create mode 100644 net/vmnet-shared.c
create mode 100644 net/vmnet_int.h
--
2.34.1.vfs.0.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v16 1/7] net/vmnet: add vmnet dependency and customizable option
2022-03-14 19:15 [PATCH v16 0/7] Add vmnet.framework based network backend Vladislav Yaroshchuk
@ 2022-03-14 19:15 ` Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 2/7] net/vmnet: add vmnet backends to qapi/net Vladislav Yaroshchuk
` (5 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Vladislav Yaroshchuk @ 2022-03-14 19:15 UTC (permalink / raw)
To: qemu-devel
Cc: jasowang, r.bolshakov, eblake, phillip.ennen, phillip,
akihiko.odaki, armbru, hsp.cat7, hello, roman, peter.maydell,
dirty, f4bug, agraf, kraxel, alex.bennee, qemu_oss,
Vladislav Yaroshchuk
vmnet.framework dependency is added with 'vmnet' option
to enable or disable it. Default value is 'auto'.
vmnet features to be used are available since macOS 11.0,
corresponding probe is created into meson.build.
Signed-off-by: Vladislav Yaroshchuk <Vladislav.Yaroshchuk@jetbrains.com>
---
meson.build | 16 +++++++++++++++-
meson_options.txt | 2 ++
scripts/meson-buildoptions.sh | 1 +
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 2d6601467f..806f3869f9 100644
--- a/meson.build
+++ b/meson.build
@@ -522,6 +522,18 @@ if cocoa.found() and get_option('gtk').enabled()
error('Cocoa and GTK+ cannot be enabled at the same time')
endif
+vmnet = dependency('appleframeworks', modules: 'vmnet', required: get_option('vmnet'))
+if vmnet.found() and not cc.has_header_symbol('vmnet/vmnet.h',
+ 'VMNET_BRIDGED_MODE',
+ dependencies: vmnet)
+ vmnet = not_found
+ if get_option('vmnet').enabled()
+ error('vmnet.framework API is outdated')
+ else
+ warning('vmnet.framework API is outdated, disabling')
+ endif
+endif
+
seccomp = not_found
if not get_option('seccomp').auto() or have_system or have_tools
seccomp = dependency('libseccomp', version: '>=2.3.0',
@@ -1550,6 +1562,7 @@ config_host_data.set('CONFIG_SNAPPY', snappy.found())
config_host_data.set('CONFIG_TPM', have_tpm)
config_host_data.set('CONFIG_USB_LIBUSB', libusb.found())
config_host_data.set('CONFIG_VDE', vde.found())
+config_host_data.set('CONFIG_VMNET', vmnet.found())
config_host_data.set('CONFIG_VHOST_USER_BLK_SERVER', have_vhost_user_blk_server)
config_host_data.set('CONFIG_VNC', vnc.found())
config_host_data.set('CONFIG_VNC_JPEG', jpeg.found())
@@ -3588,7 +3601,8 @@ summary(summary_info, bool_yn: true, section: 'Crypto')
# Libraries
summary_info = {}
if targetos == 'darwin'
- summary_info += {'Cocoa support': cocoa}
+ summary_info += {'Cocoa support': cocoa}
+ summary_info += {'vmnet.framework support': vmnet}
endif
summary_info += {'SDL support': sdl}
summary_info += {'SDL image support': sdl_image}
diff --git a/meson_options.txt b/meson_options.txt
index 52b11cead4..d2c0b6b412 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -175,6 +175,8 @@ option('netmap', type : 'feature', value : 'auto',
description: 'netmap network backend support')
option('vde', type : 'feature', value : 'auto',
description: 'vde network backend support')
+option('vmnet', type : 'feature', value : 'auto',
+ description: 'vmnet.framework network backend support')
option('virglrenderer', type : 'feature', value : 'auto',
description: 'virgl rendering support')
option('vnc', type : 'feature', value : 'auto',
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 9ee684ef03..30946f3798 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -116,6 +116,7 @@ meson_options_help() {
printf "%s\n" ' usb-redir libusbredir support'
printf "%s\n" ' vde vde network backend support'
printf "%s\n" ' vdi vdi image format support'
+ printf "%s\n" ' vmnet vmnet.framework network backend support'
printf "%s\n" ' vhost-user-blk-server'
printf "%s\n" ' build vhost-user-blk server'
printf "%s\n" ' virglrenderer virgl rendering support'
--
2.34.1.vfs.0.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v16 2/7] net/vmnet: add vmnet backends to qapi/net
2022-03-14 19:15 [PATCH v16 0/7] Add vmnet.framework based network backend Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 1/7] net/vmnet: add vmnet dependency and customizable option Vladislav Yaroshchuk
@ 2022-03-14 19:15 ` Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 3/7] net/vmnet: implement shared mode (vmnet-shared) Vladislav Yaroshchuk
` (4 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Vladislav Yaroshchuk @ 2022-03-14 19:15 UTC (permalink / raw)
To: qemu-devel
Cc: jasowang, r.bolshakov, eblake, phillip.ennen, phillip,
akihiko.odaki, armbru, hsp.cat7, hello, roman, peter.maydell,
dirty, f4bug, agraf, kraxel, alex.bennee, qemu_oss,
Vladislav Yaroshchuk
Create separate netdevs for each vmnet operating mode:
- vmnet-host
- vmnet-shared
- vmnet-bridged
Signed-off-by: Vladislav Yaroshchuk <Vladislav.Yaroshchuk@jetbrains.com>
---
net/clients.h | 11 ++++
net/meson.build | 7 +++
net/net.c | 10 ++++
net/vmnet-bridged.m | 25 +++++++++
net/vmnet-common.m | 20 +++++++
net/vmnet-host.c | 24 ++++++++
net/vmnet-shared.c | 25 +++++++++
net/vmnet_int.h | 25 +++++++++
qapi/net.json | 133 +++++++++++++++++++++++++++++++++++++++++++-
9 files changed, 278 insertions(+), 2 deletions(-)
create mode 100644 net/vmnet-bridged.m
create mode 100644 net/vmnet-common.m
create mode 100644 net/vmnet-host.c
create mode 100644 net/vmnet-shared.c
create mode 100644 net/vmnet_int.h
diff --git a/net/clients.h b/net/clients.h
index 92f9b59aed..c9157789f2 100644
--- a/net/clients.h
+++ b/net/clients.h
@@ -63,4 +63,15 @@ int net_init_vhost_user(const Netdev *netdev, const char *name,
int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp);
+#ifdef CONFIG_VMNET
+int net_init_vmnet_host(const Netdev *netdev, const char *name,
+ NetClientState *peer, Error **errp);
+
+int net_init_vmnet_shared(const Netdev *netdev, const char *name,
+ NetClientState *peer, Error **errp);
+
+int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
+ NetClientState *peer, Error **errp);
+#endif /* CONFIG_VMNET */
+
#endif /* QEMU_NET_CLIENTS_H */
diff --git a/net/meson.build b/net/meson.build
index 847bc2ac85..00a88c4951 100644
--- a/net/meson.build
+++ b/net/meson.build
@@ -42,4 +42,11 @@ softmmu_ss.add(when: 'CONFIG_POSIX', if_true: files(tap_posix))
softmmu_ss.add(when: 'CONFIG_WIN32', if_true: files('tap-win32.c'))
softmmu_ss.add(when: 'CONFIG_VHOST_NET_VDPA', if_true: files('vhost-vdpa.c'))
+vmnet_files = files(
+ 'vmnet-common.m',
+ 'vmnet-bridged.m',
+ 'vmnet-host.c',
+ 'vmnet-shared.c'
+)
+softmmu_ss.add(when: vmnet, if_true: vmnet_files)
subdir('can')
diff --git a/net/net.c b/net/net.c
index f0d14dbfc1..1dbb64b935 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1021,6 +1021,11 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
#ifdef CONFIG_L2TPV3
[NET_CLIENT_DRIVER_L2TPV3] = net_init_l2tpv3,
#endif
+#ifdef CONFIG_VMNET
+ [NET_CLIENT_DRIVER_VMNET_HOST] = net_init_vmnet_host,
+ [NET_CLIENT_DRIVER_VMNET_SHARED] = net_init_vmnet_shared,
+ [NET_CLIENT_DRIVER_VMNET_BRIDGED] = net_init_vmnet_bridged,
+#endif /* CONFIG_VMNET */
};
@@ -1106,6 +1111,11 @@ void show_netdevs(void)
#endif
#ifdef CONFIG_VHOST_VDPA
"vhost-vdpa",
+#endif
+#ifdef CONFIG_VMNET
+ "vmnet-host",
+ "vmnet-shared",
+ "vmnet-bridged",
#endif
};
diff --git a/net/vmnet-bridged.m b/net/vmnet-bridged.m
new file mode 100644
index 0000000000..c735901666
--- /dev/null
+++ b/net/vmnet-bridged.m
@@ -0,0 +1,25 @@
+/*
+ * vmnet-bridged.m
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk <vladislav.yaroshchuk@jetbrains.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-types-net.h"
+#include "vmnet_int.h"
+#include "clients.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+
+#include <vmnet/vmnet.h>
+
+int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
+ NetClientState *peer, Error **errp)
+{
+ error_setg(errp, "vmnet-bridged is not implemented yet");
+ return -1;
+}
diff --git a/net/vmnet-common.m b/net/vmnet-common.m
new file mode 100644
index 0000000000..56612c72ce
--- /dev/null
+++ b/net/vmnet-common.m
@@ -0,0 +1,20 @@
+/*
+ * vmnet-common.m - network client wrapper for Apple vmnet.framework
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk <vladislav.yaroshchuk@jetbrains.com>
+ * Copyright(c) 2021 Phillip Tennen <phillip@axleos.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-types-net.h"
+#include "vmnet_int.h"
+#include "clients.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+
+#include <vmnet/vmnet.h>
+
diff --git a/net/vmnet-host.c b/net/vmnet-host.c
new file mode 100644
index 0000000000..32dc437037
--- /dev/null
+++ b/net/vmnet-host.c
@@ -0,0 +1,24 @@
+/*
+ * vmnet-host.c
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk <vladislav.yaroshchuk@jetbrains.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-types-net.h"
+#include "vmnet_int.h"
+#include "clients.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+
+#include <vmnet/vmnet.h>
+
+int net_init_vmnet_host(const Netdev *netdev, const char *name,
+ NetClientState *peer, Error **errp) {
+ error_setg(errp, "vmnet-host is not implemented yet");
+ return -1;
+}
diff --git a/net/vmnet-shared.c b/net/vmnet-shared.c
new file mode 100644
index 0000000000..f07afaaf21
--- /dev/null
+++ b/net/vmnet-shared.c
@@ -0,0 +1,25 @@
+/*
+ * vmnet-shared.c
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk <vladislav.yaroshchuk@jetbrains.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-types-net.h"
+#include "vmnet_int.h"
+#include "clients.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+
+#include <vmnet/vmnet.h>
+
+int net_init_vmnet_shared(const Netdev *netdev, const char *name,
+ NetClientState *peer, Error **errp)
+{
+ error_setg(errp, "vmnet-shared is not implemented yet");
+ return -1;
+}
diff --git a/net/vmnet_int.h b/net/vmnet_int.h
new file mode 100644
index 0000000000..aac4d5af64
--- /dev/null
+++ b/net/vmnet_int.h
@@ -0,0 +1,25 @@
+/*
+ * vmnet_int.h
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk <vladislav.yaroshchuk@jetbrains.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#ifndef VMNET_INT_H
+#define VMNET_INT_H
+
+#include "qemu/osdep.h"
+#include "vmnet_int.h"
+#include "clients.h"
+
+#include <vmnet/vmnet.h>
+
+typedef struct VmnetCommonState {
+ NetClientState nc;
+
+} VmnetCommonState;
+
+
+#endif /* VMNET_INT_H */
diff --git a/qapi/net.json b/qapi/net.json
index 7fab2e7cd8..4bff1c4e37 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -452,6 +452,120 @@
'*vhostdev': 'str',
'*queues': 'int' } }
+##
+# @NetdevVmnetHostOptions:
+#
+# vmnet (host mode) network backend.
+#
+# Allows the vmnet interface to communicate with other vmnet
+# interfaces that are in host mode and also with the host.
+#
+# @start-address: The starting IPv4 address to use for the interface.
+# Must be in the private IP range (RFC 1918). Must be
+# specified along with @end-address and @subnet-mask.
+# This address is used as the gateway address. The
+# subsequent address up to and including end-address are
+# placed in the DHCP pool.
+#
+# @end-address: The DHCP IPv4 range end address to use for the
+# interface. Must be in the private IP range (RFC 1918).
+# Must be specified along with @start-address and
+# @subnet-mask.
+#
+# @subnet-mask: The IPv4 subnet mask to use on the interface. Must
+# be specified along with @start-address and @subnet-mask.
+#
+# @isolated: Enable isolation for this interface. Interface isolation
+# ensures that vmnet interface is not able to communicate
+# with any other vmnet interfaces. Only communication with
+# host is allowed. Requires at least macOS Big Sur 11.0.
+#
+# @net-uuid: The identifier (UUID) to uniquely identify the isolated
+# network vmnet interface should be added to. If
+# set, no DHCP service is provided for this interface and
+# network communication is allowed only with other interfaces
+# added to this network identified by the UUID. Requires
+# at least macOS Big Sur 11.0.
+#
+# Since: 7.1
+##
+{ 'struct': 'NetdevVmnetHostOptions',
+ 'data': {
+ '*start-address': 'str',
+ '*end-address': 'str',
+ '*subnet-mask': 'str',
+ '*isolated': 'bool',
+ '*net-uuid': 'str' },
+ 'if': 'CONFIG_VMNET' }
+
+##
+# @NetdevVmnetSharedOptions:
+#
+# vmnet (shared mode) network backend.
+#
+# Allows traffic originating from the vmnet interface to reach the
+# Internet through a network address translator (NAT).
+# The vmnet interface can communicate with the host and with
+# other shared mode interfaces on the same subnet. If no DHCP
+# settings, subnet mask and IPv6 prefix specified, the interface can
+# communicate with any of other interfaces in shared mode.
+#
+# @start-address: The starting IPv4 address to use for the interface.
+# Must be in the private IP range (RFC 1918). Must be
+# specified along with @end-address and @subnet-mask.
+# This address is used as the gateway address. The
+# subsequent address up to and including end-address are
+# placed in the DHCP pool.
+#
+# @end-address: The DHCP IPv4 range end address to use for the
+# interface. Must be in the private IP range (RFC 1918).
+# Must be specified along with @start-address and @subnet-mask.
+#
+# @subnet-mask: The IPv4 subnet mask to use on the interface. Must
+# be specified along with @start-address and @subnet-mask.
+#
+# @isolated: Enable isolation for this interface. Interface isolation
+# ensures that vmnet interface is not able to communicate
+# with any other vmnet interfaces. Only communication with
+# host is allowed. Requires at least macOS Big Sur 11.0.
+#
+# @nat66-prefix: The IPv6 prefix to use into guest network. Must be a
+# unique local address i.e. start with fd00::/8 and have
+# length of 64.
+#
+# Since: 7.1
+##
+{ 'struct': 'NetdevVmnetSharedOptions',
+ 'data': {
+ '*start-address': 'str',
+ '*end-address': 'str',
+ '*subnet-mask': 'str',
+ '*isolated': 'bool',
+ '*nat66-prefix': 'str' },
+ 'if': 'CONFIG_VMNET' }
+
+##
+# @NetdevVmnetBridgedOptions:
+#
+# vmnet (bridged mode) network backend.
+#
+# Bridges the vmnet interface with a physical network interface.
+#
+# @ifname: The name of the physical interface to be bridged.
+#
+# @isolated: Enable isolation for this interface. Interface isolation
+# ensures that vmnet interface is not able to communicate
+# with any other vmnet interfaces. Only communication with
+# host is allowed. Requires at least macOS Big Sur 11.0.
+#
+# Since: 7.1
+##
+{ 'struct': 'NetdevVmnetBridgedOptions',
+ 'data': {
+ 'ifname': 'str',
+ '*isolated': 'bool' },
+ 'if': 'CONFIG_VMNET' }
+
##
# @NetClientDriver:
#
@@ -460,10 +574,16 @@
# Since: 2.7
#
# @vhost-vdpa since 5.1
+# @vmnet-host since 7.1
+# @vmnet-shared since 7.1
+# @vmnet-bridged since 7.1
##
{ 'enum': 'NetClientDriver',
'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
- 'bridge', 'hubport', 'netmap', 'vhost-user', 'vhost-vdpa' ] }
+ 'bridge', 'hubport', 'netmap', 'vhost-user', 'vhost-vdpa',
+ { 'name': 'vmnet-host', 'if': 'CONFIG_VMNET' },
+ { 'name': 'vmnet-shared', 'if': 'CONFIG_VMNET' },
+ { 'name': 'vmnet-bridged', 'if': 'CONFIG_VMNET' }] }
##
# @Netdev:
@@ -477,6 +597,9 @@
# Since: 1.2
#
# 'l2tpv3' - since 2.1
+# 'vmnet-host' - since 7.1
+# 'vmnet-shared' - since 7.1
+# 'vmnet-bridged' - since 7.1
##
{ 'union': 'Netdev',
'base': { 'id': 'str', 'type': 'NetClientDriver' },
@@ -492,7 +615,13 @@
'hubport': 'NetdevHubPortOptions',
'netmap': 'NetdevNetmapOptions',
'vhost-user': 'NetdevVhostUserOptions',
- 'vhost-vdpa': 'NetdevVhostVDPAOptions' } }
+ 'vhost-vdpa': 'NetdevVhostVDPAOptions',
+ 'vmnet-host': { 'type': 'NetdevVmnetHostOptions',
+ 'if': 'CONFIG_VMNET' },
+ 'vmnet-shared': { 'type': 'NetdevVmnetSharedOptions',
+ 'if': 'CONFIG_VMNET' },
+ 'vmnet-bridged': { 'type': 'NetdevVmnetBridgedOptions',
+ 'if': 'CONFIG_VMNET' } } }
##
# @RxState:
--
2.34.1.vfs.0.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v16 3/7] net/vmnet: implement shared mode (vmnet-shared)
2022-03-14 19:15 [PATCH v16 0/7] Add vmnet.framework based network backend Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 1/7] net/vmnet: add vmnet dependency and customizable option Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 2/7] net/vmnet: add vmnet backends to qapi/net Vladislav Yaroshchuk
@ 2022-03-14 19:15 ` Vladislav Yaroshchuk
2022-03-14 19:45 ` Akihiko Odaki
2022-03-14 19:15 ` [PATCH v16 4/7] net/vmnet: implement host mode (vmnet-host) Vladislav Yaroshchuk
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Vladislav Yaroshchuk @ 2022-03-14 19:15 UTC (permalink / raw)
To: qemu-devel
Cc: jasowang, r.bolshakov, eblake, phillip.ennen, phillip,
akihiko.odaki, armbru, hsp.cat7, hello, roman, peter.maydell,
dirty, f4bug, agraf, kraxel, alex.bennee, qemu_oss,
Vladislav Yaroshchuk
vmnet.framework supports iov, but writing more than
one iov into vmnet interface fails with
'VMNET_INVALID_ARGUMENT'. Collecting provided iovs into
one and passing it to vmnet works fine. That's the
reason why receive_iov() left unimplemented. But it still
works with good enough performance having .receive()
implemented only.
Signed-off-by: Phillip Tennen <phillip@axleos.com>
Signed-off-by: Vladislav Yaroshchuk <Vladislav.Yaroshchuk@jetbrains.com>
---
net/vmnet-common.m | 298 +++++++++++++++++++++++++++++++++++++++++++++
net/vmnet-shared.c | 95 ++++++++++++++-
net/vmnet_int.h | 41 ++++++-
3 files changed, 429 insertions(+), 5 deletions(-)
diff --git a/net/vmnet-common.m b/net/vmnet-common.m
index 56612c72ce..20a33d2591 100644
--- a/net/vmnet-common.m
+++ b/net/vmnet-common.m
@@ -10,6 +10,8 @@
*/
#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
+#include "qemu/log.h"
#include "qapi/qapi-types-net.h"
#include "vmnet_int.h"
#include "clients.h"
@@ -17,4 +19,300 @@
#include "qapi/error.h"
#include <vmnet/vmnet.h>
+#include <dispatch/dispatch.h>
+static bool vmnet_qemu_send_wrapper(VmnetCommonState *s);
+
+
+static void vmnet_send_completed(NetClientState *nc, ssize_t len)
+{
+ VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
+ /* Complete sending packets left in VmnetCommonState buffers */
+ s->send_enabled = vmnet_qemu_send_wrapper(s);
+}
+
+
+static bool vmnet_qemu_send_wrapper(VmnetCommonState *s) {
+ ssize_t size;
+
+ /*
+ * Packets to send lay in [current_pos..end_pos)
+ * (including current_pos, excluding end_pos)
+ */
+ while (s->packets_send_current_pos < s->packets_send_end_pos) {
+ size = qemu_send_packet_async(&s->nc,
+ s->iov_buf[s->packets_send_current_pos].iov_base,
+ s->packets_buf[s->packets_send_current_pos].vm_pkt_size,
+ vmnet_send_completed);
+ ++s->packets_send_current_pos;
+ if (size == 0) {
+ /* QEMU is not ready - wait for completion callback call */
+ return false;
+ }
+ }
+ return true;
+}
+
+
+static void vmnet_send_bh(void *opaque)
+{
+ NetClientState *nc = (NetClientState *) opaque;
+ VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
+ struct vmpktdesc *packets = s->packets_buf;
+ vmnet_return_t status;
+ int i;
+
+ /*
+ * Do nothing if QEMU is not ready - wait
+ * for completion callback invocation
+ */
+ if (!s->send_enabled) {
+ return;
+ }
+
+ /* Read as many packets as present */
+ s->packets_send_current_pos = 0;
+ s->packets_send_end_pos = VMNET_PACKETS_LIMIT;
+ for (i = 0; i < s->packets_send_end_pos; ++i) {
+ packets[i].vm_pkt_size = s->max_packet_size;
+ packets[i].vm_pkt_iovcnt = 1;
+ packets[i].vm_flags = 0;
+ }
+
+ status = vmnet_read(s->vmnet_if, packets, &s->packets_send_end_pos);
+ if (status != VMNET_SUCCESS) {
+ error_printf("vmnet: read failed: %s\n",
+ vmnet_status_map_str(status));
+ s->packets_send_current_pos = 0;
+ s->packets_send_end_pos = 0;
+ return;
+ }
+
+ /* Send packets to QEMU */
+ s->send_enabled = vmnet_qemu_send_wrapper(s);
+}
+
+
+static void vmnet_bufs_init(VmnetCommonState *s)
+{
+ struct vmpktdesc *packets = s->packets_buf;
+ struct iovec *iov = s->iov_buf;
+ int i;
+
+ for (i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
+ iov[i].iov_len = s->max_packet_size;
+ iov[i].iov_base = g_malloc0(iov[i].iov_len);
+ packets[i].vm_pkt_iov = iov + i;
+ }
+}
+
+
+const char *vmnet_status_map_str(vmnet_return_t status)
+{
+ switch (status) {
+ case VMNET_SUCCESS:
+ return "success";
+ case VMNET_FAILURE:
+ return "general failure (possibly not enough privileges)";
+ case VMNET_MEM_FAILURE:
+ return "memory allocation failure";
+ case VMNET_INVALID_ARGUMENT:
+ return "invalid argument specified";
+ case VMNET_SETUP_INCOMPLETE:
+ return "interface setup is not complete";
+ case VMNET_INVALID_ACCESS:
+ return "invalid access, permission denied";
+ case VMNET_PACKET_TOO_BIG:
+ return "packet size is larger than MTU";
+ case VMNET_BUFFER_EXHAUSTED:
+ return "buffers exhausted in kernel";
+ case VMNET_TOO_MANY_PACKETS:
+ return "packet count exceeds limit";
+#if defined(MAC_OS_VERSION_11_0) && \
+ MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
+ case VMNET_SHARING_SERVICE_BUSY:
+ return "conflict, sharing service is in use";
+#endif
+ default:
+ return "unknown vmnet error";
+ }
+}
+
+
+int vmnet_if_create(NetClientState *nc,
+ xpc_object_t if_desc,
+ Error **errp)
+{
+ VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
+ dispatch_semaphore_t if_created_sem = dispatch_semaphore_create(0);
+ __block vmnet_return_t if_status;
+
+ s->if_queue = dispatch_queue_create(
+ "org.qemu.vmnet.if_queue",
+ DISPATCH_QUEUE_SERIAL
+ );
+
+ xpc_dictionary_set_bool(
+ if_desc,
+ vmnet_allocate_mac_address_key,
+ false
+ );
+
+#ifdef DEBUG
+ qemu_log("vmnet.start.interface_desc:\n");
+ xpc_dictionary_apply(if_desc,
+ ^bool(const char *k, xpc_object_t v) {
+ char *desc = xpc_copy_description(v);
+ qemu_log(" %s=%s\n", k, desc);
+ free(desc);
+ return true;
+ });
+#endif /* DEBUG */
+
+ s->vmnet_if = vmnet_start_interface(
+ if_desc,
+ s->if_queue,
+ ^(vmnet_return_t status, xpc_object_t interface_param) {
+ if_status = status;
+ if (status != VMNET_SUCCESS || !interface_param) {
+ dispatch_semaphore_signal(if_created_sem);
+ return;
+ }
+
+#ifdef DEBUG
+ qemu_log("vmnet.start.interface_param:\n");
+ xpc_dictionary_apply(interface_param,
+ ^bool(const char *k, xpc_object_t v) {
+ char *desc = xpc_copy_description(v);
+ qemu_log(" %s=%s\n", k, desc);
+ free(desc);
+ return true;
+ });
+#endif /* DEBUG */
+
+ s->mtu = xpc_dictionary_get_uint64(
+ interface_param,
+ vmnet_mtu_key);
+ s->max_packet_size = xpc_dictionary_get_uint64(
+ interface_param,
+ vmnet_max_packet_size_key);
+
+ dispatch_semaphore_signal(if_created_sem);
+ });
+
+ if (s->vmnet_if == NULL) {
+ dispatch_release(s->if_queue);
+ dispatch_release(if_created_sem);
+ error_setg(errp,
+ "unable to create interface with requested params");
+ return -1;
+ }
+
+ dispatch_semaphore_wait(if_created_sem, DISPATCH_TIME_FOREVER);
+ dispatch_release(if_created_sem);
+
+ if (if_status != VMNET_SUCCESS) {
+ dispatch_release(s->if_queue);
+ error_setg(errp,
+ "cannot create vmnet interface: %s",
+ vmnet_status_map_str(if_status));
+ return -1;
+ }
+
+ s->send_bh = aio_bh_new(qemu_get_aio_context(), vmnet_send_bh, nc);
+ s->send_enabled = true;
+ vmnet_bufs_init(s);
+
+ vmnet_interface_set_event_callback(
+ s->vmnet_if,
+ VMNET_INTERFACE_PACKETS_AVAILABLE,
+ s->if_queue,
+ ^(interface_event_t event_id, xpc_object_t event) {
+ assert(event_id == VMNET_INTERFACE_PACKETS_AVAILABLE);
+ /*
+ * This function is being called from a non qemu thread, so
+ * we only schedule a BH, and do the rest of the io completion
+ * handling from vmnet_send_bh() which runs in a qemu context.
+ */
+ qemu_bh_schedule(s->send_bh);
+ });
+
+ return 0;
+}
+
+
+ssize_t vmnet_receive_common(NetClientState *nc,
+ const uint8_t *buf,
+ size_t size)
+{
+ VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
+ struct vmpktdesc packet;
+ struct iovec iov;
+ int pkt_cnt;
+ vmnet_return_t if_status;
+
+ if (size > s->max_packet_size) {
+ warn_report("vmnet: packet is too big, %zu > %" PRIu64,
+ packet.vm_pkt_size,
+ s->max_packet_size);
+ return -1;
+ }
+
+ iov.iov_base = (char *) buf;
+ iov.iov_len = size;
+
+ packet.vm_pkt_iovcnt = 1;
+ packet.vm_flags = 0;
+ packet.vm_pkt_size = size;
+ packet.vm_pkt_iov = &iov;
+ pkt_cnt = 1;
+
+ if_status = vmnet_write(s->vmnet_if, &packet, &pkt_cnt);
+ if (if_status != VMNET_SUCCESS) {
+ error_report("vmnet: write error: %s\n",
+ vmnet_status_map_str(if_status));
+ return -1;
+ }
+
+ if (if_status == VMNET_SUCCESS && pkt_cnt) {
+ return size;
+ }
+ return 0;
+}
+
+
+void vmnet_cleanup_common(NetClientState *nc)
+{
+ VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
+ dispatch_semaphore_t if_stopped_sem;
+
+ if (s->vmnet_if == NULL) {
+ return;
+ }
+
+ vmnet_interface_set_event_callback(
+ s->vmnet_if,
+ VMNET_INTERFACE_PACKETS_AVAILABLE,
+ NULL,
+ NULL);
+
+ qemu_purge_queued_packets(nc);
+
+ if_stopped_sem = dispatch_semaphore_create(0);
+ vmnet_stop_interface(
+ s->vmnet_if,
+ s->if_queue,
+ ^(vmnet_return_t status) {
+ assert(status == VMNET_SUCCESS);
+ dispatch_semaphore_signal(if_stopped_sem);
+ });
+ dispatch_semaphore_wait(if_stopped_sem, DISPATCH_TIME_FOREVER);
+
+ qemu_bh_delete(s->send_bh);
+ dispatch_release(if_stopped_sem);
+ dispatch_release(s->if_queue);
+
+ for (int i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
+ g_free(s->iov_buf[i].iov_base);
+ }
+}
diff --git a/net/vmnet-shared.c b/net/vmnet-shared.c
index f07afaaf21..2f4eb1db2d 100644
--- a/net/vmnet-shared.c
+++ b/net/vmnet-shared.c
@@ -10,16 +10,103 @@
#include "qemu/osdep.h"
#include "qapi/qapi-types-net.h"
+#include "qapi/error.h"
#include "vmnet_int.h"
#include "clients.h"
-#include "qemu/error-report.h"
-#include "qapi/error.h"
#include <vmnet/vmnet.h>
+typedef struct VmnetSharedState {
+ VmnetCommonState cs;
+} VmnetSharedState;
+
+
+static bool validate_options(const Netdev *netdev, Error **errp)
+{
+ const NetdevVmnetSharedOptions *options = &(netdev->u.vmnet_shared);
+
+#if !defined(MAC_OS_VERSION_11_0) || \
+ MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_VERSION_11_0
+ if (options->has_isolated) {
+ error_setg(errp,
+ "vmnet-shared.isolated feature is "
+ "unavailable: outdated vmnet.framework API");
+ return false;
+ }
+#endif
+
+ if ((options->has_start_address ||
+ options->has_end_address ||
+ options->has_subnet_mask) &&
+ !(options->has_start_address &&
+ options->has_end_address &&
+ options->has_subnet_mask)) {
+ error_setg(errp,
+ "'start-address', 'end-address', 'subnet-mask' "
+ "should be provided together"
+ );
+ return false;
+ }
+
+ return true;
+}
+
+static xpc_object_t build_if_desc(const Netdev *netdev)
+{
+ const NetdevVmnetSharedOptions *options = &(netdev->u.vmnet_shared);
+ xpc_object_t if_desc = xpc_dictionary_create(NULL, NULL, 0);
+
+ xpc_dictionary_set_uint64(
+ if_desc,
+ vmnet_operation_mode_key,
+ VMNET_SHARED_MODE
+ );
+
+ if (options->has_nat66_prefix) {
+ xpc_dictionary_set_string(if_desc,
+ vmnet_nat66_prefix_key,
+ options->nat66_prefix);
+ }
+
+ if (options->has_start_address) {
+ xpc_dictionary_set_string(if_desc,
+ vmnet_start_address_key,
+ options->start_address);
+ xpc_dictionary_set_string(if_desc,
+ vmnet_end_address_key,
+ options->end_address);
+ xpc_dictionary_set_string(if_desc,
+ vmnet_subnet_mask_key,
+ options->subnet_mask);
+ }
+
+#if defined(MAC_OS_VERSION_11_0) && \
+ MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
+ xpc_dictionary_set_bool(
+ if_desc,
+ vmnet_enable_isolation_key,
+ options->isolated
+ );
+#endif
+
+ return if_desc;
+}
+
+static NetClientInfo net_vmnet_shared_info = {
+ .type = NET_CLIENT_DRIVER_VMNET_SHARED,
+ .size = sizeof(VmnetSharedState),
+ .receive = vmnet_receive_common,
+ .cleanup = vmnet_cleanup_common,
+};
+
int net_init_vmnet_shared(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
- error_setg(errp, "vmnet-shared is not implemented yet");
- return -1;
+ NetClientState *nc = qemu_new_net_client(&net_vmnet_shared_info,
+ peer, "vmnet-shared", name);
+ if (!validate_options(netdev, errp)) {
+ g_assert_not_reached();
+ return -1;
+ }
+ return vmnet_if_create(nc, build_if_desc(netdev), errp);
}
diff --git a/net/vmnet_int.h b/net/vmnet_int.h
index aac4d5af64..8f3321ef3e 100644
--- a/net/vmnet_int.h
+++ b/net/vmnet_int.h
@@ -15,11 +15,50 @@
#include "clients.h"
#include <vmnet/vmnet.h>
+#include <dispatch/dispatch.h>
+
+/**
+ * From vmnet.framework documentation
+ *
+ * Each read/write call allows up to 200 packets to be
+ * read or written for a maximum of 256KB.
+ *
+ * Each packet written should be a complete
+ * ethernet frame.
+ *
+ * https://developer.apple.com/documentation/vmnet
+ */
+#define VMNET_PACKETS_LIMIT 200
typedef struct VmnetCommonState {
- NetClientState nc;
+ NetClientState nc;
+ interface_ref vmnet_if;
+
+ uint64_t mtu;
+ uint64_t max_packet_size;
+ dispatch_queue_t if_queue;
+
+ QEMUBH *send_bh;
+ bool send_enabled;
+
+ struct vmpktdesc packets_buf[VMNET_PACKETS_LIMIT];
+ int packets_send_current_pos;
+ int packets_send_end_pos;
+
+ struct iovec iov_buf[VMNET_PACKETS_LIMIT];
} VmnetCommonState;
+const char *vmnet_status_map_str(vmnet_return_t status);
+
+int vmnet_if_create(NetClientState *nc,
+ xpc_object_t if_desc,
+ Error **errp);
+
+ssize_t vmnet_receive_common(NetClientState *nc,
+ const uint8_t *buf,
+ size_t size);
+
+void vmnet_cleanup_common(NetClientState *nc);
#endif /* VMNET_INT_H */
--
2.34.1.vfs.0.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v16 4/7] net/vmnet: implement host mode (vmnet-host)
2022-03-14 19:15 [PATCH v16 0/7] Add vmnet.framework based network backend Vladislav Yaroshchuk
` (2 preceding siblings ...)
2022-03-14 19:15 ` [PATCH v16 3/7] net/vmnet: implement shared mode (vmnet-shared) Vladislav Yaroshchuk
@ 2022-03-14 19:15 ` Vladislav Yaroshchuk
2022-03-14 19:55 ` Akihiko Odaki
2022-03-14 19:15 ` [PATCH v16 5/7] net/vmnet: implement bridged mode (vmnet-bridged) Vladislav Yaroshchuk
` (2 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Vladislav Yaroshchuk @ 2022-03-14 19:15 UTC (permalink / raw)
To: qemu-devel
Cc: jasowang, r.bolshakov, eblake, phillip.ennen, phillip,
akihiko.odaki, armbru, hsp.cat7, hello, roman, peter.maydell,
dirty, f4bug, agraf, kraxel, alex.bennee, qemu_oss,
Vladislav Yaroshchuk
Signed-off-by: Vladislav Yaroshchuk <Vladislav.Yaroshchuk@jetbrains.com>
---
net/vmnet-host.c | 116 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 110 insertions(+), 6 deletions(-)
diff --git a/net/vmnet-host.c b/net/vmnet-host.c
index 32dc437037..15a832701a 100644
--- a/net/vmnet-host.c
+++ b/net/vmnet-host.c
@@ -9,16 +9,120 @@
*/
#include "qemu/osdep.h"
+#include "qemu/uuid.h"
#include "qapi/qapi-types-net.h"
-#include "vmnet_int.h"
-#include "clients.h"
-#include "qemu/error-report.h"
#include "qapi/error.h"
+#include "clients.h"
+#include "vmnet_int.h"
#include <vmnet/vmnet.h>
+typedef struct VmnetHostState {
+ VmnetCommonState cs;
+ QemuUUID network_uuid;
+} VmnetHostState;
+
+static bool validate_options(const Netdev *netdev, Error **errp)
+{
+ const NetdevVmnetHostOptions *options = &(netdev->u.vmnet_host);
+ QemuUUID uuid;
+
+#if defined(MAC_OS_VERSION_11_0) && \
+ MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
+
+ if (options->has_net_uuid &&
+ qemu_uuid_parse(options->net_uuid, &uuid) < 0) {
+ error_setg(errp, "Invalid UUID provided in 'net-uuid'");
+ return false;
+ }
+#else
+ if (options->has_isolated) {
+ error_setg(errp,
+ "vmnet-host.isolated feature is "
+ "unavailable: outdated vmnet.framework API");
+ return false;
+ }
+
+ if (options->has_net_uuid) {
+ error_setg(errp,
+ "vmnet-host.net-uuid feature is "
+ "unavailable: outdated vmnet.framework API");
+ return false;
+ }
+#endif
+
+ if ((options->has_start_address ||
+ options->has_end_address ||
+ options->has_subnet_mask) &&
+ !(options->has_start_address &&
+ options->has_end_address &&
+ options->has_subnet_mask)) {
+ error_setg(errp,
+ "'start-address', 'end-address', 'subnet-mask' "
+ "should be provided together");
+ return false;
+ }
+
+ return true;
+}
+
+static xpc_object_t build_if_desc(const Netdev *netdev,
+ NetClientState *nc)
+{
+ const NetdevVmnetHostOptions *options = &(netdev->u.vmnet_host);
+ VmnetCommonState *cs = DO_UPCAST(VmnetCommonState, nc, nc);
+ VmnetHostState *hs = DO_UPCAST(VmnetHostState, cs, cs);
+ xpc_object_t if_desc = xpc_dictionary_create(NULL, NULL, 0);
+
+ xpc_dictionary_set_uint64(if_desc,
+ vmnet_operation_mode_key,
+ VMNET_HOST_MODE);
+
+#if defined(MAC_OS_VERSION_11_0) && \
+ MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
+
+ xpc_dictionary_set_bool(if_desc,
+ vmnet_enable_isolation_key,
+ options->isolated);
+
+ if (options->has_net_uuid) {
+ qemu_uuid_parse(options->net_uuid, &hs->network_uuid);
+ xpc_dictionary_set_uuid(if_desc,
+ vmnet_network_identifier_key,
+ hs->network_uuid.data);
+ }
+#endif
+
+ if (options->has_start_address) {
+ xpc_dictionary_set_string(if_desc,
+ vmnet_start_address_key,
+ options->start_address);
+ xpc_dictionary_set_string(if_desc,
+ vmnet_end_address_key,
+ options->end_address);
+ xpc_dictionary_set_string(if_desc,
+ vmnet_subnet_mask_key,
+ options->subnet_mask);
+ }
+
+ return if_desc;
+}
+
+static NetClientInfo net_vmnet_host_info = {
+ .type = NET_CLIENT_DRIVER_VMNET_HOST,
+ .size = sizeof(VmnetHostState),
+ .receive = vmnet_receive_common,
+ .cleanup = vmnet_cleanup_common,
+};
+
int net_init_vmnet_host(const Netdev *netdev, const char *name,
- NetClientState *peer, Error **errp) {
- error_setg(errp, "vmnet-host is not implemented yet");
- return -1;
+ NetClientState *peer, Error **errp)
+{
+ NetClientState *nc = qemu_new_net_client(&net_vmnet_host_info,
+ peer, "vmnet-host", name);
+ if (!validate_options(netdev, errp)) {
+ g_assert_not_reached();
+ return -1;
+ }
+ return vmnet_if_create(nc, build_if_desc(netdev, nc), errp);
}
--
2.34.1.vfs.0.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v16 5/7] net/vmnet: implement bridged mode (vmnet-bridged)
2022-03-14 19:15 [PATCH v16 0/7] Add vmnet.framework based network backend Vladislav Yaroshchuk
` (3 preceding siblings ...)
2022-03-14 19:15 ` [PATCH v16 4/7] net/vmnet: implement host mode (vmnet-host) Vladislav Yaroshchuk
@ 2022-03-14 19:15 ` Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 6/7] net/vmnet: update qemu-options.hx Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 7/7] net/vmnet: update hmp-commands.hx Vladislav Yaroshchuk
6 siblings, 0 replies; 16+ messages in thread
From: Vladislav Yaroshchuk @ 2022-03-14 19:15 UTC (permalink / raw)
To: qemu-devel
Cc: jasowang, r.bolshakov, eblake, phillip.ennen, phillip,
akihiko.odaki, armbru, hsp.cat7, hello, roman, peter.maydell,
dirty, f4bug, agraf, kraxel, alex.bennee, qemu_oss,
Vladislav Yaroshchuk
Signed-off-by: Vladislav Yaroshchuk <Vladislav.Yaroshchuk@jetbrains.com>
---
net/vmnet-bridged.m | 134 ++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 129 insertions(+), 5 deletions(-)
diff --git a/net/vmnet-bridged.m b/net/vmnet-bridged.m
index c735901666..a23e03c40d 100644
--- a/net/vmnet-bridged.m
+++ b/net/vmnet-bridged.m
@@ -10,16 +10,140 @@
#include "qemu/osdep.h"
#include "qapi/qapi-types-net.h"
-#include "vmnet_int.h"
-#include "clients.h"
-#include "qemu/error-report.h"
#include "qapi/error.h"
+#include "clients.h"
+#include "vmnet_int.h"
#include <vmnet/vmnet.h>
+
+typedef struct VmnetBridgedState {
+ VmnetCommonState cs;
+} VmnetBridgedState;
+
+
+static bool validate_ifname(const char *ifname)
+{
+ xpc_object_t shared_if_list = vmnet_copy_shared_interface_list();
+ bool match = false;
+ if (!xpc_array_get_count(shared_if_list)) {
+ goto done;
+ }
+
+ match = !xpc_array_apply(
+ shared_if_list,
+ ^bool(size_t index, xpc_object_t value) {
+ return strcmp(xpc_string_get_string_ptr(value), ifname) != 0;
+ });
+
+done:
+ xpc_release(shared_if_list);
+ return match;
+}
+
+
+static bool get_valid_ifnames(char *output_buf)
+{
+ xpc_object_t shared_if_list = vmnet_copy_shared_interface_list();
+ __block const char *ifname = NULL;
+ __block int str_offset = 0;
+ bool interfaces_available = true;
+
+ if (!xpc_array_get_count(shared_if_list)) {
+ interfaces_available = false;
+ goto done;
+ }
+
+ xpc_array_apply(
+ shared_if_list,
+ ^bool(size_t index, xpc_object_t value) {
+ /* build list of strings like "en0 en1 en2 " */
+ ifname = xpc_string_get_string_ptr(value);
+ strcpy(output_buf + str_offset, ifname);
+ strcpy(output_buf + str_offset + strlen(ifname), " ");
+ str_offset += strlen(ifname) + 1;
+ return true;
+ });
+
+done:
+ xpc_release(shared_if_list);
+ return interfaces_available;
+}
+
+
+static bool validate_options(const Netdev *netdev, Error **errp)
+{
+ const NetdevVmnetBridgedOptions *options = &(netdev->u.vmnet_bridged);
+ char ifnames[1024];
+
+ if (!validate_ifname(options->ifname)) {
+ if (get_valid_ifnames(ifnames)) {
+ error_setg(errp,
+ "unsupported ifname '%s', expected one of [ %s]",
+ options->ifname,
+ ifnames);
+ return false;
+ }
+ error_setg(errp,
+ "unsupported ifname '%s', no supported "
+ "interfaces available",
+ options->ifname);
+ return false;
+ }
+
+#if !defined(MAC_OS_VERSION_11_0) || \
+ MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_VERSION_11_0
+ if (options->has_isolated) {
+ error_setg(errp,
+ "vmnet-bridged.isolated feature is "
+ "unavailable: outdated vmnet.framework API");
+ return false;
+ }
+#endif
+ return true;
+}
+
+
+static xpc_object_t build_if_desc(const Netdev *netdev)
+{
+ const NetdevVmnetBridgedOptions *options = &(netdev->u.vmnet_bridged);
+ xpc_object_t if_desc = xpc_dictionary_create(NULL, NULL, 0);
+
+ xpc_dictionary_set_uint64(if_desc,
+ vmnet_operation_mode_key,
+ VMNET_BRIDGED_MODE
+ );
+
+ xpc_dictionary_set_string(if_desc,
+ vmnet_shared_interface_name_key,
+ options->ifname);
+
+#if defined(MAC_OS_VERSION_11_0) && \
+ MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
+ xpc_dictionary_set_bool(if_desc,
+ vmnet_enable_isolation_key,
+ options->isolated);
+#endif
+ return if_desc;
+}
+
+
+static NetClientInfo net_vmnet_bridged_info = {
+ .type = NET_CLIENT_DRIVER_VMNET_BRIDGED,
+ .size = sizeof(VmnetBridgedState),
+ .receive = vmnet_receive_common,
+ .cleanup = vmnet_cleanup_common,
+};
+
+
int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
- error_setg(errp, "vmnet-bridged is not implemented yet");
- return -1;
+ NetClientState *nc = qemu_new_net_client(&net_vmnet_bridged_info,
+ peer, "vmnet-bridged", name);
+ if (!validate_options(netdev, errp)) {
+ g_assert_not_reached();
+ return -1;
+ }
+ return vmnet_if_create(nc, build_if_desc(netdev), errp);
}
--
2.34.1.vfs.0.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v16 6/7] net/vmnet: update qemu-options.hx
2022-03-14 19:15 [PATCH v16 0/7] Add vmnet.framework based network backend Vladislav Yaroshchuk
` (4 preceding siblings ...)
2022-03-14 19:15 ` [PATCH v16 5/7] net/vmnet: implement bridged mode (vmnet-bridged) Vladislav Yaroshchuk
@ 2022-03-14 19:15 ` Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 7/7] net/vmnet: update hmp-commands.hx Vladislav Yaroshchuk
6 siblings, 0 replies; 16+ messages in thread
From: Vladislav Yaroshchuk @ 2022-03-14 19:15 UTC (permalink / raw)
To: qemu-devel
Cc: jasowang, r.bolshakov, eblake, phillip.ennen, phillip,
akihiko.odaki, armbru, hsp.cat7, hello, roman, peter.maydell,
dirty, f4bug, agraf, kraxel, alex.bennee, qemu_oss,
Vladislav Yaroshchuk
Signed-off-by: Vladislav Yaroshchuk <Vladislav.Yaroshchuk@jetbrains.com>
---
qemu-options.hx | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/qemu-options.hx b/qemu-options.hx
index 5ce0ada75e..ea00d0eeb6 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2743,6 +2743,25 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
#ifdef __linux__
"-netdev vhost-vdpa,id=str,vhostdev=/path/to/dev\n"
" configure a vhost-vdpa network,Establish a vhost-vdpa netdev\n"
+#endif
+#ifdef CONFIG_VMNET
+ "-netdev vmnet-host,id=str[,isolated=on|off][,net-uuid=uuid]\n"
+ " [,start-address=addr,end-address=addr,subnet-mask=mask]\n"
+ " configure a vmnet network backend in host mode with ID 'str',\n"
+ " isolate this interface from others with 'isolated',\n"
+ " configure the address range and choose a subnet mask,\n"
+ " specify network UUID 'uuid' to disable DHCP and interact with\n"
+ " vmnet-host interfaces within this isolated network\n"
+ "-netdev vmnet-shared,id=str[,isolated=on|off][,nat66-prefix=addr]\n"
+ " [,start-address=addr,end-address=addr,subnet-mask=mask]\n"
+ " configure a vmnet network backend in shared mode with ID 'str',\n"
+ " configure the address range and choose a subnet mask,\n"
+ " set IPv6 ULA prefix (of length 64) to use for internal network,\n"
+ " isolate this interface from others with 'isolated'\n"
+ "-netdev vmnet-bridged,id=str,ifname=name[,isolated=on|off]\n"
+ " configure a vmnet network backend in bridged mode with ID 'str',\n"
+ " use 'ifname=name' to select a physical network interface to be bridged,\n"
+ " isolate this interface from others with 'isolated'\n"
#endif
"-netdev hubport,id=str,hubid=n[,netdev=nd]\n"
" configure a hub port on the hub with ID 'n'\n", QEMU_ARCH_ALL)
@@ -2762,6 +2781,9 @@ DEF("nic", HAS_ARG, QEMU_OPTION_nic,
#endif
#ifdef CONFIG_POSIX
"vhost-user|"
+#endif
+#ifdef CONFIG_VMNET
+ "vmnet-host|vmnet-shared|vmnet-bridged|"
#endif
"socket][,option][,...][mac=macaddr]\n"
" initialize an on-board / default host NIC (using MAC address\n"
@@ -2784,6 +2806,9 @@ DEF("net", HAS_ARG, QEMU_OPTION_net,
#endif
#ifdef CONFIG_NETMAP
"netmap|"
+#endif
+#ifdef CONFIG_VMNET
+ "vmnet-host|vmnet-shared|vmnet-bridged|"
#endif
"socket][,option][,option][,...]\n"
" old way to initialize a host network interface\n"
--
2.34.1.vfs.0.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v16 7/7] net/vmnet: update hmp-commands.hx
2022-03-14 19:15 [PATCH v16 0/7] Add vmnet.framework based network backend Vladislav Yaroshchuk
` (5 preceding siblings ...)
2022-03-14 19:15 ` [PATCH v16 6/7] net/vmnet: update qemu-options.hx Vladislav Yaroshchuk
@ 2022-03-14 19:15 ` Vladislav Yaroshchuk
6 siblings, 0 replies; 16+ messages in thread
From: Vladislav Yaroshchuk @ 2022-03-14 19:15 UTC (permalink / raw)
To: qemu-devel
Cc: jasowang, r.bolshakov, eblake, phillip.ennen, phillip,
akihiko.odaki, armbru, hsp.cat7, hello, roman, peter.maydell,
dirty, f4bug, agraf, kraxel, alex.bennee, qemu_oss,
Vladislav Yaroshchuk
Signed-off-by: Vladislav Yaroshchuk <Vladislav.Yaroshchuk@jetbrains.com>
---
hmp-commands.hx | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8476277aa9..8f3d78f177 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1265,7 +1265,11 @@ ERST
{
.name = "netdev_add",
.args_type = "netdev:O",
- .params = "[user|tap|socket|vde|bridge|hubport|netmap|vhost-user],id=str[,prop=value][,...]",
+ .params = "[user|tap|socket|vde|bridge|hubport|netmap|vhost-user"
+#ifdef CONFIG_VMNET
+ "|vmnet-host|vmnet-shared|vmnet-bridged"
+#endif
+ "],id=str[,prop=value][,...]",
.help = "add host network device",
.cmd = hmp_netdev_add,
.command_completion = netdev_add_completion,
--
2.34.1.vfs.0.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v16 3/7] net/vmnet: implement shared mode (vmnet-shared)
2022-03-14 19:15 ` [PATCH v16 3/7] net/vmnet: implement shared mode (vmnet-shared) Vladislav Yaroshchuk
@ 2022-03-14 19:45 ` Akihiko Odaki
2022-03-14 21:50 ` Vladislav Yaroshchuk
0 siblings, 1 reply; 16+ messages in thread
From: Akihiko Odaki @ 2022-03-14 19:45 UTC (permalink / raw)
To: Vladislav Yaroshchuk, qemu-devel
Cc: peter.maydell, alex.bennee, jasowang, phillip.ennen, armbru,
dirty, f4bug, r.bolshakov, agraf, phillip, roman, hsp.cat7,
hello, qemu_oss, eblake, kraxel
On 2022/03/15 4:15, Vladislav Yaroshchuk wrote:
> vmnet.framework supports iov, but writing more than
> one iov into vmnet interface fails with
> 'VMNET_INVALID_ARGUMENT'. Collecting provided iovs into
> one and passing it to vmnet works fine. That's the
> reason why receive_iov() left unimplemented. But it still
> works with good enough performance having .receive()
> implemented only.
>
> Signed-off-by: Phillip Tennen <phillip@axleos.com>
> Signed-off-by: Vladislav Yaroshchuk <Vladislav.Yaroshchuk@jetbrains.com>
> ---
> net/vmnet-common.m | 298 +++++++++++++++++++++++++++++++++++++++++++++
> net/vmnet-shared.c | 95 ++++++++++++++-
> net/vmnet_int.h | 41 ++++++-
> 3 files changed, 429 insertions(+), 5 deletions(-)
>
> diff --git a/net/vmnet-common.m b/net/vmnet-common.m
> index 56612c72ce..20a33d2591 100644
> --- a/net/vmnet-common.m
> +++ b/net/vmnet-common.m
> @@ -10,6 +10,8 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
> +#include "qemu/log.h"
> #include "qapi/qapi-types-net.h"
> #include "vmnet_int.h"
> #include "clients.h"
> @@ -17,4 +19,300 @@
> #include "qapi/error.h"
>
> #include <vmnet/vmnet.h>
> +#include <dispatch/dispatch.h>
>
> +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s);
The names of vmnet_qemu_send_wrapper and vmnet_send_bh does not tell
them apart well. Since only vmnet_send_bh does reading, its name may
include "read" to clarify that. "wrapper" in vmnet_qemu_send_wrapper may
be also misleading as it does more than just calling the underlying QEMU
facility, but it also updates VmnetCommonState.
> +
> +
> +static void vmnet_send_completed(NetClientState *nc, ssize_t len)
> +{
> + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> + /* Complete sending packets left in VmnetCommonState buffers */
> + s->send_enabled = vmnet_qemu_send_wrapper(s);
It must qemu_bh_schedule(s->send_bh) after vmnet_qemu_send_wrapper.
Also, send_enabled flag can be removed as explained in:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html
> send_enabled can be eliminated. When it is enabled, packets_send_pos
> and packets_batch_size must be equal. They must not be equal
> otherwise. packets_send_pos must represent the position of the packet
> which is not sent yet, possibly in the process of sending.
> vmnet_send_completed must call qemu_send_wrapper before scheduling
> send_bh. bh_send should do nothing if s->packets_send_pos <
> s->packets_batch_size.
> +}
> +
> +
> +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s) {
> + ssize_t size;
> +
> + /*
> + * Packets to send lay in [current_pos..end_pos)
> + * (including current_pos, excluding end_pos)
> + */
> + while (s->packets_send_current_pos < s->packets_send_end_pos) {
> + size = qemu_send_packet_async(&s->nc,
> + s->iov_buf[s->packets_send_current_pos].iov_base,
> + s->packets_buf[s->packets_send_current_pos].vm_pkt_size,
> + vmnet_send_completed);
> + ++s->packets_send_current_pos;
> + if (size == 0) {
> + /* QEMU is not ready - wait for completion callback call */
> + return false;
> + }
> + }
> + return true;
> +}
> +
> +
> +static void vmnet_send_bh(void *opaque)
> +{
> + NetClientState *nc = (NetClientState *) opaque;
> + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> + struct vmpktdesc *packets = s->packets_buf;
> + vmnet_return_t status;
> + int i;
> +
> + /*
> + * Do nothing if QEMU is not ready - wait
> + * for completion callback invocation
> + */
> + if (!s->send_enabled) {
> + return;
> + }
> +
> + /* Read as many packets as present */
> + s->packets_send_current_pos = 0;
> + s->packets_send_end_pos = VMNET_PACKETS_LIMIT;
> + for (i = 0; i < s->packets_send_end_pos; ++i) {
> + packets[i].vm_pkt_size = s->max_packet_size;
> + packets[i].vm_pkt_iovcnt = 1;
> + packets[i].vm_flags = 0;
> + }
> +
> + status = vmnet_read(s->vmnet_if, packets, &s->packets_send_end_pos);
> + if (status != VMNET_SUCCESS) {
> + error_printf("vmnet: read failed: %s\n",
> + vmnet_status_map_str(status));
> + s->packets_send_current_pos = 0;
> + s->packets_send_end_pos = 0;
> + return;
> + }
> +
> + /* Send packets to QEMU */
> + s->send_enabled = vmnet_qemu_send_wrapper(s);
> +}
> +
> +
> +static void vmnet_bufs_init(VmnetCommonState *s)
> +{
> + struct vmpktdesc *packets = s->packets_buf;
> + struct iovec *iov = s->iov_buf;
> + int i;
> +
> + for (i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
> + iov[i].iov_len = s->max_packet_size;
> + iov[i].iov_base = g_malloc0(iov[i].iov_len);
> + packets[i].vm_pkt_iov = iov + i;
> + }
> +}
> +
> +
> +const char *vmnet_status_map_str(vmnet_return_t status)
> +{
> + switch (status) {
> + case VMNET_SUCCESS:
> + return "success";
> + case VMNET_FAILURE:
> + return "general failure (possibly not enough privileges)";
> + case VMNET_MEM_FAILURE:
> + return "memory allocation failure";
> + case VMNET_INVALID_ARGUMENT:
> + return "invalid argument specified";
> + case VMNET_SETUP_INCOMPLETE:
> + return "interface setup is not complete";
> + case VMNET_INVALID_ACCESS:
> + return "invalid access, permission denied";
> + case VMNET_PACKET_TOO_BIG:
> + return "packet size is larger than MTU";
> + case VMNET_BUFFER_EXHAUSTED:
> + return "buffers exhausted in kernel";
> + case VMNET_TOO_MANY_PACKETS:
> + return "packet count exceeds limit";
> +#if defined(MAC_OS_VERSION_11_0) && \
> + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> + case VMNET_SHARING_SERVICE_BUSY:
> + return "conflict, sharing service is in use";
> +#endif
> + default:
> + return "unknown vmnet error";
> + }
> +}
> +
> +
> +int vmnet_if_create(NetClientState *nc,
> + xpc_object_t if_desc,
> + Error **errp)
> +{
> + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> + dispatch_semaphore_t if_created_sem = dispatch_semaphore_create(0);
> + __block vmnet_return_t if_status;
> +
> + s->if_queue = dispatch_queue_create(
> + "org.qemu.vmnet.if_queue",
> + DISPATCH_QUEUE_SERIAL
> + );
> +
> + xpc_dictionary_set_bool(
> + if_desc,
> + vmnet_allocate_mac_address_key,
> + false
> + );
> +
> +#ifdef DEBUG
> + qemu_log("vmnet.start.interface_desc:\n");
> + xpc_dictionary_apply(if_desc,
> + ^bool(const char *k, xpc_object_t v) {
> + char *desc = xpc_copy_description(v);
> + qemu_log(" %s=%s\n", k, desc);
> + free(desc);
> + return true;
> + });
> +#endif /* DEBUG */
> +
> + s->vmnet_if = vmnet_start_interface(
> + if_desc,
> + s->if_queue,
> + ^(vmnet_return_t status, xpc_object_t interface_param) {
> + if_status = status;
> + if (status != VMNET_SUCCESS || !interface_param) {
> + dispatch_semaphore_signal(if_created_sem);
> + return;
> + }
> +
> +#ifdef DEBUG
> + qemu_log("vmnet.start.interface_param:\n");
> + xpc_dictionary_apply(interface_param,
> + ^bool(const char *k, xpc_object_t v) {
> + char *desc = xpc_copy_description(v);
> + qemu_log(" %s=%s\n", k, desc);
> + free(desc);
> + return true;
> + });
> +#endif /* DEBUG */
> +
> + s->mtu = xpc_dictionary_get_uint64(
> + interface_param,
> + vmnet_mtu_key);
> + s->max_packet_size = xpc_dictionary_get_uint64(
> + interface_param,
> + vmnet_max_packet_size_key);
> +
> + dispatch_semaphore_signal(if_created_sem);
> + });
> +
> + if (s->vmnet_if == NULL) {
> + dispatch_release(s->if_queue);
> + dispatch_release(if_created_sem);
> + error_setg(errp,
> + "unable to create interface with requested params");
> + return -1;
> + }
> +
> + dispatch_semaphore_wait(if_created_sem, DISPATCH_TIME_FOREVER);
> + dispatch_release(if_created_sem);
> +
> + if (if_status != VMNET_SUCCESS) {
> + dispatch_release(s->if_queue);
> + error_setg(errp,
> + "cannot create vmnet interface: %s",
> + vmnet_status_map_str(if_status));
> + return -1;
> + }
> +
> + s->send_bh = aio_bh_new(qemu_get_aio_context(), vmnet_send_bh, nc);
> + s->send_enabled = true;
> + vmnet_bufs_init(s);
> +
> + vmnet_interface_set_event_callback(
> + s->vmnet_if,
> + VMNET_INTERFACE_PACKETS_AVAILABLE,
> + s->if_queue,
> + ^(interface_event_t event_id, xpc_object_t event) {
> + assert(event_id == VMNET_INTERFACE_PACKETS_AVAILABLE);
> + /*
> + * This function is being called from a non qemu thread, so
> + * we only schedule a BH, and do the rest of the io completion
> + * handling from vmnet_send_bh() which runs in a qemu context.
> + */
> + qemu_bh_schedule(s->send_bh);
> + });
> +
> + return 0;
> +}
> +
> +
> +ssize_t vmnet_receive_common(NetClientState *nc,
> + const uint8_t *buf,
> + size_t size)
> +{
> + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> + struct vmpktdesc packet;
> + struct iovec iov;
> + int pkt_cnt;
> + vmnet_return_t if_status;
> +
> + if (size > s->max_packet_size) {
> + warn_report("vmnet: packet is too big, %zu > %" PRIu64,
> + packet.vm_pkt_size,
> + s->max_packet_size);
> + return -1;
> + }
> +
> + iov.iov_base = (char *) buf;
> + iov.iov_len = size;
> +
> + packet.vm_pkt_iovcnt = 1;
> + packet.vm_flags = 0;
> + packet.vm_pkt_size = size;
> + packet.vm_pkt_iov = &iov;
> + pkt_cnt = 1;
> +
> + if_status = vmnet_write(s->vmnet_if, &packet, &pkt_cnt);
> + if (if_status != VMNET_SUCCESS) {
> + error_report("vmnet: write error: %s\n",
> + vmnet_status_map_str(if_status));
> + return -1;
> + }
> +
> + if (if_status == VMNET_SUCCESS && pkt_cnt) {
`if_status == VMNET_SUCCESS` is redundant.
Regards,
Akihiko Odaki
> + return size;
> + }
> + return 0;
> +}
> +
> +
> +void vmnet_cleanup_common(NetClientState *nc)
> +{
> + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> + dispatch_semaphore_t if_stopped_sem;
> +
> + if (s->vmnet_if == NULL) {
> + return;
> + }
> +
> + vmnet_interface_set_event_callback(
> + s->vmnet_if,
> + VMNET_INTERFACE_PACKETS_AVAILABLE,
> + NULL,
> + NULL);
I don't think this vmnet_interface_set_event_callback call is necessary.
> +
> + qemu_purge_queued_packets(nc);
> +
> + if_stopped_sem = dispatch_semaphore_create(0);
> + vmnet_stop_interface(
> + s->vmnet_if,
> + s->if_queue,
> + ^(vmnet_return_t status) {
> + assert(status == VMNET_SUCCESS);
> + dispatch_semaphore_signal(if_stopped_sem);
> + });
> + dispatch_semaphore_wait(if_stopped_sem, DISPATCH_TIME_FOREVER);
> +
> + qemu_bh_delete(s->send_bh);
> + dispatch_release(if_stopped_sem);
> + dispatch_release(s->if_queue);
> +
> + for (int i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
> + g_free(s->iov_buf[i].iov_base);
> + }
> +}
> diff --git a/net/vmnet-shared.c b/net/vmnet-shared.c
> index f07afaaf21..2f4eb1db2d 100644
> --- a/net/vmnet-shared.c
> +++ b/net/vmnet-shared.c
> @@ -10,16 +10,103 @@
>
> #include "qemu/osdep.h"
> #include "qapi/qapi-types-net.h"
> +#include "qapi/error.h"
> #include "vmnet_int.h"
> #include "clients.h"
> -#include "qemu/error-report.h"
> -#include "qapi/error.h"
>
> #include <vmnet/vmnet.h>
>
> +typedef struct VmnetSharedState {
> + VmnetCommonState cs;
> +} VmnetSharedState;
> +
> +
> +static bool validate_options(const Netdev *netdev, Error **errp)
> +{
> + const NetdevVmnetSharedOptions *options = &(netdev->u.vmnet_shared);
> +
> +#if !defined(MAC_OS_VERSION_11_0) || \
> + MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_VERSION_11_0
> + if (options->has_isolated) {
> + error_setg(errp,
> + "vmnet-shared.isolated feature is "
> + "unavailable: outdated vmnet.framework API");
> + return false;
> + }
> +#endif
> +
> + if ((options->has_start_address ||
> + options->has_end_address ||
> + options->has_subnet_mask) &&
> + !(options->has_start_address &&
> + options->has_end_address &&
> + options->has_subnet_mask)) {
> + error_setg(errp,
> + "'start-address', 'end-address', 'subnet-mask' "
> + "should be provided together"
> + );
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static xpc_object_t build_if_desc(const Netdev *netdev)
> +{
> + const NetdevVmnetSharedOptions *options = &(netdev->u.vmnet_shared);
> + xpc_object_t if_desc = xpc_dictionary_create(NULL, NULL, 0);
> +
> + xpc_dictionary_set_uint64(
> + if_desc,
> + vmnet_operation_mode_key,
> + VMNET_SHARED_MODE
> + );
> +
> + if (options->has_nat66_prefix) {
> + xpc_dictionary_set_string(if_desc,
> + vmnet_nat66_prefix_key,
> + options->nat66_prefix);
> + }
> +
> + if (options->has_start_address) {
> + xpc_dictionary_set_string(if_desc,
> + vmnet_start_address_key,
> + options->start_address);
> + xpc_dictionary_set_string(if_desc,
> + vmnet_end_address_key,
> + options->end_address);
> + xpc_dictionary_set_string(if_desc,
> + vmnet_subnet_mask_key,
> + options->subnet_mask);
> + }
> +
> +#if defined(MAC_OS_VERSION_11_0) && \
> + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> + xpc_dictionary_set_bool(
> + if_desc,
> + vmnet_enable_isolation_key,
> + options->isolated
> + );
> +#endif
> +
> + return if_desc;
> +}
> +
> +static NetClientInfo net_vmnet_shared_info = {
> + .type = NET_CLIENT_DRIVER_VMNET_SHARED,
> + .size = sizeof(VmnetSharedState),
> + .receive = vmnet_receive_common,
> + .cleanup = vmnet_cleanup_common,
> +};
> +
> int net_init_vmnet_shared(const Netdev *netdev, const char *name,
> NetClientState *peer, Error **errp)
> {
> - error_setg(errp, "vmnet-shared is not implemented yet");
> - return -1;
> + NetClientState *nc = qemu_new_net_client(&net_vmnet_shared_info,
> + peer, "vmnet-shared", name);
> + if (!validate_options(netdev, errp)) {
> + g_assert_not_reached();
> + return -1;
> + }
> + return vmnet_if_create(nc, build_if_desc(netdev), errp);
> }
> diff --git a/net/vmnet_int.h b/net/vmnet_int.h
> index aac4d5af64..8f3321ef3e 100644
> --- a/net/vmnet_int.h
> +++ b/net/vmnet_int.h
> @@ -15,11 +15,50 @@
> #include "clients.h"
>
> #include <vmnet/vmnet.h>
> +#include <dispatch/dispatch.h>
> +
> +/**
> + * From vmnet.framework documentation
> + *
> + * Each read/write call allows up to 200 packets to be
> + * read or written for a maximum of 256KB.
> + *
> + * Each packet written should be a complete
> + * ethernet frame.
> + *
> + * https://developer.apple.com/documentation/vmnet
> + */
> +#define VMNET_PACKETS_LIMIT 200
>
> typedef struct VmnetCommonState {
> - NetClientState nc;
> + NetClientState nc;
> + interface_ref vmnet_if;
> +
> + uint64_t mtu;
> + uint64_t max_packet_size;
>
> + dispatch_queue_t if_queue;
> +
> + QEMUBH *send_bh;
> + bool send_enabled;
> +
> + struct vmpktdesc packets_buf[VMNET_PACKETS_LIMIT];
> + int packets_send_current_pos;
> + int packets_send_end_pos;
> +
> + struct iovec iov_buf[VMNET_PACKETS_LIMIT];
> } VmnetCommonState;
>
> +const char *vmnet_status_map_str(vmnet_return_t status);
> +
> +int vmnet_if_create(NetClientState *nc,
> + xpc_object_t if_desc,
> + Error **errp);
> +
> +ssize_t vmnet_receive_common(NetClientState *nc,
> + const uint8_t *buf,
> + size_t size);
> +
> +void vmnet_cleanup_common(NetClientState *nc);
>
> #endif /* VMNET_INT_H */
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v16 4/7] net/vmnet: implement host mode (vmnet-host)
2022-03-14 19:15 ` [PATCH v16 4/7] net/vmnet: implement host mode (vmnet-host) Vladislav Yaroshchuk
@ 2022-03-14 19:55 ` Akihiko Odaki
0 siblings, 0 replies; 16+ messages in thread
From: Akihiko Odaki @ 2022-03-14 19:55 UTC (permalink / raw)
To: Vladislav Yaroshchuk, qemu-devel
Cc: peter.maydell, alex.bennee, jasowang, phillip.ennen, armbru,
dirty, f4bug, r.bolshakov, agraf, phillip, roman, hsp.cat7,
hello, qemu_oss, eblake, kraxel
On 2022/03/15 4:15, Vladislav Yaroshchuk wrote:
> Signed-off-by: Vladislav Yaroshchuk <Vladislav.Yaroshchuk@jetbrains.com>
> ---
> net/vmnet-host.c | 116 ++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 110 insertions(+), 6 deletions(-)
>
> diff --git a/net/vmnet-host.c b/net/vmnet-host.c
> index 32dc437037..15a832701a 100644
> --- a/net/vmnet-host.c
> +++ b/net/vmnet-host.c
> @@ -9,16 +9,120 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/uuid.h"
> #include "qapi/qapi-types-net.h"
> -#include "vmnet_int.h"
> -#include "clients.h"
> -#include "qemu/error-report.h"
> #include "qapi/error.h"
> +#include "clients.h"
> +#include "vmnet_int.h"
>
> #include <vmnet/vmnet.h>
>
> +typedef struct VmnetHostState {
> + VmnetCommonState cs;
> + QemuUUID network_uuid;
> +} VmnetHostState;
> +
> +static bool validate_options(const Netdev *netdev, Error **errp)
> +{
> + const NetdevVmnetHostOptions *options = &(netdev->u.vmnet_host);
> + QemuUUID uuid;
> +
> +#if defined(MAC_OS_VERSION_11_0) && \
> + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> +
> + if (options->has_net_uuid &&
> + qemu_uuid_parse(options->net_uuid, &uuid) < 0) {
> + error_setg(errp, "Invalid UUID provided in 'net-uuid'");
> + return false;
> + }
> +#else
> + if (options->has_isolated) {
> + error_setg(errp,
> + "vmnet-host.isolated feature is "
> + "unavailable: outdated vmnet.framework API");
> + return false;
> + }
> +
> + if (options->has_net_uuid) {
> + error_setg(errp,
> + "vmnet-host.net-uuid feature is "
> + "unavailable: outdated vmnet.framework API");
> + return false;
> + }
> +#endif
> +
> + if ((options->has_start_address ||
> + options->has_end_address ||
> + options->has_subnet_mask) &&
> + !(options->has_start_address &&
> + options->has_end_address &&
> + options->has_subnet_mask)) {
> + error_setg(errp,
> + "'start-address', 'end-address', 'subnet-mask' "
> + "should be provided together");
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static xpc_object_t build_if_desc(const Netdev *netdev,
> + NetClientState *nc)
> +{
> + const NetdevVmnetHostOptions *options = &(netdev->u.vmnet_host);
> + VmnetCommonState *cs = DO_UPCAST(VmnetCommonState, nc, nc);
> + VmnetHostState *hs = DO_UPCAST(VmnetHostState, cs, cs);
> + xpc_object_t if_desc = xpc_dictionary_create(NULL, NULL, 0);
> +
> + xpc_dictionary_set_uint64(if_desc,
> + vmnet_operation_mode_key,
> + VMNET_HOST_MODE);
> +
> +#if defined(MAC_OS_VERSION_11_0) && \
> + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> +
> + xpc_dictionary_set_bool(if_desc,
> + vmnet_enable_isolation_key,
> + options->isolated);
> +
> + if (options->has_net_uuid) {
> + qemu_uuid_parse(options->net_uuid, &hs->network_uuid);
> + xpc_dictionary_set_uuid(if_desc,
> + vmnet_network_identifier_key,
> + hs->network_uuid.data);
I missed this previously, but network_uuid can be a local variable since
xpc_dictionary_set_uuid copies the UUID to a XPC UUID object box.
> + }
> +#endif
> +
> + if (options->has_start_address) {
> + xpc_dictionary_set_string(if_desc,
> + vmnet_start_address_key,
> + options->start_address);
> + xpc_dictionary_set_string(if_desc,
> + vmnet_end_address_key,
> + options->end_address);
> + xpc_dictionary_set_string(if_desc,
> + vmnet_subnet_mask_key,
> + options->subnet_mask);
> + }
> +
> + return if_desc;
> +}
> +
> +static NetClientInfo net_vmnet_host_info = {
> + .type = NET_CLIENT_DRIVER_VMNET_HOST,
> + .size = sizeof(VmnetHostState),
> + .receive = vmnet_receive_common,
> + .cleanup = vmnet_cleanup_common,
> +};
> +
> int net_init_vmnet_host(const Netdev *netdev, const char *name,
> - NetClientState *peer, Error **errp) {
> - error_setg(errp, "vmnet-host is not implemented yet");
> - return -1;
> + NetClientState *peer, Error **errp)
> +{
> + NetClientState *nc = qemu_new_net_client(&net_vmnet_host_info,
> + peer, "vmnet-host", name);
> + if (!validate_options(netdev, errp)) {
> + g_assert_not_reached();
g_assert_not_reached() is inappropriate here. It should be used to
identify programming errors, but this code path can be triggered with an
erroneous user input. It can be simply removed.
> + return -1;
> + }
> + return vmnet_if_create(nc, build_if_desc(netdev, nc), errp);
> }
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v16 3/7] net/vmnet: implement shared mode (vmnet-shared)
2022-03-14 19:45 ` Akihiko Odaki
@ 2022-03-14 21:50 ` Vladislav Yaroshchuk
2022-03-14 22:34 ` Akihiko Odaki
0 siblings, 1 reply; 16+ messages in thread
From: Vladislav Yaroshchuk @ 2022-03-14 21:50 UTC (permalink / raw)
To: Akihiko Odaki
Cc: qemu Developers, Jason Wang, Roman Bolshakov, Eric Blake,
phillip.ennen, Phillip Tennen, Markus Armbruster,
Howard Spoelstra, Alessio Dionisi, Roman Bolshakov,
Peter Maydell, Cameron Esfahani, Philippe Mathieu-Daudé,
Alexander Graf, Gerd Hoffmann, Alex Bennée,
Christian Schoenebeck
[-- Attachment #1: Type: text/plain, Size: 19635 bytes --]
Thank you, Akihiko
On Mon, Mar 14, 2022 at 10:46 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:
> On 2022/03/15 4:15, Vladislav Yaroshchuk wrote:
> > vmnet.framework supports iov, but writing more than
> > one iov into vmnet interface fails with
> > 'VMNET_INVALID_ARGUMENT'. Collecting provided iovs into
> > one and passing it to vmnet works fine. That's the
> > reason why receive_iov() left unimplemented. But it still
> > works with good enough performance having .receive()
> > implemented only.
> >
> > Signed-off-by: Phillip Tennen <phillip@axleos.com>
> > Signed-off-by: Vladislav Yaroshchuk <Vladislav.Yaroshchuk@jetbrains.com>
> > ---
> > net/vmnet-common.m | 298 +++++++++++++++++++++++++++++++++++++++++++++
> > net/vmnet-shared.c | 95 ++++++++++++++-
> > net/vmnet_int.h | 41 ++++++-
> > 3 files changed, 429 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/vmnet-common.m b/net/vmnet-common.m
> > index 56612c72ce..20a33d2591 100644
> > --- a/net/vmnet-common.m
> > +++ b/net/vmnet-common.m
> > @@ -10,6 +10,8 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "qemu/main-loop.h"
> > +#include "qemu/log.h"
> > #include "qapi/qapi-types-net.h"
> > #include "vmnet_int.h"
> > #include "clients.h"
> > @@ -17,4 +19,300 @@
> > #include "qapi/error.h"
> >
> > #include <vmnet/vmnet.h>
> > +#include <dispatch/dispatch.h>
> >
> > +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s);
>
> The names of vmnet_qemu_send_wrapper and vmnet_send_bh does not tell
> them apart well. Since only vmnet_send_bh does reading, its name may
> include "read" to clarify that. "wrapper" in vmnet_qemu_send_wrapper may
> be also misleading as it does more than just calling the underlying QEMU
> facility, but it also updates VmnetCommonState.
>
>
Ok, I'll think about how to name them better.
> > +
> > +
> > +static void vmnet_send_completed(NetClientState *nc, ssize_t len)
> > +{
> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > + /* Complete sending packets left in VmnetCommonState buffers */
> > + s->send_enabled = vmnet_qemu_send_wrapper(s);
>
> It must qemu_bh_schedule(s->send_bh) after vmnet_qemu_send_wrapper.
>
>
Agree with you, thanks.
> Also, send_enabled flag can be removed as explained in:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html
>
>
Not sure about this. Values of packets_send_current_pos
and packets_send_end_pos may be equal, but QEMU may be
not ready to receive new packets - the explanation:
1. We are sending packets to QEMU with qemu_send_packet_async:
packets_send_current_pos = 0
packets_send_end_pos = 5
2. All five packets (0, 1, 2, 3, 4) have been successfully sent to QEMU,
but qemu_send_packet_async returned 0 "no more packets" after
the last invocation
3. In spite of this, all five packets are sent and
packets_send_current_pos == packets_send_end_pos == 5
4. It seems that "pointers are equal -> QEMU is ready", but actually
it is not.
Also, hiding QEMU "ready"/"not ready" state behind pointers is a
bad choice I think. Having a concrete flag makes this more clear.
It provides understandability, not complexity (imho).
> > send_enabled can be eliminated. When it is enabled, packets_send_pos
> > and packets_batch_size must be equal. They must not be equal
> > otherwise. packets_send_pos must represent the position of the packet
> > which is not sent yet, possibly in the process of sending.
> > vmnet_send_completed must call qemu_send_wrapper before scheduling
> > send_bh. bh_send should do nothing if s->packets_send_pos <
> > s->packets_batch_size.
>
> > +}
> > +
> > +
> > +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s) {
> > + ssize_t size;
> > +
> > + /*
> > + * Packets to send lay in [current_pos..end_pos)
> > + * (including current_pos, excluding end_pos)
> > + */
> > + while (s->packets_send_current_pos < s->packets_send_end_pos) {
> > + size = qemu_send_packet_async(&s->nc,
> > +
> s->iov_buf[s->packets_send_current_pos].iov_base,
> > +
> s->packets_buf[s->packets_send_current_pos].vm_pkt_size,
> > + vmnet_send_completed);
> > + ++s->packets_send_current_pos;
> > + if (size == 0) {
> > + /* QEMU is not ready - wait for completion callback call */
> > + return false;
> > + }
> > + }
> > + return true;
> > +}
> > +
> > +
> > +static void vmnet_send_bh(void *opaque)
> > +{
> > + NetClientState *nc = (NetClientState *) opaque;
> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > + struct vmpktdesc *packets = s->packets_buf;
> > + vmnet_return_t status;
> > + int i;
> > +
> > + /*
> > + * Do nothing if QEMU is not ready - wait
> > + * for completion callback invocation
> > + */
> > + if (!s->send_enabled) {
> > + return;
> > + }
> > +
> > + /* Read as many packets as present */
> > + s->packets_send_current_pos = 0;
> > + s->packets_send_end_pos = VMNET_PACKETS_LIMIT;
> > + for (i = 0; i < s->packets_send_end_pos; ++i) {
> > + packets[i].vm_pkt_size = s->max_packet_size;
> > + packets[i].vm_pkt_iovcnt = 1;
> > + packets[i].vm_flags = 0;
> > + }
> > +
> > + status = vmnet_read(s->vmnet_if, packets, &s->packets_send_end_pos);
> > + if (status != VMNET_SUCCESS) {
> > + error_printf("vmnet: read failed: %s\n",
> > + vmnet_status_map_str(status));
> > + s->packets_send_current_pos = 0;
> > + s->packets_send_end_pos = 0;
> > + return;
> > + }
> > +
> > + /* Send packets to QEMU */
> > + s->send_enabled = vmnet_qemu_send_wrapper(s);
> > +}
> > +
> > +
> > +static void vmnet_bufs_init(VmnetCommonState *s)
> > +{
> > + struct vmpktdesc *packets = s->packets_buf;
> > + struct iovec *iov = s->iov_buf;
> > + int i;
> > +
> > + for (i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
> > + iov[i].iov_len = s->max_packet_size;
> > + iov[i].iov_base = g_malloc0(iov[i].iov_len);
> > + packets[i].vm_pkt_iov = iov + i;
> > + }
> > +}
> > +
> > +
> > +const char *vmnet_status_map_str(vmnet_return_t status)
> > +{
> > + switch (status) {
> > + case VMNET_SUCCESS:
> > + return "success";
> > + case VMNET_FAILURE:
> > + return "general failure (possibly not enough privileges)";
> > + case VMNET_MEM_FAILURE:
> > + return "memory allocation failure";
> > + case VMNET_INVALID_ARGUMENT:
> > + return "invalid argument specified";
> > + case VMNET_SETUP_INCOMPLETE:
> > + return "interface setup is not complete";
> > + case VMNET_INVALID_ACCESS:
> > + return "invalid access, permission denied";
> > + case VMNET_PACKET_TOO_BIG:
> > + return "packet size is larger than MTU";
> > + case VMNET_BUFFER_EXHAUSTED:
> > + return "buffers exhausted in kernel";
> > + case VMNET_TOO_MANY_PACKETS:
> > + return "packet count exceeds limit";
> > +#if defined(MAC_OS_VERSION_11_0) && \
> > + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> > + case VMNET_SHARING_SERVICE_BUSY:
> > + return "conflict, sharing service is in use";
> > +#endif
> > + default:
> > + return "unknown vmnet error";
> > + }
> > +}
> > +
> > +
> > +int vmnet_if_create(NetClientState *nc,
> > + xpc_object_t if_desc,
> > + Error **errp)
> > +{
> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > + dispatch_semaphore_t if_created_sem = dispatch_semaphore_create(0);
> > + __block vmnet_return_t if_status;
> > +
> > + s->if_queue = dispatch_queue_create(
> > + "org.qemu.vmnet.if_queue",
> > + DISPATCH_QUEUE_SERIAL
> > + );
> > +
> > + xpc_dictionary_set_bool(
> > + if_desc,
> > + vmnet_allocate_mac_address_key,
> > + false
> > + );
> > +
> > +#ifdef DEBUG
> > + qemu_log("vmnet.start.interface_desc:\n");
> > + xpc_dictionary_apply(if_desc,
> > + ^bool(const char *k, xpc_object_t v) {
> > + char *desc = xpc_copy_description(v);
> > + qemu_log(" %s=%s\n", k, desc);
> > + free(desc);
> > + return true;
> > + });
> > +#endif /* DEBUG */
> > +
> > + s->vmnet_if = vmnet_start_interface(
> > + if_desc,
> > + s->if_queue,
> > + ^(vmnet_return_t status, xpc_object_t interface_param) {
> > + if_status = status;
> > + if (status != VMNET_SUCCESS || !interface_param) {
> > + dispatch_semaphore_signal(if_created_sem);
> > + return;
> > + }
> > +
> > +#ifdef DEBUG
> > + qemu_log("vmnet.start.interface_param:\n");
> > + xpc_dictionary_apply(interface_param,
> > + ^bool(const char *k, xpc_object_t v) {
> > + char *desc =
> xpc_copy_description(v);
> > + qemu_log(" %s=%s\n", k, desc);
> > + free(desc);
> > + return true;
> > + });
> > +#endif /* DEBUG */
> > +
> > + s->mtu = xpc_dictionary_get_uint64(
> > + interface_param,
> > + vmnet_mtu_key);
> > + s->max_packet_size = xpc_dictionary_get_uint64(
> > + interface_param,
> > + vmnet_max_packet_size_key);
> > +
> > + dispatch_semaphore_signal(if_created_sem);
> > + });
> > +
> > + if (s->vmnet_if == NULL) {
> > + dispatch_release(s->if_queue);
> > + dispatch_release(if_created_sem);
> > + error_setg(errp,
> > + "unable to create interface with requested params");
> > + return -1;
> > + }
> > +
> > + dispatch_semaphore_wait(if_created_sem, DISPATCH_TIME_FOREVER);
> > + dispatch_release(if_created_sem);
> > +
> > + if (if_status != VMNET_SUCCESS) {
> > + dispatch_release(s->if_queue);
> > + error_setg(errp,
> > + "cannot create vmnet interface: %s",
> > + vmnet_status_map_str(if_status));
> > + return -1;
> > + }
> > +
> > + s->send_bh = aio_bh_new(qemu_get_aio_context(), vmnet_send_bh, nc);
> > + s->send_enabled = true;
> > + vmnet_bufs_init(s);
> > +
> > + vmnet_interface_set_event_callback(
> > + s->vmnet_if,
> > + VMNET_INTERFACE_PACKETS_AVAILABLE,
> > + s->if_queue,
> > + ^(interface_event_t event_id, xpc_object_t event) {
> > + assert(event_id == VMNET_INTERFACE_PACKETS_AVAILABLE);
> > + /*
> > + * This function is being called from a non qemu thread, so
> > + * we only schedule a BH, and do the rest of the io
> completion
> > + * handling from vmnet_send_bh() which runs in a qemu
> context.
> > + */
> > + qemu_bh_schedule(s->send_bh);
> > + });
> > +
> > + return 0;
> > +}
> > +
> > +
> > +ssize_t vmnet_receive_common(NetClientState *nc,
> > + const uint8_t *buf,
> > + size_t size)
> > +{
> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > + struct vmpktdesc packet;
> > + struct iovec iov;
> > + int pkt_cnt;
> > + vmnet_return_t if_status;
> > +
> > + if (size > s->max_packet_size) {
> > + warn_report("vmnet: packet is too big, %zu > %" PRIu64,
> > + packet.vm_pkt_size,
> > + s->max_packet_size);
> > + return -1;
> > + }
> > +
> > + iov.iov_base = (char *) buf;
> > + iov.iov_len = size;
> > +
> > + packet.vm_pkt_iovcnt = 1;
> > + packet.vm_flags = 0;
> > + packet.vm_pkt_size = size;
> > + packet.vm_pkt_iov = &iov;
> > + pkt_cnt = 1;
> > +
> > + if_status = vmnet_write(s->vmnet_if, &packet, &pkt_cnt);
> > + if (if_status != VMNET_SUCCESS) {
> > + error_report("vmnet: write error: %s\n",
> > + vmnet_status_map_str(if_status));
> > + return -1;
> > + }
> > +
> > + if (if_status == VMNET_SUCCESS && pkt_cnt) {
>
> `if_status == VMNET_SUCCESS` is redundant.
>
>
Missed this, thanks.
> Regards,
> Akihiko Odaki
>
> > + return size;
> > + }
> > + return 0;
> > +}
> > +
> > +
> > +void vmnet_cleanup_common(NetClientState *nc)
> > +{
> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > + dispatch_semaphore_t if_stopped_sem;
> > +
> > + if (s->vmnet_if == NULL) {
> > + return;
> > + }
> > +
> > + vmnet_interface_set_event_callback(
> > + s->vmnet_if,
> > + VMNET_INTERFACE_PACKETS_AVAILABLE,
> > + NULL,
> > + NULL);
>
> I don't think this vmnet_interface_set_event_callback call is necessary.
>
>
I kept in mind that vmnet processing lives in a separate thread
and while cleanup it may continue receiving packets. While the
queue is not empty, vmnet_stop_interface hangs. Unregistering
callback ensures that this queue will be emptied asap.
It will work without vmnet_interface_set_event_callback here,
but I think it's better to be respectful to vmnet and clean everything
we can :)
Thank you!
Best Regards,
Vladislav
> +
> > + qemu_purge_queued_packets(nc);
> > +
> > + if_stopped_sem = dispatch_semaphore_create(0);
> > + vmnet_stop_interface(
> > + s->vmnet_if,
> > + s->if_queue,
> > + ^(vmnet_return_t status) {
> > + assert(status == VMNET_SUCCESS);
> > + dispatch_semaphore_signal(if_stopped_sem);
> > + });
> > + dispatch_semaphore_wait(if_stopped_sem, DISPATCH_TIME_FOREVER);
> > +
> > + qemu_bh_delete(s->send_bh);
> > + dispatch_release(if_stopped_sem);
> > + dispatch_release(s->if_queue);
> > +
> > + for (int i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
> > + g_free(s->iov_buf[i].iov_base);
> > + }
> > +}
> > diff --git a/net/vmnet-shared.c b/net/vmnet-shared.c
> > index f07afaaf21..2f4eb1db2d 100644
> > --- a/net/vmnet-shared.c
> > +++ b/net/vmnet-shared.c
> > @@ -10,16 +10,103 @@
> >
> > #include "qemu/osdep.h"
> > #include "qapi/qapi-types-net.h"
> > +#include "qapi/error.h"
> > #include "vmnet_int.h"
> > #include "clients.h"
> > -#include "qemu/error-report.h"
> > -#include "qapi/error.h"
> >
> > #include <vmnet/vmnet.h>
> >
> > +typedef struct VmnetSharedState {
> > + VmnetCommonState cs;
> > +} VmnetSharedState;
> > +
> > +
> > +static bool validate_options(const Netdev *netdev, Error **errp)
> > +{
> > + const NetdevVmnetSharedOptions *options = &(netdev->u.vmnet_shared);
> > +
> > +#if !defined(MAC_OS_VERSION_11_0) || \
> > + MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_VERSION_11_0
> > + if (options->has_isolated) {
> > + error_setg(errp,
> > + "vmnet-shared.isolated feature is "
> > + "unavailable: outdated vmnet.framework API");
> > + return false;
> > + }
> > +#endif
> > +
> > + if ((options->has_start_address ||
> > + options->has_end_address ||
> > + options->has_subnet_mask) &&
> > + !(options->has_start_address &&
> > + options->has_end_address &&
> > + options->has_subnet_mask)) {
> > + error_setg(errp,
> > + "'start-address', 'end-address', 'subnet-mask' "
> > + "should be provided together"
> > + );
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static xpc_object_t build_if_desc(const Netdev *netdev)
> > +{
> > + const NetdevVmnetSharedOptions *options = &(netdev->u.vmnet_shared);
> > + xpc_object_t if_desc = xpc_dictionary_create(NULL, NULL, 0);
> > +
> > + xpc_dictionary_set_uint64(
> > + if_desc,
> > + vmnet_operation_mode_key,
> > + VMNET_SHARED_MODE
> > + );
> > +
> > + if (options->has_nat66_prefix) {
> > + xpc_dictionary_set_string(if_desc,
> > + vmnet_nat66_prefix_key,
> > + options->nat66_prefix);
> > + }
> > +
> > + if (options->has_start_address) {
> > + xpc_dictionary_set_string(if_desc,
> > + vmnet_start_address_key,
> > + options->start_address);
> > + xpc_dictionary_set_string(if_desc,
> > + vmnet_end_address_key,
> > + options->end_address);
> > + xpc_dictionary_set_string(if_desc,
> > + vmnet_subnet_mask_key,
> > + options->subnet_mask);
> > + }
> > +
> > +#if defined(MAC_OS_VERSION_11_0) && \
> > + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> > + xpc_dictionary_set_bool(
> > + if_desc,
> > + vmnet_enable_isolation_key,
> > + options->isolated
> > + );
> > +#endif
> > +
> > + return if_desc;
> > +}
> > +
> > +static NetClientInfo net_vmnet_shared_info = {
> > + .type = NET_CLIENT_DRIVER_VMNET_SHARED,
> > + .size = sizeof(VmnetSharedState),
> > + .receive = vmnet_receive_common,
> > + .cleanup = vmnet_cleanup_common,
> > +};
> > +
> > int net_init_vmnet_shared(const Netdev *netdev, const char *name,
> > NetClientState *peer, Error **errp)
> > {
> > - error_setg(errp, "vmnet-shared is not implemented yet");
> > - return -1;
> > + NetClientState *nc = qemu_new_net_client(&net_vmnet_shared_info,
> > + peer, "vmnet-shared",
> name);
> > + if (!validate_options(netdev, errp)) {
> > + g_assert_not_reached();
> > + return -1;
> > + }
> > + return vmnet_if_create(nc, build_if_desc(netdev), errp);
> > }
> > diff --git a/net/vmnet_int.h b/net/vmnet_int.h
> > index aac4d5af64..8f3321ef3e 100644
> > --- a/net/vmnet_int.h
> > +++ b/net/vmnet_int.h
> > @@ -15,11 +15,50 @@
> > #include "clients.h"
> >
> > #include <vmnet/vmnet.h>
> > +#include <dispatch/dispatch.h>
> > +
> > +/**
> > + * From vmnet.framework documentation
> > + *
> > + * Each read/write call allows up to 200 packets to be
> > + * read or written for a maximum of 256KB.
> > + *
> > + * Each packet written should be a complete
> > + * ethernet frame.
> > + *
> > + * https://developer.apple.com/documentation/vmnet
> > + */
> > +#define VMNET_PACKETS_LIMIT 200
> >
> > typedef struct VmnetCommonState {
> > - NetClientState nc;
> > + NetClientState nc;
> > + interface_ref vmnet_if;
> > +
> > + uint64_t mtu;
> > + uint64_t max_packet_size;
> >
> > + dispatch_queue_t if_queue;
> > +
> > + QEMUBH *send_bh;
> > + bool send_enabled;
> > +
> > + struct vmpktdesc packets_buf[VMNET_PACKETS_LIMIT];
> > + int packets_send_current_pos;
> > + int packets_send_end_pos;
> > +
> > + struct iovec iov_buf[VMNET_PACKETS_LIMIT];
> > } VmnetCommonState;
> >
> > +const char *vmnet_status_map_str(vmnet_return_t status);
> > +
> > +int vmnet_if_create(NetClientState *nc,
> > + xpc_object_t if_desc,
> > + Error **errp);
> > +
> > +ssize_t vmnet_receive_common(NetClientState *nc,
> > + const uint8_t *buf,
> > + size_t size);
> > +
> > +void vmnet_cleanup_common(NetClientState *nc);
> >
> > #endif /* VMNET_INT_H */
>
>
[-- Attachment #2: Type: text/html, Size: 26910 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v16 3/7] net/vmnet: implement shared mode (vmnet-shared)
2022-03-14 21:50 ` Vladislav Yaroshchuk
@ 2022-03-14 22:34 ` Akihiko Odaki
2022-03-14 22:37 ` Akihiko Odaki
2022-03-14 23:02 ` Vladislav Yaroshchuk
0 siblings, 2 replies; 16+ messages in thread
From: Akihiko Odaki @ 2022-03-14 22:34 UTC (permalink / raw)
To: Vladislav Yaroshchuk
Cc: Peter Maydell, Gerd Hoffmann, Alex Bennée, Jason Wang,
phillip.ennen, qemu Developers, Cameron Esfahani,
Markus Armbruster, Roman Bolshakov, Alexander Graf,
Phillip Tennen, Roman Bolshakov, Howard Spoelstra,
Alessio Dionisi, Christian Schoenebeck, Eric Blake,
Philippe Mathieu-Daudé
On 2022/03/15 6:50, Vladislav Yaroshchuk wrote:
> Thank you, Akihiko
>
> On Mon, Mar 14, 2022 at 10:46 PM Akihiko Odaki <akihiko.odaki@gmail.com
> <mailto:akihiko.odaki@gmail.com>> wrote:
>
> On 2022/03/15 4:15, Vladislav Yaroshchuk wrote:
> > vmnet.framework supports iov, but writing more than
> > one iov into vmnet interface fails with
> > 'VMNET_INVALID_ARGUMENT'. Collecting provided iovs into
> > one and passing it to vmnet works fine. That's the
> > reason why receive_iov() left unimplemented. But it still
> > works with good enough performance having .receive()
> > implemented only.
> >
> > Signed-off-by: Phillip Tennen <phillip@axleos.com
> <mailto:phillip@axleos.com>>
> > Signed-off-by: Vladislav Yaroshchuk
> <Vladislav.Yaroshchuk@jetbrains.com
> <mailto:Vladislav.Yaroshchuk@jetbrains.com>>
> > ---
> > net/vmnet-common.m | 298
> +++++++++++++++++++++++++++++++++++++++++++++
> > net/vmnet-shared.c | 95 ++++++++++++++-
> > net/vmnet_int.h | 41 ++++++-
> > 3 files changed, 429 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/vmnet-common.m b/net/vmnet-common.m
> > index 56612c72ce..20a33d2591 100644
> > --- a/net/vmnet-common.m
> > +++ b/net/vmnet-common.m
> > @@ -10,6 +10,8 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "qemu/main-loop.h"
> > +#include "qemu/log.h"
> > #include "qapi/qapi-types-net.h"
> > #include "vmnet_int.h"
> > #include "clients.h"
> > @@ -17,4 +19,300 @@
> > #include "qapi/error.h"
> >
> > #include <vmnet/vmnet.h>
> > +#include <dispatch/dispatch.h>
> >
> > +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s);
>
> The names of vmnet_qemu_send_wrapper and vmnet_send_bh does not tell
> them apart well. Since only vmnet_send_bh does reading, its name may
> include "read" to clarify that. "wrapper" in vmnet_qemu_send_wrapper
> may
> be also misleading as it does more than just calling the underlying
> QEMU
> facility, but it also updates VmnetCommonState.
>
>
> Ok, I'll think about how to name them better.
>
> > +
> > +
> > +static void vmnet_send_completed(NetClientState *nc, ssize_t len)
> > +{
> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > + /* Complete sending packets left in VmnetCommonState buffers */
> > + s->send_enabled = vmnet_qemu_send_wrapper(s);
>
> It must qemu_bh_schedule(s->send_bh) after vmnet_qemu_send_wrapper.
>
>
> Agree with you, thanks.
>
> Also, send_enabled flag can be removed as explained in:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html
> <https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html>
>
>
> Not sure about this. Values of packets_send_current_pos
> and packets_send_end_pos may be equal, but QEMU may be
> not ready to receive new packets - the explanation:
> 1. We are sending packets to QEMU with qemu_send_packet_async:
> packets_send_current_pos = 0
> packets_send_end_pos = 5
> 2. All five packets (0, 1, 2, 3, 4) have been successfully sent to QEMU,
> but qemu_send_packet_async returned 0 "no more packets" after
> the last invocation
> 3. In spite of this, all five packets are sent and
> packets_send_current_pos == packets_send_end_pos == 5
> 4. It seems that "pointers are equal -> QEMU is ready", but actually
> it is not.
>
> Also, hiding QEMU "ready"/"not ready" state behind pointers is a
> bad choice I think. Having a concrete flag makes this more clear.
> It provides understandability, not complexity (imho).
packets_send_current_pos must not be incremented if
qemu_send_packet_async returned 0. It must tell the position of the
packet currently being sent.
It would not hide the state, but it would rather make it clear that the
condition vmnet_send_bh can execute. If you see the current
implementation of vmnet_send_bh, you'll find the send_enabled flag, but
it does not tell the exact condition it requires to be enabled. You have
to then jump to all assignments for the flag to know it becomes true iff
every packets in the buffer are sent. It is obvious if vmnet_send_bh
directly states `if (packets_send_current_pos < packets_send_end_pos)`.
Eliminating the flag would also remove the possiblity of forgetting to
maintain the separate state.
>
> > send_enabled can be eliminated. When it is enabled, packets_send_pos
> > and packets_batch_size must be equal. They must not be equal
> > otherwise. packets_send_pos must represent the position of the
> packet
> > which is not sent yet, possibly in the process of sending.
> > vmnet_send_completed must call qemu_send_wrapper before scheduling
> > send_bh. bh_send should do nothing if s->packets_send_pos <
> > s->packets_batch_size.
>
> > +}
> > +
> > +
> > +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s) {
> > + ssize_t size;
> > +
> > + /*
> > + * Packets to send lay in [current_pos..end_pos)
> > + * (including current_pos, excluding end_pos)
> > + */
> > + while (s->packets_send_current_pos < s->packets_send_end_pos) {
> > + size = qemu_send_packet_async(&s->nc,
> > +
> s->iov_buf[s->packets_send_current_pos].iov_base,
> > +
> s->packets_buf[s->packets_send_current_pos].vm_pkt_size,
> > + vmnet_send_completed);
> > + ++s->packets_send_current_pos;
> > + if (size == 0) {
> > + /* QEMU is not ready - wait for completion callback
> call */
> > + return false;
> > + }
> > + }
> > + return true;
> > +}
> > +
> > +
> > +static void vmnet_send_bh(void *opaque)
> > +{
> > + NetClientState *nc = (NetClientState *) opaque;
> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > + struct vmpktdesc *packets = s->packets_buf;
> > + vmnet_return_t status;
> > + int i;
> > +
> > + /*
> > + * Do nothing if QEMU is not ready - wait
> > + * for completion callback invocation
> > + */
> > + if (!s->send_enabled) {
> > + return;
> > + }
> > +
> > + /* Read as many packets as present */
> > + s->packets_send_current_pos = 0;
> > + s->packets_send_end_pos = VMNET_PACKETS_LIMIT;
> > + for (i = 0; i < s->packets_send_end_pos; ++i) {
> > + packets[i].vm_pkt_size = s->max_packet_size;
> > + packets[i].vm_pkt_iovcnt = 1;
> > + packets[i].vm_flags = 0;
> > + }
> > +
> > + status = vmnet_read(s->vmnet_if, packets,
> &s->packets_send_end_pos);
> > + if (status != VMNET_SUCCESS) {
> > + error_printf("vmnet: read failed: %s\n",
> > + vmnet_status_map_str(status));
> > + s->packets_send_current_pos = 0;
> > + s->packets_send_end_pos = 0;
> > + return;
> > + }
> > +
> > + /* Send packets to QEMU */
> > + s->send_enabled = vmnet_qemu_send_wrapper(s);
> > +}
> > +
> > +
> > +static void vmnet_bufs_init(VmnetCommonState *s)
> > +{
> > + struct vmpktdesc *packets = s->packets_buf;
> > + struct iovec *iov = s->iov_buf;
> > + int i;
> > +
> > + for (i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
> > + iov[i].iov_len = s->max_packet_size;
> > + iov[i].iov_base = g_malloc0(iov[i].iov_len);
> > + packets[i].vm_pkt_iov = iov + i;
> > + }
> > +}
> > +
> > +
> > +const char *vmnet_status_map_str(vmnet_return_t status)
> > +{
> > + switch (status) {
> > + case VMNET_SUCCESS:
> > + return "success";
> > + case VMNET_FAILURE:
> > + return "general failure (possibly not enough privileges)";
> > + case VMNET_MEM_FAILURE:
> > + return "memory allocation failure";
> > + case VMNET_INVALID_ARGUMENT:
> > + return "invalid argument specified";
> > + case VMNET_SETUP_INCOMPLETE:
> > + return "interface setup is not complete";
> > + case VMNET_INVALID_ACCESS:
> > + return "invalid access, permission denied";
> > + case VMNET_PACKET_TOO_BIG:
> > + return "packet size is larger than MTU";
> > + case VMNET_BUFFER_EXHAUSTED:
> > + return "buffers exhausted in kernel";
> > + case VMNET_TOO_MANY_PACKETS:
> > + return "packet count exceeds limit";
> > +#if defined(MAC_OS_VERSION_11_0) && \
> > + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> > + case VMNET_SHARING_SERVICE_BUSY:
> > + return "conflict, sharing service is in use";
> > +#endif
> > + default:
> > + return "unknown vmnet error";
> > + }
> > +}
> > +
> > +
> > +int vmnet_if_create(NetClientState *nc,
> > + xpc_object_t if_desc,
> > + Error **errp)
> > +{
> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > + dispatch_semaphore_t if_created_sem =
> dispatch_semaphore_create(0);
> > + __block vmnet_return_t if_status;
> > +
> > + s->if_queue = dispatch_queue_create(
> > + "org.qemu.vmnet.if_queue",
> > + DISPATCH_QUEUE_SERIAL
> > + );
> > +
> > + xpc_dictionary_set_bool(
> > + if_desc,
> > + vmnet_allocate_mac_address_key,
> > + false
> > + );
> > +
> > +#ifdef DEBUG
> > + qemu_log("vmnet.start.interface_desc:\n");
> > + xpc_dictionary_apply(if_desc,
> > + ^bool(const char *k, xpc_object_t v) {
> > + char *desc = xpc_copy_description(v);
> > + qemu_log(" %s=%s\n", k, desc);
> > + free(desc);
> > + return true;
> > + });
> > +#endif /* DEBUG */
> > +
> > + s->vmnet_if = vmnet_start_interface(
> > + if_desc,
> > + s->if_queue,
> > + ^(vmnet_return_t status, xpc_object_t interface_param) {
> > + if_status = status;
> > + if (status != VMNET_SUCCESS || !interface_param) {
> > + dispatch_semaphore_signal(if_created_sem);
> > + return;
> > + }
> > +
> > +#ifdef DEBUG
> > + qemu_log("vmnet.start.interface_param:\n");
> > + xpc_dictionary_apply(interface_param,
> > + ^bool(const char *k,
> xpc_object_t v) {
> > + char *desc =
> xpc_copy_description(v);
> > + qemu_log(" %s=%s\n", k, desc);
> > + free(desc);
> > + return true;
> > + });
> > +#endif /* DEBUG */
> > +
> > + s->mtu = xpc_dictionary_get_uint64(
> > + interface_param,
> > + vmnet_mtu_key);
> > + s->max_packet_size = xpc_dictionary_get_uint64(
> > + interface_param,
> > + vmnet_max_packet_size_key);
> > +
> > + dispatch_semaphore_signal(if_created_sem);
> > + });
> > +
> > + if (s->vmnet_if == NULL) {
> > + dispatch_release(s->if_queue);
> > + dispatch_release(if_created_sem);
> > + error_setg(errp,
> > + "unable to create interface with requested
> params");
> > + return -1;
> > + }
> > +
> > + dispatch_semaphore_wait(if_created_sem, DISPATCH_TIME_FOREVER);
> > + dispatch_release(if_created_sem);
> > +
> > + if (if_status != VMNET_SUCCESS) {
> > + dispatch_release(s->if_queue);
> > + error_setg(errp,
> > + "cannot create vmnet interface: %s",
> > + vmnet_status_map_str(if_status));
> > + return -1;
> > + }
> > +
> > + s->send_bh = aio_bh_new(qemu_get_aio_context(),
> vmnet_send_bh, nc);
> > + s->send_enabled = true;
> > + vmnet_bufs_init(s);
> > +
> > + vmnet_interface_set_event_callback(
> > + s->vmnet_if,
> > + VMNET_INTERFACE_PACKETS_AVAILABLE,
> > + s->if_queue,
> > + ^(interface_event_t event_id, xpc_object_t event) {
> > + assert(event_id == VMNET_INTERFACE_PACKETS_AVAILABLE);
> > + /*
> > + * This function is being called from a non qemu
> thread, so
> > + * we only schedule a BH, and do the rest of the io
> completion
> > + * handling from vmnet_send_bh() which runs in a
> qemu context.
> > + */
> > + qemu_bh_schedule(s->send_bh);
> > + });
> > +
> > + return 0;
> > +}
> > +
> > +
> > +ssize_t vmnet_receive_common(NetClientState *nc,
> > + const uint8_t *buf,
> > + size_t size)
> > +{
> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > + struct vmpktdesc packet;
> > + struct iovec iov;
> > + int pkt_cnt;
> > + vmnet_return_t if_status;
> > +
> > + if (size > s->max_packet_size) {
> > + warn_report("vmnet: packet is too big, %zu > %" PRIu64,
> > + packet.vm_pkt_size,
> > + s->max_packet_size);
> > + return -1;
> > + }
> > +
> > + iov.iov_base = (char *) buf;
> > + iov.iov_len = size;
> > +
> > + packet.vm_pkt_iovcnt = 1;
> > + packet.vm_flags = 0;
> > + packet.vm_pkt_size = size;
> > + packet.vm_pkt_iov = &iov;
> > + pkt_cnt = 1;
> > +
> > + if_status = vmnet_write(s->vmnet_if, &packet, &pkt_cnt);
> > + if (if_status != VMNET_SUCCESS) {
> > + error_report("vmnet: write error: %s\n",
> > + vmnet_status_map_str(if_status));
> > + return -1;
> > + }
> > +
> > + if (if_status == VMNET_SUCCESS && pkt_cnt) {
>
> `if_status == VMNET_SUCCESS` is redundant.
>
>
> Missed this, thanks.
>
> Regards,
> Akihiko Odaki
>
> > + return size;
> > + }
> > + return 0;
> > +}
> > +
> > +
> > +void vmnet_cleanup_common(NetClientState *nc)
> > +{
> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > + dispatch_semaphore_t if_stopped_sem;
> > +
> > + if (s->vmnet_if == NULL) {
> > + return;
> > + }
> > +
> > + vmnet_interface_set_event_callback(
> > + s->vmnet_if,
> > + VMNET_INTERFACE_PACKETS_AVAILABLE,
> > + NULL,
> > + NULL);
>
> I don't think this vmnet_interface_set_event_callback call is necessary.
>
>
> I kept in mind that vmnet processing lives in a separate thread
> and while cleanup it may continue receiving packets. While the
> queue is not empty, vmnet_stop_interface hangs. Unregistering
> callback ensures that this queue will be emptied asap.
>
> It will work without vmnet_interface_set_event_callback here,
> but I think it's better to be respectful to vmnet and clean everything
> we can :)
You may put qemu_purge_queued_packets after vmnet_stop_interface if you
think about the case it keeps receving packets while cleaning up since
it is the only thing it does before calling vmnet_stop_interface.
vmnet_stop_interface would then stop things in the proper order. It may
decide to stop event callbacks first. Otherwise, it may decide to stop
some internal heavy functionality first. It is up to vmnet.framework.
Regards,
Akihiko Odaki
> Thank you!
>
> Best Regards,
>
> Vladislav
>
> > +
> > + qemu_purge_queued_packets(nc);
> > +
> > + if_stopped_sem = dispatch_semaphore_create(0);
> > + vmnet_stop_interface(
> > + s->vmnet_if,
> > + s->if_queue,
> > + ^(vmnet_return_t status) {
> > + assert(status == VMNET_SUCCESS);
> > + dispatch_semaphore_signal(if_stopped_sem);
> > + });
> > + dispatch_semaphore_wait(if_stopped_sem, DISPATCH_TIME_FOREVER);
> > +
> > + qemu_bh_delete(s->send_bh);
> > + dispatch_release(if_stopped_sem);
> > + dispatch_release(s->if_queue);
> > +
> > + for (int i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
> > + g_free(s->iov_buf[i].iov_base);
> > + }
> > +}
> > diff --git a/net/vmnet-shared.c b/net/vmnet-shared.c
> > index f07afaaf21..2f4eb1db2d 100644
> > --- a/net/vmnet-shared.c
> > +++ b/net/vmnet-shared.c
> > @@ -10,16 +10,103 @@
> >
> > #include "qemu/osdep.h"
> > #include "qapi/qapi-types-net.h"
> > +#include "qapi/error.h"
> > #include "vmnet_int.h"
> > #include "clients.h"
> > -#include "qemu/error-report.h"
> > -#include "qapi/error.h"
> >
> > #include <vmnet/vmnet.h>
> >
> > +typedef struct VmnetSharedState {
> > + VmnetCommonState cs;
> > +} VmnetSharedState;
> > +
> > +
> > +static bool validate_options(const Netdev *netdev, Error **errp)
> > +{
> > + const NetdevVmnetSharedOptions *options =
> &(netdev->u.vmnet_shared);
> > +
> > +#if !defined(MAC_OS_VERSION_11_0) || \
> > + MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_VERSION_11_0
> > + if (options->has_isolated) {
> > + error_setg(errp,
> > + "vmnet-shared.isolated feature is "
> > + "unavailable: outdated vmnet.framework API");
> > + return false;
> > + }
> > +#endif
> > +
> > + if ((options->has_start_address ||
> > + options->has_end_address ||
> > + options->has_subnet_mask) &&
> > + !(options->has_start_address &&
> > + options->has_end_address &&
> > + options->has_subnet_mask)) {
> > + error_setg(errp,
> > + "'start-address', 'end-address', 'subnet-mask' "
> > + "should be provided together"
> > + );
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static xpc_object_t build_if_desc(const Netdev *netdev)
> > +{
> > + const NetdevVmnetSharedOptions *options =
> &(netdev->u.vmnet_shared);
> > + xpc_object_t if_desc = xpc_dictionary_create(NULL, NULL, 0);
> > +
> > + xpc_dictionary_set_uint64(
> > + if_desc,
> > + vmnet_operation_mode_key,
> > + VMNET_SHARED_MODE
> > + );
> > +
> > + if (options->has_nat66_prefix) {
> > + xpc_dictionary_set_string(if_desc,
> > + vmnet_nat66_prefix_key,
> > + options->nat66_prefix);
> > + }
> > +
> > + if (options->has_start_address) {
> > + xpc_dictionary_set_string(if_desc,
> > + vmnet_start_address_key,
> > + options->start_address);
> > + xpc_dictionary_set_string(if_desc,
> > + vmnet_end_address_key,
> > + options->end_address);
> > + xpc_dictionary_set_string(if_desc,
> > + vmnet_subnet_mask_key,
> > + options->subnet_mask);
> > + }
> > +
> > +#if defined(MAC_OS_VERSION_11_0) && \
> > + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> > + xpc_dictionary_set_bool(
> > + if_desc,
> > + vmnet_enable_isolation_key,
> > + options->isolated
> > + );
> > +#endif
> > +
> > + return if_desc;
> > +}
> > +
> > +static NetClientInfo net_vmnet_shared_info = {
> > + .type = NET_CLIENT_DRIVER_VMNET_SHARED,
> > + .size = sizeof(VmnetSharedState),
> > + .receive = vmnet_receive_common,
> > + .cleanup = vmnet_cleanup_common,
> > +};
> > +
> > int net_init_vmnet_shared(const Netdev *netdev, const char *name,
> > NetClientState *peer, Error **errp)
> > {
> > - error_setg(errp, "vmnet-shared is not implemented yet");
> > - return -1;
> > + NetClientState *nc = qemu_new_net_client(&net_vmnet_shared_info,
> > + peer,
> "vmnet-shared", name);
> > + if (!validate_options(netdev, errp)) {
> > + g_assert_not_reached();
> > + return -1;
> > + }
> > + return vmnet_if_create(nc, build_if_desc(netdev), errp);
> > }
> > diff --git a/net/vmnet_int.h b/net/vmnet_int.h
> > index aac4d5af64..8f3321ef3e 100644
> > --- a/net/vmnet_int.h
> > +++ b/net/vmnet_int.h
> > @@ -15,11 +15,50 @@
> > #include "clients.h"
> >
> > #include <vmnet/vmnet.h>
> > +#include <dispatch/dispatch.h>
> > +
> > +/**
> > + * From vmnet.framework documentation
> > + *
> > + * Each read/write call allows up to 200 packets to be
> > + * read or written for a maximum of 256KB.
> > + *
> > + * Each packet written should be a complete
> > + * ethernet frame.
> > + *
> > + * https://developer.apple.com/documentation/vmnet
> <https://developer.apple.com/documentation/vmnet>
> > + */
> > +#define VMNET_PACKETS_LIMIT 200
> >
> > typedef struct VmnetCommonState {
> > - NetClientState nc;
> > + NetClientState nc;
> > + interface_ref vmnet_if;
> > +
> > + uint64_t mtu;
> > + uint64_t max_packet_size;
> >
> > + dispatch_queue_t if_queue;
> > +
> > + QEMUBH *send_bh;
> > + bool send_enabled;
> > +
> > + struct vmpktdesc packets_buf[VMNET_PACKETS_LIMIT];
> > + int packets_send_current_pos;
> > + int packets_send_end_pos;
> > +
> > + struct iovec iov_buf[VMNET_PACKETS_LIMIT];
> > } VmnetCommonState;
> >
> > +const char *vmnet_status_map_str(vmnet_return_t status);
> > +
> > +int vmnet_if_create(NetClientState *nc,
> > + xpc_object_t if_desc,
> > + Error **errp);
> > +
> > +ssize_t vmnet_receive_common(NetClientState *nc,
> > + const uint8_t *buf,
> > + size_t size);
> > +
> > +void vmnet_cleanup_common(NetClientState *nc);
> >
> > #endif /* VMNET_INT_H */
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v16 3/7] net/vmnet: implement shared mode (vmnet-shared)
2022-03-14 22:34 ` Akihiko Odaki
@ 2022-03-14 22:37 ` Akihiko Odaki
2022-03-14 23:02 ` Vladislav Yaroshchuk
1 sibling, 0 replies; 16+ messages in thread
From: Akihiko Odaki @ 2022-03-14 22:37 UTC (permalink / raw)
To: Vladislav Yaroshchuk
Cc: Peter Maydell, Gerd Hoffmann, Alex Bennée, Jason Wang,
phillip.ennen, qemu Developers, Cameron Esfahani,
Markus Armbruster, Roman Bolshakov, Alexander Graf,
Phillip Tennen, Roman Bolshakov, Howard Spoelstra,
Alessio Dionisi, Christian Schoenebeck, Eric Blake,
Philippe Mathieu-Daudé
On 2022/03/15 7:34, Akihiko Odaki wrote:
> On 2022/03/15 6:50, Vladislav Yaroshchuk wrote:
>> Thank you, Akihiko
>>
>> On Mon, Mar 14, 2022 at 10:46 PM Akihiko Odaki
>> <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>> wrote:
>>
>> On 2022/03/15 4:15, Vladislav Yaroshchuk wrote:
>> > vmnet.framework supports iov, but writing more than
>> > one iov into vmnet interface fails with
>> > 'VMNET_INVALID_ARGUMENT'. Collecting provided iovs into
>> > one and passing it to vmnet works fine. That's the
>> > reason why receive_iov() left unimplemented. But it still
>> > works with good enough performance having .receive()
>> > implemented only.
>> >
>> > Signed-off-by: Phillip Tennen <phillip@axleos.com
>> <mailto:phillip@axleos.com>>
>> > Signed-off-by: Vladislav Yaroshchuk
>> <Vladislav.Yaroshchuk@jetbrains.com
>> <mailto:Vladislav.Yaroshchuk@jetbrains.com>>
>> > ---
>> > net/vmnet-common.m | 298
>> +++++++++++++++++++++++++++++++++++++++++++++
>> > net/vmnet-shared.c | 95 ++++++++++++++-
>> > net/vmnet_int.h | 41 ++++++-
>> > 3 files changed, 429 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/net/vmnet-common.m b/net/vmnet-common.m
>> > index 56612c72ce..20a33d2591 100644
>> > --- a/net/vmnet-common.m
>> > +++ b/net/vmnet-common.m
>> > @@ -10,6 +10,8 @@
>> > */
>> >
>> > #include "qemu/osdep.h"
>> > +#include "qemu/main-loop.h"
>> > +#include "qemu/log.h"
>> > #include "qapi/qapi-types-net.h"
>> > #include "vmnet_int.h"
>> > #include "clients.h"
>> > @@ -17,4 +19,300 @@
>> > #include "qapi/error.h"
>> >
>> > #include <vmnet/vmnet.h>
>> > +#include <dispatch/dispatch.h>
>> >
>> > +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s);
>>
>> The names of vmnet_qemu_send_wrapper and vmnet_send_bh does not tell
>> them apart well. Since only vmnet_send_bh does reading, its name may
>> include "read" to clarify that. "wrapper" in vmnet_qemu_send_wrapper
>> may
>> be also misleading as it does more than just calling the underlying
>> QEMU
>> facility, but it also updates VmnetCommonState.
>>
>>
>> Ok, I'll think about how to name them better.
>>
>> > +
>> > +
>> > +static void vmnet_send_completed(NetClientState *nc, ssize_t len)
>> > +{
>> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
>> > + /* Complete sending packets left in VmnetCommonState
>> buffers */
>> > + s->send_enabled = vmnet_qemu_send_wrapper(s);
>>
>> It must qemu_bh_schedule(s->send_bh) after vmnet_qemu_send_wrapper.
>>
>>
>> Agree with you, thanks.
>>
>> Also, send_enabled flag can be removed as explained in:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html
>> <https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html>
>>
>>
>> Not sure about this. Values of packets_send_current_pos
>> and packets_send_end_pos may be equal, but QEMU may be
>> not ready to receive new packets - the explanation:
>> 1. We are sending packets to QEMU with qemu_send_packet_async:
>> packets_send_current_pos = 0
>> packets_send_end_pos = 5
>> 2. All five packets (0, 1, 2, 3, 4) have been successfully sent to QEMU,
>> but qemu_send_packet_async returned 0 "no more packets" after
>> the last invocation
>> 3. In spite of this, all five packets are sent and
>> packets_send_current_pos == packets_send_end_pos == 5
>> 4. It seems that "pointers are equal -> QEMU is ready", but actually
>> it is not.
>>
>> Also, hiding QEMU "ready"/"not ready" state behind pointers is a
>> bad choice I think. Having a concrete flag makes this more clear.
>> It provides understandability, not complexity (imho).
>
> packets_send_current_pos must not be incremented if
> qemu_send_packet_async returned 0. It must tell the position of the
> packet currently being sent.
(And of course increment it in vmnet_send_completed instead.)
>
> It would not hide the state, but it would rather make it clear that the
> condition vmnet_send_bh can execute. If you see the current
> implementation of vmnet_send_bh, you'll find the send_enabled flag, but
> it does not tell the exact condition it requires to be enabled. You have
> to then jump to all assignments for the flag to know it becomes true iff
> every packets in the buffer are sent. It is obvious if vmnet_send_bh
> directly states `if (packets_send_current_pos < packets_send_end_pos)`.
>
> Eliminating the flag would also remove the possiblity of forgetting to
> maintain the separate state.
>
>>
>> > send_enabled can be eliminated. When it is enabled,
>> packets_send_pos
>> > and packets_batch_size must be equal. They must not be equal
>> > otherwise. packets_send_pos must represent the position of the
>> packet
>> > which is not sent yet, possibly in the process of sending.
>> > vmnet_send_completed must call qemu_send_wrapper before
>> scheduling
>> > send_bh. bh_send should do nothing if s->packets_send_pos <
>> > s->packets_batch_size.
>>
>> > +}
>> > +
>> > +
>> > +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s) {
>> > + ssize_t size;
>> > +
>> > + /*
>> > + * Packets to send lay in [current_pos..end_pos)
>> > + * (including current_pos, excluding end_pos)
>> > + */
>> > + while (s->packets_send_current_pos <
>> s->packets_send_end_pos) {
>> > + size = qemu_send_packet_async(&s->nc,
>> > + s->iov_buf[s->packets_send_current_pos].iov_base,
>> > + s->packets_buf[s->packets_send_current_pos].vm_pkt_size,
>> > + vmnet_send_completed);
>> > + ++s->packets_send_current_pos;
>> > + if (size == 0) {
>> > + /* QEMU is not ready - wait for completion callback
>> call */
>> > + return false;
>> > + }
>> > + }
>> > + return true;
>> > +}
>> > +
>> > +
>> > +static void vmnet_send_bh(void *opaque)
>> > +{
>> > + NetClientState *nc = (NetClientState *) opaque;
>> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
>> > + struct vmpktdesc *packets = s->packets_buf;
>> > + vmnet_return_t status;
>> > + int i;
>> > +
>> > + /*
>> > + * Do nothing if QEMU is not ready - wait
>> > + * for completion callback invocation
>> > + */
>> > + if (!s->send_enabled) {
>> > + return;
>> > + }
>> > +
>> > + /* Read as many packets as present */
>> > + s->packets_send_current_pos = 0;
>> > + s->packets_send_end_pos = VMNET_PACKETS_LIMIT;
>> > + for (i = 0; i < s->packets_send_end_pos; ++i) {
>> > + packets[i].vm_pkt_size = s->max_packet_size;
>> > + packets[i].vm_pkt_iovcnt = 1;
>> > + packets[i].vm_flags = 0;
>> > + }
>> > +
>> > + status = vmnet_read(s->vmnet_if, packets,
>> &s->packets_send_end_pos);
>> > + if (status != VMNET_SUCCESS) {
>> > + error_printf("vmnet: read failed: %s\n",
>> > + vmnet_status_map_str(status));
>> > + s->packets_send_current_pos = 0;
>> > + s->packets_send_end_pos = 0;
>> > + return;
>> > + }
>> > +
>> > + /* Send packets to QEMU */
>> > + s->send_enabled = vmnet_qemu_send_wrapper(s);
>> > +}
>> > +
>> > +
>> > +static void vmnet_bufs_init(VmnetCommonState *s)
>> > +{
>> > + struct vmpktdesc *packets = s->packets_buf;
>> > + struct iovec *iov = s->iov_buf;
>> > + int i;
>> > +
>> > + for (i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
>> > + iov[i].iov_len = s->max_packet_size;
>> > + iov[i].iov_base = g_malloc0(iov[i].iov_len);
>> > + packets[i].vm_pkt_iov = iov + i;
>> > + }
>> > +}
>> > +
>> > +
>> > +const char *vmnet_status_map_str(vmnet_return_t status)
>> > +{
>> > + switch (status) {
>> > + case VMNET_SUCCESS:
>> > + return "success";
>> > + case VMNET_FAILURE:
>> > + return "general failure (possibly not enough
>> privileges)";
>> > + case VMNET_MEM_FAILURE:
>> > + return "memory allocation failure";
>> > + case VMNET_INVALID_ARGUMENT:
>> > + return "invalid argument specified";
>> > + case VMNET_SETUP_INCOMPLETE:
>> > + return "interface setup is not complete";
>> > + case VMNET_INVALID_ACCESS:
>> > + return "invalid access, permission denied";
>> > + case VMNET_PACKET_TOO_BIG:
>> > + return "packet size is larger than MTU";
>> > + case VMNET_BUFFER_EXHAUSTED:
>> > + return "buffers exhausted in kernel";
>> > + case VMNET_TOO_MANY_PACKETS:
>> > + return "packet count exceeds limit";
>> > +#if defined(MAC_OS_VERSION_11_0) && \
>> > + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
>> > + case VMNET_SHARING_SERVICE_BUSY:
>> > + return "conflict, sharing service is in use";
>> > +#endif
>> > + default:
>> > + return "unknown vmnet error";
>> > + }
>> > +}
>> > +
>> > +
>> > +int vmnet_if_create(NetClientState *nc,
>> > + xpc_object_t if_desc,
>> > + Error **errp)
>> > +{
>> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
>> > + dispatch_semaphore_t if_created_sem =
>> dispatch_semaphore_create(0);
>> > + __block vmnet_return_t if_status;
>> > +
>> > + s->if_queue = dispatch_queue_create(
>> > + "org.qemu.vmnet.if_queue",
>> > + DISPATCH_QUEUE_SERIAL
>> > + );
>> > +
>> > + xpc_dictionary_set_bool(
>> > + if_desc,
>> > + vmnet_allocate_mac_address_key,
>> > + false
>> > + );
>> > +
>> > +#ifdef DEBUG
>> > + qemu_log("vmnet.start.interface_desc:\n");
>> > + xpc_dictionary_apply(if_desc,
>> > + ^bool(const char *k, xpc_object_t v) {
>> > + char *desc =
>> xpc_copy_description(v);
>> > + qemu_log(" %s=%s\n", k, desc);
>> > + free(desc);
>> > + return true;
>> > + });
>> > +#endif /* DEBUG */
>> > +
>> > + s->vmnet_if = vmnet_start_interface(
>> > + if_desc,
>> > + s->if_queue,
>> > + ^(vmnet_return_t status, xpc_object_t interface_param) {
>> > + if_status = status;
>> > + if (status != VMNET_SUCCESS || !interface_param) {
>> > + dispatch_semaphore_signal(if_created_sem);
>> > + return;
>> > + }
>> > +
>> > +#ifdef DEBUG
>> > + qemu_log("vmnet.start.interface_param:\n");
>> > + xpc_dictionary_apply(interface_param,
>> > + ^bool(const char *k,
>> xpc_object_t v) {
>> > + char *desc =
>> xpc_copy_description(v);
>> > + qemu_log(" %s=%s\n", k,
>> desc);
>> > + free(desc);
>> > + return true;
>> > + });
>> > +#endif /* DEBUG */
>> > +
>> > + s->mtu = xpc_dictionary_get_uint64(
>> > + interface_param,
>> > + vmnet_mtu_key);
>> > + s->max_packet_size = xpc_dictionary_get_uint64(
>> > + interface_param,
>> > + vmnet_max_packet_size_key);
>> > +
>> > + dispatch_semaphore_signal(if_created_sem);
>> > + });
>> > +
>> > + if (s->vmnet_if == NULL) {
>> > + dispatch_release(s->if_queue);
>> > + dispatch_release(if_created_sem);
>> > + error_setg(errp,
>> > + "unable to create interface with requested
>> params");
>> > + return -1;
>> > + }
>> > +
>> > + dispatch_semaphore_wait(if_created_sem,
>> DISPATCH_TIME_FOREVER);
>> > + dispatch_release(if_created_sem);
>> > +
>> > + if (if_status != VMNET_SUCCESS) {
>> > + dispatch_release(s->if_queue);
>> > + error_setg(errp,
>> > + "cannot create vmnet interface: %s",
>> > + vmnet_status_map_str(if_status));
>> > + return -1;
>> > + }
>> > +
>> > + s->send_bh = aio_bh_new(qemu_get_aio_context(),
>> vmnet_send_bh, nc);
>> > + s->send_enabled = true;
>> > + vmnet_bufs_init(s);
>> > +
>> > + vmnet_interface_set_event_callback(
>> > + s->vmnet_if,
>> > + VMNET_INTERFACE_PACKETS_AVAILABLE,
>> > + s->if_queue,
>> > + ^(interface_event_t event_id, xpc_object_t event) {
>> > + assert(event_id ==
>> VMNET_INTERFACE_PACKETS_AVAILABLE);
>> > + /*
>> > + * This function is being called from a non qemu
>> thread, so
>> > + * we only schedule a BH, and do the rest of the io
>> completion
>> > + * handling from vmnet_send_bh() which runs in a
>> qemu context.
>> > + */
>> > + qemu_bh_schedule(s->send_bh);
>> > + });
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +
>> > +ssize_t vmnet_receive_common(NetClientState *nc,
>> > + const uint8_t *buf,
>> > + size_t size)
>> > +{
>> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
>> > + struct vmpktdesc packet;
>> > + struct iovec iov;
>> > + int pkt_cnt;
>> > + vmnet_return_t if_status;
>> > +
>> > + if (size > s->max_packet_size) {
>> > + warn_report("vmnet: packet is too big, %zu > %" PRIu64,
>> > + packet.vm_pkt_size,
>> > + s->max_packet_size);
>> > + return -1;
>> > + }
>> > +
>> > + iov.iov_base = (char *) buf;
>> > + iov.iov_len = size;
>> > +
>> > + packet.vm_pkt_iovcnt = 1;
>> > + packet.vm_flags = 0;
>> > + packet.vm_pkt_size = size;
>> > + packet.vm_pkt_iov = &iov;
>> > + pkt_cnt = 1;
>> > +
>> > + if_status = vmnet_write(s->vmnet_if, &packet, &pkt_cnt);
>> > + if (if_status != VMNET_SUCCESS) {
>> > + error_report("vmnet: write error: %s\n",
>> > + vmnet_status_map_str(if_status));
>> > + return -1;
>> > + }
>> > +
>> > + if (if_status == VMNET_SUCCESS && pkt_cnt) {
>>
>> `if_status == VMNET_SUCCESS` is redundant.
>>
>>
>> Missed this, thanks.
>>
>> Regards,
>> Akihiko Odaki
>>
>> > + return size;
>> > + }
>> > + return 0;
>> > +}
>> > +
>> > +
>> > +void vmnet_cleanup_common(NetClientState *nc)
>> > +{
>> > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
>> > + dispatch_semaphore_t if_stopped_sem;
>> > +
>> > + if (s->vmnet_if == NULL) {
>> > + return;
>> > + }
>> > +
>> > + vmnet_interface_set_event_callback(
>> > + s->vmnet_if,
>> > + VMNET_INTERFACE_PACKETS_AVAILABLE,
>> > + NULL,
>> > + NULL);
>>
>> I don't think this vmnet_interface_set_event_callback call is
>> necessary.
>>
>>
>> I kept in mind that vmnet processing lives in a separate thread
>> and while cleanup it may continue receiving packets. While the
>> queue is not empty, vmnet_stop_interface hangs. Unregistering
>> callback ensures that this queue will be emptied asap.
>>
>> It will work without vmnet_interface_set_event_callback here,
>> but I think it's better to be respectful to vmnet and clean everything
>> we can :)
>
> You may put qemu_purge_queued_packets after vmnet_stop_interface if you
> think about the case it keeps receving packets while cleaning up since
> it is the only thing it does before calling vmnet_stop_interface.
> vmnet_stop_interface would then stop things in the proper order. It may
> decide to stop event callbacks first. Otherwise, it may decide to stop
> some internal heavy functionality first. It is up to vmnet.framework.
>
> Regards,
> Akihiko Odaki
>
>> Thank you!
>>
>> Best Regards,
>>
>> Vladislav
>>
>> > +
>> > + qemu_purge_queued_packets(nc);
>> > +
>> > + if_stopped_sem = dispatch_semaphore_create(0);
>> > + vmnet_stop_interface(
>> > + s->vmnet_if,
>> > + s->if_queue,
>> > + ^(vmnet_return_t status) {
>> > + assert(status == VMNET_SUCCESS);
>> > + dispatch_semaphore_signal(if_stopped_sem);
>> > + });
>> > + dispatch_semaphore_wait(if_stopped_sem,
>> DISPATCH_TIME_FOREVER);
>> > +
>> > + qemu_bh_delete(s->send_bh);
>> > + dispatch_release(if_stopped_sem);
>> > + dispatch_release(s->if_queue);
>> > +
>> > + for (int i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
>> > + g_free(s->iov_buf[i].iov_base);
>> > + }
>> > +}
>> > diff --git a/net/vmnet-shared.c b/net/vmnet-shared.c
>> > index f07afaaf21..2f4eb1db2d 100644
>> > --- a/net/vmnet-shared.c
>> > +++ b/net/vmnet-shared.c
>> > @@ -10,16 +10,103 @@
>> >
>> > #include "qemu/osdep.h"
>> > #include "qapi/qapi-types-net.h"
>> > +#include "qapi/error.h"
>> > #include "vmnet_int.h"
>> > #include "clients.h"
>> > -#include "qemu/error-report.h"
>> > -#include "qapi/error.h"
>> >
>> > #include <vmnet/vmnet.h>
>> >
>> > +typedef struct VmnetSharedState {
>> > + VmnetCommonState cs;
>> > +} VmnetSharedState;
>> > +
>> > +
>> > +static bool validate_options(const Netdev *netdev, Error **errp)
>> > +{
>> > + const NetdevVmnetSharedOptions *options =
>> &(netdev->u.vmnet_shared);
>> > +
>> > +#if !defined(MAC_OS_VERSION_11_0) || \
>> > + MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_VERSION_11_0
>> > + if (options->has_isolated) {
>> > + error_setg(errp,
>> > + "vmnet-shared.isolated feature is "
>> > + "unavailable: outdated vmnet.framework API");
>> > + return false;
>> > + }
>> > +#endif
>> > +
>> > + if ((options->has_start_address ||
>> > + options->has_end_address ||
>> > + options->has_subnet_mask) &&
>> > + !(options->has_start_address &&
>> > + options->has_end_address &&
>> > + options->has_subnet_mask)) {
>> > + error_setg(errp,
>> > + "'start-address', 'end-address',
>> 'subnet-mask' "
>> > + "should be provided together"
>> > + );
>> > + return false;
>> > + }
>> > +
>> > + return true;
>> > +}
>> > +
>> > +static xpc_object_t build_if_desc(const Netdev *netdev)
>> > +{
>> > + const NetdevVmnetSharedOptions *options =
>> &(netdev->u.vmnet_shared);
>> > + xpc_object_t if_desc = xpc_dictionary_create(NULL, NULL, 0);
>> > +
>> > + xpc_dictionary_set_uint64(
>> > + if_desc,
>> > + vmnet_operation_mode_key,
>> > + VMNET_SHARED_MODE
>> > + );
>> > +
>> > + if (options->has_nat66_prefix) {
>> > + xpc_dictionary_set_string(if_desc,
>> > + vmnet_nat66_prefix_key,
>> > + options->nat66_prefix);
>> > + }
>> > +
>> > + if (options->has_start_address) {
>> > + xpc_dictionary_set_string(if_desc,
>> > + vmnet_start_address_key,
>> > + options->start_address);
>> > + xpc_dictionary_set_string(if_desc,
>> > + vmnet_end_address_key,
>> > + options->end_address);
>> > + xpc_dictionary_set_string(if_desc,
>> > + vmnet_subnet_mask_key,
>> > + options->subnet_mask);
>> > + }
>> > +
>> > +#if defined(MAC_OS_VERSION_11_0) && \
>> > + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
>> > + xpc_dictionary_set_bool(
>> > + if_desc,
>> > + vmnet_enable_isolation_key,
>> > + options->isolated
>> > + );
>> > +#endif
>> > +
>> > + return if_desc;
>> > +}
>> > +
>> > +static NetClientInfo net_vmnet_shared_info = {
>> > + .type = NET_CLIENT_DRIVER_VMNET_SHARED,
>> > + .size = sizeof(VmnetSharedState),
>> > + .receive = vmnet_receive_common,
>> > + .cleanup = vmnet_cleanup_common,
>> > +};
>> > +
>> > int net_init_vmnet_shared(const Netdev *netdev, const char
>> *name,
>> > NetClientState *peer, Error **errp)
>> > {
>> > - error_setg(errp, "vmnet-shared is not implemented yet");
>> > - return -1;
>> > + NetClientState *nc =
>> qemu_new_net_client(&net_vmnet_shared_info,
>> > + peer,
>> "vmnet-shared", name);
>> > + if (!validate_options(netdev, errp)) {
>> > + g_assert_not_reached();
>> > + return -1;
>> > + }
>> > + return vmnet_if_create(nc, build_if_desc(netdev), errp);
>> > }
>> > diff --git a/net/vmnet_int.h b/net/vmnet_int.h
>> > index aac4d5af64..8f3321ef3e 100644
>> > --- a/net/vmnet_int.h
>> > +++ b/net/vmnet_int.h
>> > @@ -15,11 +15,50 @@
>> > #include "clients.h"
>> >
>> > #include <vmnet/vmnet.h>
>> > +#include <dispatch/dispatch.h>
>> > +
>> > +/**
>> > + * From vmnet.framework documentation
>> > + *
>> > + * Each read/write call allows up to 200 packets to be
>> > + * read or written for a maximum of 256KB.
>> > + *
>> > + * Each packet written should be a complete
>> > + * ethernet frame.
>> > + *
>> > + * https://developer.apple.com/documentation/vmnet
>> <https://developer.apple.com/documentation/vmnet>
>> > + */
>> > +#define VMNET_PACKETS_LIMIT 200
>> >
>> > typedef struct VmnetCommonState {
>> > - NetClientState nc;
>> > + NetClientState nc;
>> > + interface_ref vmnet_if;
>> > +
>> > + uint64_t mtu;
>> > + uint64_t max_packet_size;
>> >
>> > + dispatch_queue_t if_queue;
>> > +
>> > + QEMUBH *send_bh;
>> > + bool send_enabled;
>> > +
>> > + struct vmpktdesc packets_buf[VMNET_PACKETS_LIMIT];
>> > + int packets_send_current_pos;
>> > + int packets_send_end_pos;
>> > +
>> > + struct iovec iov_buf[VMNET_PACKETS_LIMIT];
>> > } VmnetCommonState;
>> >
>> > +const char *vmnet_status_map_str(vmnet_return_t status);
>> > +
>> > +int vmnet_if_create(NetClientState *nc,
>> > + xpc_object_t if_desc,
>> > + Error **errp);
>> > +
>> > +ssize_t vmnet_receive_common(NetClientState *nc,
>> > + const uint8_t *buf,
>> > + size_t size);
>> > +
>> > +void vmnet_cleanup_common(NetClientState *nc);
>> >
>> > #endif /* VMNET_INT_H */
>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v16 3/7] net/vmnet: implement shared mode (vmnet-shared)
2022-03-14 22:34 ` Akihiko Odaki
2022-03-14 22:37 ` Akihiko Odaki
@ 2022-03-14 23:02 ` Vladislav Yaroshchuk
2022-03-14 23:06 ` Akihiko Odaki
1 sibling, 1 reply; 16+ messages in thread
From: Vladislav Yaroshchuk @ 2022-03-14 23:02 UTC (permalink / raw)
To: Akihiko Odaki
Cc: qemu Developers, Jason Wang, Roman Bolshakov, Eric Blake,
phillip.ennen, Phillip Tennen, Markus Armbruster,
Howard Spoelstra, Alessio Dionisi, Roman Bolshakov,
Peter Maydell, Cameron Esfahani, Philippe Mathieu-Daudé,
Alexander Graf, Gerd Hoffmann, Alex Bennée,
Christian Schoenebeck
[-- Attachment #1: Type: text/plain, Size: 26705 bytes --]
вт, 15 мар. 2022 г., 1:34 AM Akihiko Odaki <akihiko.odaki@gmail.com>:
> On 2022/03/15 6:50, Vladislav Yaroshchuk wrote:
> > Thank you, Akihiko
> >
> > On Mon, Mar 14, 2022 at 10:46 PM Akihiko Odaki <akihiko.odaki@gmail.com
> > <mailto:akihiko.odaki@gmail.com>> wrote:
> >
> > On 2022/03/15 4:15, Vladislav Yaroshchuk wrote:
> > > vmnet.framework supports iov, but writing more than
> > > one iov into vmnet interface fails with
> > > 'VMNET_INVALID_ARGUMENT'. Collecting provided iovs into
> > > one and passing it to vmnet works fine. That's the
> > > reason why receive_iov() left unimplemented. But it still
> > > works with good enough performance having .receive()
> > > implemented only.
> > >
> > > Signed-off-by: Phillip Tennen <phillip@axleos.com
> > <mailto:phillip@axleos.com>>
> > > Signed-off-by: Vladislav Yaroshchuk
> > <Vladislav.Yaroshchuk@jetbrains.com
> > <mailto:Vladislav.Yaroshchuk@jetbrains.com>>
> > > ---
> > > net/vmnet-common.m | 298
> > +++++++++++++++++++++++++++++++++++++++++++++
> > > net/vmnet-shared.c | 95 ++++++++++++++-
> > > net/vmnet_int.h | 41 ++++++-
> > > 3 files changed, 429 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/net/vmnet-common.m b/net/vmnet-common.m
> > > index 56612c72ce..20a33d2591 100644
> > > --- a/net/vmnet-common.m
> > > +++ b/net/vmnet-common.m
> > > @@ -10,6 +10,8 @@
> > > */
> > >
> > > #include "qemu/osdep.h"
> > > +#include "qemu/main-loop.h"
> > > +#include "qemu/log.h"
> > > #include "qapi/qapi-types-net.h"
> > > #include "vmnet_int.h"
> > > #include "clients.h"
> > > @@ -17,4 +19,300 @@
> > > #include "qapi/error.h"
> > >
> > > #include <vmnet/vmnet.h>
> > > +#include <dispatch/dispatch.h>
> > >
> > > +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s);
> >
> > The names of vmnet_qemu_send_wrapper and vmnet_send_bh does not tell
> > them apart well. Since only vmnet_send_bh does reading, its name may
> > include "read" to clarify that. "wrapper" in vmnet_qemu_send_wrapper
> > may
> > be also misleading as it does more than just calling the underlying
> > QEMU
> > facility, but it also updates VmnetCommonState.
> >
> >
> > Ok, I'll think about how to name them better.
> >
> > > +
> > > +
> > > +static void vmnet_send_completed(NetClientState *nc, ssize_t len)
> > > +{
> > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > > + /* Complete sending packets left in VmnetCommonState buffers
> */
> > > + s->send_enabled = vmnet_qemu_send_wrapper(s);
> >
> > It must qemu_bh_schedule(s->send_bh) after vmnet_qemu_send_wrapper.
> >
> >
> > Agree with you, thanks.
> >
> > Also, send_enabled flag can be removed as explained in:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html
> > <https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html>
> >
> >
> > Not sure about this. Values of packets_send_current_pos
> > and packets_send_end_pos may be equal, but QEMU may be
> > not ready to receive new packets - the explanation:
> > 1. We are sending packets to QEMU with qemu_send_packet_async:
> > packets_send_current_pos = 0
> > packets_send_end_pos = 5
> > 2. All five packets (0, 1, 2, 3, 4) have been successfully sent to QEMU,
> > but qemu_send_packet_async returned 0 "no more packets" after
> > the last invocation
> > 3. In spite of this, all five packets are sent and
> > packets_send_current_pos == packets_send_end_pos == 5
> > 4. It seems that "pointers are equal -> QEMU is ready", but actually
> > it is not.
> >
> > Also, hiding QEMU "ready"/"not ready" state behind pointers is a
> > bad choice I think. Having a concrete flag makes this more clear.
> > It provides understandability, not complexity (imho).
>
> packets_send_current_pos must not be incremented if
> qemu_send_packet_async returned 0. It must tell the position of the
> packet currently being sent.
>
>
>
> must be incremented
It cannot.
If qemu_send_packet_async returns 0,
it still consumes (!) (queues internally) the packet.
So the packets_send_current_pos must be
incremented
to prevent sending same packet multiple times.
The idea is simple:
1. We've sent the packet - increment
2. Packed is not send - not increment
qemu_send_packet_async with 0 returned meets
the 1st case, because it still queues the packet.
While the increment action is not depends on the
returned value, we cannot use position pointers as a
criteria to send more packets or not. Another state
storage (flag) is required.
If You are not against, I'll cover this with proper
documentation (comments) to simplify future support
and make it more clear.
Best regards,
Vladislav Yaroshchuk
>
> It would not hide the state, but it would rather make it clear that the
> condition vmnet_send_bh can execute. If you see the current
> implementation of vmnet_send_bh, you'll find the send_enabled flag, but
> it does not tell the exact condition it requires to be enabled. You have
> to then jump to all assignments for the flag to know it becomes true iff
> every packets in the buffer are sent. It is obvious if vmnet_send_bh
> directly states `if (packets_send_current_pos < packets_send_end_pos)`.
>
> Eliminating the flag would also remove the possiblity of forgetting to
> maintain the separate state.
>
>
> >
> > > send_enabled can be eliminated. When it is enabled,
> packets_send_pos
> > > and packets_batch_size must be equal. They must not be equal
> > > otherwise. packets_send_pos must represent the position of the
> > packet
> > > which is not sent yet, possibly in the process of sending.
> > > vmnet_send_completed must call qemu_send_wrapper before
> scheduling
> > > send_bh. bh_send should do nothing if s->packets_send_pos <
> > > s->packets_batch_size.
> >
> > > +}
> > > +
> > > +
> > > +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s) {
> > > + ssize_t size;
> > > +
> > > + /*
> > > + * Packets to send lay in [current_pos..end_pos)
> > > + * (including current_pos, excluding end_pos)
> > > + */
> > > + while (s->packets_send_current_pos <
> s->packets_send_end_pos) {
> > > + size = qemu_send_packet_async(&s->nc,
> > > +
> > s->iov_buf[s->packets_send_current_pos].iov_base,
> > > +
> > s->packets_buf[s->packets_send_current_pos].vm_pkt_size,
> > > + vmnet_send_completed);
> > > + ++s->packets_send_current_pos;
> > > + if (size == 0) {
> > > + /* QEMU is not ready - wait for completion callback
> > call */
> > > + return false;
> > > + }
> > > + }
> > > + return true;
> > > +}
> > > +
> > > +
> > > +static void vmnet_send_bh(void *opaque)
> > > +{
> > > + NetClientState *nc = (NetClientState *) opaque;
> > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > > + struct vmpktdesc *packets = s->packets_buf;
> > > + vmnet_return_t status;
> > > + int i;
> > > +
> > > + /*
> > > + * Do nothing if QEMU is not ready - wait
> > > + * for completion callback invocation
> > > + */
> > > + if (!s->send_enabled) {
> > > + return;
> > > + }
> > > +
> > > + /* Read as many packets as present */
> > > + s->packets_send_current_pos = 0;
> > > + s->packets_send_end_pos = VMNET_PACKETS_LIMIT;
> > > + for (i = 0; i < s->packets_send_end_pos; ++i) {
> > > + packets[i].vm_pkt_size = s->max_packet_size;
> > > + packets[i].vm_pkt_iovcnt = 1;
> > > + packets[i].vm_flags = 0;
> > > + }
> > > +
> > > + status = vmnet_read(s->vmnet_if, packets,
> > &s->packets_send_end_pos);
> > > + if (status != VMNET_SUCCESS) {
> > > + error_printf("vmnet: read failed: %s\n",
> > > + vmnet_status_map_str(status));
> > > + s->packets_send_current_pos = 0;
> > > + s->packets_send_end_pos = 0;
> > > + return;
> > > + }
> > > +
> > > + /* Send packets to QEMU */
> > > + s->send_enabled = vmnet_qemu_send_wrapper(s);
> > > +}
> > > +
> > > +
> > > +static void vmnet_bufs_init(VmnetCommonState *s)
> > > +{
> > > + struct vmpktdesc *packets = s->packets_buf;
> > > + struct iovec *iov = s->iov_buf;
> > > + int i;
> > > +
> > > + for (i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
> > > + iov[i].iov_len = s->max_packet_size;
> > > + iov[i].iov_base = g_malloc0(iov[i].iov_len);
> > > + packets[i].vm_pkt_iov = iov + i;
> > > + }
> > > +}
> > > +
> > > +
> > > +const char *vmnet_status_map_str(vmnet_return_t status)
> > > +{
> > > + switch (status) {
> > > + case VMNET_SUCCESS:
> > > + return "success";
> > > + case VMNET_FAILURE:
> > > + return "general failure (possibly not enough
> privileges)";
> > > + case VMNET_MEM_FAILURE:
> > > + return "memory allocation failure";
> > > + case VMNET_INVALID_ARGUMENT:
> > > + return "invalid argument specified";
> > > + case VMNET_SETUP_INCOMPLETE:
> > > + return "interface setup is not complete";
> > > + case VMNET_INVALID_ACCESS:
> > > + return "invalid access, permission denied";
> > > + case VMNET_PACKET_TOO_BIG:
> > > + return "packet size is larger than MTU";
> > > + case VMNET_BUFFER_EXHAUSTED:
> > > + return "buffers exhausted in kernel";
> > > + case VMNET_TOO_MANY_PACKETS:
> > > + return "packet count exceeds limit";
> > > +#if defined(MAC_OS_VERSION_11_0) && \
> > > + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> > > + case VMNET_SHARING_SERVICE_BUSY:
> > > + return "conflict, sharing service is in use";
> > > +#endif
> > > + default:
> > > + return "unknown vmnet error";
> > > + }
> > > +}
> > > +
> > > +
> > > +int vmnet_if_create(NetClientState *nc,
> > > + xpc_object_t if_desc,
> > > + Error **errp)
> > > +{
> > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > > + dispatch_semaphore_t if_created_sem =
> > dispatch_semaphore_create(0);
> > > + __block vmnet_return_t if_status;
> > > +
> > > + s->if_queue = dispatch_queue_create(
> > > + "org.qemu.vmnet.if_queue",
> > > + DISPATCH_QUEUE_SERIAL
> > > + );
> > > +
> > > + xpc_dictionary_set_bool(
> > > + if_desc,
> > > + vmnet_allocate_mac_address_key,
> > > + false
> > > + );
> > > +
> > > +#ifdef DEBUG
> > > + qemu_log("vmnet.start.interface_desc:\n");
> > > + xpc_dictionary_apply(if_desc,
> > > + ^bool(const char *k, xpc_object_t v) {
> > > + char *desc =
> xpc_copy_description(v);
> > > + qemu_log(" %s=%s\n", k, desc);
> > > + free(desc);
> > > + return true;
> > > + });
> > > +#endif /* DEBUG */
> > > +
> > > + s->vmnet_if = vmnet_start_interface(
> > > + if_desc,
> > > + s->if_queue,
> > > + ^(vmnet_return_t status, xpc_object_t interface_param) {
> > > + if_status = status;
> > > + if (status != VMNET_SUCCESS || !interface_param) {
> > > + dispatch_semaphore_signal(if_created_sem);
> > > + return;
> > > + }
> > > +
> > > +#ifdef DEBUG
> > > + qemu_log("vmnet.start.interface_param:\n");
> > > + xpc_dictionary_apply(interface_param,
> > > + ^bool(const char *k,
> > xpc_object_t v) {
> > > + char *desc =
> > xpc_copy_description(v);
> > > + qemu_log(" %s=%s\n", k,
> desc);
> > > + free(desc);
> > > + return true;
> > > + });
> > > +#endif /* DEBUG */
> > > +
> > > + s->mtu = xpc_dictionary_get_uint64(
> > > + interface_param,
> > > + vmnet_mtu_key);
> > > + s->max_packet_size = xpc_dictionary_get_uint64(
> > > + interface_param,
> > > + vmnet_max_packet_size_key);
> > > +
> > > + dispatch_semaphore_signal(if_created_sem);
> > > + });
> > > +
> > > + if (s->vmnet_if == NULL) {
> > > + dispatch_release(s->if_queue);
> > > + dispatch_release(if_created_sem);
> > > + error_setg(errp,
> > > + "unable to create interface with requested
> > params");
> > > + return -1;
> > > + }
> > > +
> > > + dispatch_semaphore_wait(if_created_sem,
> DISPATCH_TIME_FOREVER);
> > > + dispatch_release(if_created_sem);
> > > +
> > > + if (if_status != VMNET_SUCCESS) {
> > > + dispatch_release(s->if_queue);
> > > + error_setg(errp,
> > > + "cannot create vmnet interface: %s",
> > > + vmnet_status_map_str(if_status));
> > > + return -1;
> > > + }
> > > +
> > > + s->send_bh = aio_bh_new(qemu_get_aio_context(),
> > vmnet_send_bh, nc);
> > > + s->send_enabled = true;
> > > + vmnet_bufs_init(s);
> > > +
> > > + vmnet_interface_set_event_callback(
> > > + s->vmnet_if,
> > > + VMNET_INTERFACE_PACKETS_AVAILABLE,
> > > + s->if_queue,
> > > + ^(interface_event_t event_id, xpc_object_t event) {
> > > + assert(event_id ==
> VMNET_INTERFACE_PACKETS_AVAILABLE);
> > > + /*
> > > + * This function is being called from a non qemu
> > thread, so
> > > + * we only schedule a BH, and do the rest of the io
> > completion
> > > + * handling from vmnet_send_bh() which runs in a
> > qemu context.
> > > + */
> > > + qemu_bh_schedule(s->send_bh);
> > > + });
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +
> > > +ssize_t vmnet_receive_common(NetClientState *nc,
> > > + const uint8_t *buf,
> > > + size_t size)
> > > +{
> > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > > + struct vmpktdesc packet;
> > > + struct iovec iov;
> > > + int pkt_cnt;
> > > + vmnet_return_t if_status;
> > > +
> > > + if (size > s->max_packet_size) {
> > > + warn_report("vmnet: packet is too big, %zu > %" PRIu64,
> > > + packet.vm_pkt_size,
> > > + s->max_packet_size);
> > > + return -1;
> > > + }
> > > +
> > > + iov.iov_base = (char *) buf;
> > > + iov.iov_len = size;
> > > +
> > > + packet.vm_pkt_iovcnt = 1;
> > > + packet.vm_flags = 0;
> > > + packet.vm_pkt_size = size;
> > > + packet.vm_pkt_iov = &iov;
> > > + pkt_cnt = 1;
> > > +
> > > + if_status = vmnet_write(s->vmnet_if, &packet, &pkt_cnt);
> > > + if (if_status != VMNET_SUCCESS) {
> > > + error_report("vmnet: write error: %s\n",
> > > + vmnet_status_map_str(if_status));
> > > + return -1;
> > > + }
> > > +
> > > + if (if_status == VMNET_SUCCESS && pkt_cnt) {
> >
> > `if_status == VMNET_SUCCESS` is redundant.
> >
> >
> > Missed this, thanks.
> >
> > Regards,
> > Akihiko Odaki
> >
> > > + return size;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +
> > > +void vmnet_cleanup_common(NetClientState *nc)
> > > +{
> > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
> > > + dispatch_semaphore_t if_stopped_sem;
> > > +
> > > + if (s->vmnet_if == NULL) {
> > > + return;
> > > + }
> > > +
> > > + vmnet_interface_set_event_callback(
> > > + s->vmnet_if,
> > > + VMNET_INTERFACE_PACKETS_AVAILABLE,
> > > + NULL,
> > > + NULL);
> >
> > I don't think this vmnet_interface_set_event_callback call is
> necessary.
> >
> >
> > I kept in mind that vmnet processing lives in a separate thread
> > and while cleanup it may continue receiving packets. While the
> > queue is not empty, vmnet_stop_interface hangs. Unregistering
> > callback ensures that this queue will be emptied asap.
> >
> > It will work without vmnet_interface_set_event_callback here,
> > but I think it's better to be respectful to vmnet and clean everything
> > we can :)
>
> You may put qemu_purge_queued_packets after vmnet_stop_interface if you
> think about the case it keeps receving packets while cleaning up since
> it is the only thing it does before calling vmnet_stop_interface.
> vmnet_stop_interface would then stop things in the proper order. It may
> decide to stop event callbacks first. Otherwise, it may decide to stop
> some internal heavy functionality first. It is up to vmnet.framework.
>
> Regards,
> Akihiko Odaki
>
> > Thank you!
> >
> > Best Regards,
> >
> > Vladislav
> >
> > > +
> > > + qemu_purge_queued_packets(nc);
> > > +
> > > + if_stopped_sem = dispatch_semaphore_create(0);
> > > + vmnet_stop_interface(
> > > + s->vmnet_if,
> > > + s->if_queue,
> > > + ^(vmnet_return_t status) {
> > > + assert(status == VMNET_SUCCESS);
> > > + dispatch_semaphore_signal(if_stopped_sem);
> > > + });
> > > + dispatch_semaphore_wait(if_stopped_sem,
> DISPATCH_TIME_FOREVER);
> > > +
> > > + qemu_bh_delete(s->send_bh);
> > > + dispatch_release(if_stopped_sem);
> > > + dispatch_release(s->if_queue);
> > > +
> > > + for (int i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
> > > + g_free(s->iov_buf[i].iov_base);
> > > + }
> > > +}
> > > diff --git a/net/vmnet-shared.c b/net/vmnet-shared.c
> > > index f07afaaf21..2f4eb1db2d 100644
> > > --- a/net/vmnet-shared.c
> > > +++ b/net/vmnet-shared.c
> > > @@ -10,16 +10,103 @@
> > >
> > > #include "qemu/osdep.h"
> > > #include "qapi/qapi-types-net.h"
> > > +#include "qapi/error.h"
> > > #include "vmnet_int.h"
> > > #include "clients.h"
> > > -#include "qemu/error-report.h"
> > > -#include "qapi/error.h"
> > >
> > > #include <vmnet/vmnet.h>
> > >
> > > +typedef struct VmnetSharedState {
> > > + VmnetCommonState cs;
> > > +} VmnetSharedState;
> > > +
> > > +
> > > +static bool validate_options(const Netdev *netdev, Error **errp)
> > > +{
> > > + const NetdevVmnetSharedOptions *options =
> > &(netdev->u.vmnet_shared);
> > > +
> > > +#if !defined(MAC_OS_VERSION_11_0) || \
> > > + MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_VERSION_11_0
> > > + if (options->has_isolated) {
> > > + error_setg(errp,
> > > + "vmnet-shared.isolated feature is "
> > > + "unavailable: outdated vmnet.framework API");
> > > + return false;
> > > + }
> > > +#endif
> > > +
> > > + if ((options->has_start_address ||
> > > + options->has_end_address ||
> > > + options->has_subnet_mask) &&
> > > + !(options->has_start_address &&
> > > + options->has_end_address &&
> > > + options->has_subnet_mask)) {
> > > + error_setg(errp,
> > > + "'start-address', 'end-address',
> 'subnet-mask' "
> > > + "should be provided together"
> > > + );
> > > + return false;
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +static xpc_object_t build_if_desc(const Netdev *netdev)
> > > +{
> > > + const NetdevVmnetSharedOptions *options =
> > &(netdev->u.vmnet_shared);
> > > + xpc_object_t if_desc = xpc_dictionary_create(NULL, NULL, 0);
> > > +
> > > + xpc_dictionary_set_uint64(
> > > + if_desc,
> > > + vmnet_operation_mode_key,
> > > + VMNET_SHARED_MODE
> > > + );
> > > +
> > > + if (options->has_nat66_prefix) {
> > > + xpc_dictionary_set_string(if_desc,
> > > + vmnet_nat66_prefix_key,
> > > + options->nat66_prefix);
> > > + }
> > > +
> > > + if (options->has_start_address) {
> > > + xpc_dictionary_set_string(if_desc,
> > > + vmnet_start_address_key,
> > > + options->start_address);
> > > + xpc_dictionary_set_string(if_desc,
> > > + vmnet_end_address_key,
> > > + options->end_address);
> > > + xpc_dictionary_set_string(if_desc,
> > > + vmnet_subnet_mask_key,
> > > + options->subnet_mask);
> > > + }
> > > +
> > > +#if defined(MAC_OS_VERSION_11_0) && \
> > > + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> > > + xpc_dictionary_set_bool(
> > > + if_desc,
> > > + vmnet_enable_isolation_key,
> > > + options->isolated
> > > + );
> > > +#endif
> > > +
> > > + return if_desc;
> > > +}
> > > +
> > > +static NetClientInfo net_vmnet_shared_info = {
> > > + .type = NET_CLIENT_DRIVER_VMNET_SHARED,
> > > + .size = sizeof(VmnetSharedState),
> > > + .receive = vmnet_receive_common,
> > > + .cleanup = vmnet_cleanup_common,
> > > +};
> > > +
> > > int net_init_vmnet_shared(const Netdev *netdev, const char
> *name,
> > > NetClientState *peer, Error **errp)
> > > {
> > > - error_setg(errp, "vmnet-shared is not implemented yet");
> > > - return -1;
> > > + NetClientState *nc =
> qemu_new_net_client(&net_vmnet_shared_info,
> > > + peer,
> > "vmnet-shared", name);
> > > + if (!validate_options(netdev, errp)) {
> > > + g_assert_not_reached();
> > > + return -1;
> > > + }
> > > + return vmnet_if_create(nc, build_if_desc(netdev), errp);
> > > }
> > > diff --git a/net/vmnet_int.h b/net/vmnet_int.h
> > > index aac4d5af64..8f3321ef3e 100644
> > > --- a/net/vmnet_int.h
> > > +++ b/net/vmnet_int.h
> > > @@ -15,11 +15,50 @@
> > > #include "clients.h"
> > >
> > > #include <vmnet/vmnet.h>
> > > +#include <dispatch/dispatch.h>
> > > +
> > > +/**
> > > + * From vmnet.framework documentation
> > > + *
> > > + * Each read/write call allows up to 200 packets to be
> > > + * read or written for a maximum of 256KB.
> > > + *
> > > + * Each packet written should be a complete
> > > + * ethernet frame.
> > > + *
> > > + * https://developer.apple.com/documentation/vmnet
> > <https://developer.apple.com/documentation/vmnet>
> > > + */
> > > +#define VMNET_PACKETS_LIMIT 200
> > >
> > > typedef struct VmnetCommonState {
> > > - NetClientState nc;
> > > + NetClientState nc;
> > > + interface_ref vmnet_if;
> > > +
> > > + uint64_t mtu;
> > > + uint64_t max_packet_size;
> > >
> > > + dispatch_queue_t if_queue;
> > > +
> > > + QEMUBH *send_bh;
> > > + bool send_enabled;
> > > +
> > > + struct vmpktdesc packets_buf[VMNET_PACKETS_LIMIT];
> > > + int packets_send_current_pos;
> > > + int packets_send_end_pos;
> > > +
> > > + struct iovec iov_buf[VMNET_PACKETS_LIMIT];
> > > } VmnetCommonState;
> > >
> > > +const char *vmnet_status_map_str(vmnet_return_t status);
> > > +
> > > +int vmnet_if_create(NetClientState *nc,
> > > + xpc_object_t if_desc,
> > > + Error **errp);
> > > +
> > > +ssize_t vmnet_receive_common(NetClientState *nc,
> > > + const uint8_t *buf,
> > > + size_t size);
> > > +
> > > +void vmnet_cleanup_common(NetClientState *nc);
> > >
> > > #endif /* VMNET_INT_H */
> >
>
>
[-- Attachment #2: Type: text/html, Size: 38176 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v16 3/7] net/vmnet: implement shared mode (vmnet-shared)
2022-03-14 23:02 ` Vladislav Yaroshchuk
@ 2022-03-14 23:06 ` Akihiko Odaki
2022-03-14 23:18 ` Vladislav Yaroshchuk
0 siblings, 1 reply; 16+ messages in thread
From: Akihiko Odaki @ 2022-03-14 23:06 UTC (permalink / raw)
To: Vladislav Yaroshchuk
Cc: Peter Maydell, Gerd Hoffmann, Alex Bennée, Jason Wang,
phillip.ennen, qemu Developers, Cameron Esfahani,
Markus Armbruster, Roman Bolshakov, Alexander Graf,
Phillip Tennen, Roman Bolshakov, Howard Spoelstra,
Alessio Dionisi, Christian Schoenebeck, Eric Blake,
Philippe Mathieu-Daudé
On 2022/03/15 8:02, Vladislav Yaroshchuk wrote:
>
>
> вт, 15 мар. 2022 г., 1:34 AM Akihiko Odaki <akihiko.odaki@gmail.com
> <mailto:akihiko.odaki@gmail.com>>:
>
> On 2022/03/15 6:50, Vladislav Yaroshchuk wrote:
> > Thank you, Akihiko
> >
> > On Mon, Mar 14, 2022 at 10:46 PM Akihiko Odaki
> <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
> > <mailto:akihiko.odaki@gmail.com
> <mailto:akihiko.odaki@gmail.com>>> wrote:
> >
> > On 2022/03/15 4:15, Vladislav Yaroshchuk wrote:
> > > vmnet.framework supports iov, but writing more than
> > > one iov into vmnet interface fails with
> > > 'VMNET_INVALID_ARGUMENT'. Collecting provided iovs into
> > > one and passing it to vmnet works fine. That's the
> > > reason why receive_iov() left unimplemented. But it still
> > > works with good enough performance having .receive()
> > > implemented only.
> > >
> > > Signed-off-by: Phillip Tennen <phillip@axleos.com
> <mailto:phillip@axleos.com>
> > <mailto:phillip@axleos.com <mailto:phillip@axleos.com>>>
> > > Signed-off-by: Vladislav Yaroshchuk
> > <Vladislav.Yaroshchuk@jetbrains.com
> <mailto:Vladislav.Yaroshchuk@jetbrains.com>
> > <mailto:Vladislav.Yaroshchuk@jetbrains.com
> <mailto:Vladislav.Yaroshchuk@jetbrains.com>>>
> > > ---
> > > net/vmnet-common.m | 298
> > +++++++++++++++++++++++++++++++++++++++++++++
> > > net/vmnet-shared.c | 95 ++++++++++++++-
> > > net/vmnet_int.h | 41 ++++++-
> > > 3 files changed, 429 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/net/vmnet-common.m b/net/vmnet-common.m
> > > index 56612c72ce..20a33d2591 100644
> > > --- a/net/vmnet-common.m
> > > +++ b/net/vmnet-common.m
> > > @@ -10,6 +10,8 @@
> > > */
> > >
> > > #include "qemu/osdep.h"
> > > +#include "qemu/main-loop.h"
> > > +#include "qemu/log.h"
> > > #include "qapi/qapi-types-net.h"
> > > #include "vmnet_int.h"
> > > #include "clients.h"
> > > @@ -17,4 +19,300 @@
> > > #include "qapi/error.h"
> > >
> > > #include <vmnet/vmnet.h>
> > > +#include <dispatch/dispatch.h>
> > >
> > > +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s);
> >
> > The names of vmnet_qemu_send_wrapper and vmnet_send_bh does
> not tell
> > them apart well. Since only vmnet_send_bh does reading, its
> name may
> > include "read" to clarify that. "wrapper" in
> vmnet_qemu_send_wrapper
> > may
> > be also misleading as it does more than just calling the
> underlying
> > QEMU
> > facility, but it also updates VmnetCommonState.
> >
> >
> > Ok, I'll think about how to name them better.
> >
> > > +
> > > +
> > > +static void vmnet_send_completed(NetClientState *nc,
> ssize_t len)
> > > +{
> > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc,
> nc);
> > > + /* Complete sending packets left in VmnetCommonState
> buffers */
> > > + s->send_enabled = vmnet_qemu_send_wrapper(s);
> >
> > It must qemu_bh_schedule(s->send_bh) after
> vmnet_qemu_send_wrapper.
> >
> >
> > Agree with you, thanks.
> >
> > Also, send_enabled flag can be removed as explained in:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html
> <https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html>
> >
> <https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html
> <https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html>>
> >
> >
> > Not sure about this. Values of packets_send_current_pos
> > and packets_send_end_pos may be equal, but QEMU may be
> > not ready to receive new packets - the explanation:
> > 1. We are sending packets to QEMU with qemu_send_packet_async:
> > packets_send_current_pos = 0
> > packets_send_end_pos = 5
> > 2. All five packets (0, 1, 2, 3, 4) have been successfully sent
> to QEMU,
> > but qemu_send_packet_async returned 0 "no more packets" after
> > the last invocation
> > 3. In spite of this, all five packets are sent and
> > packets_send_current_pos == packets_send_end_pos == 5
> > 4. It seems that "pointers are equal -> QEMU is ready", but actually
> > it is not.
> >
> > Also, hiding QEMU "ready"/"not ready" state behind pointers is a
> > bad choice I think. Having a concrete flag makes this more clear.
> > It provides understandability, not complexity (imho).
>
> packets_send_current_pos must not be incremented if
> qemu_send_packet_async returned 0. It must tell the position of the
> packet currently being sent.
>
>
>
> > must be incremented
> It cannot.
>
> If qemu_send_packet_async returns 0,
> it still consumes (!) (queues internally) the packet.
> So the packets_send_current_pos must be
> incremented
> to prevent sending same packet multiple times.
>
> The idea is simple:
> 1. We've sent the packet - increment
> 2. Packed is not send - not increment
>
> qemu_send_packet_async with 0 returned meets
> the 1st case, because it still queues the packet.
>
> While the increment action is not depends on the
> returned value, we cannot use position pointers as a
> criteria to send more packets or not. Another state
> storage (flag) is required.
>
>
> If You are not against, I'll cover this with proper
> documentation (comments) to simplify future support
> and make it more clear.
I forgot to note that packets_send_current_pos should be incremented in
vmnet_send_completed instead. It would make packets_send_current_pos
properly represent case 1.
>
>
> Best regards,
>
> Vladislav Yaroshchuk
>
>
>
> It would not hide the state, but it would rather make it clear that the
> condition vmnet_send_bh can execute. If you see the current
> implementation of vmnet_send_bh, you'll find the send_enabled flag, but
> it does not tell the exact condition it requires to be enabled. You
> have
> to then jump to all assignments for the flag to know it becomes true
> iff
> every packets in the buffer are sent. It is obvious if vmnet_send_bh
> directly states `if (packets_send_current_pos < packets_send_end_pos)`.
>
> Eliminating the flag would also remove the possiblity of forgetting to
> maintain the separate state.
>
>
> >
> > > send_enabled can be eliminated. When it is enabled,
> packets_send_pos
> > > and packets_batch_size must be equal. They must not be equal
> > > otherwise. packets_send_pos must represent the position
> of the
> > packet
> > > which is not sent yet, possibly in the process of sending.
> > > vmnet_send_completed must call qemu_send_wrapper before
> scheduling
> > > send_bh. bh_send should do nothing if s->packets_send_pos <
> > > s->packets_batch_size.
> >
> > > +}
> > > +
> > > +
> > > +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s) {
> > > + ssize_t size;
> > > +
> > > + /*
> > > + * Packets to send lay in [current_pos..end_pos)
> > > + * (including current_pos, excluding end_pos)
> > > + */
> > > + while (s->packets_send_current_pos <
> s->packets_send_end_pos) {
> > > + size = qemu_send_packet_async(&s->nc,
> > > +
> > s->iov_buf[s->packets_send_current_pos].iov_base,
> > > +
> > s->packets_buf[s->packets_send_current_pos].vm_pkt_size,
> > > + vmnet_send_completed);
> > > + ++s->packets_send_current_pos;
> > > + if (size == 0) {
> > > + /* QEMU is not ready - wait for completion
> callback
> > call */
> > > + return false;
> > > + }
> > > + }
> > > + return true;
> > > +}
> > > +
> > > +
> > > +static void vmnet_send_bh(void *opaque)
> > > +{
> > > + NetClientState *nc = (NetClientState *) opaque;
> > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc,
> nc);
> > > + struct vmpktdesc *packets = s->packets_buf;
> > > + vmnet_return_t status;
> > > + int i;
> > > +
> > > + /*
> > > + * Do nothing if QEMU is not ready - wait
> > > + * for completion callback invocation
> > > + */
> > > + if (!s->send_enabled) {
> > > + return;
> > > + }
> > > +
> > > + /* Read as many packets as present */
> > > + s->packets_send_current_pos = 0;
> > > + s->packets_send_end_pos = VMNET_PACKETS_LIMIT;
> > > + for (i = 0; i < s->packets_send_end_pos; ++i) {
> > > + packets[i].vm_pkt_size = s->max_packet_size;
> > > + packets[i].vm_pkt_iovcnt = 1;
> > > + packets[i].vm_flags = 0;
> > > + }
> > > +
> > > + status = vmnet_read(s->vmnet_if, packets,
> > &s->packets_send_end_pos);
> > > + if (status != VMNET_SUCCESS) {
> > > + error_printf("vmnet: read failed: %s\n",
> > > + vmnet_status_map_str(status));
> > > + s->packets_send_current_pos = 0;
> > > + s->packets_send_end_pos = 0;
> > > + return;
> > > + }
> > > +
> > > + /* Send packets to QEMU */
> > > + s->send_enabled = vmnet_qemu_send_wrapper(s);
> > > +}
> > > +
> > > +
> > > +static void vmnet_bufs_init(VmnetCommonState *s)
> > > +{
> > > + struct vmpktdesc *packets = s->packets_buf;
> > > + struct iovec *iov = s->iov_buf;
> > > + int i;
> > > +
> > > + for (i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
> > > + iov[i].iov_len = s->max_packet_size;
> > > + iov[i].iov_base = g_malloc0(iov[i].iov_len);
> > > + packets[i].vm_pkt_iov = iov + i;
> > > + }
> > > +}
> > > +
> > > +
> > > +const char *vmnet_status_map_str(vmnet_return_t status)
> > > +{
> > > + switch (status) {
> > > + case VMNET_SUCCESS:
> > > + return "success";
> > > + case VMNET_FAILURE:
> > > + return "general failure (possibly not enough
> privileges)";
> > > + case VMNET_MEM_FAILURE:
> > > + return "memory allocation failure";
> > > + case VMNET_INVALID_ARGUMENT:
> > > + return "invalid argument specified";
> > > + case VMNET_SETUP_INCOMPLETE:
> > > + return "interface setup is not complete";
> > > + case VMNET_INVALID_ACCESS:
> > > + return "invalid access, permission denied";
> > > + case VMNET_PACKET_TOO_BIG:
> > > + return "packet size is larger than MTU";
> > > + case VMNET_BUFFER_EXHAUSTED:
> > > + return "buffers exhausted in kernel";
> > > + case VMNET_TOO_MANY_PACKETS:
> > > + return "packet count exceeds limit";
> > > +#if defined(MAC_OS_VERSION_11_0) && \
> > > + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> > > + case VMNET_SHARING_SERVICE_BUSY:
> > > + return "conflict, sharing service is in use";
> > > +#endif
> > > + default:
> > > + return "unknown vmnet error";
> > > + }
> > > +}
> > > +
> > > +
> > > +int vmnet_if_create(NetClientState *nc,
> > > + xpc_object_t if_desc,
> > > + Error **errp)
> > > +{
> > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc,
> nc);
> > > + dispatch_semaphore_t if_created_sem =
> > dispatch_semaphore_create(0);
> > > + __block vmnet_return_t if_status;
> > > +
> > > + s->if_queue = dispatch_queue_create(
> > > + "org.qemu.vmnet.if_queue",
> > > + DISPATCH_QUEUE_SERIAL
> > > + );
> > > +
> > > + xpc_dictionary_set_bool(
> > > + if_desc,
> > > + vmnet_allocate_mac_address_key,
> > > + false
> > > + );
> > > +
> > > +#ifdef DEBUG
> > > + qemu_log("vmnet.start.interface_desc:\n");
> > > + xpc_dictionary_apply(if_desc,
> > > + ^bool(const char *k,
> xpc_object_t v) {
> > > + char *desc =
> xpc_copy_description(v);
> > > + qemu_log(" %s=%s\n", k, desc);
> > > + free(desc);
> > > + return true;
> > > + });
> > > +#endif /* DEBUG */
> > > +
> > > + s->vmnet_if = vmnet_start_interface(
> > > + if_desc,
> > > + s->if_queue,
> > > + ^(vmnet_return_t status, xpc_object_t
> interface_param) {
> > > + if_status = status;
> > > + if (status != VMNET_SUCCESS ||
> !interface_param) {
> > > + dispatch_semaphore_signal(if_created_sem);
> > > + return;
> > > + }
> > > +
> > > +#ifdef DEBUG
> > > + qemu_log("vmnet.start.interface_param:\n");
> > > + xpc_dictionary_apply(interface_param,
> > > + ^bool(const char *k,
> > xpc_object_t v) {
> > > + char *desc =
> > xpc_copy_description(v);
> > > + qemu_log("
> %s=%s\n", k, desc);
> > > + free(desc);
> > > + return true;
> > > + });
> > > +#endif /* DEBUG */
> > > +
> > > + s->mtu = xpc_dictionary_get_uint64(
> > > + interface_param,
> > > + vmnet_mtu_key);
> > > + s->max_packet_size = xpc_dictionary_get_uint64(
> > > + interface_param,
> > > + vmnet_max_packet_size_key);
> > > +
> > > + dispatch_semaphore_signal(if_created_sem);
> > > + });
> > > +
> > > + if (s->vmnet_if == NULL) {
> > > + dispatch_release(s->if_queue);
> > > + dispatch_release(if_created_sem);
> > > + error_setg(errp,
> > > + "unable to create interface with requested
> > params");
> > > + return -1;
> > > + }
> > > +
> > > + dispatch_semaphore_wait(if_created_sem,
> DISPATCH_TIME_FOREVER);
> > > + dispatch_release(if_created_sem);
> > > +
> > > + if (if_status != VMNET_SUCCESS) {
> > > + dispatch_release(s->if_queue);
> > > + error_setg(errp,
> > > + "cannot create vmnet interface: %s",
> > > + vmnet_status_map_str(if_status));
> > > + return -1;
> > > + }
> > > +
> > > + s->send_bh = aio_bh_new(qemu_get_aio_context(),
> > vmnet_send_bh, nc);
> > > + s->send_enabled = true;
> > > + vmnet_bufs_init(s);
> > > +
> > > + vmnet_interface_set_event_callback(
> > > + s->vmnet_if,
> > > + VMNET_INTERFACE_PACKETS_AVAILABLE,
> > > + s->if_queue,
> > > + ^(interface_event_t event_id, xpc_object_t event) {
> > > + assert(event_id ==
> VMNET_INTERFACE_PACKETS_AVAILABLE);
> > > + /*
> > > + * This function is being called from a non qemu
> > thread, so
> > > + * we only schedule a BH, and do the rest of
> the io
> > completion
> > > + * handling from vmnet_send_bh() which runs in a
> > qemu context.
> > > + */
> > > + qemu_bh_schedule(s->send_bh);
> > > + });
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +
> > > +ssize_t vmnet_receive_common(NetClientState *nc,
> > > + const uint8_t *buf,
> > > + size_t size)
> > > +{
> > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc,
> nc);
> > > + struct vmpktdesc packet;
> > > + struct iovec iov;
> > > + int pkt_cnt;
> > > + vmnet_return_t if_status;
> > > +
> > > + if (size > s->max_packet_size) {
> > > + warn_report("vmnet: packet is too big, %zu > %"
> PRIu64,
> > > + packet.vm_pkt_size,
> > > + s->max_packet_size);
> > > + return -1;
> > > + }
> > > +
> > > + iov.iov_base = (char *) buf;
> > > + iov.iov_len = size;
> > > +
> > > + packet.vm_pkt_iovcnt = 1;
> > > + packet.vm_flags = 0;
> > > + packet.vm_pkt_size = size;
> > > + packet.vm_pkt_iov = &iov;
> > > + pkt_cnt = 1;
> > > +
> > > + if_status = vmnet_write(s->vmnet_if, &packet, &pkt_cnt);
> > > + if (if_status != VMNET_SUCCESS) {
> > > + error_report("vmnet: write error: %s\n",
> > > + vmnet_status_map_str(if_status));
> > > + return -1;
> > > + }
> > > +
> > > + if (if_status == VMNET_SUCCESS && pkt_cnt) {
> >
> > `if_status == VMNET_SUCCESS` is redundant.
> >
> >
> > Missed this, thanks.
> >
> > Regards,
> > Akihiko Odaki
> >
> > > + return size;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +
> > > +void vmnet_cleanup_common(NetClientState *nc)
> > > +{
> > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc,
> nc);
> > > + dispatch_semaphore_t if_stopped_sem;
> > > +
> > > + if (s->vmnet_if == NULL) {
> > > + return;
> > > + }
> > > +
> > > + vmnet_interface_set_event_callback(
> > > + s->vmnet_if,
> > > + VMNET_INTERFACE_PACKETS_AVAILABLE,
> > > + NULL,
> > > + NULL);
> >
> > I don't think this vmnet_interface_set_event_callback call is
> necessary.
> >
> >
> > I kept in mind that vmnet processing lives in a separate thread
> > and while cleanup it may continue receiving packets. While the
> > queue is not empty, vmnet_stop_interface hangs. Unregistering
> > callback ensures that this queue will be emptied asap.
> >
> > It will work without vmnet_interface_set_event_callback here,
> > but I think it's better to be respectful to vmnet and clean
> everything
> > we can :)
>
> You may put qemu_purge_queued_packets after vmnet_stop_interface if you
> think about the case it keeps receving packets while cleaning up since
> it is the only thing it does before calling vmnet_stop_interface.
> vmnet_stop_interface would then stop things in the proper order. It may
> decide to stop event callbacks first. Otherwise, it may decide to stop
> some internal heavy functionality first. It is up to vmnet.framework.
>
> Regards,
> Akihiko Odaki
>
> > Thank you!
> >
> > Best Regards,
> >
> > Vladislav
> >
> > > +
> > > + qemu_purge_queued_packets(nc);
> > > +
> > > + if_stopped_sem = dispatch_semaphore_create(0);
> > > + vmnet_stop_interface(
> > > + s->vmnet_if,
> > > + s->if_queue,
> > > + ^(vmnet_return_t status) {
> > > + assert(status == VMNET_SUCCESS);
> > > + dispatch_semaphore_signal(if_stopped_sem);
> > > + });
> > > + dispatch_semaphore_wait(if_stopped_sem,
> DISPATCH_TIME_FOREVER);
> > > +
> > > + qemu_bh_delete(s->send_bh);
> > > + dispatch_release(if_stopped_sem);
> > > + dispatch_release(s->if_queue);
> > > +
> > > + for (int i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
> > > + g_free(s->iov_buf[i].iov_base);
> > > + }
> > > +}
> > > diff --git a/net/vmnet-shared.c b/net/vmnet-shared.c
> > > index f07afaaf21..2f4eb1db2d 100644
> > > --- a/net/vmnet-shared.c
> > > +++ b/net/vmnet-shared.c
> > > @@ -10,16 +10,103 @@
> > >
> > > #include "qemu/osdep.h"
> > > #include "qapi/qapi-types-net.h"
> > > +#include "qapi/error.h"
> > > #include "vmnet_int.h"
> > > #include "clients.h"
> > > -#include "qemu/error-report.h"
> > > -#include "qapi/error.h"
> > >
> > > #include <vmnet/vmnet.h>
> > >
> > > +typedef struct VmnetSharedState {
> > > + VmnetCommonState cs;
> > > +} VmnetSharedState;
> > > +
> > > +
> > > +static bool validate_options(const Netdev *netdev, Error
> **errp)
> > > +{
> > > + const NetdevVmnetSharedOptions *options =
> > &(netdev->u.vmnet_shared);
> > > +
> > > +#if !defined(MAC_OS_VERSION_11_0) || \
> > > + MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_VERSION_11_0
> > > + if (options->has_isolated) {
> > > + error_setg(errp,
> > > + "vmnet-shared.isolated feature is "
> > > + "unavailable: outdated vmnet.framework
> API");
> > > + return false;
> > > + }
> > > +#endif
> > > +
> > > + if ((options->has_start_address ||
> > > + options->has_end_address ||
> > > + options->has_subnet_mask) &&
> > > + !(options->has_start_address &&
> > > + options->has_end_address &&
> > > + options->has_subnet_mask)) {
> > > + error_setg(errp,
> > > + "'start-address', 'end-address',
> 'subnet-mask' "
> > > + "should be provided together"
> > > + );
> > > + return false;
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +static xpc_object_t build_if_desc(const Netdev *netdev)
> > > +{
> > > + const NetdevVmnetSharedOptions *options =
> > &(netdev->u.vmnet_shared);
> > > + xpc_object_t if_desc = xpc_dictionary_create(NULL,
> NULL, 0);
> > > +
> > > + xpc_dictionary_set_uint64(
> > > + if_desc,
> > > + vmnet_operation_mode_key,
> > > + VMNET_SHARED_MODE
> > > + );
> > > +
> > > + if (options->has_nat66_prefix) {
> > > + xpc_dictionary_set_string(if_desc,
> > > + vmnet_nat66_prefix_key,
> > > + options->nat66_prefix);
> > > + }
> > > +
> > > + if (options->has_start_address) {
> > > + xpc_dictionary_set_string(if_desc,
> > > + vmnet_start_address_key,
> > > + options->start_address);
> > > + xpc_dictionary_set_string(if_desc,
> > > + vmnet_end_address_key,
> > > + options->end_address);
> > > + xpc_dictionary_set_string(if_desc,
> > > + vmnet_subnet_mask_key,
> > > + options->subnet_mask);
> > > + }
> > > +
> > > +#if defined(MAC_OS_VERSION_11_0) && \
> > > + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> > > + xpc_dictionary_set_bool(
> > > + if_desc,
> > > + vmnet_enable_isolation_key,
> > > + options->isolated
> > > + );
> > > +#endif
> > > +
> > > + return if_desc;
> > > +}
> > > +
> > > +static NetClientInfo net_vmnet_shared_info = {
> > > + .type = NET_CLIENT_DRIVER_VMNET_SHARED,
> > > + .size = sizeof(VmnetSharedState),
> > > + .receive = vmnet_receive_common,
> > > + .cleanup = vmnet_cleanup_common,
> > > +};
> > > +
> > > int net_init_vmnet_shared(const Netdev *netdev, const
> char *name,
> > > NetClientState *peer, Error
> **errp)
> > > {
> > > - error_setg(errp, "vmnet-shared is not implemented yet");
> > > - return -1;
> > > + NetClientState *nc =
> qemu_new_net_client(&net_vmnet_shared_info,
> > > + peer,
> > "vmnet-shared", name);
> > > + if (!validate_options(netdev, errp)) {
> > > + g_assert_not_reached();
> > > + return -1;
> > > + }
> > > + return vmnet_if_create(nc, build_if_desc(netdev), errp);
> > > }
> > > diff --git a/net/vmnet_int.h b/net/vmnet_int.h
> > > index aac4d5af64..8f3321ef3e 100644
> > > --- a/net/vmnet_int.h
> > > +++ b/net/vmnet_int.h
> > > @@ -15,11 +15,50 @@
> > > #include "clients.h"
> > >
> > > #include <vmnet/vmnet.h>
> > > +#include <dispatch/dispatch.h>
> > > +
> > > +/**
> > > + * From vmnet.framework documentation
> > > + *
> > > + * Each read/write call allows up to 200 packets to be
> > > + * read or written for a maximum of 256KB.
> > > + *
> > > + * Each packet written should be a complete
> > > + * ethernet frame.
> > > + *
> > > + * https://developer.apple.com/documentation/vmnet
> <https://developer.apple.com/documentation/vmnet>
> > <https://developer.apple.com/documentation/vmnet
> <https://developer.apple.com/documentation/vmnet>>
> > > + */
> > > +#define VMNET_PACKETS_LIMIT 200
> > >
> > > typedef struct VmnetCommonState {
> > > - NetClientState nc;
> > > + NetClientState nc;
> > > + interface_ref vmnet_if;
> > > +
> > > + uint64_t mtu;
> > > + uint64_t max_packet_size;
> > >
> > > + dispatch_queue_t if_queue;
> > > +
> > > + QEMUBH *send_bh;
> > > + bool send_enabled;
> > > +
> > > + struct vmpktdesc packets_buf[VMNET_PACKETS_LIMIT];
> > > + int packets_send_current_pos;
> > > + int packets_send_end_pos;
> > > +
> > > + struct iovec iov_buf[VMNET_PACKETS_LIMIT];
> > > } VmnetCommonState;
> > >
> > > +const char *vmnet_status_map_str(vmnet_return_t status);
> > > +
> > > +int vmnet_if_create(NetClientState *nc,
> > > + xpc_object_t if_desc,
> > > + Error **errp);
> > > +
> > > +ssize_t vmnet_receive_common(NetClientState *nc,
> > > + const uint8_t *buf,
> > > + size_t size);
> > > +
> > > +void vmnet_cleanup_common(NetClientState *nc);
> > >
> > > #endif /* VMNET_INT_H */
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v16 3/7] net/vmnet: implement shared mode (vmnet-shared)
2022-03-14 23:06 ` Akihiko Odaki
@ 2022-03-14 23:18 ` Vladislav Yaroshchuk
0 siblings, 0 replies; 16+ messages in thread
From: Vladislav Yaroshchuk @ 2022-03-14 23:18 UTC (permalink / raw)
To: Akihiko Odaki
Cc: qemu Developers, Jason Wang, Roman Bolshakov, Eric Blake,
phillip.ennen, Phillip Tennen, Markus Armbruster,
Howard Spoelstra, Alessio Dionisi, Roman Bolshakov,
Peter Maydell, Cameron Esfahani, Philippe Mathieu-Daudé,
Alexander Graf, Gerd Hoffmann, Alex Bennée,
Christian Schoenebeck
[-- Attachment #1: Type: text/plain, Size: 32621 bytes --]
вт, 15 мар. 2022 г., 2:06 AM Akihiko Odaki <akihiko.odaki@gmail.com>:
> On 2022/03/15 8:02, Vladislav Yaroshchuk wrote:
> >
> >
> > вт, 15 мар. 2022 г., 1:34 AM Akihiko Odaki <akihiko.odaki@gmail.com
> > <mailto:akihiko.odaki@gmail.com>>:
> >
> > On 2022/03/15 6:50, Vladislav Yaroshchuk wrote:
> > > Thank you, Akihiko
> > >
> > > On Mon, Mar 14, 2022 at 10:46 PM Akihiko Odaki
> > <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
> > > <mailto:akihiko.odaki@gmail.com
> > <mailto:akihiko.odaki@gmail.com>>> wrote:
> > >
> > > On 2022/03/15 4:15, Vladislav Yaroshchuk wrote:
> > > > vmnet.framework supports iov, but writing more than
> > > > one iov into vmnet interface fails with
> > > > 'VMNET_INVALID_ARGUMENT'. Collecting provided iovs into
> > > > one and passing it to vmnet works fine. That's the
> > > > reason why receive_iov() left unimplemented. But it still
> > > > works with good enough performance having .receive()
> > > > implemented only.
> > > >
> > > > Signed-off-by: Phillip Tennen <phillip@axleos.com
> > <mailto:phillip@axleos.com>
> > > <mailto:phillip@axleos.com <mailto:phillip@axleos.com>>>
> > > > Signed-off-by: Vladislav Yaroshchuk
> > > <Vladislav.Yaroshchuk@jetbrains.com
> > <mailto:Vladislav.Yaroshchuk@jetbrains.com>
> > > <mailto:Vladislav.Yaroshchuk@jetbrains.com
> > <mailto:Vladislav.Yaroshchuk@jetbrains.com>>>
> > > > ---
> > > > net/vmnet-common.m | 298
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > net/vmnet-shared.c | 95 ++++++++++++++-
> > > > net/vmnet_int.h | 41 ++++++-
> > > > 3 files changed, 429 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/net/vmnet-common.m b/net/vmnet-common.m
> > > > index 56612c72ce..20a33d2591 100644
> > > > --- a/net/vmnet-common.m
> > > > +++ b/net/vmnet-common.m
> > > > @@ -10,6 +10,8 @@
> > > > */
> > > >
> > > > #include "qemu/osdep.h"
> > > > +#include "qemu/main-loop.h"
> > > > +#include "qemu/log.h"
> > > > #include "qapi/qapi-types-net.h"
> > > > #include "vmnet_int.h"
> > > > #include "clients.h"
> > > > @@ -17,4 +19,300 @@
> > > > #include "qapi/error.h"
> > > >
> > > > #include <vmnet/vmnet.h>
> > > > +#include <dispatch/dispatch.h>
> > > >
> > > > +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s);
> > >
> > > The names of vmnet_qemu_send_wrapper and vmnet_send_bh does
> > not tell
> > > them apart well. Since only vmnet_send_bh does reading, its
> > name may
> > > include "read" to clarify that. "wrapper" in
> > vmnet_qemu_send_wrapper
> > > may
> > > be also misleading as it does more than just calling the
> > underlying
> > > QEMU
> > > facility, but it also updates VmnetCommonState.
> > >
> > >
> > > Ok, I'll think about how to name them better.
> > >
> > > > +
> > > > +
> > > > +static void vmnet_send_completed(NetClientState *nc,
> > ssize_t len)
> > > > +{
> > > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc,
> > nc);
> > > > + /* Complete sending packets left in VmnetCommonState
> > buffers */
> > > > + s->send_enabled = vmnet_qemu_send_wrapper(s);
> > >
> > > It must qemu_bh_schedule(s->send_bh) after
> > vmnet_qemu_send_wrapper.
> > >
> > >
> > > Agree with you, thanks.
> > >
> > > Also, send_enabled flag can be removed as explained in:
> > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html
> > <https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html>
> > >
> > <https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html
> > <https://www.mail-archive.com/qemu-devel@nongnu.org/msg873923.html>>
> > >
> > >
> > > Not sure about this. Values of packets_send_current_pos
> > > and packets_send_end_pos may be equal, but QEMU may be
> > > not ready to receive new packets - the explanation:
> > > 1. We are sending packets to QEMU with qemu_send_packet_async:
> > > packets_send_current_pos = 0
> > > packets_send_end_pos = 5
> > > 2. All five packets (0, 1, 2, 3, 4) have been successfully sent
> > to QEMU,
> > > but qemu_send_packet_async returned 0 "no more packets" after
> > > the last invocation
> > > 3. In spite of this, all five packets are sent and
> > > packets_send_current_pos == packets_send_end_pos == 5
> > > 4. It seems that "pointers are equal -> QEMU is ready", but
> actually
> > > it is not.
> > >
> > > Also, hiding QEMU "ready"/"not ready" state behind pointers is a
> > > bad choice I think. Having a concrete flag makes this more clear.
> > > It provides understandability, not complexity (imho).
> >
> > packets_send_current_pos must not be incremented if
> > qemu_send_packet_async returned 0. It must tell the position of the
> > packet currently being sent.
> >
> >
> >
> > > must be incremented
> > It cannot.
> >
> > If qemu_send_packet_async returns 0,
> > it still consumes (!) (queues internally) the packet.
> > So the packets_send_current_pos must be
> > incremented
> > to prevent sending same packet multiple times.
> >
> > The idea is simple:
> > 1. We've sent the packet - increment
> > 2. Packed is not send - not increment
> >
> > qemu_send_packet_async with 0 returned meets
> > the 1st case, because it still queues the packet.
> >
> > While the increment action is not depends on the
> > returned value, we cannot use position pointers as a
> > criteria to send more packets or not. Another state
> > storage (flag) is required.
> >
> >
> > If You are not against, I'll cover this with proper
> > documentation (comments) to simplify future support
> > and make it more clear.
>
> I forgot to note that packets_send_current_pos should be incremented in
> vmnet_send_completed instead. It would make packets_send_current_pos
> properly represent case 1.
>
>
>
The truth begins from discussion :)
I'll try to refactor and document this part a bit,
to make it more clear since it is not. Thank you!
Best regards,
Vladislav Yaroshchuk
>
> >
> > Best regards,
> >
> > Vladislav Yaroshchuk
> >
> >
> >
> > It would not hide the state, but it would rather make it clear that
> the
> > condition vmnet_send_bh can execute. If you see the current
> > implementation of vmnet_send_bh, you'll find the send_enabled flag,
> but
> > it does not tell the exact condition it requires to be enabled. You
> > have
> > to then jump to all assignments for the flag to know it becomes true
> > iff
> > every packets in the buffer are sent. It is obvious if vmnet_send_bh
> > directly states `if (packets_send_current_pos <
> packets_send_end_pos)`.
> >
> > Eliminating the flag would also remove the possiblity of forgetting
> to
> > maintain the separate state.
> >
> >
> > >
> > > > send_enabled can be eliminated. When it is enabled,
> > packets_send_pos
> > > > and packets_batch_size must be equal. They must not be
> equal
> > > > otherwise. packets_send_pos must represent the position
> > of the
> > > packet
> > > > which is not sent yet, possibly in the process of sending.
> > > > vmnet_send_completed must call qemu_send_wrapper before
> > scheduling
> > > > send_bh. bh_send should do nothing if s->packets_send_pos
> <
> > > > s->packets_batch_size.
> > >
> > > > +}
> > > > +
> > > > +
> > > > +static bool vmnet_qemu_send_wrapper(VmnetCommonState *s) {
> > > > + ssize_t size;
> > > > +
> > > > + /*
> > > > + * Packets to send lay in [current_pos..end_pos)
> > > > + * (including current_pos, excluding end_pos)
> > > > + */
> > > > + while (s->packets_send_current_pos <
> > s->packets_send_end_pos) {
> > > > + size = qemu_send_packet_async(&s->nc,
> > > > +
> > > s->iov_buf[s->packets_send_current_pos].iov_base,
> > > > +
> > > s->packets_buf[s->packets_send_current_pos].vm_pkt_size,
> > > > +
> vmnet_send_completed);
> > > > + ++s->packets_send_current_pos;
> > > > + if (size == 0) {
> > > > + /* QEMU is not ready - wait for completion
> > callback
> > > call */
> > > > + return false;
> > > > + }
> > > > + }
> > > > + return true;
> > > > +}
> > > > +
> > > > +
> > > > +static void vmnet_send_bh(void *opaque)
> > > > +{
> > > > + NetClientState *nc = (NetClientState *) opaque;
> > > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc,
> > nc);
> > > > + struct vmpktdesc *packets = s->packets_buf;
> > > > + vmnet_return_t status;
> > > > + int i;
> > > > +
> > > > + /*
> > > > + * Do nothing if QEMU is not ready - wait
> > > > + * for completion callback invocation
> > > > + */
> > > > + if (!s->send_enabled) {
> > > > + return;
> > > > + }
> > > > +
> > > > + /* Read as many packets as present */
> > > > + s->packets_send_current_pos = 0;
> > > > + s->packets_send_end_pos = VMNET_PACKETS_LIMIT;
> > > > + for (i = 0; i < s->packets_send_end_pos; ++i) {
> > > > + packets[i].vm_pkt_size = s->max_packet_size;
> > > > + packets[i].vm_pkt_iovcnt = 1;
> > > > + packets[i].vm_flags = 0;
> > > > + }
> > > > +
> > > > + status = vmnet_read(s->vmnet_if, packets,
> > > &s->packets_send_end_pos);
> > > > + if (status != VMNET_SUCCESS) {
> > > > + error_printf("vmnet: read failed: %s\n",
> > > > + vmnet_status_map_str(status));
> > > > + s->packets_send_current_pos = 0;
> > > > + s->packets_send_end_pos = 0;
> > > > + return;
> > > > + }
> > > > +
> > > > + /* Send packets to QEMU */
> > > > + s->send_enabled = vmnet_qemu_send_wrapper(s);
> > > > +}
> > > > +
> > > > +
> > > > +static void vmnet_bufs_init(VmnetCommonState *s)
> > > > +{
> > > > + struct vmpktdesc *packets = s->packets_buf;
> > > > + struct iovec *iov = s->iov_buf;
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
> > > > + iov[i].iov_len = s->max_packet_size;
> > > > + iov[i].iov_base = g_malloc0(iov[i].iov_len);
> > > > + packets[i].vm_pkt_iov = iov + i;
> > > > + }
> > > > +}
> > > > +
> > > > +
> > > > +const char *vmnet_status_map_str(vmnet_return_t status)
> > > > +{
> > > > + switch (status) {
> > > > + case VMNET_SUCCESS:
> > > > + return "success";
> > > > + case VMNET_FAILURE:
> > > > + return "general failure (possibly not enough
> > privileges)";
> > > > + case VMNET_MEM_FAILURE:
> > > > + return "memory allocation failure";
> > > > + case VMNET_INVALID_ARGUMENT:
> > > > + return "invalid argument specified";
> > > > + case VMNET_SETUP_INCOMPLETE:
> > > > + return "interface setup is not complete";
> > > > + case VMNET_INVALID_ACCESS:
> > > > + return "invalid access, permission denied";
> > > > + case VMNET_PACKET_TOO_BIG:
> > > > + return "packet size is larger than MTU";
> > > > + case VMNET_BUFFER_EXHAUSTED:
> > > > + return "buffers exhausted in kernel";
> > > > + case VMNET_TOO_MANY_PACKETS:
> > > > + return "packet count exceeds limit";
> > > > +#if defined(MAC_OS_VERSION_11_0) && \
> > > > + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> > > > + case VMNET_SHARING_SERVICE_BUSY:
> > > > + return "conflict, sharing service is in use";
> > > > +#endif
> > > > + default:
> > > > + return "unknown vmnet error";
> > > > + }
> > > > +}
> > > > +
> > > > +
> > > > +int vmnet_if_create(NetClientState *nc,
> > > > + xpc_object_t if_desc,
> > > > + Error **errp)
> > > > +{
> > > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc,
> > nc);
> > > > + dispatch_semaphore_t if_created_sem =
> > > dispatch_semaphore_create(0);
> > > > + __block vmnet_return_t if_status;
> > > > +
> > > > + s->if_queue = dispatch_queue_create(
> > > > + "org.qemu.vmnet.if_queue",
> > > > + DISPATCH_QUEUE_SERIAL
> > > > + );
> > > > +
> > > > + xpc_dictionary_set_bool(
> > > > + if_desc,
> > > > + vmnet_allocate_mac_address_key,
> > > > + false
> > > > + );
> > > > +
> > > > +#ifdef DEBUG
> > > > + qemu_log("vmnet.start.interface_desc:\n");
> > > > + xpc_dictionary_apply(if_desc,
> > > > + ^bool(const char *k,
> > xpc_object_t v) {
> > > > + char *desc =
> > xpc_copy_description(v);
> > > > + qemu_log(" %s=%s\n", k,
> desc);
> > > > + free(desc);
> > > > + return true;
> > > > + });
> > > > +#endif /* DEBUG */
> > > > +
> > > > + s->vmnet_if = vmnet_start_interface(
> > > > + if_desc,
> > > > + s->if_queue,
> > > > + ^(vmnet_return_t status, xpc_object_t
> > interface_param) {
> > > > + if_status = status;
> > > > + if (status != VMNET_SUCCESS ||
> > !interface_param) {
> > > > + dispatch_semaphore_signal(if_created_sem);
> > > > + return;
> > > > + }
> > > > +
> > > > +#ifdef DEBUG
> > > > + qemu_log("vmnet.start.interface_param:\n");
> > > > + xpc_dictionary_apply(interface_param,
> > > > + ^bool(const char *k,
> > > xpc_object_t v) {
> > > > + char *desc =
> > > xpc_copy_description(v);
> > > > + qemu_log("
> > %s=%s\n", k, desc);
> > > > + free(desc);
> > > > + return true;
> > > > + });
> > > > +#endif /* DEBUG */
> > > > +
> > > > + s->mtu = xpc_dictionary_get_uint64(
> > > > + interface_param,
> > > > + vmnet_mtu_key);
> > > > + s->max_packet_size =
> xpc_dictionary_get_uint64(
> > > > + interface_param,
> > > > + vmnet_max_packet_size_key);
> > > > +
> > > > + dispatch_semaphore_signal(if_created_sem);
> > > > + });
> > > > +
> > > > + if (s->vmnet_if == NULL) {
> > > > + dispatch_release(s->if_queue);
> > > > + dispatch_release(if_created_sem);
> > > > + error_setg(errp,
> > > > + "unable to create interface with
> requested
> > > params");
> > > > + return -1;
> > > > + }
> > > > +
> > > > + dispatch_semaphore_wait(if_created_sem,
> > DISPATCH_TIME_FOREVER);
> > > > + dispatch_release(if_created_sem);
> > > > +
> > > > + if (if_status != VMNET_SUCCESS) {
> > > > + dispatch_release(s->if_queue);
> > > > + error_setg(errp,
> > > > + "cannot create vmnet interface: %s",
> > > > + vmnet_status_map_str(if_status));
> > > > + return -1;
> > > > + }
> > > > +
> > > > + s->send_bh = aio_bh_new(qemu_get_aio_context(),
> > > vmnet_send_bh, nc);
> > > > + s->send_enabled = true;
> > > > + vmnet_bufs_init(s);
> > > > +
> > > > + vmnet_interface_set_event_callback(
> > > > + s->vmnet_if,
> > > > + VMNET_INTERFACE_PACKETS_AVAILABLE,
> > > > + s->if_queue,
> > > > + ^(interface_event_t event_id, xpc_object_t event)
> {
> > > > + assert(event_id ==
> > VMNET_INTERFACE_PACKETS_AVAILABLE);
> > > > + /*
> > > > + * This function is being called from a non
> qemu
> > > thread, so
> > > > + * we only schedule a BH, and do the rest of
> > the io
> > > completion
> > > > + * handling from vmnet_send_bh() which runs
> in a
> > > qemu context.
> > > > + */
> > > > + qemu_bh_schedule(s->send_bh);
> > > > + });
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +
> > > > +ssize_t vmnet_receive_common(NetClientState *nc,
> > > > + const uint8_t *buf,
> > > > + size_t size)
> > > > +{
> > > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc,
> > nc);
> > > > + struct vmpktdesc packet;
> > > > + struct iovec iov;
> > > > + int pkt_cnt;
> > > > + vmnet_return_t if_status;
> > > > +
> > > > + if (size > s->max_packet_size) {
> > > > + warn_report("vmnet: packet is too big, %zu > %"
> > PRIu64,
> > > > + packet.vm_pkt_size,
> > > > + s->max_packet_size);
> > > > + return -1;
> > > > + }
> > > > +
> > > > + iov.iov_base = (char *) buf;
> > > > + iov.iov_len = size;
> > > > +
> > > > + packet.vm_pkt_iovcnt = 1;
> > > > + packet.vm_flags = 0;
> > > > + packet.vm_pkt_size = size;
> > > > + packet.vm_pkt_iov = &iov;
> > > > + pkt_cnt = 1;
> > > > +
> > > > + if_status = vmnet_write(s->vmnet_if, &packet,
> &pkt_cnt);
> > > > + if (if_status != VMNET_SUCCESS) {
> > > > + error_report("vmnet: write error: %s\n",
> > > > + vmnet_status_map_str(if_status));
> > > > + return -1;
> > > > + }
> > > > +
> > > > + if (if_status == VMNET_SUCCESS && pkt_cnt) {
> > >
> > > `if_status == VMNET_SUCCESS` is redundant.
> > >
> > >
> > > Missed this, thanks.
> > >
> > > Regards,
> > > Akihiko Odaki
> > >
> > > > + return size;
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > > > +
> > > > +void vmnet_cleanup_common(NetClientState *nc)
> > > > +{
> > > > + VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc,
> > nc);
> > > > + dispatch_semaphore_t if_stopped_sem;
> > > > +
> > > > + if (s->vmnet_if == NULL) {
> > > > + return;
> > > > + }
> > > > +
> > > > + vmnet_interface_set_event_callback(
> > > > + s->vmnet_if,
> > > > + VMNET_INTERFACE_PACKETS_AVAILABLE,
> > > > + NULL,
> > > > + NULL);
> > >
> > > I don't think this vmnet_interface_set_event_callback call is
> > necessary.
> > >
> > >
> > > I kept in mind that vmnet processing lives in a separate thread
> > > and while cleanup it may continue receiving packets. While the
> > > queue is not empty, vmnet_stop_interface hangs. Unregistering
> > > callback ensures that this queue will be emptied asap.
> > >
> > > It will work without vmnet_interface_set_event_callback here,
> > > but I think it's better to be respectful to vmnet and clean
> > everything
> > > we can :)
> >
> > You may put qemu_purge_queued_packets after vmnet_stop_interface if
> you
> > think about the case it keeps receving packets while cleaning up
> since
> > it is the only thing it does before calling vmnet_stop_interface.
> > vmnet_stop_interface would then stop things in the proper order. It
> may
> > decide to stop event callbacks first. Otherwise, it may decide to
> stop
> > some internal heavy functionality first. It is up to vmnet.framework.
> >
> > Regards,
> > Akihiko Odaki
> >
> > > Thank you!
> > >
> > > Best Regards,
> > >
> > > Vladislav
> > >
> > > > +
> > > > + qemu_purge_queued_packets(nc);
> > > > +
> > > > + if_stopped_sem = dispatch_semaphore_create(0);
> > > > + vmnet_stop_interface(
> > > > + s->vmnet_if,
> > > > + s->if_queue,
> > > > + ^(vmnet_return_t status) {
> > > > + assert(status == VMNET_SUCCESS);
> > > > + dispatch_semaphore_signal(if_stopped_sem);
> > > > + });
> > > > + dispatch_semaphore_wait(if_stopped_sem,
> > DISPATCH_TIME_FOREVER);
> > > > +
> > > > + qemu_bh_delete(s->send_bh);
> > > > + dispatch_release(if_stopped_sem);
> > > > + dispatch_release(s->if_queue);
> > > > +
> > > > + for (int i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
> > > > + g_free(s->iov_buf[i].iov_base);
> > > > + }
> > > > +}
> > > > diff --git a/net/vmnet-shared.c b/net/vmnet-shared.c
> > > > index f07afaaf21..2f4eb1db2d 100644
> > > > --- a/net/vmnet-shared.c
> > > > +++ b/net/vmnet-shared.c
> > > > @@ -10,16 +10,103 @@
> > > >
> > > > #include "qemu/osdep.h"
> > > > #include "qapi/qapi-types-net.h"
> > > > +#include "qapi/error.h"
> > > > #include "vmnet_int.h"
> > > > #include "clients.h"
> > > > -#include "qemu/error-report.h"
> > > > -#include "qapi/error.h"
> > > >
> > > > #include <vmnet/vmnet.h>
> > > >
> > > > +typedef struct VmnetSharedState {
> > > > + VmnetCommonState cs;
> > > > +} VmnetSharedState;
> > > > +
> > > > +
> > > > +static bool validate_options(const Netdev *netdev, Error
> > **errp)
> > > > +{
> > > > + const NetdevVmnetSharedOptions *options =
> > > &(netdev->u.vmnet_shared);
> > > > +
> > > > +#if !defined(MAC_OS_VERSION_11_0) || \
> > > > + MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_VERSION_11_0
> > > > + if (options->has_isolated) {
> > > > + error_setg(errp,
> > > > + "vmnet-shared.isolated feature is "
> > > > + "unavailable: outdated vmnet.framework
> > API");
> > > > + return false;
> > > > + }
> > > > +#endif
> > > > +
> > > > + if ((options->has_start_address ||
> > > > + options->has_end_address ||
> > > > + options->has_subnet_mask) &&
> > > > + !(options->has_start_address &&
> > > > + options->has_end_address &&
> > > > + options->has_subnet_mask)) {
> > > > + error_setg(errp,
> > > > + "'start-address', 'end-address',
> > 'subnet-mask' "
> > > > + "should be provided together"
> > > > + );
> > > > + return false;
> > > > + }
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > +static xpc_object_t build_if_desc(const Netdev *netdev)
> > > > +{
> > > > + const NetdevVmnetSharedOptions *options =
> > > &(netdev->u.vmnet_shared);
> > > > + xpc_object_t if_desc = xpc_dictionary_create(NULL,
> > NULL, 0);
> > > > +
> > > > + xpc_dictionary_set_uint64(
> > > > + if_desc,
> > > > + vmnet_operation_mode_key,
> > > > + VMNET_SHARED_MODE
> > > > + );
> > > > +
> > > > + if (options->has_nat66_prefix) {
> > > > + xpc_dictionary_set_string(if_desc,
> > > > + vmnet_nat66_prefix_key,
> > > > + options->nat66_prefix);
> > > > + }
> > > > +
> > > > + if (options->has_start_address) {
> > > > + xpc_dictionary_set_string(if_desc,
> > > > + vmnet_start_address_key,
> > > > + options->start_address);
> > > > + xpc_dictionary_set_string(if_desc,
> > > > + vmnet_end_address_key,
> > > > + options->end_address);
> > > > + xpc_dictionary_set_string(if_desc,
> > > > + vmnet_subnet_mask_key,
> > > > + options->subnet_mask);
> > > > + }
> > > > +
> > > > +#if defined(MAC_OS_VERSION_11_0) && \
> > > > + MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> > > > + xpc_dictionary_set_bool(
> > > > + if_desc,
> > > > + vmnet_enable_isolation_key,
> > > > + options->isolated
> > > > + );
> > > > +#endif
> > > > +
> > > > + return if_desc;
> > > > +}
> > > > +
> > > > +static NetClientInfo net_vmnet_shared_info = {
> > > > + .type = NET_CLIENT_DRIVER_VMNET_SHARED,
> > > > + .size = sizeof(VmnetSharedState),
> > > > + .receive = vmnet_receive_common,
> > > > + .cleanup = vmnet_cleanup_common,
> > > > +};
> > > > +
> > > > int net_init_vmnet_shared(const Netdev *netdev, const
> > char *name,
> > > > NetClientState *peer, Error
> > **errp)
> > > > {
> > > > - error_setg(errp, "vmnet-shared is not implemented yet");
> > > > - return -1;
> > > > + NetClientState *nc =
> > qemu_new_net_client(&net_vmnet_shared_info,
> > > > + peer,
> > > "vmnet-shared", name);
> > > > + if (!validate_options(netdev, errp)) {
> > > > + g_assert_not_reached();
> > > > + return -1;
> > > > + }
> > > > + return vmnet_if_create(nc, build_if_desc(netdev),
> errp);
> > > > }
> > > > diff --git a/net/vmnet_int.h b/net/vmnet_int.h
> > > > index aac4d5af64..8f3321ef3e 100644
> > > > --- a/net/vmnet_int.h
> > > > +++ b/net/vmnet_int.h
> > > > @@ -15,11 +15,50 @@
> > > > #include "clients.h"
> > > >
> > > > #include <vmnet/vmnet.h>
> > > > +#include <dispatch/dispatch.h>
> > > > +
> > > > +/**
> > > > + * From vmnet.framework documentation
> > > > + *
> > > > + * Each read/write call allows up to 200 packets to be
> > > > + * read or written for a maximum of 256KB.
> > > > + *
> > > > + * Each packet written should be a complete
> > > > + * ethernet frame.
> > > > + *
> > > > + * https://developer.apple.com/documentation/vmnet
> > <https://developer.apple.com/documentation/vmnet>
> > > <https://developer.apple.com/documentation/vmnet
> > <https://developer.apple.com/documentation/vmnet>>
> > > > + */
> > > > +#define VMNET_PACKETS_LIMIT 200
> > > >
> > > > typedef struct VmnetCommonState {
> > > > - NetClientState nc;
> > > > + NetClientState nc;
> > > > + interface_ref vmnet_if;
> > > > +
> > > > + uint64_t mtu;
> > > > + uint64_t max_packet_size;
> > > >
> > > > + dispatch_queue_t if_queue;
> > > > +
> > > > + QEMUBH *send_bh;
> > > > + bool send_enabled;
> > > > +
> > > > + struct vmpktdesc packets_buf[VMNET_PACKETS_LIMIT];
> > > > + int packets_send_current_pos;
> > > > + int packets_send_end_pos;
> > > > +
> > > > + struct iovec iov_buf[VMNET_PACKETS_LIMIT];
> > > > } VmnetCommonState;
> > > >
> > > > +const char *vmnet_status_map_str(vmnet_return_t status);
> > > > +
> > > > +int vmnet_if_create(NetClientState *nc,
> > > > + xpc_object_t if_desc,
> > > > + Error **errp);
> > > > +
> > > > +ssize_t vmnet_receive_common(NetClientState *nc,
> > > > + const uint8_t *buf,
> > > > + size_t size);
> > > > +
> > > > +void vmnet_cleanup_common(NetClientState *nc);
> > > >
> > > > #endif /* VMNET_INT_H */
> > >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 48445 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-03-14 23:19 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-14 19:15 [PATCH v16 0/7] Add vmnet.framework based network backend Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 1/7] net/vmnet: add vmnet dependency and customizable option Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 2/7] net/vmnet: add vmnet backends to qapi/net Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 3/7] net/vmnet: implement shared mode (vmnet-shared) Vladislav Yaroshchuk
2022-03-14 19:45 ` Akihiko Odaki
2022-03-14 21:50 ` Vladislav Yaroshchuk
2022-03-14 22:34 ` Akihiko Odaki
2022-03-14 22:37 ` Akihiko Odaki
2022-03-14 23:02 ` Vladislav Yaroshchuk
2022-03-14 23:06 ` Akihiko Odaki
2022-03-14 23:18 ` Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 4/7] net/vmnet: implement host mode (vmnet-host) Vladislav Yaroshchuk
2022-03-14 19:55 ` Akihiko Odaki
2022-03-14 19:15 ` [PATCH v16 5/7] net/vmnet: implement bridged mode (vmnet-bridged) Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 6/7] net/vmnet: update qemu-options.hx Vladislav Yaroshchuk
2022-03-14 19:15 ` [PATCH v16 7/7] net/vmnet: update hmp-commands.hx Vladislav Yaroshchuk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.