Hi On Wed, Dec 2, 2020 at 12:23 AM Jagannathan Raman wrote: > From: Elena Ufimtseva > > Defines a PCI Device proxy object as a child of TYPE_PCI_DEVICE. > > Signed-off-by: Elena Ufimtseva > Signed-off-by: Jagannathan Raman > Signed-off-by: John G Johnson > Reviewed-by: Stefan Hajnoczi > --- > include/hw/remote/proxy.h | 36 +++++++++++++++++ > hw/remote/proxy.c | 98 > +++++++++++++++++++++++++++++++++++++++++++++++ > MAINTAINERS | 2 + > hw/remote/meson.build | 1 + > 4 files changed, 137 insertions(+) > create mode 100644 include/hw/remote/proxy.h > create mode 100644 hw/remote/proxy.c > > diff --git a/include/hw/remote/proxy.h b/include/hw/remote/proxy.h > new file mode 100644 > index 0000000..923432a > --- /dev/null > +++ b/include/hw/remote/proxy.h > @@ -0,0 +1,36 @@ > +/* > + * Copyright © 2018, 2020 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 PROXY_H > +#define PROXY_H > + > +#include "hw/pci/pci.h" > +#include "io/channel.h" > + > +#define TYPE_PCI_PROXY_DEV "x-pci-proxy-dev" > + > +#define PCI_PROXY_DEV(obj) \ > + OBJECT_CHECK(PCIProxyDev, (obj), TYPE_PCI_PROXY_DEV) > +typedef struct PCIProxyDev PCIProxyDev; > + > +struct PCIProxyDev { > + PCIDevice parent_dev; > + char *fd; > + > + /* > + * Mutex used to protect the QIOChannel fd from > + * the concurrent access by the VCPUs since proxy > + * blocks while awaiting for the replies from the > + * process remote. > + */ > + QemuMutex io_mutex; > + QIOChannel *ioc; > + Error *migration_blocker; > +}; > + > +#endif /* PROXY_H */ > diff --git a/hw/remote/proxy.c b/hw/remote/proxy.c > new file mode 100644 > index 0000000..29100bc > --- /dev/null > +++ b/hw/remote/proxy.c > @@ -0,0 +1,98 @@ > +/* > + * Copyright © 2018, 2020 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 "qemu-common.h" > + > +#include "hw/remote/proxy.h" > +#include "hw/pci/pci.h" > +#include "qapi/error.h" > +#include "io/channel-util.h" > +#include "hw/qdev-properties.h" > +#include "monitor/monitor.h" > +#include "migration/blocker.h" > + > +static void proxy_set_socket(PCIProxyDev *pdev, int fd, Error **errp) > +{ > + pdev->ioc = qio_channel_new_fd(fd, errp); > +} > That one line function (called once) should be inlined. + > +static Property proxy_properties[] = { > + DEFINE_PROP_STRING("fd", PCIProxyDev, fd), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > Generally we put properties just above class_init, where it is used. +static void pci_proxy_dev_realize(PCIDevice *device, Error **errp) > +{ > (errp shouldn't be NULL, but ERRP_GUARD would be safer) + PCIProxyDev *dev = PCI_PROXY_DEV(device); > + int fd; > + > + if (dev->fd) { > + fd = monitor_fd_param(monitor_cur(), dev->fd, errp); > + if (fd == -1) { > + error_prepend(errp, "proxy: unable to parse fd: "); > + return; > + } > + proxy_set_socket(dev, fd, errp); > + } else { > + error_setg(errp, "fd parameter not specified for %s", > + DEVICE(device)->id); > + return; > We prefer early return, to keep the code easy to read. if (!dev->fd) { return error... } the_normal_thing(); ... + } > + > + error_setg(&dev->migration_blocker, "%s does not support migration", > + TYPE_PCI_PROXY_DEV); > + if (migrate_add_blocker(dev->migration_blocker, errp)) { > + error_free(dev->migration_blocker); > leave that free to dev_exit() ? > + error_free(*errp); > + dev->migration_blocker = NULL; > + error_setg(errp, "Failed to set migration blocker"); > Why not use the error from migrate_add_blocker? + } > + > + qemu_mutex_init(&dev->io_mutex); > + qio_channel_set_blocking(dev->ioc, true, NULL); > +} > + > +static void pci_proxy_dev_exit(PCIDevice *pdev) > +{ > + PCIProxyDev *dev = PCI_PROXY_DEV(pdev); > + > + qio_channel_close(dev->ioc, NULL); > on early return in realize, dev->ioc is NULL. This will crash. + > + migrate_del_blocker(dev->migration_blocker); > So is migration_blocker, but this should be safe with NULL > + > + error_free(dev->migration_blocker); > +} > + > +static void pci_proxy_dev_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + > + k->realize = pci_proxy_dev_realize; > + k->exit = pci_proxy_dev_exit; > + device_class_set_props(dc, proxy_properties); > +} > + > +static const TypeInfo pci_proxy_dev_type_info = { > + .name = TYPE_PCI_PROXY_DEV, > + .parent = TYPE_PCI_DEVICE, > + .instance_size = sizeof(PCIProxyDev), > + .class_init = pci_proxy_dev_class_init, > + .interfaces = (InterfaceInfo[]) { > + { INTERFACE_CONVENTIONAL_PCI_DEVICE }, > + { }, > + }, > +}; > + > +static void pci_proxy_dev_register_types(void) > +{ > + type_register_static(&pci_proxy_dev_type_info); > +} > + > +type_init(pci_proxy_dev_register_types) > diff --git a/MAINTAINERS b/MAINTAINERS > index 24cb36e..ebd1d1d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3148,6 +3148,8 @@ F: include/hw/remote/remote-obj.h > F: hw/remote/remote-obj.c > F: include/hw/remote/memory.h > F: hw/remote/memory.c > +F: hw/remote/proxy.c > +F: include/hw/remote/proxy.h > > Build and test automation > ------------------------- > diff --git a/hw/remote/meson.build b/hw/remote/meson.build > index 64da16c..569cd20 100644 > --- a/hw/remote/meson.build > +++ b/hw/remote/meson.build > @@ -4,6 +4,7 @@ remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: > files('machine.c')) > remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: > files('mpqemu-link.c')) > 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')) > > specific_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('memory.c')) > > -- > 1.8.3.1 > > -- Marc-André Lureau