Re: [PATCH 08/29] hw/core: Setup Remote Port I/O channels

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Feb 05, 2026 at 08:58:03PM +0100, Ruslan Ruslichenko wrote:
> From: Ruslan Ruslichenko <Ruslan_Ruslichenko@epam.com>
> 
> Add initialization of communication channels with
> remote peer.
> 
> This includes character device backend, which can be
> configured based on QOM properties or automatic socket
> creation based on the machine path. The patch also
> initializes the signaling event pipes.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> Signed-off-by: Takahiro Nakata <takahiro.nakata.wr@renesas.com>
> Signed-off-by: Ruslan Ruslichenko <Ruslan_Ruslichenko@epam.com>
> ---
>  hw/core/remote-port.c         | 187 ++++++++++++++++++++++++++++++++++
>  include/hw/core/remote-port.h |  16 +++
>  2 files changed, 203 insertions(+)
> 
> diff --git a/hw/core/remote-port.c b/hw/core/remote-port.c
> index c909a825f3..5154c1bc2a 100644
> --- a/hw/core/remote-port.c
> +++ b/hw/core/remote-port.c
> @@ -52,6 +52,58 @@
>  #define REMOTE_PORT_CLASS(klass)    \
>       OBJECT_CLASS_CHECK(RemotePortClass, (klass), TYPE_REMOTE_PORT)
>  
> +static char *rp_sanitize_prefix(RemotePort *s)
> +{
> +    char *sanitized_name;
> +    char *c;
> +
> +    sanitized_name = g_strdup(s->prefix);
> +    for (c = sanitized_name; *c != '\0'; c++) {
> +        if (*c == '/') {
> +            *c = '_';
> +        }
> +    }
> +    return sanitized_name;
> +}
> +
> +static char *rp_autocreate_chardesc(RemotePort *s, bool server)
> +{
> +    char *prefix;
> +    char *chardesc;
> +    int r;
> +
> +    prefix = rp_sanitize_prefix(s);
> +    r = asprintf(&chardesc, "unix:%s/qemu-rport-%s%s",
> +                 machine_path, prefix, server ? ",wait,server" : "");
> +    assert(r > 0);
> +    free(prefix);
> +    return chardesc;
> +}
> +
> +static Chardev *rp_autocreate_chardev(RemotePort *s, char *name)
> +{
> +    Chardev *chr = NULL;
> +    char *chardesc;
> +    char *s_path;
> +    int r;
> +
> +    r = asprintf(&s_path, "%s/qemu-rport-%s", machine_path,
> +                 rp_sanitize_prefix(s));
> +    assert(r > 0);
> +    if (g_file_test(s_path, G_FILE_TEST_EXISTS)) {
> +        chardesc = rp_autocreate_chardesc(s, false);
> +        chr = qemu_chr_new_noreplay(name, chardesc, false, NULL);
> +        free(chardesc);
> +    }
> +    free(s_path);
> +
> +    if (!chr) {
> +        chardesc = rp_autocreate_chardesc(s, true);
> +        chr = qemu_chr_new_noreplay(name, chardesc, false, NULL);
> +        free(chardesc);
> +    }
> +    return chr;
> +}
>  
>  static void rp_reset(DeviceState *dev)
>  {
> @@ -66,6 +118,127 @@ static void rp_reset(DeviceState *dev)
>  
>  static void rp_realize(DeviceState *dev, Error **errp)
>  {
> +    RemotePort *s = REMOTE_PORT(dev);
> +    int r;
> +    Error *err = NULL;
> +
> +    s->prefix = object_get_canonical_path(OBJECT(dev));
> +
> +    if (!qemu_chr_fe_get_driver(&s->chr)) {
> +        char *name;
> +        Chardev *chr = NULL;
> +        static int nr;
> +
> +        r = asprintf(&name, "rport%d", nr);
> +        nr++;
> +        assert(r > 0);
> +
> +        if (s->chrdev_id) {
> +            chr = qemu_chr_find(s->chrdev_id);
> +        }
> +
> +        if (chr) {
> +            /* Found the chardev via commandline */
> +        } else if (s->chardesc) {
> +            chr = qemu_chr_new(name, s->chardesc, NULL);
> +        } else {
> +            if (!machine_path) {
> +                error_report("%s: Missing chardesc prop."
> +                             " Forgot -machine-path?",
> +                             s->prefix);
> +                exit(EXIT_FAILURE);
> +            }
> +            chr = rp_autocreate_chardev(s, name);
> +        }

Having three different ways to configure the chardev rather
feels like overkill. IMHO it should be sufficient to just
accept thue chardev ID as a mandatory argument, and not
attempt to create chardevs from this code. That would
in turn seem to avoid the need for the -machine-path arg
to exist ?


> +#ifdef _WIN32
> +    /*
> +     * Create a socket connection between two sockets. We auto-bind
> +     * and read out the port selected by the kernel.
> +     */
> +    {
> +        char *name;
> +        SocketAddress *sock;
> +        int port;
> +        int listen_sk;
> +
> +        sock = socket_parse("127.0.0.1:0", &error_abort);
> +        listen_sk = socket_listen(sock, 1, &error_abort);

Please avoid introducing new usage of low level sockets APIs. The higher
level QIOChannelSocket APIs is preferred, that said.....

> +
> +        if (s->event.pipe.read < 0) {
> +            perror("socket read");
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        {
> +            struct sockaddr_in saddr;
> +            socklen_t slen = sizeof saddr;
> +            int r;
> +
> +            r = getsockname(listen_sk, (struct sockaddr *) &saddr, &slen);
> +            if (r < 0) {
> +                perror("getsockname");
> +                exit(EXIT_FAILURE);
> +            }
> +            port = htons(saddr.sin_port);
> +        }
> +
> +        name = g_strdup_printf("127.0.0.1:%d", port);
> +        s->event.pipe.write = inet_connect(name, &error_abort);
> +        g_free(name);
> +        if (s->event.pipe.write < 0) {
> +            perror("socket write");
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        for (;;) {
> +            struct sockaddr_in saddr;
> +            socklen_t slen = sizeof saddr;
> +            int fd;
> +
> +            slen = sizeof(saddr);
> +            fd = qemu_accept(listen_sk, (struct sockaddr *)&saddr, &slen);
> +            if (fd < 0 && errno != EINTR) {
> +                close(listen_sk);
> +                return;
> +            } else if (fd >= 0) {
> +                close(listen_sk);
> +                s->event.pipe.read = fd;
> +                break;
> +            }
> +        }
> +
> +        if (!qemu_set_blocking(s->event.pipe.read, false, &err)) {
> +            error_report("%s: Unable to set non-block for internal pipes",
> +                        s->prefix);
> +            exit(EXIT_FAILURE);
> +        }
> +    }
> +#else
> +    if (!g_unix_open_pipe(s->event.pipes, FD_CLOEXEC, NULL)) {
> +        error_report("%s: Unable to create remort-port internal pipes",
> +                    s->prefix);
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    if (!qemu_set_blocking(s->event.pipe.read, false, &err)) {
> +        error_report("%s: Unable to set non-block for internal pipes",
> +                    s->prefix);
> +        exit(EXIT_FAILURE);
> +    }
> +
> +#endif

... I can't help thinking we have this code "self event pipe" design
somewhere else in QEMU. Ideally we would have a helper API for this
task and avoid OS conditional code in this device impl.


>  }
>  
>  static void rp_unrealize(DeviceState *dev)
> @@ -73,6 +246,13 @@ static void rp_unrealize(DeviceState *dev)
>      RemotePort *s = REMOTE_PORT(dev);
>  
>      s->finalizing = true;
> +
> +    info_report("%s: Wait for remote-port to disconnect", s->prefix);
> +    qemu_chr_fe_disconnect(&s->chr);
> +
> +    close(s->event.pipe.read);
> +    close(s->event.pipe.write);
> +    object_unparent(OBJECT(s->chrdev));
>  }
>  
>  static const VMStateDescription vmstate_rp = {
> @@ -84,6 +264,12 @@ static const VMStateDescription vmstate_rp = {
>      }
>  };
>  
> +static Property rp_properties[] = {
> +    DEFINE_PROP_CHR("chardev", RemotePort, chr),
> +    DEFINE_PROP_STRING("chardesc", RemotePort, chardesc),
> +    DEFINE_PROP_STRING("chrdev-id", RemotePort, chrdev_id),
> +};

This really feels lik

> +
>  static void rp_prop_allow_set_link(const Object *obj, const char *name,
>                                     Object *val, Error **errp)
>  {
> @@ -112,6 +298,7 @@ static void rp_class_init(ObjectClass *klass, const void *data)
>      dc->realize = rp_realize;
>      dc->unrealize = rp_unrealize;
>      dc->vmsd = &vmstate_rp;
> +    device_class_set_props_n(dc, rp_properties, ARRAY_SIZE(rp_properties));
>  }
>  
>  static const TypeInfo rp_info = {
> diff --git a/include/hw/core/remote-port.h b/include/hw/core/remote-port.h
> index db71071c8e..0f40018cdb 100644
> --- a/include/hw/core/remote-port.h
> +++ b/include/hw/core/remote-port.h
> @@ -56,8 +56,24 @@ typedef struct RemotePortDeviceClass {
>  struct RemotePort {
>      DeviceState parent;
>  
> +    union {
> +       int pipes[2];
> +       struct {
> +           int read;
> +           int write;
> +       } pipe;
> +    } event;
> +    Chardev *chrdev;
> +    CharFrontend chr;
>      bool finalizing;
>  
> +    char *chardesc;
> +    char *chrdev_id;
> +
> +    const char *prefix;
> +    const char *remote_prefix;
> +
> +    uint32_t current_id;
>      bool reset_done;
>  
>  #define REMOTE_PORT_MAX_DEVS 1024
> -- 
> 2.43.0
> 
> 

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 :|