[PATCH v9 00/17] vfio-user server in QEMU

[PATCH v9 00/17] vfio-user server in QEMU

[Jagannathan Raman]

Hi,

This is v9 of the server side changes to enable vfio-user in QEMU.

Thank you very much for reviewing the last revision of this series!

We've made the following changes in this revision:

[PATCH v9 02/17] qdev: unplug blocker for devices
  - updated commit message with more details

[PATCH v9 06/17] vfio-user: build library
  - updated commit message with license information

[PATCH v9 07/17] vfio-user: define vfio-user-server object
  - fixed type with libvfio-user library name in comments for
    VfioUserServerProperties

[PATCH v9 10/17] vfio-user: run vfio-user context
  - added the QOM patchs of the PCI device and server to
    VFU_CLIENT_HANGUP event

[PATCH v9 12/17] vfio-user: IOMMU support for remote device
  - added comments to describe the design of the remote machine's IOMMU

[PATCH v9 14/17] vfio-user: handle PCI BAR accesses
  - unref memory region during early exit in vfu_object_bar_rw()

Jagannathan Raman (17):
  tests/avocado: Specify target VM argument to helper routines
  qdev: unplug blocker for devices
  remote/machine: add HotplugHandler for remote machine
  remote/machine: add vfio-user property
  configure: require cmake 3.19 or newer
  vfio-user: build library
  vfio-user: define vfio-user-server object
  vfio-user: instantiate vfio-user context
  vfio-user: find and init PCI device
  vfio-user: run vfio-user context
  vfio-user: handle PCI config space accesses
  vfio-user: IOMMU support for remote device
  vfio-user: handle DMA mappings
  vfio-user: handle PCI BAR accesses
  vfio-user: handle device interrupts
  vfio-user: handle reset of remote device
  vfio-user: avocado tests for vfio-user

 configure                                  |  36 +-
 meson.build                                |  44 +-
 qapi/misc.json                             |  30 +
 qapi/qom.json                              |  20 +-
 include/exec/memory.h                      |   3 +
 include/hw/pci/pci.h                       |  13 +
 include/hw/qdev-core.h                     |  29 +
 include/hw/remote/iommu.h                  |  40 +
 include/hw/remote/machine.h                |   4 +
 include/hw/remote/vfio-user-obj.h          |   6 +
 hw/core/qdev.c                             |  24 +
 hw/pci/msi.c                               |  16 +-
 hw/pci/msix.c                              |  10 +-
 hw/pci/pci.c                               |  13 +
 hw/remote/iommu.c                          | 131 +++
 hw/remote/machine.c                        |  88 +-
 hw/remote/vfio-user-obj.c                  | 898 +++++++++++++++++++++
 softmmu/physmem.c                          |   4 +-
 softmmu/qdev-monitor.c                     |   4 +
 stubs/vfio-user-obj.c                      |   6 +
 tests/qtest/fuzz/generic_fuzz.c            |   9 +-
 .gitlab-ci.d/buildtest.yml                 |   2 +
 .gitmodules                                |   3 +
 Kconfig.host                               |   4 +
 MAINTAINERS                                |   6 +
 hw/remote/Kconfig                          |   4 +
 hw/remote/meson.build                      |   4 +
 hw/remote/trace-events                     |  11 +
 meson_options.txt                          |   3 +
 stubs/meson.build                          |   1 +
 subprojects/libvfio-user                   |   1 +
 tests/avocado/avocado_qemu/__init__.py     |  14 +-
 tests/avocado/vfio-user.py                 | 164 ++++
 tests/docker/dockerfiles/centos8.docker    |   2 +
 tests/docker/dockerfiles/ubuntu2004.docker |   2 +
 35 files changed, 1625 insertions(+), 24 deletions(-)
 create mode 100644 include/hw/remote/iommu.h
 create mode 100644 include/hw/remote/vfio-user-obj.h
 create mode 100644 hw/remote/iommu.c
 create mode 100644 hw/remote/vfio-user-obj.c
 create mode 100644 stubs/vfio-user-obj.c
 create mode 160000 subprojects/libvfio-user
 create mode 100644 tests/avocado/vfio-user.py

-- 
2.20.1

[PATCH v9 01/17] tests/avocado: Specify target VM argument to helper routines

[Jagannathan Raman]

