[PULL,v5,06/11] hw/xen/xen-hvm-common: skip ioreq creation on ioreq registration failure

Commit Message

Stefano Stabellini June 15, 2023, 11:52 p.m. UTC

From: Stefano Stabellini <[e-mail]>

On ARM it is possible to have a functioning xenpv machine with only the
PV backends and no IOREQ server. If the IOREQ server creation fails continue
to the PV backends initialization.

Also, moved the IOREQ registration and mapping subroutine to new function
xen_do_ioreq_register().

Signed-off-by: Stefano Stabellini <[e-mail]>
Signed-off-by: Vikram Garhwal <[e-mail]>
Reviewed-by: Stefano Stabellini <[e-mail]>
Reviewed-by: Paul Durrant <[e-mail]>
---
 hw/xen/xen-hvm-common.c | 57 +++++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 19 deletions(-)

Comments

Peter Maydell June 20, 2023, 10:40 a.m. UTC | #1

On Fri, 16 Jun 2023 at 00:53, Stefano Stabellini <[e-mail]> wrote:
>
> From: Stefano Stabellini <[e-mail]>
>
> On ARM it is possible to have a functioning xenpv machine with only the
> PV backends and no IOREQ server. If the IOREQ server creation fails continue
> to the PV backends initialization.
>
> Also, moved the IOREQ registration and mapping subroutine to new function
> xen_do_ioreq_register().

Hi. This refactoring has prompted Coverity to notice that
we're passing around a large structure by value here:
MemoryListener is 192 bytes in size, and we pass it by value
into xen_register_ioreq() and now through into xen_do_ioreq_register().

Does this really need to be pass by value? Could we pass a pointer
instead?

(CID 1513106, 1513107).

I suspect also that the argument should be a const pointer,
and the callsites should mark the structs they're passing as
const.

> -void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
> -                        MemoryListener xen_memory_listener)
> +static void xen_do_ioreq_register(XenIOState *state,
> +                                           unsigned int max_cpus,
> +                                           MemoryListener xen_memory_listener)

thanks
-- PMM

Patch

diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index a31b067404..cb82f4b83d 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -764,27 +764,12 @@  void xen_shutdown_fatal_error(const char *fmt, ...)
     qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
 }
 
-void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
-                        MemoryListener xen_memory_listener)
+static void xen_do_ioreq_register(XenIOState *state,
+                                           unsigned int max_cpus,
+                                           MemoryListener xen_memory_listener)
 {
     int i, rc;
 
-    setup_xen_backend_ops();
-
-    state->xce_handle = qemu_xen_evtchn_open();
-    if (state->xce_handle == NULL) {
-        perror("xen: event channel open");
-        goto err;
-    }
-
-    state->xenstore = xs_daemon_open();
-    if (state->xenstore == NULL) {
-        perror("xen: xenstore open");
-        goto err;
-    }
-
-    xen_create_ioreq_server(xen_domid, &state->ioservid);
-
     state->exit.notify = xen_exit_notifier;
     qemu_add_exit_notifier(&state->exit);
 
@@ -849,12 +834,46 @@  void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
     QLIST_INIT(&state->dev_list);
     device_listener_register(&state->device_listener);
 
+    return;
+
+err:
+    error_report("xen hardware virtual machine initialisation failed");
+    exit(1);
+}
+
+void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
+                        MemoryListener xen_memory_listener)
+{
+    int rc;
+
+    setup_xen_backend_ops();
+
+    state->xce_handle = qemu_xen_evtchn_open();
+    if (state->xce_handle == NULL) {
+        perror("xen: event channel open");
+        goto err;
+    }
+
+    state->xenstore = xs_daemon_open();
+    if (state->xenstore == NULL) {
+        perror("xen: xenstore open");
+        goto err;
+    }
+
+    rc = xen_create_ioreq_server(xen_domid, &state->ioservid);
+    if (!rc) {
+        xen_do_ioreq_register(state, max_cpus, xen_memory_listener);
+    } else {
+        warn_report("xen: failed to create ioreq server");
+    }
+
     xen_bus_init();
 
     xen_be_init();
 
     return;
+
 err:
-    error_report("xen hardware virtual machine initialisation failed");
+    error_report("xen hardware virtual machine backend registration failed");
     exit(1);
 }