Specify target VM for exec_command and
exec_command_and_wait_for_pattern routines

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Beraldo Leal <bleal@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/avocado/avocado_qemu/__init__.py | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
index 39f15c1d51..340a345799 100644
--- a/tests/avocado/avocado_qemu/__init__.py
+++ b/tests/avocado/avocado_qemu/__init__.py
@@ -198,7 +198,7 @@ def wait_for_console_pattern(test, success_message, failure_message=None,
     """
     _console_interaction(test, success_message, failure_message, None, vm=vm)
 
-def exec_command(test, command):
+def exec_command(test, command, vm=None):
     """
     Send a command to a console (appending CRLF characters), while logging
     the content.
@@ -207,11 +207,14 @@ def exec_command(test, command):
     :type test: :class:`avocado_qemu.QemuSystemTest`
     :param command: the command to send
     :type command: str
+    :param vm: target vm
+    :type vm: :class:`qemu.machine.QEMUMachine`
     """
-    _console_interaction(test, None, None, command + '\r')
+    _console_interaction(test, None, None, command + '\r', vm=vm)
 
 def exec_command_and_wait_for_pattern(test, command,
-                                      success_message, failure_message=None):
+                                      success_message, failure_message=None,
+                                      vm=None):
     """
     Send a command to a console (appending CRLF characters), then wait
     for success_message to appear on the console, while logging the.
@@ -223,8 +226,11 @@ def exec_command_and_wait_for_pattern(test, command,
     :param command: the command to send
     :param success_message: if this message appears, test succeeds
     :param failure_message: if this message appears, test fails
+    :param vm: target vm
+    :type vm: :class:`qemu.machine.QEMUMachine`
     """
-    _console_interaction(test, success_message, failure_message, command + '\r')
+    _console_interaction(test, success_message, failure_message, command + '\r',
+                         vm=vm)
 
 class QemuBaseTest(avocado.Test):
     def _get_unique_tag_val(self, tag_name):
-- 
2.20.1

[PATCH v9 02/17] qdev: unplug blocker for devices

[Jagannathan Raman]

Add blocker to prevent hot-unplug of devices

TYPE_VFIO_USER_SERVER, which is introduced shortly, attaches itself to a
PCIDevice on which it depends. If the attached PCIDevice gets removed
while the server in use, it could cause it crash. To prevent this,
TYPE_VFIO_USER_SERVER adds an unplug blocker for the PCIDevice.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/qdev-core.h | 29 +++++++++++++++++++++++++++++
 hw/core/qdev.c         | 24 ++++++++++++++++++++++++
 softmmu/qdev-monitor.c |  4 ++++
 3 files changed, 57 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 92c3d65208..1b9fa25e5c 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -193,6 +193,7 @@ struct DeviceState {
     int instance_id_alias;
     int alias_required_for_version;
     ResettableState reset;
+    GSList *unplug_blockers;
 };
 
 struct DeviceListener {
@@ -419,6 +420,34 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
 void qdev_machine_creation_done(void);
 bool qdev_machine_modified(void);
 
+/*
+ * qdev_add_unplug_blocker: Adds an unplug blocker to a device
+ *
+ * @dev: Device to be blocked from unplug
+ * @reason: Reason for blocking
+ */
+void qdev_add_unplug_blocker(DeviceState *dev, Error *reason);
+
+/*
+ * qdev_del_unplug_blocker: Removes an unplug blocker from a device
+ *
+ * @dev: Device to be unblocked
+ * @reason: Pointer to the Error used with qdev_add_unplug_blocker.
+ *          Used as a handle to lookup the blocker for deletion.
+ */
+void qdev_del_unplug_blocker(DeviceState *dev, Error *reason);
+
+/*
+ * qdev_unplug_blocked: Confirms if a device is blocked from unplug
+ *
+ * @dev: Device to be tested
+ * @reason: Returns one of the reasons why the device is blocked,
+ *          if any
+ *
+ * Returns: true if device is blocked from unplug, false otherwise
+ */
+bool qdev_unplug_blocked(DeviceState *dev, Error **errp);
+
 /**
  * GpioPolarity: Polarity of a GPIO line
  *
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 84f3019440..0806d8fcaa 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -468,6 +468,28 @@ char *qdev_get_dev_path(DeviceState *dev)
     return NULL;
 }
 
+void qdev_add_unplug_blocker(DeviceState *dev, Error *reason)
+{
+    dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason);
+}
+
+void qdev_del_unplug_blocker(DeviceState *dev, Error *reason)
+{
+    dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason);
+}
+
+bool qdev_unplug_blocked(DeviceState *dev, Error **errp)
+{
+    ERRP_GUARD();
+
+    if (dev->unplug_blockers) {
+        error_propagate(errp, error_copy(dev->unplug_blockers->data));
+        return true;
+    }
+
+    return false;
+}
+
 static bool device_get_realized(Object *obj, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
@@ -704,6 +726,8 @@ static void device_finalize(Object *obj)
 
     DeviceState *dev = DEVICE(obj);
 
+    g_assert(!dev->unplug_blockers);
+
     QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
         QLIST_REMOVE(ngl, node);
         qemu_free_irqs(ngl->in, ngl->num_in);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 12fe60c467..9cfd59d17c 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -898,6 +898,10 @@ void qdev_unplug(DeviceState *dev, Error **errp)
     HotplugHandlerClass *hdc;
     Error *local_err = NULL;
 
+    if (qdev_unplug_blocked(dev, errp)) {
+        return;
+    }
+
     if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
         error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
         return;
-- 
2.20.1

[PATCH v9 03/17] remote/machine: add HotplugHandler for remote machine

[Jagannathan Raman]

Allow hotplugging of PCI(e) devices to remote machine

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/remote/machine.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/remote/machine.c b/hw/remote/machine.c
index 92d71d47bb..a97e53e250 100644
--- a/hw/remote/machine.c
+++ b/hw/remote/machine.c
@@ -20,6 +20,7 @@
 #include "qapi/error.h"
 #include "hw/pci/pci_host.h"
 #include "hw/remote/iohub.h"
+#include "hw/qdev-core.h"
 
 static void remote_machine_init(MachineState *machine)
 {
@@ -53,14 +54,19 @@ static void remote_machine_init(MachineState *machine)
 
     pci_bus_irqs(pci_host->bus, remote_iohub_set_irq, remote_iohub_map_irq,
                  &s->iohub, REMOTE_IOHUB_NB_PIRQS);
+
+    qbus_set_hotplug_handler(BUS(pci_host->bus), OBJECT(s));
 }
 
 static void remote_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     mc->init = remote_machine_init;
     mc->desc = "Experimental remote machine";
+
+    hc->unplug = qdev_simple_device_unplug_cb;
 }
 
 static const TypeInfo remote_machine = {
@@ -68,6 +74,10 @@ static const TypeInfo remote_machine = {
     .parent = TYPE_MACHINE,
     .instance_size = sizeof(RemoteMachineState),
     .class_init = remote_machine_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void remote_machine_register_types(void)
-- 
2.20.1

[PATCH v9 04/17] remote/machine: add vfio-user property

[Jagannathan Raman]

Add vfio-user to x-remote machine. It is a boolean, which indicates if
the machine supports vfio-user protocol. The machine configures the bus
differently vfio-user and multiprocess protocols, so this property
informs it on how to configure the bus.

This property should be short lived. Once vfio-user fully replaces
multiprocess, this property could be removed.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/remote/machine.h |  2 ++
 hw/remote/machine.c         | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/include/hw/remote/machine.h b/include/hw/remote/machine.h
index 2a2a33c4b2..8d0fa98d33 100644
--- a/include/hw/remote/machine.h
+++ b/include/hw/remote/machine.h
@@ -22,6 +22,8 @@ struct RemoteMachineState {
 
     RemotePCIHost *host;
     RemoteIOHubState iohub;
+
+    bool vfio_user;
 };
 
 /* Used to pass to co-routine device and ioc. */
diff --git a/hw/remote/machine.c b/hw/remote/machine.c
index a97e53e250..9f3cdc55c3 100644
--- a/hw/remote/machine.c
+++ b/hw/remote/machine.c
@@ -58,6 +58,25 @@ static void remote_machine_init(MachineState *machine)
     qbus_set_hotplug_handler(BUS(pci_host->bus), OBJECT(s));
 }
 
+static bool remote_machine_get_vfio_user(Object *obj, Error **errp)
+{
+    RemoteMachineState *s = REMOTE_MACHINE(obj);
+
+    return s->vfio_user;
+}
+
+static void remote_machine_set_vfio_user(Object *obj, bool value, Error **errp)
+{
+    RemoteMachineState *s = REMOTE_MACHINE(obj);
+
+    if (phase_check(PHASE_MACHINE_CREATED)) {
+        error_setg(errp, "Error enabling vfio-user - machine already created");
+        return;
+    }
+
+    s->vfio_user = value;
+}
+
 static void remote_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -67,6 +86,10 @@ static void remote_machine_class_init(ObjectClass *oc, void *data)
     mc->desc = "Experimental remote machine";
 
     hc->unplug = qdev_simple_device_unplug_cb;
+
+    object_class_property_add_bool(oc, "vfio-user",
+                                   remote_machine_get_vfio_user,
+                                   remote_machine_set_vfio_user);
 }
 
 static const TypeInfo remote_machine = {
-- 
2.20.1

[PATCH v9 05/17] configure: require cmake 3.19 or newer

[Jagannathan Raman]

cmake needs to accept the compiler flags specified with
CMAKE_<LANG>_COMPILER variable. It does so starting with
version 3.19

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
---
 configure | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/configure b/configure
index 59c43bea05..7cefab289d 100755
--- a/configure
+++ b/configure
@@ -249,6 +249,7 @@ stack_protector=""
 safe_stack=""
 use_containers="yes"
 gdb_bin=$(command -v "gdb-multiarch" || command -v "gdb")
+cmake_required="no"
 
 if test -e "$source_path/.git"
 then
@@ -2503,6 +2504,21 @@ if !(GIT="$git" "$source_path/scripts/git-submodule.sh" "$git_submodules_action"
     exit 1
 fi
 
+# Per cmake spec, CMAKE_<LANG>_COMPILER variable may include "mandatory" compiler
+# flags. QEMU needs to specify these flags to correctly configure the build
+# environment. cmake 3.19 allows specifying these mandatory compiler flags,
+# and as such 3.19 or newer is required to build QEMU.
+if test "$cmake_required" = "yes" ; then
+    cmake_bin=$(command -v "cmake")
+    if [ -z "$cmake_bin" ]; then
+        error_exit "cmake not found"
+    fi
+    cmake_version=$($cmake_bin --version | head -n 1)
+    if ! version_ge ${cmake_version##* } 3.19; then
+        error_exit "QEMU needs cmake 3.19 or newer"
+    fi
+fi
+
 config_host_mak="config-host.mak"
 
 echo "# Automatically generated by configure - do not modify" > $config_host_mak
-- 
2.20.1

[PATCH v9 06/17] vfio-user: build library

[Jagannathan Raman]

add the libvfio-user library as a submodule. build it as a cmake
subproject.

libvfio-user is distributed with BSD 3-Clause license and
json-c with MIT (Expat) license

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 configure                                  | 20 +++++++++-
 meson.build                                | 44 +++++++++++++++++++++-
 .gitlab-ci.d/buildtest.yml                 |  2 +
 .gitmodules                                |  3 ++
 Kconfig.host                               |  4 ++
 MAINTAINERS                                |  1 +
 hw/remote/Kconfig                          |  4 ++
 hw/remote/meson.build                      |  2 +
 meson_options.txt                          |  3 ++
 subprojects/libvfio-user                   |  1 +
 tests/docker/dockerfiles/centos8.docker    |  2 +
 tests/docker/dockerfiles/ubuntu2004.docker |  2 +
 12 files changed, 86 insertions(+), 2 deletions(-)
 create mode 160000 subprojects/libvfio-user

diff --git a/configure b/configure
index 7cefab289d..3b096f1b94 100755
--- a/configure
+++ b/configure
@@ -326,6 +326,7 @@ meson=""
 meson_args=""
 ninja=""
 skip_meson=no
+vfio_user_server="disabled"
 
 # The following Meson options are handled manually (still they
 # are included in the automatically generated help message)
@@ -1008,6 +1009,11 @@ for opt do
   ;;
   --disable-blobs) meson_option_parse --disable-install-blobs ""
   ;;
+  --enable-vfio-user-server) vfio_user_server="enabled"
+                             cmake_required="yes"
+  ;;
+  --disable-vfio-user-server) vfio_user_server="disabled"
+  ;;
   --enable-tcmalloc) meson_option_parse --enable-malloc=tcmalloc tcmalloc
   ;;
   --enable-jemalloc) meson_option_parse --enable-malloc=jemalloc jemalloc
@@ -1226,6 +1232,7 @@ cat << EOF
   vhost-kernel    vhost kernel backend support
   vhost-user      vhost-user backend support
   vhost-vdpa      vhost-vdpa kernel backend support
+  vfio-user-server    vfio-user server support
 
 NOTE: The object files are built at the place where configure is launched
 EOF
@@ -2350,6 +2357,17 @@ case "$slirp" in
     ;;
 esac
 
+##########################################
+# check for vfio_user_server
+
+case "$vfio_user_server" in
+  auto | enabled )
+    if test "$git_submodules_action" != "ignore"; then
+      git_submodules="${git_submodules} subprojects/libvfio-user"
+    fi
+    ;;
+esac
+
 ##########################################
 # End of CC checks
 # After here, no more $cc or $ld runs
@@ -2854,7 +2872,7 @@ if test "$skip_meson" = no; then
         -Db_pie=$(if test "$pie" = yes; then echo true; else echo false; fi) \
         -Db_coverage=$(if test "$gcov" = yes; then echo true; else echo false; fi) \
         -Db_lto=$lto -Dcfi=$cfi -Dtcg=$tcg -Dxen=$xen \
-        -Dcapstone=$capstone -Dfdt=$fdt -Dslirp=$slirp \
+        -Dcapstone=$capstone -Dfdt=$fdt -Dslirp=$slirp -Dvfio_user_server=$vfio_user_server \
         $(test -n "${LIB_FUZZING_ENGINE+xxx}" && echo "-Dfuzzing_engine=$LIB_FUZZING_ENGINE") \
         $(if test "$default_feature" = no; then echo "-Dauto_features=disabled"; fi) \
         "$@" $cross_arg "$PWD" "$source_path"
diff --git a/meson.build b/meson.build
index 1fe7d257ff..55b872b51e 100644
--- a/meson.build
+++ b/meson.build
@@ -298,6 +298,11 @@ have_tpm = get_option('tpm') \
   .require(targetos != 'windows', error_message: 'TPM emulation only available on POSIX systems') \
   .allowed()
 
+if targetos != 'linux' and get_option('vfio_user_server').enabled()
+  error('vfio-user server is supported only on Linux')
+endif
+vfio_user_server_allowed = targetos == 'linux' and not get_option('vfio_user_server').disabled()
+
 # Target-specific libraries and flags
 libm = cc.find_library('m', required: false)
 threads = dependency('threads')
@@ -2204,7 +2209,8 @@ host_kconfig = \
   (have_virtfs ? ['CONFIG_VIRTFS=y'] : []) + \
   ('CONFIG_LINUX' in config_host ? ['CONFIG_LINUX=y'] : []) + \
   (have_pvrdma ? ['CONFIG_PVRDMA=y'] : []) + \
-  (multiprocess_allowed ? ['CONFIG_MULTIPROCESS_ALLOWED=y'] : [])
+  (multiprocess_allowed ? ['CONFIG_MULTIPROCESS_ALLOWED=y'] : []) + \
+  (vfio_user_server_allowed ? ['CONFIG_VFIO_USER_SERVER_ALLOWED=y'] : [])
 
 ignored = [ 'TARGET_XML_FILES', 'TARGET_ABI_DIR', 'TARGET_ARCH' ]
 
@@ -2596,6 +2602,41 @@ if get_option('cfi') and slirp_opt == 'system'
          + ' Please configure with --enable-slirp=git')
 endif
 
+vfiouser = not_found
+if have_system and vfio_user_server_allowed
+  have_internal = fs.exists(meson.current_source_dir() / 'subprojects/libvfio-user/Makefile')
+
+  if not have_internal
+    error('libvfio-user source not found - please pull git submodule')
+  endif
+
+  json_c = dependency('json-c', required: false)
+  if not json_c.found()
+    json_c = dependency('libjson-c', required: false)
+  endif
+  if not json_c.found()
+    json_c = dependency('libjson-c-dev', required: false)
+  endif
+
+  if not json_c.found()
+    error('Unable to find json-c package')
+  endif
+
+  cmake = import('cmake')
+
+  vfiouser_subproj = cmake.subproject('libvfio-user')
+
+  vfiouser_sl = vfiouser_subproj.dependency('vfio-user-static')
+
+  # Although cmake links the json-c library with vfio-user-static
+  # target, that info is not available to meson via cmake.subproject.
+  # As such, we have to separately declare the json-c dependency here.
+  # This appears to be a current limitation of using cmake inside meson.
+  # libvfio-user is planning a switch to meson in the future, which
+  # would address this item automatically.
+  vfiouser = declare_dependency(dependencies: [vfiouser_sl, json_c])
+endif
+
 fdt = not_found
 if have_system
   fdt_opt = get_option('fdt')
@@ -3708,6 +3749,7 @@ summary_info += {'target list':       ' '.join(target_dirs)}
 if have_system
   summary_info += {'default devices':   get_option('default_devices')}
   summary_info += {'out of process emulation': multiprocess_allowed}
+  summary_info += {'vfio-user server': vfio_user_server_allowed}
 endif
 summary(summary_info, bool_yn: true, section: 'Targets and accelerators')
 
diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index 0aea7ab84c..671a5d8fa4 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -42,6 +42,7 @@ build-system-ubuntu:
   variables:
     IMAGE: ubuntu2004
     CONFIGURE_ARGS: --enable-docs --enable-fdt=system --enable-slirp=system
+                    --enable-vfio-user-server
     TARGETS: aarch64-softmmu alpha-softmmu cris-softmmu hppa-softmmu
       microblazeel-softmmu mips64el-softmmu
     MAKE_CHECK_ARGS: check-build
@@ -165,6 +166,7 @@ build-system-centos:
     IMAGE: centos8
     CONFIGURE_ARGS: --disable-nettle --enable-gcrypt --enable-fdt=system
       --enable-modules --enable-trace-backends=dtrace --enable-docs
+      --enable-vfio-user-server
     TARGETS: ppc64-softmmu or1k-softmmu s390x-softmmu
       x86_64-softmmu rx-softmmu sh4-softmmu nios2-softmmu
     MAKE_CHECK_ARGS: check-build
diff --git a/.gitmodules b/.gitmodules
index f4b6a9b401..d66af96dc9 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -67,3 +67,6 @@
 [submodule "tests/lcitool/libvirt-ci"]
 	path = tests/lcitool/libvirt-ci
 	url = https://gitlab.com/libvirt/libvirt-ci.git
+[submodule "subprojects/libvfio-user"]
+	path = subprojects/libvfio-user
+	url = https://github.com/nutanix/libvfio-user.git
diff --git a/Kconfig.host b/Kconfig.host
index 60b9c07b5e..f2da8bcf8a 100644
--- a/Kconfig.host
+++ b/Kconfig.host
@@ -45,3 +45,7 @@ config MULTIPROCESS_ALLOWED
 config FUZZ
     bool
     select SPARSE_MEM
+
+config VFIO_USER_SERVER_ALLOWED
+    bool
+    imply VFIO_USER_SERVER
diff --git a/MAINTAINERS b/MAINTAINERS
index 294c88ace9..84b0e019e8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3598,6 +3598,7 @@ F: hw/remote/proxy-memory-listener.c
 F: include/hw/remote/proxy-memory-listener.h
 F: hw/remote/iohub.c
 F: include/hw/remote/iohub.h
+F: subprojects/libvfio-user
 
 EBPF:
 M: Jason Wang <jasowang@redhat.com>
diff --git a/hw/remote/Kconfig b/hw/remote/Kconfig
index 08c16e235f..2d6b4f4cf4 100644
--- a/hw/remote/Kconfig
+++ b/hw/remote/Kconfig
@@ -2,3 +2,7 @@ config MULTIPROCESS
     bool
     depends on PCI && PCI_EXPRESS && KVM
     select REMOTE_PCIHOST
+
+config VFIO_USER_SERVER
+    bool
+    depends on MULTIPROCESS
diff --git a/hw/remote/meson.build b/hw/remote/meson.build
index e6a5574242..dfea6b533b 100644
--- a/hw/remote/meson.build
+++ b/hw/remote/meson.build
@@ -7,6 +7,8 @@ remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('remote-obj.c'))
 remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('proxy.c'))
 remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('iohub.c'))
 
+remote_ss.add(when: 'CONFIG_VFIO_USER_SERVER', if_true: vfiouser)
+
 specific_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('memory.c'))
 specific_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('proxy-memory-listener.c'))
 
diff --git a/meson_options.txt b/meson_options.txt
index af432a4ee6..e1315f9a07 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -95,6 +95,9 @@ option('avx512f', type: 'feature', value: 'disabled',
 option('keyring', type: 'feature', value: 'auto',
        description: 'Linux keyring support')
 
+option('vfio_user_server', type: 'feature', value: 'auto',
+       description: 'vfio-user server support')
+
 option('attr', type : 'feature', value : 'auto',
        description: 'attr/xattr support')
 option('auth_pam', type : 'feature', value : 'auto',
diff --git a/subprojects/libvfio-user b/subprojects/libvfio-user
new file mode 160000
index 0000000000..030d2f6e79
--- /dev/null
+++ b/subprojects/libvfio-user
@@ -0,0 +1 @@
+Subproject commit 030d2f6e7978b8ca7577b81d4f48e2771bcd8f47
diff --git a/tests/docker/dockerfiles/centos8.docker b/tests/docker/dockerfiles/centos8.docker
index 4b20925bbf..300833d8e0 100644
--- a/tests/docker/dockerfiles/centos8.docker
+++ b/tests/docker/dockerfiles/centos8.docker
@@ -23,6 +23,7 @@ RUN dnf update -y && \
         capstone-devel \
         ccache \
         clang \
+        cmake \
         ctags \
         cyrus-sasl-devel \
         daxctl-devel \
@@ -45,6 +46,7 @@ RUN dnf update -y && \
         gtk3-devel \
         hostname \
         jemalloc-devel \
+        json-c-devel \
         libaio-devel \
         libasan \
         libattr-devel \
diff --git a/tests/docker/dockerfiles/ubuntu2004.docker b/tests/docker/dockerfiles/ubuntu2004.docker
index a3b38884e3..7c6131686a 100644
--- a/tests/docker/dockerfiles/ubuntu2004.docker
+++ b/tests/docker/dockerfiles/ubuntu2004.docker
@@ -18,6 +18,7 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
             ca-certificates \
             ccache \
             clang \
+            cmake \
             dbus \
             debianutils \
             diffutils \
@@ -58,6 +59,7 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
             libiscsi-dev \
             libjemalloc-dev \
             libjpeg-turbo8-dev \
+            libjson-c-dev \
             liblttng-ust-dev \
             liblzo2-dev \
             libncursesw5-dev \
-- 
2.20.1

[PATCH v9 07/17] vfio-user: define vfio-user-server object

[Jagannathan Raman]

Define vfio-user object which is remote process server for QEMU. Setup
object initialization functions and properties necessary to instantiate
the object

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
---
 qapi/qom.json               |  20 +++-
 include/hw/remote/machine.h |   2 +
 hw/remote/machine.c         |  27 +++++
 hw/remote/vfio-user-obj.c   | 210 ++++++++++++++++++++++++++++++++++++
 MAINTAINERS                 |   1 +
 hw/remote/meson.build       |   1 +
 hw/remote/trace-events      |   3 +
 7 files changed, 262 insertions(+), 2 deletions(-)
 create mode 100644 hw/remote/vfio-user-obj.c

diff --git a/qapi/qom.json b/qapi/qom.json
index eeb5395ff3..582def0522 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -703,6 +703,20 @@
 { 'struct': 'RemoteObjectProperties',
   'data': { 'fd': 'str', 'devid': 'str' } }
 
+##
+# @VfioUserServerProperties:
+#
+# Properties for x-vfio-user-server objects.
+#
+# @socket: socket to be used by the libvfio-user library
+#
+# @device: the id of the device to be emulated at the server
+#
+# Since: 7.1
+##
+{ 'struct': 'VfioUserServerProperties',
+  'data': { 'socket': 'SocketAddress', 'device': 'str' } }
+
 ##
 # @RngProperties:
 #
@@ -842,7 +856,8 @@
     'tls-creds-psk',
     'tls-creds-x509',
     'tls-cipher-suites',
-    { 'name': 'x-remote-object', 'features': [ 'unstable' ] }
+    { 'name': 'x-remote-object', 'features': [ 'unstable' ] },
+    { 'name': 'x-vfio-user-server', 'features': [ 'unstable' ] }
   ] }
 
 ##
@@ -905,7 +920,8 @@
       'tls-creds-psk':              'TlsCredsPskProperties',
       'tls-creds-x509':             'TlsCredsX509Properties',
       'tls-cipher-suites':          'TlsCredsProperties',
-      'x-remote-object':            'RemoteObjectProperties'
+      'x-remote-object':            'RemoteObjectProperties',
+      'x-vfio-user-server':         'VfioUserServerProperties'
   } }
 
 ##
diff --git a/include/hw/remote/machine.h b/include/hw/remote/machine.h
index 8d0fa98d33..ac32fda387 100644
--- a/include/hw/remote/machine.h
+++ b/include/hw/remote/machine.h
@@ -24,6 +24,8 @@ struct RemoteMachineState {
     RemoteIOHubState iohub;
 
     bool vfio_user;
+
+    bool auto_shutdown;
 };
 
 /* Used to pass to co-routine device and ioc. */
diff --git a/hw/remote/machine.c b/hw/remote/machine.c
index 9f3cdc55c3..4d008ed721 100644
--- a/hw/remote/machine.c
+++ b/hw/remote/machine.c
@@ -77,6 +77,28 @@ static void remote_machine_set_vfio_user(Object *obj, bool value, Error **errp)
     s->vfio_user = value;
 }
 
+static bool remote_machine_get_auto_shutdown(Object *obj, Error **errp)
+{
+    RemoteMachineState *s = REMOTE_MACHINE(obj);
+
+    return s->auto_shutdown;
+}
+
+static void remote_machine_set_auto_shutdown(Object *obj, bool value,
+                                             Error **errp)
+{
+    RemoteMachineState *s = REMOTE_MACHINE(obj);
+
+    s->auto_shutdown = value;
+}
+
+static void remote_machine_instance_init(Object *obj)
+{
+    RemoteMachineState *s = REMOTE_MACHINE(obj);
+
+    s->auto_shutdown = true;
+}
+
 static void remote_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -90,12 +112,17 @@ static void remote_machine_class_init(ObjectClass *oc, void *data)
     object_class_property_add_bool(oc, "vfio-user",
                                    remote_machine_get_vfio_user,
                                    remote_machine_set_vfio_user);
+
+    object_class_property_add_bool(oc, "auto-shutdown",
+                                   remote_machine_get_auto_shutdown,
+                                   remote_machine_set_auto_shutdown);
 }
 
 static const TypeInfo remote_machine = {
     .name = TYPE_REMOTE_MACHINE,
     .parent = TYPE_MACHINE,
     .instance_size = sizeof(RemoteMachineState),
+    .instance_init = remote_machine_instance_init,
     .class_init = remote_machine_class_init,
     .interfaces = (InterfaceInfo[]) {
         { TYPE_HOTPLUG_HANDLER },
diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
new file mode 100644
index 0000000000..bc49adcc27
--- /dev/null
+++ b/hw/remote/vfio-user-obj.c
@@ -0,0 +1,210 @@
+/**
+ * QEMU vfio-user-server server object
+ *
+ * Copyright © 2022 Oracle and/or its affiliates.
+ *
+ * This work is licensed under the terms of the GNU GPL-v2, version 2 or later.
+ *
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+/**
+ * Usage: add options:
+ *     -machine x-remote,vfio-user=on,auto-shutdown=on
+ *     -device <PCI-device>,id=<pci-dev-id>
+ *     -object x-vfio-user-server,id=<id>,type=unix,path=<socket-path>,
+ *             device=<pci-dev-id>
+ *
+ * Note that x-vfio-user-server object must be used with x-remote machine only.
+ * This server could only support PCI devices for now.
+ *
+ * type - SocketAddress type - presently "unix" alone is supported. Required
+ *        option
+ *
+ * path - named unix socket, it will be created by the server. It is
+ *        a required option
+ *
+ * device - id of a device on the server, a required option. PCI devices
+ *          alone are supported presently.
+ */
+
+#include "qemu/osdep.h"
+
+#include "qom/object.h"
+#include "qom/object_interfaces.h"
+#include "qemu/error-report.h"
+#include "trace.h"
+#include "sysemu/runstate.h"
+#include "hw/boards.h"
+#include "hw/remote/machine.h"
+#include "qapi/error.h"
+#include "qapi/qapi-visit-sockets.h"
+
+#define TYPE_VFU_OBJECT "x-vfio-user-server"
+OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
+
+/**
+ * VFU_OBJECT_ERROR - reports an error message. If auto_shutdown
+ * is set, it aborts the machine on error. Otherwise, it logs an
+ * error message without aborting.
+ */
+#define VFU_OBJECT_ERROR(o, fmt, ...)                                     \
+    {                                                                     \
+        if (vfu_object_auto_shutdown()) {                                 \
+            error_setg(&error_abort, (fmt), ## __VA_ARGS__);              \
+        } else {                                                          \
+            error_report((fmt), ## __VA_ARGS__);                          \
+        }                                                                 \
+    }                                                                     \
+
+struct VfuObjectClass {
+    ObjectClass parent_class;
+
+    unsigned int nr_devs;
+};
+
+struct VfuObject {
+    /* private */
+    Object parent;
+
+    SocketAddress *socket;
+
+    char *device;
+
+    Error *err;
+};
+
+static bool vfu_object_auto_shutdown(void)
+{
+    bool auto_shutdown = true;
+    Error *local_err = NULL;
+
+    if (!current_machine) {
+        return auto_shutdown;
+    }
+
+    auto_shutdown = object_property_get_bool(OBJECT(current_machine),
+                                             "auto-shutdown",
+                                             &local_err);
+
+    /*
+     * local_err would be set if no such property exists - safe to ignore.
+     * Unlikely scenario as auto-shutdown is always defined for
+     * TYPE_REMOTE_MACHINE, and  TYPE_VFU_OBJECT only works with
+     * TYPE_REMOTE_MACHINE
+     */
+    if (local_err) {
+        auto_shutdown = true;
+        error_free(local_err);
+    }
+
+    return auto_shutdown;
+}
+
+static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name,
+                                  void *opaque, Error **errp)
+{
+    VfuObject *o = VFU_OBJECT(obj);
+
+    qapi_free_SocketAddress(o->socket);
+
+    o->socket = NULL;
+
+    visit_type_SocketAddress(v, name, &o->socket, errp);
+
+    if (o->socket->type != SOCKET_ADDRESS_TYPE_UNIX) {
+        error_setg(errp, "vfu: Unsupported socket type - %s",
+                   SocketAddressType_str(o->socket->type));
+        qapi_free_SocketAddress(o->socket);
+        o->socket = NULL;
+        return;
+    }
+
+    trace_vfu_prop("socket", o->socket->u.q_unix.path);
+}
+
+static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
+{
+    VfuObject *o = VFU_OBJECT(obj);
+
+    g_free(o->device);
+
+    o->device = g_strdup(str);
+
+    trace_vfu_prop("device", str);
+}
+
+static void vfu_object_init(Object *obj)
+{
+    VfuObjectClass *k = VFU_OBJECT_GET_CLASS(obj);
+    VfuObject *o = VFU_OBJECT(obj);
+
+    k->nr_devs++;
+
+    if (!object_dynamic_cast(OBJECT(current_machine), TYPE_REMOTE_MACHINE)) {
+        error_setg(&o->err, "vfu: %s only compatible with %s machine",
+                   TYPE_VFU_OBJECT, TYPE_REMOTE_MACHINE);
+        return;
+    }
+}
+
+static void vfu_object_finalize(Object *obj)
+{
+    VfuObjectClass *k = VFU_OBJECT_GET_CLASS(obj);
+    VfuObject *o = VFU_OBJECT(obj);
+
+    k->nr_devs--;
+
+    qapi_free_SocketAddress(o->socket);
+
+    o->socket = NULL;
+
+    g_free(o->device);
+
+    o->device = NULL;
+
+    if (!k->nr_devs && vfu_object_auto_shutdown()) {
+        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+    }
+}
+
+static void vfu_object_class_init(ObjectClass *klass, void *data)
+{
+    VfuObjectClass *k = VFU_OBJECT_CLASS(klass);
+
+    k->nr_devs = 0;
+
+    object_class_property_add(klass, "socket", "SocketAddress", NULL,
+                              vfu_object_set_socket, NULL, NULL);
+    object_class_property_set_description(klass, "socket",
+                                          "SocketAddress "
+                                          "(ex: type=unix,path=/tmp/sock). "
+                                          "Only UNIX is presently supported");
+    object_class_property_add_str(klass, "device", NULL,
+                                  vfu_object_set_device);
+    object_class_property_set_description(klass, "device",
+                                          "device ID - only PCI devices "
+                                          "are presently supported");
+}
+
+static const TypeInfo vfu_object_info = {
+    .name = TYPE_VFU_OBJECT,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(VfuObject),
+    .instance_init = vfu_object_init,
+    .instance_finalize = vfu_object_finalize,
+    .class_size = sizeof(VfuObjectClass),
+    .class_init = vfu_object_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
+static void vfu_register_types(void)
+{
+    type_register_static(&vfu_object_info);
+}
+
+type_init(vfu_register_types);
diff --git a/MAINTAINERS b/MAINTAINERS
index 84b0e019e8..1b5719cd89 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3599,6 +3599,7 @@ F: include/hw/remote/proxy-memory-listener.h
 F: hw/remote/iohub.c
 F: include/hw/remote/iohub.h
 F: subprojects/libvfio-user
+F: hw/remote/vfio-user-obj.c
 
 EBPF:
 M: Jason Wang <jasowang@redhat.com>
diff --git a/hw/remote/meson.build b/hw/remote/meson.build
index dfea6b533b..534ac5df79 100644
--- a/hw/remote/meson.build
+++ b/hw/remote/meson.build
@@ -6,6 +6,7 @@ remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('message.c'))
 remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('remote-obj.c'))
 remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('proxy.c'))
 remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('iohub.c'))
+remote_ss.add(when: 'CONFIG_VFIO_USER_SERVER', if_true: files('vfio-user-obj.c'))
 
 remote_ss.add(when: 'CONFIG_VFIO_USER_SERVER', if_true: vfiouser)
 
diff --git a/hw/remote/trace-events b/hw/remote/trace-events
index 0b23974f90..7da12f0d96 100644
--- a/hw/remote/trace-events
+++ b/hw/remote/trace-events
@@ -2,3 +2,6 @@
 
 mpqemu_send_io_error(int cmd, int size, int nfds) "send command %d size %d, %d file descriptors to remote process"
 mpqemu_recv_io_error(int cmd, int size, int nfds) "failed to receive %d size %d, %d file descriptors to remote process"
+
+# vfio-user-obj.c
+vfu_prop(const char *prop, const char *val) "vfu: setting %s as %s"
-- 
2.20.1

[PATCH v9 08/17] vfio-user: instantiate vfio-user context

[Jagannathan Raman]

create a context with the vfio-user library to run a PCI device

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/remote/vfio-user-obj.c | 82 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index bc49adcc27..68f8a9dfa9 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -40,6 +40,9 @@
 #include "hw/remote/machine.h"
 #include "qapi/error.h"
 #include "qapi/qapi-visit-sockets.h"
+#include "qemu/notify.h"
+#include "sysemu/sysemu.h"
+#include "libvfio-user.h"
 
 #define TYPE_VFU_OBJECT "x-vfio-user-server"
 OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
@@ -73,8 +76,14 @@ struct VfuObject {
     char *device;
 
     Error *err;
+
+    Notifier machine_done;
+
+    vfu_ctx_t *vfu_ctx;
 };
 
+static void vfu_object_init_ctx(VfuObject *o, Error **errp);
+
 static bool vfu_object_auto_shutdown(void)
 {
     bool auto_shutdown = true;
@@ -107,6 +116,11 @@ static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name,
 {
     VfuObject *o = VFU_OBJECT(obj);
 
+    if (o->vfu_ctx) {
+        error_setg(errp, "vfu: Unable to set socket property - server busy");
+        return;
+    }
+
     qapi_free_SocketAddress(o->socket);
 
     o->socket = NULL;
@@ -122,17 +136,69 @@ static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name,
     }
 
     trace_vfu_prop("socket", o->socket->u.q_unix.path);
+
+    vfu_object_init_ctx(o, errp);
 }
 
 static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
 {
     VfuObject *o = VFU_OBJECT(obj);
 
+    if (o->vfu_ctx) {
+        error_setg(errp, "vfu: Unable to set device property - server busy");
+        return;
+    }
+
     g_free(o->device);
 
     o->device = g_strdup(str);
 
     trace_vfu_prop("device", str);
+
+    vfu_object_init_ctx(o, errp);
+}
+
+/*
+ * TYPE_VFU_OBJECT depends on the availability of the 'socket' and 'device'
+ * properties. It also depends on devices instantiated in QEMU. These
+ * dependencies are not available during the instance_init phase of this
+ * object's life-cycle. As such, the server is initialized after the
+ * machine is setup. machine_init_done_notifier notifies TYPE_VFU_OBJECT
+ * when the machine is setup, and the dependencies are available.
+ */
+static void vfu_object_machine_done(Notifier *notifier, void *data)
+{
+    VfuObject *o = container_of(notifier, VfuObject, machine_done);
+    Error *err = NULL;
+
+    vfu_object_init_ctx(o, &err);
+
+    if (err) {
+        error_propagate(&error_abort, err);
+    }
+}
+
+static void vfu_object_init_ctx(VfuObject *o, Error **errp)
+{
+    ERRP_GUARD();
+
+    if (o->vfu_ctx || !o->socket || !o->device ||
+            !phase_check(PHASE_MACHINE_READY)) {
+        return;
+    }
+
+    if (o->err) {
+        error_propagate(errp, o->err);
+        o->err = NULL;
+        return;
+    }
+
+    o->vfu_ctx = vfu_create_ctx(VFU_TRANS_SOCK, o->socket->u.q_unix.path, 0,
+                                o, VFU_DEV_TYPE_PCI);
+    if (o->vfu_ctx == NULL) {
+        error_setg(errp, "vfu: Failed to create context - %s", strerror(errno));
+        return;
+    }
 }
 
 static void vfu_object_init(Object *obj)
@@ -147,6 +213,12 @@ static void vfu_object_init(Object *obj)
                    TYPE_VFU_OBJECT, TYPE_REMOTE_MACHINE);
         return;
     }
+
+    if (!phase_check(PHASE_MACHINE_READY)) {
+        o->machine_done.notify = vfu_object_machine_done;
+        qemu_add_machine_init_done_notifier(&o->machine_done);
+    }
+
 }
 
 static void vfu_object_finalize(Object *obj)
@@ -160,6 +232,11 @@ static void vfu_object_finalize(Object *obj)
 
     o->socket = NULL;
 
+    if (o->vfu_ctx) {
+        vfu_destroy_ctx(o->vfu_ctx);
+        o->vfu_ctx = NULL;
+    }
+
     g_free(o->device);
 
     o->device = NULL;
@@ -167,6 +244,11 @@ static void vfu_object_finalize(Object *obj)
     if (!k->nr_devs && vfu_object_auto_shutdown()) {
         qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
     }
+
+    if (o->machine_done.notify) {
+        qemu_remove_machine_init_done_notifier(&o->machine_done);
+        o->machine_done.notify = NULL;
+    }
 }
 
 static void vfu_object_class_init(ObjectClass *klass, void *data)
-- 
2.20.1

[PATCH v9 09/17] vfio-user: find and init PCI device

[Jagannathan Raman]

Find the PCI device with specified id. Initialize the device context
with the QEMU PCI device

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/remote/vfio-user-obj.c | 67 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index 68f8a9dfa9..3ca6aa2b45 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -43,6 +43,8 @@
 #include "qemu/notify.h"
 #include "sysemu/sysemu.h"
 #include "libvfio-user.h"
+#include "hw/qdev-core.h"
+#include "hw/pci/pci.h"
 
 #define TYPE_VFU_OBJECT "x-vfio-user-server"
 OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
@@ -80,6 +82,10 @@ struct VfuObject {
     Notifier machine_done;
 
     vfu_ctx_t *vfu_ctx;
+
+    PCIDevice *pci_dev;
+
+    Error *unplug_blocker;
 };
 
 static void vfu_object_init_ctx(VfuObject *o, Error **errp);
@@ -181,6 +187,9 @@ static void vfu_object_machine_done(Notifier *notifier, void *data)
 static void vfu_object_init_ctx(VfuObject *o, Error **errp)
 {
     ERRP_GUARD();
+    DeviceState *dev = NULL;
+    vfu_pci_type_t pci_type = VFU_PCI_TYPE_CONVENTIONAL;
+    int ret;
 
     if (o->vfu_ctx || !o->socket || !o->device ||
             !phase_check(PHASE_MACHINE_READY)) {
@@ -199,6 +208,53 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
         error_setg(errp, "vfu: Failed to create context - %s", strerror(errno));
         return;
     }
+
+    dev = qdev_find_recursive(sysbus_get_default(), o->device);
+    if (dev == NULL) {
+        error_setg(errp, "vfu: Device %s not found", o->device);
+        goto fail;
+    }
+
+    if (!object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        error_setg(errp, "vfu: %s not a PCI device", o->device);
+        goto fail;
+    }
+
+    o->pci_dev = PCI_DEVICE(dev);
+
+    object_ref(OBJECT(o->pci_dev));
+
+    if (pci_is_express(o->pci_dev)) {
+        pci_type = VFU_PCI_TYPE_EXPRESS;
+    }
+
+    ret = vfu_pci_init(o->vfu_ctx, pci_type, PCI_HEADER_TYPE_NORMAL, 0);
+    if (ret < 0) {
+        error_setg(errp,
+                   "vfu: Failed to attach PCI device %s to context - %s",
+                   o->device, strerror(errno));
+        goto fail;
+    }
+
+    error_setg(&o->unplug_blocker,
+               "vfu: %s for %s must be deleted before unplugging",
+               TYPE_VFU_OBJECT, o->device);
+    qdev_add_unplug_blocker(DEVICE(o->pci_dev), o->unplug_blocker);
+
+    return;
+
+fail:
+    vfu_destroy_ctx(o->vfu_ctx);
+    if (o->unplug_blocker && o->pci_dev) {
+        qdev_del_unplug_blocker(DEVICE(o->pci_dev), o->unplug_blocker);
+        error_free(o->unplug_blocker);
+        o->unplug_blocker = NULL;
+    }
+    if (o->pci_dev) {
+        object_unref(OBJECT(o->pci_dev));
+        o->pci_dev = NULL;
+    }
+    o->vfu_ctx = NULL;
 }
 
 static void vfu_object_init(Object *obj)
@@ -241,6 +297,17 @@ static void vfu_object_finalize(Object *obj)
 
     o->device = NULL;
 
+    if (o->unplug_blocker && o->pci_dev) {
+        qdev_del_unplug_blocker(DEVICE(o->pci_dev), o->unplug_blocker);
+        error_free(o->unplug_blocker);
+        o->unplug_blocker = NULL;
+    }
+
+    if (o->pci_dev) {
+        object_unref(OBJECT(o->pci_dev));
+        o->pci_dev = NULL;
+    }
+
     if (!k->nr_devs && vfu_object_auto_shutdown()) {
         qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
     }
-- 
2.20.1

[PATCH v9 10/17] vfio-user: run vfio-user context

[Jagannathan Raman]

Setup a handler to run vfio-user context. The context is driven by
messages to the file descriptor associated with it - get the fd for
the context and hook up the handler with it

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qapi/misc.json            |  30 +++++++++++
 hw/remote/vfio-user-obj.c | 102 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 131 insertions(+), 1 deletion(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index b83cc39029..fa49f2876a 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -553,3 +553,33 @@
 ##
 { 'event': 'RTC_CHANGE',
   'data': { 'offset': 'int', 'qom-path': 'str' } }
+
+##
+# @VFU_CLIENT_HANGUP:
+#
+# Emitted when the client of a TYPE_VFIO_USER_SERVER closes the
+# communication channel
+#
+# @vfu-id: ID of the TYPE_VFIO_USER_SERVER object
+#
+# @vfu-qom-path: path to the TYPE_VFIO_USER_SERVER object in the QOM tree
+#
+# @dev-id: ID of attached PCI device
+#
+# @dev-qom-path: path to attached PCI device in the QOM tree
+#
+# Since: 7.1
+#
+# Example:
+#
+# <- { "event": "VFU_CLIENT_HANGUP",
+#      "data": { "vfu-id": "vfu1",
+#                "vfu-qom-path": "/objects/vfu1",
+#                "dev-id": "sas1",
+#                "dev-qom-path": "/machine/peripheral/sas1" },
+#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+#
+##
+{ 'event': 'VFU_CLIENT_HANGUP',
+  'data': { 'vfu-id': 'str', 'vfu-qom-path': 'str',
+            'dev-id': 'str', 'dev-qom-path': 'str' } }
diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index 3ca6aa2b45..3a4c6a9fa0 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -27,6 +27,9 @@
  *
  * device - id of a device on the server, a required option. PCI devices
  *          alone are supported presently.
+ *
+ * notes - x-vfio-user-server could block IO and monitor during the
+ *         initialization phase.
  */
 
 #include "qemu/osdep.h"
@@ -40,11 +43,14 @@
 #include "hw/remote/machine.h"
 #include "qapi/error.h"
 #include "qapi/qapi-visit-sockets.h"
+#include "qapi/qapi-events-misc.h"
 #include "qemu/notify.h"
+#include "qemu/thread.h"
 #include "sysemu/sysemu.h"
 #include "libvfio-user.h"
 #include "hw/qdev-core.h"
 #include "hw/pci/pci.h"
+#include "qemu/timer.h"
 
 #define TYPE_VFU_OBJECT "x-vfio-user-server"
 OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
@@ -86,6 +92,8 @@ struct VfuObject {
     PCIDevice *pci_dev;
 
     Error *unplug_blocker;
+
+    int vfu_poll_fd;
 };
 
 static void vfu_object_init_ctx(VfuObject *o, Error **errp);
@@ -164,6 +172,76 @@ static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
     vfu_object_init_ctx(o, errp);
 }
 
+static void vfu_object_ctx_run(void *opaque)
+{
+    VfuObject *o = opaque;
+    const char *vfu_id;
+    char *vfu_path, *pci_dev_path;
+    int ret = -1;
+
+    while (ret != 0) {
+        ret = vfu_run_ctx(o->vfu_ctx);
+        if (ret < 0) {
+            if (errno == EINTR) {
+                continue;
+            } else if (errno == ENOTCONN) {
+                vfu_id = object_get_canonical_path_component(OBJECT(o));
+                vfu_path = object_get_canonical_path(OBJECT(o));
+                g_assert(o->pci_dev);
+                pci_dev_path = object_get_canonical_path(OBJECT(o->pci_dev));
+                qapi_event_send_vfu_client_hangup(vfu_id, vfu_path,
+                                                  o->device, pci_dev_path);
+                qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL);
+                o->vfu_poll_fd = -1;
+                object_unparent(OBJECT(o));
+                g_free(vfu_path);
+                g_free(pci_dev_path);
+                break;
+            } else {
+                VFU_OBJECT_ERROR(o, "vfu: Failed to run device %s - %s",
+                                 o->device, strerror(errno));
+                break;
+            }
+        }
+    }
+}
+
+static void vfu_object_attach_ctx(void *opaque)
+{
+    VfuObject *o = opaque;
+    GPollFD pfds[1];
+    int ret;
+
+    qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL);
+
+    pfds[0].fd = o->vfu_poll_fd;
+    pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
+
+retry_attach:
+    ret = vfu_attach_ctx(o->vfu_ctx);
+    if (ret < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) {
+        /**
+         * vfu_object_attach_ctx can block QEMU's main loop
+         * during attach - the monitor and other IO
+         * could be unresponsive during this time.
+         */
+        (void)qemu_poll_ns(pfds, 1, 500 * (int64_t)SCALE_MS);
+        goto retry_attach;
+    } else if (ret < 0) {
+        VFU_OBJECT_ERROR(o, "vfu: Failed to attach device %s to context - %s",
+                         o->device, strerror(errno));
+        return;
+    }
+
+    o->vfu_poll_fd = vfu_get_poll_fd(o->vfu_ctx);
+    if (o->vfu_poll_fd < 0) {
+        VFU_OBJECT_ERROR(o, "vfu: Failed to get poll fd %s", o->device);
+        return;
+    }
+
+    qemu_set_fd_handler(o->vfu_poll_fd, vfu_object_ctx_run, NULL, o);
+}
+
 /*
  * TYPE_VFU_OBJECT depends on the availability of the 'socket' and 'device'
  * properties. It also depends on devices instantiated in QEMU. These
@@ -202,7 +280,8 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
         return;
     }
 
-    o->vfu_ctx = vfu_create_ctx(VFU_TRANS_SOCK, o->socket->u.q_unix.path, 0,
+    o->vfu_ctx = vfu_create_ctx(VFU_TRANS_SOCK, o->socket->u.q_unix.path,
+                                LIBVFIO_USER_FLAG_ATTACH_NB,
                                 o, VFU_DEV_TYPE_PCI);
     if (o->vfu_ctx == NULL) {
         error_setg(errp, "vfu: Failed to create context - %s", strerror(errno));
@@ -241,6 +320,21 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
                TYPE_VFU_OBJECT, o->device);
     qdev_add_unplug_blocker(DEVICE(o->pci_dev), o->unplug_blocker);
 
+    ret = vfu_realize_ctx(o->vfu_ctx);
+    if (ret < 0) {
+        error_setg(errp, "vfu: Failed to realize device %s- %s",
+                   o->device, strerror(errno));
+        goto fail;
+    }
+
+    o->vfu_poll_fd = vfu_get_poll_fd(o->vfu_ctx);
+    if (o->vfu_poll_fd < 0) {
+        error_setg(errp, "vfu: Failed to get poll fd %s", o->device);
+        goto fail;
+    }
+
+    qemu_set_fd_handler(o->vfu_poll_fd, vfu_object_attach_ctx, NULL, o);
+
     return;
 
 fail:
@@ -275,6 +369,7 @@ static void vfu_object_init(Object *obj)
         qemu_add_machine_init_done_notifier(&o->machine_done);
     }
 
+    o->vfu_poll_fd = -1;
 }
 
 static void vfu_object_finalize(Object *obj)
@@ -288,6 +383,11 @@ static void vfu_object_finalize(Object *obj)
 
     o->socket = NULL;
 
+    if (o->vfu_poll_fd != -1) {
+        qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL);
+        o->vfu_poll_fd = -1;
+    }
+
     if (o->vfu_ctx) {
         vfu_destroy_ctx(o->vfu_ctx);
         o->vfu_ctx = NULL;
-- 
2.20.1

[PATCH v9 11/17] vfio-user: handle PCI config space accesses

[Jagannathan Raman]

Define and register handlers for PCI config space accesses

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/remote/vfio-user-obj.c | 51 +++++++++++++++++++++++++++++++++++++++
 hw/remote/trace-events    |  2 ++
 2 files changed, 53 insertions(+)

diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index 3a4c6a9fa0..c81a76094c 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -46,6 +46,7 @@
 #include "qapi/qapi-events-misc.h"
 #include "qemu/notify.h"
 #include "qemu/thread.h"
+#include "qemu/main-loop.h"
 #include "sysemu/sysemu.h"
 #include "libvfio-user.h"
 #include "hw/qdev-core.h"
@@ -242,6 +243,45 @@ retry_attach:
     qemu_set_fd_handler(o->vfu_poll_fd, vfu_object_ctx_run, NULL, o);
 }
 
+static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx, char * const buf,
+                                     size_t count, loff_t offset,
+                                     const bool is_write)
+{
+    VfuObject *o = vfu_get_private(vfu_ctx);
+    uint32_t pci_access_width = sizeof(uint32_t);
+    size_t bytes = count;
+    uint32_t val = 0;
+    char *ptr = buf;
+    int len;
+
+    /*
+     * Writes to the BAR registers would trigger an update to the
+     * global Memory and IO AddressSpaces. But the remote device
+     * never uses the global AddressSpaces, therefore overlapping
+     * memory regions are not a problem
+     */
+    while (bytes > 0) {
+        len = (bytes > pci_access_width) ? pci_access_width : bytes;
+        if (is_write) {
+            memcpy(&val, ptr, len);
+            pci_host_config_write_common(o->pci_dev, offset,
+                                         pci_config_size(o->pci_dev),
+                                         val, len);
+            trace_vfu_cfg_write(offset, val);
+        } else {
+            val = pci_host_config_read_common(o->pci_dev, offset,
+                                              pci_config_size(o->pci_dev), len);
+            memcpy(ptr, &val, len);
+            trace_vfu_cfg_read(offset, val);
+        }
+        offset += len;
+        ptr += len;
+        bytes -= len;
+    }
+
+    return count;
+}
+
 /*
  * TYPE_VFU_OBJECT depends on the availability of the 'socket' and 'device'
  * properties. It also depends on devices instantiated in QEMU. These
@@ -320,6 +360,17 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
                TYPE_VFU_OBJECT, o->device);
     qdev_add_unplug_blocker(DEVICE(o->pci_dev), o->unplug_blocker);
 
+    ret = vfu_setup_region(o->vfu_ctx, VFU_PCI_DEV_CFG_REGION_IDX,
+                           pci_config_size(o->pci_dev), &vfu_object_cfg_access,
+                           VFU_REGION_FLAG_RW | VFU_REGION_FLAG_ALWAYS_CB,
+                           NULL, 0, -1, 0);
+    if (ret < 0) {
+        error_setg(errp,
+                   "vfu: Failed to setup config space handlers for %s- %s",
+                   o->device, strerror(errno));
+        goto fail;
+    }
+
     ret = vfu_realize_ctx(o->vfu_ctx);
     if (ret < 0) {
         error_setg(errp, "vfu: Failed to realize device %s- %s",
diff --git a/hw/remote/trace-events b/hw/remote/trace-events
index 7da12f0d96..2ef7884346 100644
--- a/hw/remote/trace-events
+++ b/hw/remote/trace-events
@@ -5,3 +5,5 @@ mpqemu_recv_io_error(int cmd, int size, int nfds) "failed to receive %d size %d,
 
 # vfio-user-obj.c
 vfu_prop(const char *prop, const char *val) "vfu: setting %s as %s"
+vfu_cfg_read(uint32_t offset, uint32_t val) "vfu: cfg: 0x%u -> 0x%x"
+vfu_cfg_write(uint32_t offset, uint32_t val) "vfu: cfg: 0x%u <- 0x%x"
-- 
2.20.1

[PATCH v9 12/17] vfio-user: IOMMU support for remote device

[Jagannathan Raman]

Assign separate address space for each device in the remote processes.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
---
 include/hw/remote/iommu.h |  40 ++++++++++++
 hw/remote/iommu.c         | 131 ++++++++++++++++++++++++++++++++++++++
 hw/remote/machine.c       |  13 +++-
 MAINTAINERS               |   2 +
 hw/remote/meson.build     |   1 +
 5 files changed, 186 insertions(+), 1 deletion(-)
 create mode 100644 include/hw/remote/iommu.h
 create mode 100644 hw/remote/iommu.c

diff --git a/include/hw/remote/iommu.h b/include/hw/remote/iommu.h
new file mode 100644
index 0000000000..33b68a8f4b
--- /dev/null
+++ b/include/hw/remote/iommu.h
@@ -0,0 +1,40 @@
+/**
+ * Copyright © 2022 Oracle and/or its affiliates.
+ *
+ * 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 REMOTE_IOMMU_H
+#define REMOTE_IOMMU_H
+
+#include "hw/pci/pci_bus.h"
+#include "hw/pci/pci.h"
+
+#ifndef INT2VOIDP
+#define INT2VOIDP(i) (void *)(uintptr_t)(i)
+#endif
+
+typedef struct RemoteIommuElem {
+    MemoryRegion *mr;
+
+    AddressSpace as;
+} RemoteIommuElem;
+
+#define TYPE_REMOTE_IOMMU "x-remote-iommu"
+OBJECT_DECLARE_SIMPLE_TYPE(RemoteIommu, REMOTE_IOMMU)
+
+struct RemoteIommu {
+    Object parent;
+
+    GHashTable *elem_by_devfn;
+
+    QemuMutex lock;
+};
+
+void remote_iommu_setup(PCIBus *pci_bus);
+
+void remote_iommu_unplug_dev(PCIDevice *pci_dev);
+
+#endif
diff --git a/hw/remote/iommu.c b/hw/remote/iommu.c
new file mode 100644
index 0000000000..fd723d91f3
--- /dev/null
+++ b/hw/remote/iommu.c
@@ -0,0 +1,131 @@
+/**
+ * IOMMU for remote device
+ *
+ * Copyright © 2022 Oracle and/or its affiliates.
+ *
+ * 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 "hw/remote/iommu.h"
+#include "hw/pci/pci_bus.h"
+#include "hw/pci/pci.h"
+#include "exec/memory.h"
+#include "exec/address-spaces.h"
+#include "trace.h"
+
+/**
+ * IOMMU for TYPE_REMOTE_MACHINE - manages DMA address space isolation
+ *     for remote machine. It is used by TYPE_VFIO_USER_SERVER.
+ *
+ * - Each TYPE_VFIO_USER_SERVER instance handles one PCIDevice on a PCIBus.
+ *   There is one RemoteIommu per PCIBus, so the RemoteIommu tracks multiple
+ *   PCIDevices by maintaining a ->elem_by_devfn mapping.
+ *
+ * - memory_region_init_iommu() is not used because vfio-user MemoryRegions
+ *   will be added to the elem->mr container instead. This is more natural
+ *   than implementing the IOMMUMemoryRegionClass APIs since vfio-user
+ *   provides something that is close to a full-fledged MemoryRegion and
+ *   not like an IOMMU mapping.
+ *
+ * - When a device is hot unplugged, the elem->mr reference is dropped so
+ *   all vfio-user MemoryRegions associated with this vfio-user server are
+ *   destroyed.
+ */
+
+static AddressSpace *remote_iommu_find_add_as(PCIBus *pci_bus,
+                                              void *opaque, int devfn)
+{
+    RemoteIommu *iommu = opaque;
+    RemoteIommuElem *elem = NULL;
+
+    qemu_mutex_lock(&iommu->lock);
+
+    elem = g_hash_table_lookup(iommu->elem_by_devfn, INT2VOIDP(devfn));
+
+    if (!elem) {
+        elem = g_malloc0(sizeof(RemoteIommuElem));
+        g_hash_table_insert(iommu->elem_by_devfn, INT2VOIDP(devfn), elem);
+    }
+
+    if (!elem->mr) {
+        elem->mr = MEMORY_REGION(object_new(TYPE_MEMORY_REGION));
+        memory_region_set_size(elem->mr, UINT64_MAX);
+        address_space_init(&elem->as, elem->mr, NULL);
+    }
+
+    qemu_mutex_unlock(&iommu->lock);
+
+    return &elem->as;
+}
+
+void remote_iommu_unplug_dev(PCIDevice *pci_dev)
+{
+    AddressSpace *as = pci_device_iommu_address_space(pci_dev);
+    RemoteIommuElem *elem = NULL;
+
+    if (as == &address_space_memory) {
+        return;
+    }
+
+    elem = container_of(as, RemoteIommuElem, as);
+
+    address_space_destroy(&elem->as);
+
+    object_unref(elem->mr);
+
+    elem->mr = NULL;
+}
+
+static void remote_iommu_init(Object *obj)
+{
+    RemoteIommu *iommu = REMOTE_IOMMU(obj);
+
+    iommu->elem_by_devfn = g_hash_table_new_full(NULL, NULL, NULL, g_free);
+
+    qemu_mutex_init(&iommu->lock);
+}
+
+static void remote_iommu_finalize(Object *obj)
+{
+    RemoteIommu *iommu = REMOTE_IOMMU(obj);
+
+    qemu_mutex_destroy(&iommu->lock);
+
+    g_hash_table_destroy(iommu->elem_by_devfn);
+
+    iommu->elem_by_devfn = NULL;
+}
+
+void remote_iommu_setup(PCIBus *pci_bus)
+{
+    RemoteIommu *iommu = NULL;
+
+    g_assert(pci_bus);
+
+    iommu = REMOTE_IOMMU(object_new(TYPE_REMOTE_IOMMU));
+
+    pci_setup_iommu(pci_bus, remote_iommu_find_add_as, iommu);
+
+    object_property_add_child(OBJECT(pci_bus), "remote-iommu", OBJECT(iommu));
+
+    object_unref(OBJECT(iommu));
+}
+
+static const TypeInfo remote_iommu_info = {
+    .name = TYPE_REMOTE_IOMMU,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(RemoteIommu),
+    .instance_init = remote_iommu_init,
+    .instance_finalize = remote_iommu_finalize,
+};
+
+static void remote_iommu_register_types(void)
+{
+    type_register_static(&remote_iommu_info);
+}
+
+type_init(remote_iommu_register_types)
diff --git a/hw/remote/machine.c b/hw/remote/machine.c
index 4d008ed721..cbb2add291 100644
--- a/hw/remote/machine.c
+++ b/hw/remote/machine.c
@@ -20,6 +20,7 @@
 #include "qapi/error.h"
 #include "hw/pci/pci_host.h"
 #include "hw/remote/iohub.h"
+#include "hw/remote/iommu.h"
 #include "hw/qdev-core.h"
 
 static void remote_machine_init(MachineState *machine)
@@ -99,6 +100,16 @@ static void remote_machine_instance_init(Object *obj)
     s->auto_shutdown = true;
 }
 
+static void remote_machine_dev_unplug_cb(HotplugHandler *hotplug_dev,
+                                         DeviceState *dev, Error **errp)
+{
+    qdev_unrealize(dev);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        remote_iommu_unplug_dev(PCI_DEVICE(dev));
+    }
+}
+
 static void remote_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -107,7 +118,7 @@ static void remote_machine_class_init(ObjectClass *oc, void *data)
     mc->init = remote_machine_init;
     mc->desc = "Experimental remote machine";
 
-    hc->unplug = qdev_simple_device_unplug_cb;
+    hc->unplug = remote_machine_dev_unplug_cb;
 
     object_class_property_add_bool(oc, "vfio-user",
                                    remote_machine_get_vfio_user,
diff --git a/MAINTAINERS b/MAINTAINERS
index 1b5719cd89..f257afcf8a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3600,6 +3600,8 @@ F: hw/remote/iohub.c
 F: include/hw/remote/iohub.h
 F: subprojects/libvfio-user
 F: hw/remote/vfio-user-obj.c
+F: hw/remote/iommu.c
+F: include/hw/remote/iommu.h
 
 EBPF:
 M: Jason Wang <jasowang@redhat.com>
diff --git a/hw/remote/meson.build b/hw/remote/meson.build
index 534ac5df79..bcef83c8cc 100644
--- a/hw/remote/meson.build
+++ b/hw/remote/meson.build
@@ -6,6 +6,7 @@ remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('message.c'))
 remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('remote-obj.c'))
 remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('proxy.c'))
 remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('iohub.c'))
+remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('iommu.c'))
 remote_ss.add(when: 'CONFIG_VFIO_USER_SERVER', if_true: files('vfio-user-obj.c'))
 
 remote_ss.add(when: 'CONFIG_VFIO_USER_SERVER', if_true: vfiouser)
-- 
2.20.1

[PATCH v9 13/17] vfio-user: handle DMA mappings

[Jagannathan Raman]

Define and register callbacks to manage the RAM regions used for
device DMA

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/remote/machine.c       |  5 ++++
 hw/remote/vfio-user-obj.c | 55 +++++++++++++++++++++++++++++++++++++++
 hw/remote/trace-events    |  2 ++
 3 files changed, 62 insertions(+)

diff --git a/hw/remote/machine.c b/hw/remote/machine.c
index cbb2add291..645b54343d 100644
--- a/hw/remote/machine.c
+++ b/hw/remote/machine.c
@@ -22,6 +22,7 @@
 #include "hw/remote/iohub.h"
 #include "hw/remote/iommu.h"
 #include "hw/qdev-core.h"
+#include "hw/remote/iommu.h"
 
 static void remote_machine_init(MachineState *machine)
 {
@@ -51,6 +52,10 @@ static void remote_machine_init(MachineState *machine)
 
     pci_host = PCI_HOST_BRIDGE(rem_host);
 
+    if (s->vfio_user) {
+        remote_iommu_setup(pci_host->bus);
+    }
+
     remote_iohub_init(&s->iohub);
 
     pci_bus_irqs(pci_host->bus, remote_iohub_set_irq, remote_iohub_map_irq,
diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index c81a76094c..736339c74a 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -282,6 +282,54 @@ static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx, char * const buf,
     return count;
 }
 
+static void dma_register(vfu_ctx_t *vfu_ctx, vfu_dma_info_t *info)
+{
+    VfuObject *o = vfu_get_private(vfu_ctx);
+    AddressSpace *dma_as = NULL;
+    MemoryRegion *subregion = NULL;
+    g_autofree char *name = NULL;
+    struct iovec *iov = &info->iova;
+
+    if (!info->vaddr) {
+        return;
+    }
+
+    name = g_strdup_printf("mem-%s-%"PRIx64"", o->device,
+                           (uint64_t)info->vaddr);
+
+    subregion = g_new0(MemoryRegion, 1);
+
+    memory_region_init_ram_ptr(subregion, NULL, name,
+                               iov->iov_len, info->vaddr);
+
+    dma_as = pci_device_iommu_address_space(o->pci_dev);
+
+    memory_region_add_subregion(dma_as->root, (hwaddr)iov->iov_base, subregion);
+
+    trace_vfu_dma_register((uint64_t)iov->iov_base, iov->iov_len);
+}
+
+static void dma_unregister(vfu_ctx_t *vfu_ctx, vfu_dma_info_t *info)
+{
+    VfuObject *o = vfu_get_private(vfu_ctx);
+    AddressSpace *dma_as = NULL;
+    MemoryRegion *mr = NULL;
+    ram_addr_t offset;
+
+    mr = memory_region_from_host(info->vaddr, &offset);
+    if (!mr) {
+        return;
+    }
+
+    dma_as = pci_device_iommu_address_space(o->pci_dev);
+
+    memory_region_del_subregion(dma_as->root, mr);
+
+    object_unparent((OBJECT(mr)));
+
+    trace_vfu_dma_unregister((uint64_t)info->iova.iov_base);
+}
+
 /*
  * TYPE_VFU_OBJECT depends on the availability of the 'socket' and 'device'
  * properties. It also depends on devices instantiated in QEMU. These
@@ -371,6 +419,13 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
         goto fail;
     }
 
+    ret = vfu_setup_device_dma(o->vfu_ctx, &dma_register, &dma_unregister);
+    if (ret < 0) {
+        error_setg(errp, "vfu: Failed to setup DMA handlers for %s",
+                   o->device);
+        goto fail;
+    }
+
     ret = vfu_realize_ctx(o->vfu_ctx);
     if (ret < 0) {
         error_setg(errp, "vfu: Failed to realize device %s- %s",
diff --git a/hw/remote/trace-events b/hw/remote/trace-events
index 2ef7884346..f945c7e33b 100644
--- a/hw/remote/trace-events
+++ b/hw/remote/trace-events
@@ -7,3 +7,5 @@ mpqemu_recv_io_error(int cmd, int size, int nfds) "failed to receive %d size %d,
 vfu_prop(const char *prop, const char *val) "vfu: setting %s as %s"
 vfu_cfg_read(uint32_t offset, uint32_t val) "vfu: cfg: 0x%u -> 0x%x"
 vfu_cfg_write(uint32_t offset, uint32_t val) "vfu: cfg: 0x%u <- 0x%x"
+vfu_dma_register(uint64_t gpa, size_t len) "vfu: registering GPA 0x%"PRIx64", %zu bytes"
+vfu_dma_unregister(uint64_t gpa) "vfu: unregistering GPA 0x%"PRIx64""
-- 
2.20.1

[PATCH v9 14/17] vfio-user: handle PCI BAR accesses

[Jagannathan Raman]

Determine the BARs used by the PCI device and register handlers to
manage the access to the same.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
---
 include/exec/memory.h           |   3 +
 hw/remote/vfio-user-obj.c       | 190 ++++++++++++++++++++++++++++++++
 softmmu/physmem.c               |   4 +-
 tests/qtest/fuzz/generic_fuzz.c |   9 +-
 hw/remote/trace-events          |   3 +
 5 files changed, 203 insertions(+), 6 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index f1c19451bc..a6a0f4d8ad 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2810,6 +2810,9 @@ MemTxResult address_space_write_cached_slow(MemoryRegionCache *cache,
                                             hwaddr addr, const void *buf,
                                             hwaddr len);
 
+int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr);
+bool prepare_mmio_access(MemoryRegion *mr);
+
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
     if (is_write) {
diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index 736339c74a..f5ca909e68 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -52,6 +52,7 @@
 #include "hw/qdev-core.h"
 #include "hw/pci/pci.h"
 #include "qemu/timer.h"
+#include "exec/memory.h"
 
 #define TYPE_VFU_OBJECT "x-vfio-user-server"
 OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
@@ -330,6 +331,193 @@ static void dma_unregister(vfu_ctx_t *vfu_ctx, vfu_dma_info_t *info)
     trace_vfu_dma_unregister((uint64_t)info->iova.iov_base);
 }
 
+static int vfu_object_mr_rw(MemoryRegion *mr, uint8_t *buf, hwaddr offset,
+                            hwaddr size, const bool is_write)
+{
+    uint8_t *ptr = buf;
+    bool release_lock = false;
+    uint8_t *ram_ptr = NULL;
+    MemTxResult result;
+    int access_size;
+    uint64_t val;
+
+    if (memory_access_is_direct(mr, is_write)) {
+        /**
+         * Some devices expose a PCI expansion ROM, which could be buffer
+         * based as compared to other regions which are primarily based on
+         * MemoryRegionOps. memory_region_find() would already check
+         * for buffer overflow, we don't need to repeat it here.
+         */
+        ram_ptr = memory_region_get_ram_ptr(mr);
+
+        if (is_write) {
+            memcpy((ram_ptr + offset), buf, size);
+        } else {
+            memcpy(buf, (ram_ptr + offset), size);
+        }
+
+        return 0;
+    }
+
+    while (size) {
+        /**
+         * The read/write logic used below is similar to the ones in
+         * flatview_read/write_continue()
+         */
+        release_lock = prepare_mmio_access(mr);
+
+        access_size = memory_access_size(mr, size, offset);
+
+        if (is_write) {
+            val = ldn_he_p(ptr, access_size);
+
+            result = memory_region_dispatch_write(mr, offset, val,
+                                                  size_memop(access_size),
+                                                  MEMTXATTRS_UNSPECIFIED);
+        } else {
+            result = memory_region_dispatch_read(mr, offset, &val,
+                                                 size_memop(access_size),
+                                                 MEMTXATTRS_UNSPECIFIED);
+
+            stn_he_p(ptr, access_size, val);
+        }
+
+        if (release_lock) {
+            qemu_mutex_unlock_iothread();
+            release_lock = false;
+        }
+
+        if (result != MEMTX_OK) {
+            return -1;
+        }
+
+        size -= access_size;
+        ptr += access_size;
+        offset += access_size;
+    }
+
+    return 0;
+}
+
+static size_t vfu_object_bar_rw(PCIDevice *pci_dev, int pci_bar,
+                                hwaddr bar_offset, char * const buf,
+                                hwaddr len, const bool is_write)
+{
+    MemoryRegionSection section = { 0 };
+    uint8_t *ptr = (uint8_t *)buf;
+    MemoryRegion *section_mr = NULL;
+    uint64_t section_size;
+    hwaddr section_offset;
+    hwaddr size = 0;
+
+    while (len) {
+        section = memory_region_find(pci_dev->io_regions[pci_bar].memory,
+                                     bar_offset, len);
+
+        if (!section.mr) {
+            warn_report("vfu: invalid address 0x%"PRIx64"", bar_offset);
+            return size;
+        }
+
+        section_mr = section.mr;
+        section_offset = section.offset_within_region;
+        section_size = int128_get64(section.size);
+
+        if (is_write && section_mr->readonly) {
+            warn_report("vfu: attempting to write to readonly region in "
+                        "bar %d - [0x%"PRIx64" - 0x%"PRIx64"]",
+                        pci_bar, bar_offset,
+                        (bar_offset + section_size));
+            memory_region_unref(section_mr);
+            return size;
+        }
+
+        if (vfu_object_mr_rw(section_mr, ptr, section_offset,
+                             section_size, is_write)) {
+            warn_report("vfu: failed to %s "
+                        "[0x%"PRIx64" - 0x%"PRIx64"] in bar %d",
+                        is_write ? "write to" : "read from", bar_offset,
+                        (bar_offset + section_size), pci_bar);
+            memory_region_unref(section_mr);
+            return size;
+        }
+
+        size += section_size;
+        bar_offset += section_size;
+        ptr += section_size;
+        len -= section_size;
+
+        memory_region_unref(section_mr);
+    }
+
+    return size;
+}
+
+/**
+ * VFU_OBJECT_BAR_HANDLER - macro for defining handlers for PCI BARs.
+ *
+ * To create handler for BAR number 2, VFU_OBJECT_BAR_HANDLER(2) would
+ * define vfu_object_bar2_handler
+ */
+#define VFU_OBJECT_BAR_HANDLER(BAR_NO)                                         \
+    static ssize_t vfu_object_bar##BAR_NO##_handler(vfu_ctx_t *vfu_ctx,        \
+                                        char * const buf, size_t count,        \
+                                        loff_t offset, const bool is_write)    \
+    {                                                                          \
+        VfuObject *o = vfu_get_private(vfu_ctx);                               \
+        PCIDevice *pci_dev = o->pci_dev;                                       \
+                                                                               \
+        return vfu_object_bar_rw(pci_dev, BAR_NO, offset,                      \
+                                 buf, count, is_write);                        \
+    }                                                                          \
+
+VFU_OBJECT_BAR_HANDLER(0)
+VFU_OBJECT_BAR_HANDLER(1)
+VFU_OBJECT_BAR_HANDLER(2)
+VFU_OBJECT_BAR_HANDLER(3)
+VFU_OBJECT_BAR_HANDLER(4)
+VFU_OBJECT_BAR_HANDLER(5)
+VFU_OBJECT_BAR_HANDLER(6)
+
+static vfu_region_access_cb_t *vfu_object_bar_handlers[PCI_NUM_REGIONS] = {
+    &vfu_object_bar0_handler,
+    &vfu_object_bar1_handler,
+    &vfu_object_bar2_handler,
+    &vfu_object_bar3_handler,
+    &vfu_object_bar4_handler,
+    &vfu_object_bar5_handler,
+    &vfu_object_bar6_handler,
+};
+
+/**
+ * vfu_object_register_bars - Identify active BAR regions of pdev and setup
+ *                            callbacks to handle read/write accesses
+ */
+static void vfu_object_register_bars(vfu_ctx_t *vfu_ctx, PCIDevice *pdev)
+{
+    int flags = VFU_REGION_FLAG_RW;
+    int i;
+
+    for (i = 0; i < PCI_NUM_REGIONS; i++) {
+        if (!pdev->io_regions[i].size) {
+            continue;
+        }
+
+        if ((i == VFU_PCI_DEV_ROM_REGION_IDX) ||
+            pdev->io_regions[i].memory->readonly) {
+            flags &= ~VFU_REGION_FLAG_WRITE;
+        }
+
+        vfu_setup_region(vfu_ctx, VFU_PCI_DEV_BAR0_REGION_IDX + i,
+                         (size_t)pdev->io_regions[i].size,
+                         vfu_object_bar_handlers[i],
+                         flags, NULL, 0, -1, 0);
+
+        trace_vfu_bar_register(i, pdev->io_regions[i].addr,
+                               pdev->io_regions[i].size);
+    }
+}
+
 /*
  * TYPE_VFU_OBJECT depends on the availability of the 'socket' and 'device'
  * properties. It also depends on devices instantiated in QEMU. These
@@ -426,6 +614,8 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
         goto fail;
     }
 
+    vfu_object_register_bars(o->vfu_ctx, o->pci_dev);
+
     ret = vfu_realize_ctx(o->vfu_ctx);
     if (ret < 0) {
         error_setg(errp, "vfu: Failed to realize device %s- %s",
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 657841eed0..fb16be57a6 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2719,7 +2719,7 @@ void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size)
     invalidate_and_set_dirty(mr, addr, size);
 }
 
-static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
+int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 {
     unsigned access_size_max = mr->ops->valid.max_access_size;
 
@@ -2746,7 +2746,7 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
     return l;
 }
 
-static bool prepare_mmio_access(MemoryRegion *mr)
+bool prepare_mmio_access(MemoryRegion *mr)
 {
     bool release_lock = false;
 
diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
index bce8360482..8cd33f6c7e 100644
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -144,7 +144,7 @@ static void *pattern_alloc(pattern p, size_t len)
     return buf;
 }
 
-static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
+static int fuzz_memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 {
     unsigned access_size_max = mr->ops->valid.max_access_size;
 
@@ -242,11 +242,12 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr)
 
         /*
          *  If mr1 isn't RAM, address_space_translate doesn't update l. Use
-         *  memory_access_size to identify the number of bytes that it is safe
-         *  to write without accidentally writing to another MemoryRegion.
+         *  fuzz_memory_access_size to identify the number of bytes that it
+         *  is safe to write without accidentally writing to another
+         *  MemoryRegion.
          */
         if (!memory_region_is_ram(mr1)) {
-            l = memory_access_size(mr1, l, addr1);
+            l = fuzz_memory_access_size(mr1, l, addr1);
         }
         if (memory_region_is_ram(mr1) ||
             memory_region_is_romd(mr1) ||
diff --git a/hw/remote/trace-events b/hw/remote/trace-events
index f945c7e33b..847d50d88f 100644
--- a/hw/remote/trace-events
+++ b/hw/remote/trace-events
@@ -9,3 +9,6 @@ vfu_cfg_read(uint32_t offset, uint32_t val) "vfu: cfg: 0x%u -> 0x%x"
 vfu_cfg_write(uint32_t offset, uint32_t val) "vfu: cfg: 0x%u <- 0x%x"
 vfu_dma_register(uint64_t gpa, size_t len) "vfu: registering GPA 0x%"PRIx64", %zu bytes"
 vfu_dma_unregister(uint64_t gpa) "vfu: unregistering GPA 0x%"PRIx64""
+vfu_bar_register(int i, uint64_t addr, uint64_t size) "vfu: BAR %d: addr 0x%"PRIx64" size 0x%"PRIx64""
+vfu_bar_rw_enter(const char *op, uint64_t addr) "vfu: %s request for BAR address 0x%"PRIx64""
+vfu_bar_rw_exit(const char *op, uint64_t addr) "vfu: Finished %s of BAR address 0x%"PRIx64""
-- 
2.20.1

[PATCH v9 15/17] vfio-user: handle device interrupts

[Jagannathan Raman]

Forward remote device's interrupts to the guest

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
---
 include/hw/pci/pci.h              |  13 ++++
 include/hw/remote/vfio-user-obj.h |   6 ++
 hw/pci/msi.c                      |  16 ++--
 hw/pci/msix.c                     |  10 ++-
 hw/pci/pci.c                      |  13 ++++
 hw/remote/machine.c               |  14 +++-
 hw/remote/vfio-user-obj.c         | 123 ++++++++++++++++++++++++++++++
 stubs/vfio-user-obj.c             |   6 ++
 MAINTAINERS                       |   1 +
 hw/remote/trace-events            |   1 +
 stubs/meson.build                 |   1 +
 11 files changed, 193 insertions(+), 11 deletions(-)
 create mode 100644 include/hw/remote/vfio-user-obj.h
 create mode 100644 stubs/vfio-user-obj.c

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 3a32b8dd40..7595c05c98 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -16,6 +16,7 @@ extern bool pci_available;
 #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
 #define PCI_FUNC(devfn)         ((devfn) & 0x07)
 #define PCI_BUILD_BDF(bus, devfn)     ((bus << 8) | (devfn))
+#define PCI_BDF_TO_DEVFN(x)     ((x) & 0xff)
 #define PCI_BUS_MAX             256
 #define PCI_DEVFN_MAX           256
 #define PCI_SLOT_MAX            32
@@ -127,6 +128,10 @@ typedef void PCIMapIORegionFunc(PCIDevice *pci_dev, int region_num,
                                 pcibus_t addr, pcibus_t size, int type);
 typedef void PCIUnregisterFunc(PCIDevice *pci_dev);
 
+typedef void MSITriggerFunc(PCIDevice *dev, MSIMessage msg);
+typedef MSIMessage MSIPrepareMessageFunc(PCIDevice *dev, unsigned vector);
+typedef MSIMessage MSIxPrepareMessageFunc(PCIDevice *dev, unsigned vector);
+
 typedef struct PCIIORegion {
     pcibus_t addr; /* current PCI mapping address. -1 means not mapped */
 #define PCI_BAR_UNMAPPED (~(pcibus_t)0)
@@ -321,6 +326,14 @@ struct PCIDevice {
     /* Space to store MSIX table & pending bit array */
     uint8_t *msix_table;
     uint8_t *msix_pba;
+
+    /* May be used by INTx or MSI during interrupt notification */
+    void *irq_opaque;
+
+    MSITriggerFunc *msi_trigger;
+    MSIPrepareMessageFunc *msi_prepare_message;
+    MSIxPrepareMessageFunc *msix_prepare_message;
+
     /* MemoryRegion container for msix exclusive BAR setup */
     MemoryRegion msix_exclusive_bar;
     /* Memory Regions for MSIX table and pending bit entries. */
diff --git a/include/hw/remote/vfio-user-obj.h b/include/hw/remote/vfio-user-obj.h
new file mode 100644
index 0000000000..87ab78b875
--- /dev/null
+++ b/include/hw/remote/vfio-user-obj.h
@@ -0,0 +1,6 @@
+#ifndef VFIO_USER_OBJ_H
+#define VFIO_USER_OBJ_H
+
+void vfu_object_set_bus_irq(PCIBus *pci_bus);
+
+#endif
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index 47d2b0f33c..d556e17a09 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -134,7 +134,7 @@ void msi_set_message(PCIDevice *dev, MSIMessage msg)
     pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
 }
 
-MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector)
+static MSIMessage msi_prepare_message(PCIDevice *dev, unsigned int vector)
 {
     uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
     bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
@@ -159,6 +159,11 @@ MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector)
     return msg;
 }
 
+MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector)
+{
+    return dev->msi_prepare_message(dev, vector);
+}
+
 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&
@@ -241,6 +246,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
                      0xffffffff >> (PCI_MSI_VECTORS_MAX - nr_vectors));
     }
 
+    dev->msi_prepare_message = msi_prepare_message;
+
     return 0;
 }
 
@@ -256,6 +263,7 @@ void msi_uninit(struct PCIDevice *dev)
     cap_size = msi_cap_sizeof(flags);
     pci_del_capability(dev, PCI_CAP_ID_MSI, cap_size);
     dev->cap_present &= ~QEMU_PCI_CAP_MSI;
+    dev->msi_prepare_message = NULL;
 
     MSI_DEV_PRINTF(dev, "uninit\n");
 }
@@ -334,11 +342,7 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
 
 void msi_send_message(PCIDevice *dev, MSIMessage msg)
 {
-    MemTxAttrs attrs = {};
-
-    attrs.requester_id = pci_requester_id(dev);
-    address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
-                         attrs, NULL);
+    dev->msi_trigger(dev, msg);
 }
 
 /* Normally called by pci_default_write_config(). */
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index ae9331cd0b..6f85192d6f 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -31,7 +31,7 @@
 #define MSIX_ENABLE_MASK (PCI_MSIX_FLAGS_ENABLE >> 8)
 #define MSIX_MASKALL_MASK (PCI_MSIX_FLAGS_MASKALL >> 8)
 
-MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
+static MSIMessage msix_prepare_message(PCIDevice *dev, unsigned vector)
 {
     uint8_t *table_entry = dev->msix_table + vector * PCI_MSIX_ENTRY_SIZE;
     MSIMessage msg;
@@ -41,6 +41,11 @@ MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
+{
+    return dev->msix_prepare_message(dev, vector);
+}
+
 /*
  * Special API for POWER to configure the vectors through
  * a side channel. Should never be used by devices.
@@ -344,6 +349,8 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
                           "msix-pba", pba_size);
     memory_region_add_subregion(pba_bar, pba_offset, &dev->msix_pba_mmio);
 
+    dev->msix_prepare_message = msix_prepare_message;
+
     return 0;
 }
 
@@ -429,6 +436,7 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
     g_free(dev->msix_entry_used);
     dev->msix_entry_used = NULL;
     dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
+    dev->msix_prepare_message = NULL;
 }
 
 void msix_uninit_exclusive_bar(PCIDevice *dev)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e99417e501..f451300ce7 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -306,6 +306,15 @@ void pci_device_deassert_intx(PCIDevice *dev)
     }
 }
 
+static void pci_msi_trigger(PCIDevice *dev, MSIMessage msg)
+{
+    MemTxAttrs attrs = {};
+
+    attrs.requester_id = pci_requester_id(dev);
+    address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
+                         attrs, NULL);
+}
+
 static void pci_reset_regions(PCIDevice *dev)
 {
     int r;
@@ -1201,6 +1210,8 @@ static void pci_qdev_unrealize(DeviceState *dev)
 
     pci_device_deassert_intx(pci_dev);
     do_pci_unregister_device(pci_dev);
+
+    pci_dev->msi_trigger = NULL;
 }
 
 void pci_register_bar(PCIDevice *pci_dev, int region_num,
@@ -2235,6 +2246,8 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
     }
 
     pci_set_power(pci_dev, true);
+
+    pci_dev->msi_trigger = pci_msi_trigger;
 }
 
 PCIDevice *pci_new_multifunction(int devfn, bool multifunction,
diff --git a/hw/remote/machine.c b/hw/remote/machine.c
index 645b54343d..75d550daae 100644
--- a/hw/remote/machine.c
+++ b/hw/remote/machine.c
@@ -23,6 +23,8 @@
 #include "hw/remote/iommu.h"
 #include "hw/qdev-core.h"
 #include "hw/remote/iommu.h"
+#include "hw/remote/vfio-user-obj.h"
+#include "hw/pci/msi.h"
 
 static void remote_machine_init(MachineState *machine)
 {
@@ -54,12 +56,16 @@ static void remote_machine_init(MachineState *machine)
 
     if (s->vfio_user) {
         remote_iommu_setup(pci_host->bus);
-    }
 
-    remote_iohub_init(&s->iohub);
+        msi_nonbroken = true;
+
+        vfu_object_set_bus_irq(pci_host->bus);
+    } else {
+        remote_iohub_init(&s->iohub);
 
-    pci_bus_irqs(pci_host->bus, remote_iohub_set_irq, remote_iohub_map_irq,
-                 &s->iohub, REMOTE_IOHUB_NB_PIRQS);
+        pci_bus_irqs(pci_host->bus, remote_iohub_set_irq, remote_iohub_map_irq,
+                     &s->iohub, REMOTE_IOHUB_NB_PIRQS);
+    }
 
     qbus_set_hotplug_handler(BUS(pci_host->bus), OBJECT(s));
 }
diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index f5ca909e68..d351b1daa3 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -53,6 +53,9 @@
 #include "hw/pci/pci.h"
 #include "qemu/timer.h"
 #include "exec/memory.h"
+#include "hw/pci/msi.h"
+#include "hw/pci/msix.h"
+#include "hw/remote/vfio-user-obj.h"
 
 #define TYPE_VFU_OBJECT "x-vfio-user-server"
 OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
@@ -96,6 +99,10 @@ struct VfuObject {
     Error *unplug_blocker;
 
     int vfu_poll_fd;
+
+    MSITriggerFunc *default_msi_trigger;
+    MSIPrepareMessageFunc *default_msi_prepare_message;
+    MSIxPrepareMessageFunc *default_msix_prepare_message;
 };
 
 static void vfu_object_init_ctx(VfuObject *o, Error **errp);
@@ -518,6 +525,111 @@ static void vfu_object_register_bars(vfu_ctx_t *vfu_ctx, PCIDevice *pdev)
     }
 }
 
+static int vfu_object_map_irq(PCIDevice *pci_dev, int intx)
+{
+    int pci_bdf = PCI_BUILD_BDF(pci_bus_num(pci_get_bus(pci_dev)),
+                                pci_dev->devfn);
+
+    return pci_bdf;
+}
+
+static void vfu_object_set_irq(void *opaque, int pirq, int level)
+{
+    PCIBus *pci_bus = opaque;
+    PCIDevice *pci_dev = NULL;
+    vfu_ctx_t *vfu_ctx = NULL;
+    int pci_bus_num, devfn;
+
+    if (level) {
+        pci_bus_num = PCI_BUS_NUM(pirq);
+        devfn = PCI_BDF_TO_DEVFN(pirq);
+
+        /*
+         * pci_find_device() performs at O(1) if the device is attached
+         * to the root PCI bus. Whereas, if the device is attached to a
+         * secondary PCI bus (such as when a root port is involved),
+         * finding the parent PCI bus could take O(n)
+         */
+        pci_dev = pci_find_device(pci_bus, pci_bus_num, devfn);
+
+        vfu_ctx = pci_dev->irq_opaque;
+
+        g_assert(vfu_ctx);
+
+        vfu_irq_trigger(vfu_ctx, 0);
+    }
+}
+
+static MSIMessage vfu_object_msi_prepare_msg(PCIDevice *pci_dev,
+                                             unsigned int vector)
+{
+    MSIMessage msg;
+
+    msg.address = 0;
+    msg.data = vector;
+
+    return msg;
+}
+
+static void vfu_object_msi_trigger(PCIDevice *pci_dev, MSIMessage msg)
+{
+    vfu_ctx_t *vfu_ctx = pci_dev->irq_opaque;
+
+    vfu_irq_trigger(vfu_ctx, msg.data);
+}
+
+static void vfu_object_setup_msi_cbs(VfuObject *o)
+{
+    o->default_msi_trigger = o->pci_dev->msi_trigger;
+    o->default_msi_prepare_message = o->pci_dev->msi_prepare_message;
+    o->default_msix_prepare_message = o->pci_dev->msix_prepare_message;
+
+    o->pci_dev->msi_trigger = vfu_object_msi_trigger;
+    o->pci_dev->msi_prepare_message = vfu_object_msi_prepare_msg;
+    o->pci_dev->msix_prepare_message = vfu_object_msi_prepare_msg;
+}
+
+static void vfu_object_restore_msi_cbs(VfuObject *o)
+{
+    o->pci_dev->msi_trigger = o->default_msi_trigger;
+    o->pci_dev->msi_prepare_message = o->default_msi_prepare_message;
+    o->pci_dev->msix_prepare_message = o->default_msix_prepare_message;
+}
+
+static int vfu_object_setup_irqs(VfuObject *o, PCIDevice *pci_dev)
+{
+    vfu_ctx_t *vfu_ctx = o->vfu_ctx;
+    int ret;
+
+    ret = vfu_setup_device_nr_irqs(vfu_ctx, VFU_DEV_INTX_IRQ, 1);
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (msix_nr_vectors_allocated(pci_dev)) {
+        ret = vfu_setup_device_nr_irqs(vfu_ctx, VFU_DEV_MSIX_IRQ,
+                                       msix_nr_vectors_allocated(pci_dev));
+    } else if (msi_nr_vectors_allocated(pci_dev)) {
+        ret = vfu_setup_device_nr_irqs(vfu_ctx, VFU_DEV_MSI_IRQ,
+                                       msi_nr_vectors_allocated(pci_dev));
+    }
+
+    if (ret < 0) {
+        return ret;
+    }
+
+    vfu_object_setup_msi_cbs(o);
+
+    pci_dev->irq_opaque = vfu_ctx;
+
+    return 0;
+}
+
+void vfu_object_set_bus_irq(PCIBus *pci_bus)
+{
+    pci_bus_irqs(pci_bus, vfu_object_set_irq, vfu_object_map_irq, pci_bus, 1);
+}
+
 /*
  * TYPE_VFU_OBJECT depends on the availability of the 'socket' and 'device'
  * properties. It also depends on devices instantiated in QEMU. These
@@ -616,6 +728,13 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
 
     vfu_object_register_bars(o->vfu_ctx, o->pci_dev);
 
+    ret = vfu_object_setup_irqs(o, o->pci_dev);
+    if (ret < 0) {
+        error_setg(errp, "vfu: Failed to setup interrupts for %s",
+                   o->device);
+        goto fail;
+    }
+
     ret = vfu_realize_ctx(o->vfu_ctx);
     if (ret < 0) {
         error_setg(errp, "vfu: Failed to realize device %s- %s",
@@ -641,6 +760,8 @@ fail:
         o->unplug_blocker = NULL;
     }
     if (o->pci_dev) {
+        vfu_object_restore_msi_cbs(o);
+        o->pci_dev->irq_opaque = NULL;
         object_unref(OBJECT(o->pci_dev));
         o->pci_dev = NULL;
     }
@@ -700,6 +821,8 @@ static void vfu_object_finalize(Object *obj)
     }
 
     if (o->pci_dev) {
+        vfu_object_restore_msi_cbs(o);
+        o->pci_dev->irq_opaque = NULL;
         object_unref(OBJECT(o->pci_dev));
         o->pci_dev = NULL;
     }
diff --git a/stubs/vfio-user-obj.c b/stubs/vfio-user-obj.c
new file mode 100644
index 0000000000..79100d768e
--- /dev/null
+++ b/stubs/vfio-user-obj.c
@@ -0,0 +1,6 @@
+#include "qemu/osdep.h"
+#include "hw/remote/vfio-user-obj.h"
+
+void vfu_object_set_bus_irq(PCIBus *pci_bus)
+{
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index f257afcf8a..d2e977affb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3600,6 +3600,7 @@ F: hw/remote/iohub.c
 F: include/hw/remote/iohub.h
 F: subprojects/libvfio-user
 F: hw/remote/vfio-user-obj.c
+F: include/hw/remote/vfio-user-obj.h
 F: hw/remote/iommu.c
 F: include/hw/remote/iommu.h
 
diff --git a/hw/remote/trace-events b/hw/remote/trace-events
index 847d50d88f..c167b3c7a5 100644
--- a/hw/remote/trace-events
+++ b/hw/remote/trace-events
@@ -12,3 +12,4 @@ vfu_dma_unregister(uint64_t gpa) "vfu: unregistering GPA 0x%"PRIx64""
 vfu_bar_register(int i, uint64_t addr, uint64_t size) "vfu: BAR %d: addr 0x%"PRIx64" size 0x%"PRIx64""
 vfu_bar_rw_enter(const char *op, uint64_t addr) "vfu: %s request for BAR address 0x%"PRIx64""
 vfu_bar_rw_exit(const char *op, uint64_t addr) "vfu: Finished %s of BAR address 0x%"PRIx64""
+vfu_interrupt(int pirq) "vfu: sending interrupt to device - PIRQ %d"
diff --git a/stubs/meson.build b/stubs/meson.build
index 6f80fec761..d8f3fd5c44 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -60,3 +60,4 @@ if have_system
 else
   stub_ss.add(files('qdev.c'))
 endif
+stub_ss.add(when: 'CONFIG_VFIO_USER_SERVER', if_false: files('vfio-user-obj.c'))
-- 
2.20.1

[PATCH v9 16/17] vfio-user: handle reset of remote device

[Jagannathan Raman]

Adds handler to reset a remote device

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/remote/vfio-user-obj.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index d351b1daa3..15b06744f9 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -630,6 +630,20 @@ void vfu_object_set_bus_irq(PCIBus *pci_bus)
     pci_bus_irqs(pci_bus, vfu_object_set_irq, vfu_object_map_irq, pci_bus, 1);
 }
 
+static int vfu_object_device_reset(vfu_ctx_t *vfu_ctx, vfu_reset_type_t type)
+{
+    VfuObject *o = vfu_get_private(vfu_ctx);
+
+    /* vfu_object_ctx_run() handles lost connection */
+    if (type == VFU_RESET_LOST_CONN) {
+        return 0;
+    }
+
+    qdev_reset_all(DEVICE(o->pci_dev));
+
+    return 0;
+}
+
 /*
  * TYPE_VFU_OBJECT depends on the availability of the 'socket' and 'device'
  * properties. It also depends on devices instantiated in QEMU. These
@@ -735,6 +749,12 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
         goto fail;
     }
 
+    ret = vfu_setup_device_reset_cb(o->vfu_ctx, &vfu_object_device_reset);
+    if (ret < 0) {
+        error_setg(errp, "vfu: Failed to setup reset callback");
+        goto fail;
+    }
+
     ret = vfu_realize_ctx(o->vfu_ctx);
     if (ret < 0) {
         error_setg(errp, "vfu: Failed to realize device %s- %s",
-- 
2.20.1

[PATCH v9 17/17] vfio-user: avocado tests for vfio-user

[Jagannathan Raman]

Avocado tests for libvfio-user in QEMU - tests startup,
hotplug and migration of the server object

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
---
 MAINTAINERS                |   1 +
 tests/avocado/vfio-user.py | 164 +++++++++++++++++++++++++++++++++++++
 2 files changed, 165 insertions(+)
 create mode 100644 tests/avocado/vfio-user.py

diff --git a/MAINTAINERS b/MAINTAINERS
index d2e977affb..103fcb472f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3603,6 +3603,7 @@ F: hw/remote/vfio-user-obj.c
 F: include/hw/remote/vfio-user-obj.h
 F: hw/remote/iommu.c
 F: include/hw/remote/iommu.h
+F: tests/avocado/vfio-user.py
 
 EBPF:
 M: Jason Wang <jasowang@redhat.com>
diff --git a/tests/avocado/vfio-user.py b/tests/avocado/vfio-user.py
new file mode 100644
index 0000000000..ced304d770
--- /dev/null
+++ b/tests/avocado/vfio-user.py
@@ -0,0 +1,164 @@
+# vfio-user protocol sanity test
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+
+
+import os
+import socket
+import uuid
+
+from avocado_qemu import QemuSystemTest
+from avocado_qemu import wait_for_console_pattern
+from avocado_qemu import exec_command
+from avocado_qemu import exec_command_and_wait_for_pattern
+
+from avocado.utils import network
+from avocado.utils import wait
+
+class VfioUser(QemuSystemTest):
+    """
+    :avocado: tags=vfiouser
+    """
+    KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
+    timeout = 20
+
+    def _get_free_port(self):
+        port = network.find_free_port()
+        if port is None:
+            self.cancel('Failed to find a free port')
+        return port
+
+    def validate_vm_launch(self, vm):
+        wait_for_console_pattern(self, 'as init process',
+                                 'Kernel panic - not syncing', vm=vm)
+        exec_command(self, 'mount -t sysfs sysfs /sys', vm=vm)
+        exec_command_and_wait_for_pattern(self,
+                                          'cat /sys/bus/pci/devices/*/uevent',
+                                          'PCI_ID=1000:0060', vm=vm)
+
+    def launch_server_startup(self, socket, *opts):
+        server_vm = self.get_vm()
+        server_vm.add_args('-machine', 'x-remote,vfio-user=on')
+        server_vm.add_args('-nodefaults')
+        server_vm.add_args('-device', 'megasas,id=sas1')
+        server_vm.add_args('-object', 'x-vfio-user-server,id=vfioobj1,'
+                           'type=unix,path='+socket+',device=sas1')
+        for opt in opts:
+            server_vm.add_args(opt)
+        server_vm.launch()
+        return server_vm
+
+    def launch_server_hotplug(self, socket):
+        server_vm = self.get_vm()
+        server_vm.add_args('-machine', 'x-remote,vfio-user=on')
+        server_vm.add_args('-nodefaults')
+        server_vm.launch()
+        server_vm.qmp('device_add', args_dict=None, conv_keys=None,
+                      driver='megasas', id='sas1')
+        obj_add_opts = {'qom-type': 'x-vfio-user-server',
+                        'id': 'vfioobj', 'device': 'sas1',
+                        'socket': {'type': 'unix', 'path': socket}}
+        server_vm.qmp('object-add', args_dict=obj_add_opts)
+        return server_vm
+
+    def launch_client(self, kernel_path, initrd_path, kernel_command_line,
+                      machine_type, socket, *opts):
+        client_vm = self.get_vm()
+        client_vm.set_console()
+        client_vm.add_args('-machine', machine_type)
+        client_vm.add_args('-accel', 'kvm')
+        client_vm.add_args('-cpu', 'host')
+        client_vm.add_args('-object',
+                           'memory-backend-memfd,id=sysmem-file,size=2G')
+        client_vm.add_args('--numa', 'node,memdev=sysmem-file')
+        client_vm.add_args('-m', '2048')
+        client_vm.add_args('-kernel', kernel_path,
+                           '-initrd', initrd_path,
+                           '-append', kernel_command_line)
+        client_vm.add_args('-device',
+                           'vfio-user-pci,socket='+socket)
+        for opt in opts:
+            client_vm.add_args(opt)
+        client_vm.launch()
+        return client_vm
+
+    def do_test_startup(self, kernel_url, initrd_url, kernel_command_line,
+                machine_type):
+        self.require_accelerator('kvm')
+
+        kernel_path = self.fetch_asset(kernel_url)
+        initrd_path = self.fetch_asset(initrd_url)
+        socket = os.path.join('/tmp', str(uuid.uuid4()))
+        if os.path.exists(socket):
+            os.remove(socket)
+        self.launch_server_startup(socket)
+        client = self.launch_client(kernel_path, initrd_path,
+                                    kernel_command_line, machine_type, socket)
+        self.validate_vm_launch(client)
+
+    def do_test_hotplug(self, kernel_url, initrd_url, kernel_command_line,
+                machine_type):
+        self.require_accelerator('kvm')
+
+        kernel_path = self.fetch_asset(kernel_url)
+        initrd_path = self.fetch_asset(initrd_url)
+        socket = os.path.join('/tmp', str(uuid.uuid4()))
+        if os.path.exists(socket):
+            os.remove(socket)
+        self.launch_server_hotplug(socket)
+        client = self.launch_client(kernel_path, initrd_path,
+                                    kernel_command_line, machine_type, socket)
+        self.validate_vm_launch(client)
+
+    def test_vfio_user_x86_64(self):
+        """
+        :avocado: tags=arch:x86_64
+        :avocado: tags=distro:centos
+        """
+        kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
+                      '/linux/releases/31/Everything/x86_64/os/images'
+                      '/pxeboot/vmlinuz')
+        initrd_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
+                      '/linux/releases/31/Everything/x86_64/os/images'
+                      '/pxeboot/initrd.img')
+        kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
+                               'console=ttyS0 rdinit=/bin/bash')
+        machine_type = 'pc'
+        self.do_test_startup(kernel_url, initrd_url, kernel_command_line,
+                             machine_type)
+
+    def test_vfio_user_aarch64(self):
+        """
+        :avocado: tags=arch:aarch64
+        :avocado: tags=distro:ubuntu
+        """
+        kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
+                      '/linux/releases/31/Everything/aarch64/os/images'
+                      '/pxeboot/vmlinuz')
+        initrd_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
+                      '/linux/releases/31/Everything/aarch64/os/images'
+                      '/pxeboot/initrd.img')
+        kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
+                               'rdinit=/bin/bash console=ttyAMA0')
+        machine_type = 'virt,gic-version=3'
+        self.do_test_startup(kernel_url, initrd_url, kernel_command_line,
+                             machine_type)
+
+    def test_vfio_user_hotplug_x86_64(self):
+        """
+        :avocado: tags=arch:x86_64
+        :avocado: tags=distro:centos
+        """
+        kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
+                      '/linux/releases/31/Everything/x86_64/os/images'
+                      '/pxeboot/vmlinuz')
+        initrd_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
+                      '/linux/releases/31/Everything/x86_64/os/images'
+                      '/pxeboot/initrd.img')
+        kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
+                               'console=ttyS0 rdinit=/bin/bash')
+        machine_type = 'pc'
+        self.do_test_hotplug(kernel_url, initrd_url, kernel_command_line,
+                             machine_type)
+
-- 
2.20.1

Re: [PATCH v9 02/17] qdev: unplug blocker for devices

[Markus Armbruster]

Jagannathan Raman <jag.raman@oracle.com> writes:

> Add blocker to prevent hot-unplug of devices
>
> TYPE_VFIO_USER_SERVER, which is introduced shortly, attaches itself to a
> PCIDevice on which it depends. If the attached PCIDevice gets removed
> while the server in use, it could cause it crash. To prevent this,
> TYPE_VFIO_USER_SERVER adds an unplug blocker for the PCIDevice.

Appreciate the explanation :)

>
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/hw/qdev-core.h | 29 +++++++++++++++++++++++++++++
>  hw/core/qdev.c         | 24 ++++++++++++++++++++++++
>  softmmu/qdev-monitor.c |  4 ++++
>  3 files changed, 57 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 92c3d65208..1b9fa25e5c 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -193,6 +193,7 @@ struct DeviceState {
>      int instance_id_alias;
>      int alias_required_for_version;
>      ResettableState reset;
> +    GSList *unplug_blockers;
>  };
>  
>  struct DeviceListener {
> @@ -419,6 +420,34 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>  void qdev_machine_creation_done(void);
>  bool qdev_machine_modified(void);
>  
> +/*

Use /** here like we do in the other function comments nearby.

In case you're curious: it's GTK-Doc format.  It's intended for
generating documentation from doc comments.  Which we don't do, and
perhaps never will.  But let's be locally consistent.

> + * qdev_add_unplug_blocker: Adds an unplug blocker to a device

Recommend imperative mood for function comments: "Add an unplug
blocker to a device".

More of the same below.

[...]

Re: [PATCH v9 00/17] vfio-user server in QEMU

[Markus Armbruster]

Does not apply for me.  What's your base?

Re: [PATCH v9 10/17] vfio-user: run vfio-user context

[Markus Armbruster]

Jagannathan Raman <jag.raman@oracle.com> writes:

> Setup a handler to run vfio-user context. The context is driven by
> messages to the file descriptor associated with it - get the fd for
> the context and hook up the handler with it
>
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qapi/misc.json            |  30 +++++++++++
>  hw/remote/vfio-user-obj.c | 102 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 131 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index b83cc39029..fa49f2876a 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -553,3 +553,33 @@
>  ##
>  { 'event': 'RTC_CHANGE',
>    'data': { 'offset': 'int', 'qom-path': 'str' } }
> +
> +##
> +# @VFU_CLIENT_HANGUP:
> +#
> +# Emitted when the client of a TYPE_VFIO_USER_SERVER closes the
> +# communication channel
> +#
> +# @vfu-id: ID of the TYPE_VFIO_USER_SERVER object
> +#
> +# @vfu-qom-path: path to the TYPE_VFIO_USER_SERVER object in the QOM tree
> +#
> +# @dev-id: ID of attached PCI device
> +#
> +# @dev-qom-path: path to attached PCI device in the QOM tree

I'm still unsure what kind(s) of ID @vfu-id and @dev-id are.  See below.

> +#
> +# Since: 7.1
> +#
> +# Example:
> +#
> +# <- { "event": "VFU_CLIENT_HANGUP",
> +#      "data": { "vfu-id": "vfu1",
> +#                "vfu-qom-path": "/objects/vfu1",
> +#                "dev-id": "sas1",
> +#                "dev-qom-path": "/machine/peripheral/sas1" },
> +#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +#
> +##
> +{ 'event': 'VFU_CLIENT_HANGUP',
> +  'data': { 'vfu-id': 'str', 'vfu-qom-path': 'str',
> +            'dev-id': 'str', 'dev-qom-path': 'str' } }
> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
> index 3ca6aa2b45..3a4c6a9fa0 100644
> --- a/hw/remote/vfio-user-obj.c
> +++ b/hw/remote/vfio-user-obj.c
> @@ -27,6 +27,9 @@
>   *
>   * device - id of a device on the server, a required option. PCI devices
>   *          alone are supported presently.
> + *
> + * notes - x-vfio-user-server could block IO and monitor during the
> + *         initialization phase.
>   */
>  
>  #include "qemu/osdep.h"
> @@ -40,11 +43,14 @@
>  #include "hw/remote/machine.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-visit-sockets.h"
> +#include "qapi/qapi-events-misc.h"
>  #include "qemu/notify.h"
> +#include "qemu/thread.h"
>  #include "sysemu/sysemu.h"
>  #include "libvfio-user.h"
>  #include "hw/qdev-core.h"
>  #include "hw/pci/pci.h"
> +#include "qemu/timer.h"
>  
>  #define TYPE_VFU_OBJECT "x-vfio-user-server"
>  OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
> @@ -86,6 +92,8 @@ struct VfuObject {
>      PCIDevice *pci_dev;
>  
>      Error *unplug_blocker;
> +
> +    int vfu_poll_fd;
>  };
>  
>  static void vfu_object_init_ctx(VfuObject *o, Error **errp);
> @@ -164,6 +172,76 @@ static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
>      vfu_object_init_ctx(o, errp);
>  }
>  
> +static void vfu_object_ctx_run(void *opaque)
> +{
> +    VfuObject *o = opaque;
> +    const char *vfu_id;
> +    char *vfu_path, *pci_dev_path;
> +    int ret = -1;
> +
> +    while (ret != 0) {
> +        ret = vfu_run_ctx(o->vfu_ctx);
> +        if (ret < 0) {
> +            if (errno == EINTR) {
> +                continue;
> +            } else if (errno == ENOTCONN) {
> +                vfu_id = object_get_canonical_path_component(OBJECT(o));
> +                vfu_path = object_get_canonical_path(OBJECT(o));

Hmm.  @vfu_id is always the last component of @vfu_path.  Why do we need
to send both?

> +                g_assert(o->pci_dev);
> +                pci_dev_path = object_get_canonical_path(OBJECT(o->pci_dev));
> +                qapi_event_send_vfu_client_hangup(vfu_id, vfu_path,
> +                                                  o->device, pci_dev_path);

Where is o->device set?  I'm asking because I it must not be null here,
and that's not locally obvious.

> +                qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL);
> +                o->vfu_poll_fd = -1;
> +                object_unparent(OBJECT(o));
> +                g_free(vfu_path);
> +                g_free(pci_dev_path);
> +                break;
> +            } else {
> +                VFU_OBJECT_ERROR(o, "vfu: Failed to run device %s - %s",
> +                                 o->device, strerror(errno));
> +                break;
> +            }
> +        }
> +    }
> +}

[...]

Re: [PATCH v9 07/17] vfio-user: define vfio-user-server object

[Markus Armbruster]

Jagannathan Raman <jag.raman@oracle.com> writes:

> Define vfio-user object which is remote process server for QEMU. Setup
> object initialization functions and properties necessary to instantiate
> the object
>
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> ---
>  qapi/qom.json               |  20 +++-
>  include/hw/remote/machine.h |   2 +
>  hw/remote/machine.c         |  27 +++++
>  hw/remote/vfio-user-obj.c   | 210 ++++++++++++++++++++++++++++++++++++
>  MAINTAINERS                 |   1 +
>  hw/remote/meson.build       |   1 +
>  hw/remote/trace-events      |   3 +
>  7 files changed, 262 insertions(+), 2 deletions(-)
>  create mode 100644 hw/remote/vfio-user-obj.c
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index eeb5395ff3..582def0522 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -703,6 +703,20 @@
>  { 'struct': 'RemoteObjectProperties',
>    'data': { 'fd': 'str', 'devid': 'str' } }
>  
> +##
> +# @VfioUserServerProperties:
> +#
> +# Properties for x-vfio-user-server objects.
> +#
> +# @socket: socket to be used by the libvfio-user library
> +#
> +# @device: the id of the device to be emulated at the server

Suggest "the ID", because "id" is not a word.

What kind of ID is this?  The kind set with -device id=...?

> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'VfioUserServerProperties',
> +  'data': { 'socket': 'SocketAddress', 'device': 'str' } }
> +
>  ##
>  # @RngProperties:
>  #
> @@ -842,7 +856,8 @@
>      'tls-creds-psk',
>      'tls-creds-x509',
>      'tls-cipher-suites',
> -    { 'name': 'x-remote-object', 'features': [ 'unstable' ] }
> +    { 'name': 'x-remote-object', 'features': [ 'unstable' ] },
> +    { 'name': 'x-vfio-user-server', 'features': [ 'unstable' ] }
>    ] }
>  
>  ##
> @@ -905,7 +920,8 @@
>        'tls-creds-psk':              'TlsCredsPskProperties',
>        'tls-creds-x509':             'TlsCredsX509Properties',
>        'tls-cipher-suites':          'TlsCredsProperties',
> -      'x-remote-object':            'RemoteObjectProperties'
> +      'x-remote-object':            'RemoteObjectProperties',
> +      'x-vfio-user-server':         'VfioUserServerProperties'
>    } }
>  
>  ##

[...]

Re: [PATCH v9 02/17] qdev: unplug blocker for devices

[Jag Raman]

> On May 4, 2022, at 7:13 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Jagannathan Raman <jag.raman@oracle.com> writes:
> 
>> Add blocker to prevent hot-unplug of devices
>> 
>> TYPE_VFIO_USER_SERVER, which is introduced shortly, attaches itself to a
>> PCIDevice on which it depends. If the attached PCIDevice gets removed
>> while the server in use, it could cause it crash. To prevent this,
>> TYPE_VFIO_USER_SERVER adds an unplug blocker for the PCIDevice.
> 
> Appreciate the explanation :)
> 
>> 
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>> include/hw/qdev-core.h | 29 +++++++++++++++++++++++++++++
>> hw/core/qdev.c         | 24 ++++++++++++++++++++++++
>> softmmu/qdev-monitor.c |  4 ++++
>> 3 files changed, 57 insertions(+)
>> 
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 92c3d65208..1b9fa25e5c 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -193,6 +193,7 @@ struct DeviceState {
>>     int instance_id_alias;
>>     int alias_required_for_version;
>>     ResettableState reset;
>> +    GSList *unplug_blockers;
>> };
>> 
>> struct DeviceListener {
>> @@ -419,6 +420,34 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>> void qdev_machine_creation_done(void);
>> bool qdev_machine_modified(void);
>> 
>> +/*
> 
> Use /** here like we do in the other function comments nearby.
> 
> In case you're curious: it's GTK-Doc format.  It's intended for
> generating documentation from doc comments.  Which we don't do, and
> perhaps never will.  But let's be locally consistent.

Sure, will do.

> 
>> + * qdev_add_unplug_blocker: Adds an unplug blocker to a device
> 
> Recommend imperative mood for function comments: "Add an unplug
> blocker to a device".

OK, got it. Will update the comments. :)

--
Jag

> 
> More of the same below.
> 
> [...]
> 

Re: [PATCH v9 00/17] vfio-user server in QEMU

[Jag Raman]

> On May 4, 2022, at 7:37 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Does not apply for me.  What's your base?

The base I used is f5643914a9e8f79c606a76e6a9d7ea82a3fc3e65.

The patchew tool says it was able to apply the patches:
https://patchew.org/QEMU/cover.1651586203.git.jag.raman@oracle.com/

The client side changes required are still under review. But the
following repo has this series applied on top of the client:
repo URL: https://github.com/oracle/qemu
branch: vfio-user-v5client-v9server

Thank you!
--
Jag

> 

Re: [PATCH v9 00/17] vfio-user server in QEMU

[Markus Armbruster]

Jag Raman <jag.raman@oracle.com> writes:

>> On May 4, 2022, at 7:37 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> Does not apply for me.  What's your base?
>
> The base I used is f5643914a9e8f79c606a76e6a9d7ea82a3fc3e65.
>
> The patchew tool says it was able to apply the patches:
> https://patchew.org/QEMU/cover.1651586203.git.jag.raman@oracle.com/

Local tooling error, sorry for the noise.

Re: [PATCH v9 10/17] vfio-user: run vfio-user context

[Jag Raman]

> On May 4, 2022, at 7:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Jagannathan Raman <jag.raman@oracle.com> writes:
> 
>> Setup a handler to run vfio-user context. The context is driven by
>> messages to the file descriptor associated with it - get the fd for
>> the context and hook up the handler with it
>> 
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>> qapi/misc.json | 30 +++++++++++
>> hw/remote/vfio-user-obj.c | 102 +++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 131 insertions(+), 1 deletion(-)
>> 
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index b83cc39029..fa49f2876a 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json
>> @@ -553,3 +553,33 @@
>> ##
>> { 'event': 'RTC_CHANGE',
>> 'data': { 'offset': 'int', 'qom-path': 'str' } }
>> +
>> +##
>> +# @VFU_CLIENT_HANGUP:
>> +#
>> +# Emitted when the client of a TYPE_VFIO_USER_SERVER closes the
>> +# communication channel
>> +#
>> +# @vfu-id: ID of the TYPE_VFIO_USER_SERVER object
>> +#
>> +# @vfu-qom-path: path to the TYPE_VFIO_USER_SERVER object in the QOM tree
>> +#
>> +# @dev-id: ID of attached PCI device
>> +#
>> +# @dev-qom-path: path to attached PCI device in the QOM tree
> 
> I'm still unsure what kind(s) of ID @vfu-id and @dev-id are. See below.

I’m not sure what you mean by kind of ID - I thought of ID as a
unique string. I’ll try my best to explain.

dev-id and vfu-id are the “id" sub-options of “-device” and “-object” command-line
options respectively.

"dev-id” is the “id” member of “DeviceState” which QEMU sets using
qdev_set_id() when the device is added. 

The Object ID (vfu-id in this case) is a bit tricky. It’s also the “id”
command-line sub-option, but QEMU stores it as a child property
of the parent object.

> 
>> +#
>> +# Since: 7.1
>> +#
>> +# Example:
>> +#
>> +# <- { "event": "VFU_CLIENT_HANGUP",
>> +# "data": { "vfu-id": "vfu1",
>> +# "vfu-qom-path": "/objects/vfu1",
>> +# "dev-id": "sas1",
>> +# "dev-qom-path": "/machine/peripheral/sas1" },
>> +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>> +#
>> +##
>> +{ 'event': 'VFU_CLIENT_HANGUP',
>> + 'data': { 'vfu-id': 'str', 'vfu-qom-path': 'str',
>> + 'dev-id': 'str', 'dev-qom-path': 'str' } }
>> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>> index 3ca6aa2b45..3a4c6a9fa0 100644
>> --- a/hw/remote/vfio-user-obj.c
>> +++ b/hw/remote/vfio-user-obj.c
>> @@ -27,6 +27,9 @@
>> *
>> * device - id of a device on the server, a required option. PCI devices
>> * alone are supported presently.
>> + *
>> + * notes - x-vfio-user-server could block IO and monitor during the
>> + * initialization phase.
>> */
>> 
>> #include "qemu/osdep.h"
>> @@ -40,11 +43,14 @@
>> #include "hw/remote/machine.h"
>> #include "qapi/error.h"
>> #include "qapi/qapi-visit-sockets.h"
>> +#include "qapi/qapi-events-misc.h"
>> #include "qemu/notify.h"
>> +#include "qemu/thread.h"
>> #include "sysemu/sysemu.h"
>> #include "libvfio-user.h"
>> #include "hw/qdev-core.h"
>> #include "hw/pci/pci.h"
>> +#include "qemu/timer.h"
>> 
>> #define TYPE_VFU_OBJECT "x-vfio-user-server"
>> OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
>> @@ -86,6 +92,8 @@ struct VfuObject {
>> PCIDevice *pci_dev;
>> 
>> Error *unplug_blocker;
>> +
>> + int vfu_poll_fd;
>> };
>> 
>> static void vfu_object_init_ctx(VfuObject *o, Error **errp);
>> @@ -164,6 +172,76 @@ static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
>> vfu_object_init_ctx(o, errp);
>> }
>> 
>> +static void vfu_object_ctx_run(void *opaque)
>> +{
>> + VfuObject *o = opaque;
>> + const char *vfu_id;
>> + char *vfu_path, *pci_dev_path;
>> + int ret = -1;
>> +
>> + while (ret != 0) {
>> + ret = vfu_run_ctx(o->vfu_ctx);
>> + if (ret < 0) {
>> + if (errno == EINTR) {
>> + continue;
>> + } else if (errno == ENOTCONN) {
>> + vfu_id = object_get_canonical_path_component(OBJECT(o));
>> + vfu_path = object_get_canonical_path(OBJECT(o));
> 
> Hmm. @vfu_id is always the last component of @vfu_path. Why do we need
> to send both?

vfu_id is the ID that the user/Orchestrator passed as a command-line option
during addition/creation. So it made sense to report back with the same ID
that they used. But I’m OK with dropping this if that’s what you prefer.

> 
>> + g_assert(o->pci_dev);
>> + pci_dev_path = object_get_canonical_path(OBJECT(o->pci_dev));
>> + qapi_event_send_vfu_client_hangup(vfu_id, vfu_path,
>> + o->device, pci_dev_path);
> 
> Where is o->device set? I'm asking because I it must not be null here,
> and that's not locally obvious.

Yeah, it’s not obvious from this patch that o->device is guaranteed to be
non-NULL. It’s set by vfu_object_set_device(). Please see the following
patches in the series:
vfio-user: define vfio-user-server object
vfio-user: instantiate vfio-user context

There’s already an assert for o->pci_dev here, but we could add one
for o->device too?

Thank you!
—
Jag

> 
>> + qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL);
>> + o->vfu_poll_fd = -1;
>> + object_unparent(OBJECT(o));
>> + g_free(vfu_path);
>> + g_free(pci_dev_path);
>> + break;
>> + } else {
>> + VFU_OBJECT_ERROR(o, "vfu: Failed to run device %s - %s",
>> + o->device, strerror(errno));
>> + break;
>> + }
>> + }
>> + }
>> +}
> 
> [...]

Re: [PATCH v9 07/17] vfio-user: define vfio-user-server object

[Jag Raman]

> On May 4, 2022, at 7:45 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Jagannathan Raman <jag.raman@oracle.com> writes:
> 
>> Define vfio-user object which is remote process server for QEMU. Setup
>> object initialization functions and properties necessary to instantiate
>> the object
>> 
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> ---
>> qapi/qom.json | 20 +++-
>> include/hw/remote/machine.h | 2 +
>> hw/remote/machine.c | 27 +++++
>> hw/remote/vfio-user-obj.c | 210 ++++++++++++++++++++++++++++++++++++
>> MAINTAINERS | 1 +
>> hw/remote/meson.build | 1 +
>> hw/remote/trace-events | 3 +
>> 7 files changed, 262 insertions(+), 2 deletions(-)
>> create mode 100644 hw/remote/vfio-user-obj.c
>> 
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index eeb5395ff3..582def0522 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -703,6 +703,20 @@
>> { 'struct': 'RemoteObjectProperties',
>> 'data': { 'fd': 'str', 'devid': 'str' } }
>> 
>> +##
>> +# @VfioUserServerProperties:
>> +#
>> +# Properties for x-vfio-user-server objects.
>> +#
>> +# @socket: socket to be used by the libvfio-user library
>> +#
>> +# @device: the id of the device to be emulated at the server
> 
> Suggest "the ID", because "id" is not a word.
> 
> What kind of ID is this? The kind set with -device id=...?

Yes, it’s the “id” sub-option of the “-device” option. Will update this comment.

Thank you!
--
Jag

> 
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'struct': 'VfioUserServerProperties',
>> + 'data': { 'socket': 'SocketAddress', 'device': 'str' } }
>> +
>> ##
>> # @RngProperties:
>> #
>> @@ -842,7 +856,8 @@
>> 'tls-creds-psk',
>> 'tls-creds-x509',
>> 'tls-cipher-suites',
>> - { 'name': 'x-remote-object', 'features': [ 'unstable' ] }
>> + { 'name': 'x-remote-object', 'features': [ 'unstable' ] },
>> + { 'name': 'x-vfio-user-server', 'features': [ 'unstable' ] }
>> ] }
>> 
>> ##
>> @@ -905,7 +920,8 @@
>> 'tls-creds-psk': 'TlsCredsPskProperties',
>> 'tls-creds-x509': 'TlsCredsX509Properties',
>> 'tls-cipher-suites': 'TlsCredsProperties',
>> - 'x-remote-object': 'RemoteObjectProperties'
>> + 'x-remote-object': 'RemoteObjectProperties',
>> + 'x-vfio-user-server': 'VfioUserServerProperties'
>> } }
>> 
>> ##
> 
> [...]

Re: [PATCH v9 07/17] vfio-user: define vfio-user-server object

[Markus Armbruster]

Jag Raman <jag.raman@oracle.com> writes:

>> On May 4, 2022, at 7:45 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> Jagannathan Raman <jag.raman@oracle.com> writes:
>> 
>>> Define vfio-user object which is remote process server for QEMU. Setup
>>> object initialization functions and properties necessary to instantiate
>>> the object
>>> 
>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>> ---
>>> qapi/qom.json | 20 +++-
>>> include/hw/remote/machine.h | 2 +
>>> hw/remote/machine.c | 27 +++++
>>> hw/remote/vfio-user-obj.c | 210 ++++++++++++++++++++++++++++++++++++
>>> MAINTAINERS | 1 +
>>> hw/remote/meson.build | 1 +
>>> hw/remote/trace-events | 3 +
>>> 7 files changed, 262 insertions(+), 2 deletions(-)
>>> create mode 100644 hw/remote/vfio-user-obj.c
>>> 
>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>> index eeb5395ff3..582def0522 100644
>>> --- a/qapi/qom.json
>>> +++ b/qapi/qom.json
>>> @@ -703,6 +703,20 @@
>>> { 'struct': 'RemoteObjectProperties',
>>> 'data': { 'fd': 'str', 'devid': 'str' } }
>>> 
>>> +##
>>> +# @VfioUserServerProperties:
>>> +#
>>> +# Properties for x-vfio-user-server objects.
>>> +#
>>> +# @socket: socket to be used by the libvfio-user library
>>> +#
>>> +# @device: the id of the device to be emulated at the server
>> 
>> Suggest "the ID", because "id" is not a word.
>> 
>> What kind of ID is this? The kind set with -device id=...?
>
> Yes, it’s the “id” sub-option of the “-device” option. Will update this comment.

I was just double-checking.  I think the comment is okay with "the ID".

> Thank you!

You're welcome!

Re: [PATCH v9 10/17] vfio-user: run vfio-user context

[Markus Armbruster]

Jag Raman <jag.raman@oracle.com> writes:

>> On May 4, 2022, at 7:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> Jagannathan Raman <jag.raman@oracle.com> writes:
>> 
>>> Setup a handler to run vfio-user context. The context is driven by
>>> messages to the file descriptor associated with it - get the fd for
>>> the context and hook up the handler with it
>>> 
>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>> qapi/misc.json | 30 +++++++++++
>>> hw/remote/vfio-user-obj.c | 102 +++++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 131 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/qapi/misc.json b/qapi/misc.json
>>> index b83cc39029..fa49f2876a 100644
>>> --- a/qapi/misc.json
>>> +++ b/qapi/misc.json
>>> @@ -553,3 +553,33 @@
>>> ##
>>> { 'event': 'RTC_CHANGE',
>>> 'data': { 'offset': 'int', 'qom-path': 'str' } }
>>> +
>>> +##
>>> +# @VFU_CLIENT_HANGUP:
>>> +#
>>> +# Emitted when the client of a TYPE_VFIO_USER_SERVER closes the
>>> +# communication channel
>>> +#
>>> +# @vfu-id: ID of the TYPE_VFIO_USER_SERVER object
>>> +#
>>> +# @vfu-qom-path: path to the TYPE_VFIO_USER_SERVER object in the QOM tree
>>> +#
>>> +# @dev-id: ID of attached PCI device
>>> +#
>>> +# @dev-qom-path: path to attached PCI device in the QOM tree
>> 
>> I'm still unsure what kind(s) of ID @vfu-id and @dev-id are. See below.
>
> I’m not sure what you mean by kind of ID - I thought of ID as a
> unique string. I’ll try my best to explain.

Okay, let me try to clarify.

We have many, many ID namespaces, each associated with a certain kind of
object: device IDs for TYPE_DEVICE, object IDs for TYPE_OBJECT
implementing TYPE_USER_CREATABLE), block backend node names for
BlockDriverState, ...

Aside: I believe a single namespace would have been a wiser design
choice, but that ship sailed long ago.

To which of these namespaces do these two IDs belong, respectively?

> dev-id and vfu-id are the “id" sub-options of “-device” and “-object” command-line
> options respectively.

This answers my question.

> "dev-id” is the “id” member of “DeviceState” which QEMU sets using
> qdev_set_id() when the device is added. 
>
> The Object ID (vfu-id in this case) is a bit tricky. It’s also the “id”
> command-line sub-option, but QEMU stores it as a child property
> of the parent object.
>
>> 
>>> +#
>>> +# Since: 7.1
>>> +#
>>> +# Example:
>>> +#
>>> +# <- { "event": "VFU_CLIENT_HANGUP",
>>> +# "data": { "vfu-id": "vfu1",
>>> +# "vfu-qom-path": "/objects/vfu1",
>>> +# "dev-id": "sas1",
>>> +# "dev-qom-path": "/machine/peripheral/sas1" },
>>> +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>>> +#
>>> +##
>>> +{ 'event': 'VFU_CLIENT_HANGUP',
>>> + 'data': { 'vfu-id': 'str', 'vfu-qom-path': 'str',
>>> + 'dev-id': 'str', 'dev-qom-path': 'str' } }
>>> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>>> index 3ca6aa2b45..3a4c6a9fa0 100644
>>> --- a/hw/remote/vfio-user-obj.c
>>> +++ b/hw/remote/vfio-user-obj.c
>>> @@ -27,6 +27,9 @@
>>> *
>>> * device - id of a device on the server, a required option. PCI devices
>>> * alone are supported presently.
>>> + *
>>> + * notes - x-vfio-user-server could block IO and monitor during the
>>> + * initialization phase.
>>> */
>>> 
>>> #include "qemu/osdep.h"
>>> @@ -40,11 +43,14 @@
>>> #include "hw/remote/machine.h"
>>> #include "qapi/error.h"
>>> #include "qapi/qapi-visit-sockets.h"
>>> +#include "qapi/qapi-events-misc.h"
>>> #include "qemu/notify.h"
>>> +#include "qemu/thread.h"
>>> #include "sysemu/sysemu.h"
>>> #include "libvfio-user.h"
>>> #include "hw/qdev-core.h"
>>> #include "hw/pci/pci.h"
>>> +#include "qemu/timer.h"
>>> 
>>> #define TYPE_VFU_OBJECT "x-vfio-user-server"
>>> OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
>>> @@ -86,6 +92,8 @@ struct VfuObject {
>>> PCIDevice *pci_dev;
>>> 
>>> Error *unplug_blocker;
>>> +
>>> + int vfu_poll_fd;
>>> };
>>> 
>>> static void vfu_object_init_ctx(VfuObject *o, Error **errp);
>>> @@ -164,6 +172,76 @@ static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
>>> vfu_object_init_ctx(o, errp);
>>> }
>>> 
>>> +static void vfu_object_ctx_run(void *opaque)
>>> +{
>>> + VfuObject *o = opaque;
>>> + const char *vfu_id;
>>> + char *vfu_path, *pci_dev_path;
>>> + int ret = -1;
>>> +
>>> + while (ret != 0) {
>>> + ret = vfu_run_ctx(o->vfu_ctx);
>>> + if (ret < 0) {
>>> + if (errno == EINTR) {
>>> + continue;
>>> + } else if (errno == ENOTCONN) {
>>> + vfu_id = object_get_canonical_path_component(OBJECT(o));
>>> + vfu_path = object_get_canonical_path(OBJECT(o));
>> 
>> Hmm. @vfu_id is always the last component of @vfu_path. Why do we need
>> to send both?
>
> vfu_id is the ID that the user/Orchestrator passed as a command-line option
> during addition/creation. So it made sense to report back with the same ID
> that they used. But I’m OK with dropping this if that’s what you prefer.

Matter of taste, I guess.  I'd drop it simply to saves us the trouble of
documenting it.

If we decide to keep it, then I think we should document it's always the
last component of @vfu_path.

>>> + g_assert(o->pci_dev);
>>> + pci_dev_path = object_get_canonical_path(OBJECT(o->pci_dev));
>>> + qapi_event_send_vfu_client_hangup(vfu_id, vfu_path,
>>> + o->device, pci_dev_path);
>> 
>> Where is o->device set? I'm asking because I it must not be null here,
>> and that's not locally obvious.
>
> Yeah, it’s not obvious from this patch that o->device is guaranteed to be
> non-NULL. It’s set by vfu_object_set_device(). Please see the following
> patches in the series:
> vfio-user: define vfio-user-server object
> vfio-user: instantiate vfio-user context

vfu_object_set_device() is a QOM property setter.  It gets called if and
only if the property is set.  If it's never set, ->device remains null.
What ensures it's always set?

> There’s already an assert for o->pci_dev here, but we could add one
> for o->device too?

I'll make up my mind when I'm convinced o->device can't be null here.

> Thank you!

You're welcome!

Re: [PATCH v9 10/17] vfio-user: run vfio-user context

[Jag Raman]

> On May 5, 2022, at 3:44 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Jag Raman <jag.raman@oracle.com> writes:
> 
>>> On May 4, 2022, at 7:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> 
>>> Jagannathan Raman <jag.raman@oracle.com> writes:
>>> 
>>>> Setup a handler to run vfio-user context. The context is driven by
>>>> messages to the file descriptor associated with it - get the fd for
>>>> the context and hook up the handler with it
>>>> 
>>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>> qapi/misc.json | 30 +++++++++++
>>>> hw/remote/vfio-user-obj.c | 102 +++++++++++++++++++++++++++++++++++++-
>>>> 2 files changed, 131 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/qapi/misc.json b/qapi/misc.json
>>>> index b83cc39029..fa49f2876a 100644
>>>> --- a/qapi/misc.json
>>>> +++ b/qapi/misc.json
>>>> @@ -553,3 +553,33 @@
>>>> ##
>>>> { 'event': 'RTC_CHANGE',
>>>> 'data': { 'offset': 'int', 'qom-path': 'str' } }
>>>> +
>>>> +##
>>>> +# @VFU_CLIENT_HANGUP:
>>>> +#
>>>> +# Emitted when the client of a TYPE_VFIO_USER_SERVER closes the
>>>> +# communication channel
>>>> +#
>>>> +# @vfu-id: ID of the TYPE_VFIO_USER_SERVER object
>>>> +#
>>>> +# @vfu-qom-path: path to the TYPE_VFIO_USER_SERVER object in the QOM tree
>>>> +#
>>>> +# @dev-id: ID of attached PCI device
>>>> +#
>>>> +# @dev-qom-path: path to attached PCI device in the QOM tree
>>> 
>>> I'm still unsure what kind(s) of ID @vfu-id and @dev-id are. See below.
>> 
>> I’m not sure what you mean by kind of ID - I thought of ID as a
>> unique string. I’ll try my best to explain.
> 
> Okay, let me try to clarify.
> 
> We have many, many ID namespaces, each associated with a certain kind of
> object: device IDs for TYPE_DEVICE, object IDs for TYPE_OBJECT
> implementing TYPE_USER_CREATABLE), block backend node names for
> BlockDriverState, ...
> 
> Aside: I believe a single namespace would have been a wiser design
> choice, but that ship sailed long ago.
> 
> To which of these namespaces do these two IDs belong, respectively?
> 
>> dev-id and vfu-id are the “id" sub-options of “-device” and “-object” command-line
>> options respectively.
> 
> This answers my question.
> 
>> "dev-id” is the “id” member of “DeviceState” which QEMU sets using
>> qdev_set_id() when the device is added. 
>> 
>> The Object ID (vfu-id in this case) is a bit tricky. It’s also the “id”
>> command-line sub-option, but QEMU stores it as a child property
>> of the parent object.
>> 
>>> 
>>>> +#
>>>> +# Since: 7.1
>>>> +#
>>>> +# Example:
>>>> +#
>>>> +# <- { "event": "VFU_CLIENT_HANGUP",
>>>> +# "data": { "vfu-id": "vfu1",
>>>> +# "vfu-qom-path": "/objects/vfu1",
>>>> +# "dev-id": "sas1",
>>>> +# "dev-qom-path": "/machine/peripheral/sas1" },
>>>> +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>>>> +#
>>>> +##
>>>> +{ 'event': 'VFU_CLIENT_HANGUP',
>>>> + 'data': { 'vfu-id': 'str', 'vfu-qom-path': 'str',
>>>> + 'dev-id': 'str', 'dev-qom-path': 'str' } }
>>>> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>>>> index 3ca6aa2b45..3a4c6a9fa0 100644
>>>> --- a/hw/remote/vfio-user-obj.c
>>>> +++ b/hw/remote/vfio-user-obj.c
>>>> @@ -27,6 +27,9 @@
>>>> *
>>>> * device - id of a device on the server, a required option. PCI devices
>>>> * alone are supported presently.
>>>> + *
>>>> + * notes - x-vfio-user-server could block IO and monitor during the
>>>> + * initialization phase.
>>>> */
>>>> 
>>>> #include "qemu/osdep.h"
>>>> @@ -40,11 +43,14 @@
>>>> #include "hw/remote/machine.h"
>>>> #include "qapi/error.h"
>>>> #include "qapi/qapi-visit-sockets.h"
>>>> +#include "qapi/qapi-events-misc.h"
>>>> #include "qemu/notify.h"
>>>> +#include "qemu/thread.h"
>>>> #include "sysemu/sysemu.h"
>>>> #include "libvfio-user.h"
>>>> #include "hw/qdev-core.h"
>>>> #include "hw/pci/pci.h"
>>>> +#include "qemu/timer.h"
>>>> 
>>>> #define TYPE_VFU_OBJECT "x-vfio-user-server"
>>>> OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
>>>> @@ -86,6 +92,8 @@ struct VfuObject {
>>>> PCIDevice *pci_dev;
>>>> 
>>>> Error *unplug_blocker;
>>>> +
>>>> + int vfu_poll_fd;
>>>> };
>>>> 
>>>> static void vfu_object_init_ctx(VfuObject *o, Error **errp);
>>>> @@ -164,6 +172,76 @@ static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
>>>> vfu_object_init_ctx(o, errp);
>>>> }
>>>> 
>>>> +static void vfu_object_ctx_run(void *opaque)
>>>> +{
>>>> + VfuObject *o = opaque;
>>>> + const char *vfu_id;
>>>> + char *vfu_path, *pci_dev_path;
>>>> + int ret = -1;
>>>> +
>>>> + while (ret != 0) {
>>>> + ret = vfu_run_ctx(o->vfu_ctx);
>>>> + if (ret < 0) {
>>>> + if (errno == EINTR) {
>>>> + continue;
>>>> + } else if (errno == ENOTCONN) {
>>>> + vfu_id = object_get_canonical_path_component(OBJECT(o));
>>>> + vfu_path = object_get_canonical_path(OBJECT(o));
>>> 
>>> Hmm. @vfu_id is always the last component of @vfu_path. Why do we need
>>> to send both?
>> 
>> vfu_id is the ID that the user/Orchestrator passed as a command-line option
>> during addition/creation. So it made sense to report back with the same ID
>> that they used. But I’m OK with dropping this if that’s what you prefer.
> 
> Matter of taste, I guess.  I'd drop it simply to saves us the trouble of
> documenting it.
> 
> If we decide to keep it, then I think we should document it's always the
> last component of @vfu_path.
> 
>>>> + g_assert(o->pci_dev);
>>>> + pci_dev_path = object_get_canonical_path(OBJECT(o->pci_dev));
>>>> + qapi_event_send_vfu_client_hangup(vfu_id, vfu_path,
>>>> + o->device, pci_dev_path);
>>> 
>>> Where is o->device set? I'm asking because I it must not be null here,
>>> and that's not locally obvious.
>> 
>> Yeah, it’s not obvious from this patch that o->device is guaranteed to be
>> non-NULL. It’s set by vfu_object_set_device(). Please see the following
>> patches in the series:
>> vfio-user: define vfio-user-server object
>> vfio-user: instantiate vfio-user context
> 
> vfu_object_set_device() is a QOM property setter.  It gets called if and
> only if the property is set.  If it's never set, ->device remains null.
> What ensures it's always set?

That’s a good question - it’s not obvious from this patch.

The code would not reach here if o->device is not set. If o->device is NULL,
vfu_object_init_ctx() would bail out early without setting up
vfu_object_attach_ctx() and vfu_object_ctx_run() (this function) handlers.

Also, device is a required parameter. QEMU would not initialize this object
without it. Please see the definition of VfioUserServerProperties in the
following patch - noting that optional parameters are prefixed with a ‘*’:
[PATCH v9 07/17] vfio-user: define vfio-user-server object.

May be we should add a comment here to explain why o->device
wouldn’t be NULL?

Thank you!

> 
>> There’s already an assert for o->pci_dev here, but we could add one
>> for o->device too?
> 
> I'll make up my mind when I'm convinced o->device can't be null here.
> 
>> Thank you!
> 
> You're welcome!
> 

Re: [PATCH v9 10/17] vfio-user: run vfio-user context

[Markus Armbruster]

Jag Raman <jag.raman@oracle.com> writes:

>> On May 5, 2022, at 3:44 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> Jag Raman <jag.raman@oracle.com> writes:
>> 
>>>> On May 4, 2022, at 7:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> 
>>>> Jagannathan Raman <jag.raman@oracle.com> writes:
>>>> 
>>>>> Setup a handler to run vfio-user context. The context is driven by
>>>>> messages to the file descriptor associated with it - get the fd for
>>>>> the context and hook up the handler with it
>>>>> 
>>>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>>>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[...]

>>>>> @@ -164,6 +172,76 @@ static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
>>>>> vfu_object_init_ctx(o, errp);
>>>>> }
>>>>> 
>>>>> +static void vfu_object_ctx_run(void *opaque)
>>>>> +{
>>>>> + VfuObject *o = opaque;
>>>>> + const char *vfu_id;
>>>>> + char *vfu_path, *pci_dev_path;
>>>>> + int ret = -1;
>>>>> +
>>>>> + while (ret != 0) {
>>>>> + ret = vfu_run_ctx(o->vfu_ctx);
>>>>> + if (ret < 0) {
>>>>> + if (errno == EINTR) {
>>>>> + continue;
>>>>> + } else if (errno == ENOTCONN) {
>>>>> + vfu_id = object_get_canonical_path_component(OBJECT(o));
>>>>> + vfu_path = object_get_canonical_path(OBJECT(o));
>>>> 
>>>> Hmm. @vfu_id is always the last component of @vfu_path. Why do we need
>>>> to send both?
>>> 
>>> vfu_id is the ID that the user/Orchestrator passed as a command-line option
>>> during addition/creation. So it made sense to report back with the same ID
>>> that they used. But I’m OK with dropping this if that’s what you prefer.
>> 
>> Matter of taste, I guess.  I'd drop it simply to saves us the trouble of
>> documenting it.
>> 
>> If we decide to keep it, then I think we should document it's always the
>> last component of @vfu_path.
>> 
>>>>> + g_assert(o->pci_dev);
>>>>> + pci_dev_path = object_get_canonical_path(OBJECT(o->pci_dev));
>>>>> + qapi_event_send_vfu_client_hangup(vfu_id, vfu_path,
>>>>> + o->device, pci_dev_path);
>>>> 
>>>> Where is o->device set? I'm asking because I it must not be null here,
>>>> and that's not locally obvious.
>>> 
>>> Yeah, it’s not obvious from this patch that o->device is guaranteed to be
>>> non-NULL. It’s set by vfu_object_set_device(). Please see the following
>>> patches in the series:
>>> vfio-user: define vfio-user-server object
>>> vfio-user: instantiate vfio-user context
>> 
>> vfu_object_set_device() is a QOM property setter.  It gets called if and
>> only if the property is set.  If it's never set, ->device remains null.
>> What ensures it's always set?
>
> That’s a good question - it’s not obvious from this patch.
>
> The code would not reach here if o->device is not set. If o->device is NULL,
> vfu_object_init_ctx() would bail out early without setting up
> vfu_object_attach_ctx() and vfu_object_ctx_run() (this function)
> handlers.

Yes:

    static void vfu_object_init_ctx(VfuObject *o, Error **errp)
    {
        ERRP_GUARD();
        DeviceState *dev = NULL;
        vfu_pci_type_t pci_type = VFU_PCI_TYPE_CONVENTIONAL;
        int ret;

        if (o->vfu_ctx || !o->socket || !o->device ||
                !phase_check(PHASE_MACHINE_READY)) {
            return;
        }

Bails out without setting an error.  Sure that's appropriate?

> Also, device is a required parameter. QEMU would not initialize this object
> without it. Please see the definition of VfioUserServerProperties in the
> following patch - noting that optional parameters are prefixed with a ‘*’:
> [PATCH v9 07/17] vfio-user: define vfio-user-server object.
>
> May be we should add a comment here to explain why o->device
> wouldn’t be NULL?

Perhaps assertion with a comment explaining why it holds.

> Thank you!

You're welcome!

Re: [PATCH v9 07/17] vfio-user: define vfio-user-server object

[Stefan Hajnoczi]

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

On Wed, May 04, 2022 at 01:45:07PM +0200, Markus Armbruster wrote:
> Jagannathan Raman <jag.raman@oracle.com> writes:
> 
> > Define vfio-user object which is remote process server for QEMU. Setup
> > object initialization functions and properties necessary to instantiate
> > the object
> >
> > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> > ---
> >  qapi/qom.json               |  20 +++-
> >  include/hw/remote/machine.h |   2 +
> >  hw/remote/machine.c         |  27 +++++
> >  hw/remote/vfio-user-obj.c   | 210 ++++++++++++++++++++++++++++++++++++
> >  MAINTAINERS                 |   1 +
> >  hw/remote/meson.build       |   1 +
> >  hw/remote/trace-events      |   3 +
> >  7 files changed, 262 insertions(+), 2 deletions(-)
> >  create mode 100644 hw/remote/vfio-user-obj.c
> >
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index eeb5395ff3..582def0522 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -703,6 +703,20 @@
> >  { 'struct': 'RemoteObjectProperties',
> >    'data': { 'fd': 'str', 'devid': 'str' } }
> >  
> > +##
> > +# @VfioUserServerProperties:
> > +#
> > +# Properties for x-vfio-user-server objects.
> > +#
> > +# @socket: socket to be used by the libvfio-user library
> > +#
> > +# @device: the id of the device to be emulated at the server
> 
> Suggest "the ID", because "id" is not a word.

id (noun)
1. In Freudian theory, the division of the psyche that is totally unconscious and serves as the source of instinctual impulses and demands for immediate satisfaction of primitive needs.
2. In Weismann's doctrine of germ-plasm, the substance of inheritance or the bearer , in the germ-plasm, of the hereditary qualities of a single complete organism.
3. In the somatic idioplasm of the hereditary qualities of a group of cells or a part of a developing embryo or growing organism.
https://duckduckgo.com/?q=define+id&ia=definition

:D :D :D

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v9 07/17] vfio-user: define vfio-user-server object

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Wed, May 04, 2022 at 01:45:07PM +0200, Markus Armbruster wrote:
>> Jagannathan Raman <jag.raman@oracle.com> writes:
>> 
>> > Define vfio-user object which is remote process server for QEMU. Setup
>> > object initialization functions and properties necessary to instantiate
>> > the object
>> >
>> > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> > ---
>> >  qapi/qom.json               |  20 +++-
>> >  include/hw/remote/machine.h |   2 +
>> >  hw/remote/machine.c         |  27 +++++
>> >  hw/remote/vfio-user-obj.c   | 210 ++++++++++++++++++++++++++++++++++++
>> >  MAINTAINERS                 |   1 +
>> >  hw/remote/meson.build       |   1 +
>> >  hw/remote/trace-events      |   3 +
>> >  7 files changed, 262 insertions(+), 2 deletions(-)
>> >  create mode 100644 hw/remote/vfio-user-obj.c
>> >
>> > diff --git a/qapi/qom.json b/qapi/qom.json
>> > index eeb5395ff3..582def0522 100644
>> > --- a/qapi/qom.json
>> > +++ b/qapi/qom.json
>> > @@ -703,6 +703,20 @@
>> >  { 'struct': 'RemoteObjectProperties',
>> >    'data': { 'fd': 'str', 'devid': 'str' } }
>> >  
>> > +##
>> > +# @VfioUserServerProperties:
>> > +#
>> > +# Properties for x-vfio-user-server objects.
>> > +#
>> > +# @socket: socket to be used by the libvfio-user library
>> > +#
>> > +# @device: the id of the device to be emulated at the server
>> 
>> Suggest "the ID", because "id" is not a word.
>
> id (noun)
> 1. In Freudian theory, the division of the psyche that is totally unconscious and serves as the source of instinctual impulses and demands for immediate satisfaction of primitive needs.
> 2. In Weismann's doctrine of germ-plasm, the substance of inheritance or the bearer , in the germ-plasm, of the hereditary qualities of a single complete organism.
> 3. In the somatic idioplasm of the hereditary qualities of a group of cells or a part of a developing embryo or growing organism.
> https://duckduckgo.com/?q=define+id&ia=definition
>
> :D :D :D

I stand corrected!

%-)

Re: [PATCH v9 05/17] configure: require cmake 3.19 or newer

[Stefan Hajnoczi]

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

On Tue, May 03, 2022 at 10:16:46AM -0400, Jagannathan Raman wrote:
> cmake needs to accept the compiler flags specified with
> CMAKE_<LANG>_COMPILER variable. It does so starting with
> version 3.19
>
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> ---
>  configure | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v9 10/17] vfio-user: run vfio-user context

[Jag Raman]

> On May 5, 2022, at 10:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Jag Raman <jag.raman@oracle.com> writes:
> 
>>> On May 5, 2022, at 3:44 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> 
>>> Jag Raman <jag.raman@oracle.com> writes:
>>> 
>>>>> On May 4, 2022, at 7:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> 
>>>>> Jagannathan Raman <jag.raman@oracle.com> writes:
>>>>> 
>>>>>> Setup a handler to run vfio-user context. The context is driven by
>>>>>> messages to the file descriptor associated with it - get the fd for
>>>>>> the context and hook up the handler with it
>>>>>> 
>>>>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>>>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>>>>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> [...]
> 
>>>>>> @@ -164,6 +172,76 @@ static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
>>>>>> vfu_object_init_ctx(o, errp);
>>>>>> }
>>>>>> 
>>>>>> +static void vfu_object_ctx_run(void *opaque)
>>>>>> +{
>>>>>> + VfuObject *o = opaque;
>>>>>> + const char *vfu_id;
>>>>>> + char *vfu_path, *pci_dev_path;
>>>>>> + int ret = -1;
>>>>>> +
>>>>>> + while (ret != 0) {
>>>>>> + ret = vfu_run_ctx(o->vfu_ctx);
>>>>>> + if (ret < 0) {
>>>>>> + if (errno == EINTR) {
>>>>>> + continue;
>>>>>> + } else if (errno == ENOTCONN) {
>>>>>> + vfu_id = object_get_canonical_path_component(OBJECT(o));
>>>>>> + vfu_path = object_get_canonical_path(OBJECT(o));
>>>>> 
>>>>> Hmm. @vfu_id is always the last component of @vfu_path. Why do we need
>>>>> to send both?
>>>> 
>>>> vfu_id is the ID that the user/Orchestrator passed as a command-line option
>>>> during addition/creation. So it made sense to report back with the same ID
>>>> that they used. But I’m OK with dropping this if that’s what you prefer.
>>> 
>>> Matter of taste, I guess.  I'd drop it simply to saves us the trouble of
>>> documenting it.
>>> 
>>> If we decide to keep it, then I think we should document it's always the
>>> last component of @vfu_path.
>>> 
>>>>>> + g_assert(o->pci_dev);
>>>>>> + pci_dev_path = object_get_canonical_path(OBJECT(o->pci_dev));
>>>>>> + qapi_event_send_vfu_client_hangup(vfu_id, vfu_path,
>>>>>> + o->device, pci_dev_path);
>>>>> 
>>>>> Where is o->device set? I'm asking because I it must not be null here,
>>>>> and that's not locally obvious.
>>>> 
>>>> Yeah, it’s not obvious from this patch that o->device is guaranteed to be
>>>> non-NULL. It’s set by vfu_object_set_device(). Please see the following
>>>> patches in the series:
>>>> vfio-user: define vfio-user-server object
>>>> vfio-user: instantiate vfio-user context
>>> 
>>> vfu_object_set_device() is a QOM property setter.  It gets called if and
>>> only if the property is set.  If it's never set, ->device remains null.
>>> What ensures it's always set?
>> 
>> That’s a good question - it’s not obvious from this patch.
>> 
>> The code would not reach here if o->device is not set. If o->device is NULL,
>> vfu_object_init_ctx() would bail out early without setting up
>> vfu_object_attach_ctx() and vfu_object_ctx_run() (this function)
>> handlers.
> 
> Yes:
> 
>    static void vfu_object_init_ctx(VfuObject *o, Error **errp)
>    {
>        ERRP_GUARD();
>        DeviceState *dev = NULL;
>        vfu_pci_type_t pci_type = VFU_PCI_TYPE_CONVENTIONAL;
>        int ret;
> 
>        if (o->vfu_ctx || !o->socket || !o->device ||
>                !phase_check(PHASE_MACHINE_READY)) {
>            return;
>        }
> 
> Bails out without setting an error.  Sure that's appropriate?

It’s not an error per se - vfu_object_init_ctx() doesn’t proceed
further if device/socket is not set or if the machine is not ready.

Both socket and device are required properties, so they would
eventually be set by vfu_object_set_socket() /
vfu_object_set_device() - and these setters eventually kick
vfu_object_init_ctx().

> 
>> Also, device is a required parameter. QEMU would not initialize this object
>> without it. Please see the definition of VfioUserServerProperties in the
>> following patch - noting that optional parameters are prefixed with a ‘*’:
>> [PATCH v9 07/17] vfio-user: define vfio-user-server object.
>> 
>> May be we should add a comment here to explain why o->device
>> wouldn’t be NULL?
> 
> Perhaps assertion with a comment explaining why it holds.

OK, will do.

--
Jag

> 
>> Thank you!
> 
> You're welcome!
> 

Re: [PATCH v9 07/17] vfio-user: define vfio-user-server object

[Jag Raman]

> On May 5, 2022, at 11:22 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
>> On Wed, May 04, 2022 at 01:45:07PM +0200, Markus Armbruster wrote:
>>> Jagannathan Raman <jag.raman@oracle.com> writes:
>>> 
>>>> Define vfio-user object which is remote process server for QEMU. Setup
>>>> object initialization functions and properties necessary to instantiate
>>>> the object
>>>> 
>>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>>> ---
>>>> qapi/qom.json               |  20 +++-
>>>> include/hw/remote/machine.h |   2 +
>>>> hw/remote/machine.c         |  27 +++++
>>>> hw/remote/vfio-user-obj.c   | 210 ++++++++++++++++++++++++++++++++++++
>>>> MAINTAINERS                 |   1 +
>>>> hw/remote/meson.build       |   1 +
>>>> hw/remote/trace-events      |   3 +
>>>> 7 files changed, 262 insertions(+), 2 deletions(-)
>>>> create mode 100644 hw/remote/vfio-user-obj.c
>>>> 
>>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>>> index eeb5395ff3..582def0522 100644
>>>> --- a/qapi/qom.json
>>>> +++ b/qapi/qom.json
>>>> @@ -703,6 +703,20 @@
>>>> { 'struct': 'RemoteObjectProperties',
>>>>   'data': { 'fd': 'str', 'devid': 'str' } }
>>>> 
>>>> +##
>>>> +# @VfioUserServerProperties:
>>>> +#
>>>> +# Properties for x-vfio-user-server objects.
>>>> +#
>>>> +# @socket: socket to be used by the libvfio-user library
>>>> +#
>>>> +# @device: the id of the device to be emulated at the server
>>> 
>>> Suggest "the ID", because "id" is not a word.
>> 
>> id (noun)
>> 1. In Freudian theory, the division of the psyche that is totally unconscious and serves as the source of instinctual impulses and demands for immediate satisfaction of primitive needs.
>> 2. In Weismann's doctrine of germ-plasm, the substance of inheritance or the bearer , in the germ-plasm, of the hereditary qualities of a single complete organism.
>> 3. In the somatic idioplasm of the hereditary qualities of a group of cells or a part of a developing embryo or growing organism.
>> https://duckduckgo.com/?q=define+id&ia=definition
>> 
>> :D :D :D
> 
> I stand corrected!
> 
> %-)

You guys are funny. :)

> 

Re: [PATCH v9 06/17] vfio-user: build library

[Stefan Hajnoczi]

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

On Tue, May 03, 2022 at 10:16:47AM -0400, Jagannathan Raman wrote:
> diff --git a/subprojects/libvfio-user b/subprojects/libvfio-user
> new file mode 160000
> index 0000000000..030d2f6e79
> --- /dev/null
> +++ b/subprojects/libvfio-user
> @@ -0,0 +1 @@
> +Subproject commit 030d2f6e7978b8ca7577b81d4f48e2771bcd8f47
> diff --git a/tests/docker/dockerfiles/centos8.docker b/tests/docker/dockerfiles/centos8.docker
> index 4b20925bbf..300833d8e0 100644
> --- a/tests/docker/dockerfiles/centos8.docker
> +++ b/tests/docker/dockerfiles/centos8.docker
> @@ -23,6 +23,7 @@ RUN dnf update -y && \
>          capstone-devel \
>          ccache \
>          clang \
> +        cmake \

Good, CentOS 8 has CMake 3.20.0.

> diff --git a/tests/docker/dockerfiles/ubuntu2004.docker b/tests/docker/dockerfiles/ubuntu2004.docker
> index a3b38884e3..7c6131686a 100644
> --- a/tests/docker/dockerfiles/ubuntu2004.docker
> +++ b/tests/docker/dockerfiles/ubuntu2004.docker
> @@ -18,6 +18,7 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
>              ca-certificates \
>              ccache \
>              clang \
> +            cmake \

Ubuntu 20.04LTS has CMake 3.16.3:
https://packages.ubuntu.com/focal/cmake

That does not meet the minimum version requirement in this patch series
(3.19.0).

Please re-run container build to check if Ubuntu actually works.

Hopefully libvfio-user will support meson and CMake can be dropped from
this patch series.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v9 06/17] vfio-user: build library

[Daniel P. Berrangé]

On Thu, May 05, 2022 at 04:39:35PM +0100, Stefan Hajnoczi wrote:
> On Tue, May 03, 2022 at 10:16:47AM -0400, Jagannathan Raman wrote:
> > diff --git a/subprojects/libvfio-user b/subprojects/libvfio-user
> > new file mode 160000
> > index 0000000000..030d2f6e79
> > --- /dev/null
> > +++ b/subprojects/libvfio-user
> > @@ -0,0 +1 @@
> > +Subproject commit 030d2f6e7978b8ca7577b81d4f48e2771bcd8f47
> > diff --git a/tests/docker/dockerfiles/centos8.docker b/tests/docker/dockerfiles/centos8.docker
> > index 4b20925bbf..300833d8e0 100644
> > --- a/tests/docker/dockerfiles/centos8.docker
> > +++ b/tests/docker/dockerfiles/centos8.docker
> > @@ -23,6 +23,7 @@ RUN dnf update -y && \
> >          capstone-devel \
> >          ccache \
> >          clang \
> > +        cmake \
> 
> Good, CentOS 8 has CMake 3.20.0.
> 
> > diff --git a/tests/docker/dockerfiles/ubuntu2004.docker b/tests/docker/dockerfiles/ubuntu2004.docker
> > index a3b38884e3..7c6131686a 100644
> > --- a/tests/docker/dockerfiles/ubuntu2004.docker
> > +++ b/tests/docker/dockerfiles/ubuntu2004.docker
> > @@ -18,6 +18,7 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
> >              ca-certificates \
> >              ccache \
> >              clang \
> > +            cmake \
> 
> Ubuntu 20.04LTS has CMake 3.16.3:
> https://packages.ubuntu.com/focal/cmake
> 
> That does not meet the minimum version requirement in this patch series
> (3.19.0).
> 
> Please re-run container build to check if Ubuntu actually works.
> 
> Hopefully libvfio-user will support meson and CMake can be dropped from
> this patch series.

That's something I have proposed here:

  https://github.com/nutanix/libvfio-user/pull/666


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v9 07/17] vfio-user: define vfio-user-server object

[Stefan Hajnoczi]

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

On Tue, May 03, 2022 at 10:16:48AM -0400, Jagannathan Raman wrote:
> Define vfio-user object which is remote process server for QEMU. Setup
> object initialization functions and properties necessary to instantiate
> the object
> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> ---
>  qapi/qom.json               |  20 +++-
>  include/hw/remote/machine.h |   2 +
>  hw/remote/machine.c         |  27 +++++
>  hw/remote/vfio-user-obj.c   | 210 ++++++++++++++++++++++++++++++++++++
>  MAINTAINERS                 |   1 +
>  hw/remote/meson.build       |   1 +
>  hw/remote/trace-events      |   3 +
>  7 files changed, 262 insertions(+), 2 deletions(-)
>  create mode 100644 hw/remote/vfio-user-obj.c

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v9 14/17] vfio-user: handle PCI BAR accesses

[Stefan Hajnoczi]

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

On Tue, May 03, 2022 at 10:16:55AM -0400, Jagannathan Raman wrote:
> Determine the BARs used by the PCI device and register handlers to
> manage the access to the same.
> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> ---
>  include/exec/memory.h           |   3 +
>  hw/remote/vfio-user-obj.c       | 190 ++++++++++++++++++++++++++++++++
>  softmmu/physmem.c               |   4 +-
>  tests/qtest/fuzz/generic_fuzz.c |   9 +-
>  hw/remote/trace-events          |   3 +
>  5 files changed, 203 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v9 17/17] vfio-user: avocado tests for vfio-user

[Stefan Hajnoczi]

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

On Tue, May 03, 2022 at 10:16:58AM -0400, Jagannathan Raman wrote:
> +    def launch_client(self, kernel_path, initrd_path, kernel_command_line,
> +                      machine_type, socket, *opts):
> +        client_vm = self.get_vm()
> +        client_vm.set_console()
> +        client_vm.add_args('-machine', machine_type)
> +        client_vm.add_args('-accel', 'kvm')
> +        client_vm.add_args('-cpu', 'host')
> +        client_vm.add_args('-object',
> +                           'memory-backend-memfd,id=sysmem-file,size=2G')
> +        client_vm.add_args('--numa', 'node,memdev=sysmem-file')
> +        client_vm.add_args('-m', '2048')
> +        client_vm.add_args('-kernel', kernel_path,
> +                           '-initrd', initrd_path,
> +                           '-append', kernel_command_line)
> +        client_vm.add_args('-device',
> +                           'vfio-user-pci,socket='+socket)

This patch series is just the server. Does this test case work yet?

If not, please defer it to the client series.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v9 12/17] vfio-user: IOMMU support for remote device

[Stefan Hajnoczi]

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

On Tue, May 03, 2022 at 10:16:53AM -0400, Jagannathan Raman wrote:
> Assign separate address space for each device in the remote processes.
> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> ---
>  include/hw/remote/iommu.h |  40 ++++++++++++
>  hw/remote/iommu.c         | 131 ++++++++++++++++++++++++++++++++++++++
>  hw/remote/machine.c       |  13 +++-
>  MAINTAINERS               |   2 +
>  hw/remote/meson.build     |   1 +
>  5 files changed, 186 insertions(+), 1 deletion(-)
>  create mode 100644 include/hw/remote/iommu.h
>  create mode 100644 hw/remote/iommu.c

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v9 06/17] vfio-user: build library

[Peter Maydell]

On Thu, 5 May 2022 at 16:44, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Tue, May 03, 2022 at 10:16:47AM -0400, Jagannathan Raman wrote:
> Ubuntu 20.04LTS has CMake 3.16.3:
> https://packages.ubuntu.com/focal/cmake
>
> That does not meet the minimum version requirement in this patch series
> (3.19.0).
>
> Please re-run container build to check if Ubuntu actually works.
>
> Hopefully libvfio-user will support meson and CMake can be dropped from
> this patch series.

Yes, please. I really strongly don't want QEMU to acquire
a dependency on yet another build system.

-- PMM

Re: [PATCH v9 17/17] vfio-user: avocado tests for vfio-user

[Jag Raman]

> On May 5, 2022, at 12:04 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> On Tue, May 03, 2022 at 10:16:58AM -0400, Jagannathan Raman wrote:
>> +    def launch_client(self, kernel_path, initrd_path, kernel_command_line,
>> +                      machine_type, socket, *opts):
>> +        client_vm = self.get_vm()
>> +        client_vm.set_console()
>> +        client_vm.add_args('-machine', machine_type)
>> +        client_vm.add_args('-accel', 'kvm')
>> +        client_vm.add_args('-cpu', 'host')
>> +        client_vm.add_args('-object',
>> +                           'memory-backend-memfd,id=sysmem-file,size=2G')
>> +        client_vm.add_args('--numa', 'node,memdev=sysmem-file')
>> +        client_vm.add_args('-m', '2048')
>> +        client_vm.add_args('-kernel', kernel_path,
>> +                           '-initrd', initrd_path,
>> +                           '-append', kernel_command_line)
>> +        client_vm.add_args('-device',
>> +                           'vfio-user-pci,socket='+socket)
> 
> This patch series is just the server. Does this test case work yet?
> 
> If not, please defer it to the client series.

It needs the client to work - will defer this till the client series.

--
Jag

> 
> Stefan

Re: [PATCH v9 10/17] vfio-user: run vfio-user context

[Markus Armbruster]

Jag Raman <jag.raman@oracle.com> writes:

>> On May 5, 2022, at 10:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> Jag Raman <jag.raman@oracle.com> writes:
>> 
>>>> On May 5, 2022, at 3:44 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> 
>>>> Jag Raman <jag.raman@oracle.com> writes:
>>>> 
>>>>>> On May 4, 2022, at 7:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>> 
>>>>>> Jagannathan Raman <jag.raman@oracle.com> writes:
>>>>>> 
>>>>>>> Setup a handler to run vfio-user context. The context is driven by
>>>>>>> messages to the file descriptor associated with it - get the fd for
>>>>>>> the context and hook up the handler with it
>>>>>>> 
>>>>>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>>>>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>>>>>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>>>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> 
>> [...]
>> 
>>>>>>> @@ -164,6 +172,76 @@ static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
>>>>>>> vfu_object_init_ctx(o, errp);
>>>>>>> }
>>>>>>> 
>>>>>>> +static void vfu_object_ctx_run(void *opaque)
>>>>>>> +{
>>>>>>> + VfuObject *o = opaque;
>>>>>>> + const char *vfu_id;
>>>>>>> + char *vfu_path, *pci_dev_path;
>>>>>>> + int ret = -1;
>>>>>>> +
>>>>>>> + while (ret != 0) {
>>>>>>> + ret = vfu_run_ctx(o->vfu_ctx);
>>>>>>> + if (ret < 0) {
>>>>>>> + if (errno == EINTR) {
>>>>>>> + continue;
>>>>>>> + } else if (errno == ENOTCONN) {
>>>>>>> + vfu_id = object_get_canonical_path_component(OBJECT(o));
>>>>>>> + vfu_path = object_get_canonical_path(OBJECT(o));
>>>>>> 
>>>>>> Hmm. @vfu_id is always the last component of @vfu_path. Why do we need
>>>>>> to send both?
>>>>> 
>>>>> vfu_id is the ID that the user/Orchestrator passed as a command-line option
>>>>> during addition/creation. So it made sense to report back with the same ID
>>>>> that they used. But I’m OK with dropping this if that’s what you prefer.
>>>> 
>>>> Matter of taste, I guess.  I'd drop it simply to saves us the trouble of
>>>> documenting it.
>>>> 
>>>> If we decide to keep it, then I think we should document it's always the
>>>> last component of @vfu_path.
>>>> 
>>>>>>> + g_assert(o->pci_dev);
>>>>>>> + pci_dev_path = object_get_canonical_path(OBJECT(o->pci_dev));
>>>>>>> + qapi_event_send_vfu_client_hangup(vfu_id, vfu_path,
>>>>>>> + o->device, pci_dev_path);
>>>>>> 
>>>>>> Where is o->device set? I'm asking because I it must not be null here,
>>>>>> and that's not locally obvious.
>>>>> 
>>>>> Yeah, it’s not obvious from this patch that o->device is guaranteed to be
>>>>> non-NULL. It’s set by vfu_object_set_device(). Please see the following
>>>>> patches in the series:
>>>>> vfio-user: define vfio-user-server object
>>>>> vfio-user: instantiate vfio-user context
>>>> 
>>>> vfu_object_set_device() is a QOM property setter.  It gets called if and
>>>> only if the property is set.  If it's never set, ->device remains null.
>>>> What ensures it's always set?
>>> 
>>> That’s a good question - it’s not obvious from this patch.
>>> 
>>> The code would not reach here if o->device is not set. If o->device is NULL,
>>> vfu_object_init_ctx() would bail out early without setting up
>>> vfu_object_attach_ctx() and vfu_object_ctx_run() (this function)
>>> handlers.
>> 
>> Yes:
>> 
>>    static void vfu_object_init_ctx(VfuObject *o, Error **errp)
>>    {
>>        ERRP_GUARD();
>>        DeviceState *dev = NULL;
>>        vfu_pci_type_t pci_type = VFU_PCI_TYPE_CONVENTIONAL;
>>        int ret;
>> 
>>        if (o->vfu_ctx || !o->socket || !o->device ||
>>                !phase_check(PHASE_MACHINE_READY)) {
>>            return;
>>        }
>> 
>> Bails out without setting an error.  Sure that's appropriate?
>
> It’s not an error per se - vfu_object_init_ctx() doesn’t proceed
> further if device/socket is not set or if the machine is not ready.
>
> Both socket and device are required properties, so they would
> eventually be set by vfu_object_set_socket() /
> vfu_object_set_device() - and these setters eventually kick
> vfu_object_init_ctx().

Early returns from a function that sets error on failure triggers bug
spider sense, because forgetting to set an error on failure is a fairly
common mistake.

The root of the problem is of course that the function's contract is not
obvious.  The name vfu_object_init_ctx() suggests it initializes
something.  But it clearly doesn't unless certain conditions are met.
The reader is left to wonder whether that's a bug, or whether that's
what it is supposed to do.

A function contract spelling out when the function is supposed to do
what (including "nothing") would help.

[...]

Re: [PATCH v9 10/17] vfio-user: run vfio-user context

[Jag Raman]

> On May 6, 2022, at 1:44 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Jag Raman <jag.raman@oracle.com> writes:
> 
>>> On May 5, 2022, at 10:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> 
>>> Jag Raman <jag.raman@oracle.com> writes:
>>> 
>>>>> On May 5, 2022, at 3:44 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> 
>>>>> Jag Raman <jag.raman@oracle.com> writes:
>>>>> 
>>>>>>> On May 4, 2022, at 7:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>>> 
>>>>>>> Jagannathan Raman <jag.raman@oracle.com> writes:
>>>>>>> 
>>>>>>>> Setup a handler to run vfio-user context. The context is driven by
>>>>>>>> messages to the file descriptor associated with it - get the fd for
>>>>>>>> the context and hook up the handler with it
>>>>>>>> 
>>>>>>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>>>>>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>>>>>>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>>>>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> 
>>> [...]
>>> 
>>>>>>>> @@ -164,6 +172,76 @@ static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
>>>>>>>> vfu_object_init_ctx(o, errp);
>>>>>>>> }
>>>>>>>> 
>>>>>>>> +static void vfu_object_ctx_run(void *opaque)
>>>>>>>> +{
>>>>>>>> + VfuObject *o = opaque;
>>>>>>>> + const char *vfu_id;
>>>>>>>> + char *vfu_path, *pci_dev_path;
>>>>>>>> + int ret = -1;
>>>>>>>> +
>>>>>>>> + while (ret != 0) {
>>>>>>>> + ret = vfu_run_ctx(o->vfu_ctx);
>>>>>>>> + if (ret < 0) {
>>>>>>>> + if (errno == EINTR) {
>>>>>>>> + continue;
>>>>>>>> + } else if (errno == ENOTCONN) {
>>>>>>>> + vfu_id = object_get_canonical_path_component(OBJECT(o));
>>>>>>>> + vfu_path = object_get_canonical_path(OBJECT(o));
>>>>>>> 
>>>>>>> Hmm. @vfu_id is always the last component of @vfu_path. Why do we need
>>>>>>> to send both?
>>>>>> 
>>>>>> vfu_id is the ID that the user/Orchestrator passed as a command-line option
>>>>>> during addition/creation. So it made sense to report back with the same ID
>>>>>> that they used. But I’m OK with dropping this if that’s what you prefer.
>>>>> 
>>>>> Matter of taste, I guess.  I'd drop it simply to saves us the trouble of
>>>>> documenting it.
>>>>> 
>>>>> If we decide to keep it, then I think we should document it's always the
>>>>> last component of @vfu_path.
>>>>> 
>>>>>>>> + g_assert(o->pci_dev);
>>>>>>>> + pci_dev_path = object_get_canonical_path(OBJECT(o->pci_dev));
>>>>>>>> + qapi_event_send_vfu_client_hangup(vfu_id, vfu_path,
>>>>>>>> + o->device, pci_dev_path);
>>>>>>> 
>>>>>>> Where is o->device set? I'm asking because I it must not be null here,
>>>>>>> and that's not locally obvious.
>>>>>> 
>>>>>> Yeah, it’s not obvious from this patch that o->device is guaranteed to be
>>>>>> non-NULL. It’s set by vfu_object_set_device(). Please see the following
>>>>>> patches in the series:
>>>>>> vfio-user: define vfio-user-server object
>>>>>> vfio-user: instantiate vfio-user context
>>>>> 
>>>>> vfu_object_set_device() is a QOM property setter.  It gets called if and
>>>>> only if the property is set.  If it's never set, ->device remains null.
>>>>> What ensures it's always set?
>>>> 
>>>> That’s a good question - it’s not obvious from this patch.
>>>> 
>>>> The code would not reach here if o->device is not set. If o->device is NULL,
>>>> vfu_object_init_ctx() would bail out early without setting up
>>>> vfu_object_attach_ctx() and vfu_object_ctx_run() (this function)
>>>> handlers.
>>> 
>>> Yes:
>>> 
>>>   static void vfu_object_init_ctx(VfuObject *o, Error **errp)
>>>   {
>>>       ERRP_GUARD();
>>>       DeviceState *dev = NULL;
>>>       vfu_pci_type_t pci_type = VFU_PCI_TYPE_CONVENTIONAL;
>>>       int ret;
>>> 
>>>       if (o->vfu_ctx || !o->socket || !o->device ||
>>>               !phase_check(PHASE_MACHINE_READY)) {
>>>           return;
>>>       }
>>> 
>>> Bails out without setting an error.  Sure that's appropriate?
>> 
>> It’s not an error per se - vfu_object_init_ctx() doesn’t proceed
>> further if device/socket is not set or if the machine is not ready.
>> 
>> Both socket and device are required properties, so they would
>> eventually be set by vfu_object_set_socket() /
>> vfu_object_set_device() - and these setters eventually kick
>> vfu_object_init_ctx().
> 
> Early returns from a function that sets error on failure triggers bug
> spider sense, because forgetting to set an error on failure is a fairly
> common mistake.
> 
> The root of the problem is of course that the function's contract is not
> obvious.  The name vfu_object_init_ctx() suggests it initializes
> something.  But it clearly doesn't unless certain conditions are met.
> The reader is left to wonder whether that's a bug, or whether that's
> what it is supposed to do.
> 
> A function contract spelling out when the function is supposed to do
> what (including "nothing") would help.

Sure, will add a comment explaining what this function is
supposed to do.

--
Jag

> 
> [...]
> 

Re: [PATCH v9 06/17] vfio-user: build library

[Daniel P. Berrangé]

On Thu, May 05, 2022 at 05:17:01PM +0100, Peter Maydell wrote:
> On Thu, 5 May 2022 at 16:44, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Tue, May 03, 2022 at 10:16:47AM -0400, Jagannathan Raman wrote:
> > Ubuntu 20.04LTS has CMake 3.16.3:
> > https://packages.ubuntu.com/focal/cmake
> >
> > That does not meet the minimum version requirement in this patch series
> > (3.19.0).
> >
> > Please re-run container build to check if Ubuntu actually works.
> >
> > Hopefully libvfio-user will support meson and CMake can be dropped from
> > this patch series.
> 
> Yes, please. I really strongly don't want QEMU to acquire
> a dependency on yet another build system.

As of today, John has mergd my libvfio-user pull request to replace
cmake with meson, so we're sorted on that front now. 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v9 06/17] vfio-user: build library

[Jag Raman]

> On May 10, 2022, at 9:22 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> On Thu, May 05, 2022 at 05:17:01PM +0100, Peter Maydell wrote:
>> On Thu, 5 May 2022 at 16:44, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>> 
>>> On Tue, May 03, 2022 at 10:16:47AM -0400, Jagannathan Raman wrote:
>>> Ubuntu 20.04LTS has CMake 3.16.3:
>>> https://packages.ubuntu.com/focal/cmake
>>> 
>>> That does not meet the minimum version requirement in this patch series
>>> (3.19.0).
>>> 
>>> Please re-run container build to check if Ubuntu actually works.
>>> 
>>> Hopefully libvfio-user will support meson and CMake can be dropped from
>>> this patch series.
>> 
>> Yes, please. I really strongly don't want QEMU to acquire
>> a dependency on yet another build system.
> 
> As of today, John has mergd my libvfio-user pull request to replace
> cmake with meson, so we're sorted on that front now. 

That’s awesome! Will send patches to replace cmake with meson.

--
Jag

> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>