Re: [PATCH RESEND v6 17/36] multi-process: introduce proxy object

From: Jag Raman <jag.raman@oracle.com>
To: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Elena Ufimtseva <elena.ufimtseva@oracle.com>,
	fam@euphon.net, swapnil.ingle@nutanix.com,
	John G Johnson <john.g.johnson@oracle.com>,
	qemu-devel@nongnu.org, kraxel@redhat.com, quintela@redhat.com,
	mst@redhat.com, armbru@redhat.com, kanth.ghatraju@oracle.com,
	felipe@nutanix.com, thuth@redhat.com, ehabkost@redhat.com,
	konrad.wilk@oracle.com, dgilbert@redhat.com,
	liran.alon@oracle.com, thanos.makatos@nutanix.com,
	rth@twiddle.net, kwolf@redhat.com, berrange@redhat.com,
	mreitz@redhat.com, ross.lagerwall@citrix.com,
	marcandre.lureau@gmail.com, pbonzini@redhat.com
Subject: Re: [PATCH RESEND v6 17/36] multi-process: introduce proxy object
Date: Tue, 12 May 2020 08:35:15 -0400	[thread overview]
Message-ID: <73837C4E-28A6-4A65-9626-86498A5D5A3B@oracle.com> (raw)
In-Reply-To: <20200512122327.GG300009@stefanha-x1.localdomain>

> On May 12, 2020, at 8:23 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> On Wed, Apr 22, 2020 at 09:13:52PM -0700, elena.ufimtseva@oracle.com wrote:
>> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> 
>> Defines a PCI Device proxy object as a parent of TYPE_PCI_DEVICE.
> 
> s/parent/child/
> 
>> 
>> PCI Proxy Object registers as a PCI device with QEMU and forwards all
>> PCI accesses to the remote process using the communication channel.
> 
> Please include that functionality in this patch. The code below just
> sets up a skeleton PCI device. There is no code that forwards accesses
> to the remote process.
> 
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> ---
>> MAINTAINERS                   |  3 ++
>> hw/Makefile.objs              |  2 ++
>> hw/proxy/Makefile.objs        |  1 +
>> hw/proxy/qemu-proxy.c         | 56 +++++++++++++++++++++++++++++++++++
>> include/hw/proxy/qemu-proxy.h | 46 ++++++++++++++++++++++++++++
>> include/io/mpqemu-link.h      |  1 +
>> 6 files changed, 109 insertions(+)
>> create mode 100644 hw/proxy/Makefile.objs
>> create mode 100644 hw/proxy/qemu-proxy.c
>> create mode 100644 include/hw/proxy/qemu-proxy.h
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 96f8d7ff19..3da3dcd311 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2866,6 +2866,9 @@ F: include/remote/machine.h
>> F: remote/machine.c
>> F: include/remote/memory.h
>> F: remote/memory.c
>> +F: hw/proxy/Makefile.objs
>> +F: hw/proxy/qemu-proxy.c
>> +F: include/hw/proxy/qemu-proxy.h
> 
> It's a generic PCI device. hw/pci/proxy.c would be a good location for
> it.
> 
> By the way an alternative to the "proxy"/"remote" terminology is
> RemotePCIClient/RemotePCIServer. That makes it more obvious that "proxy"
> is related the "remote" feature. Feel free to keep the existing
> terminology, I just wanted to suggest another possibility.

OK, got it.

> 
>> 
>> Build and test automation
>> -------------------------
>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>> index af9235b6f2..7b489b12a5 100644
>> --- a/hw/Makefile.objs
>> +++ b/hw/Makefile.objs
>> @@ -45,6 +45,8 @@ endif
>> common-obj-y += $(devices-dirs-y)
>> obj-y += $(devices-dirs-y)
>> 
>> +common-obj-$(CONFIG_MPQEMU) += proxy/
>> +
>> remote-pci-obj-$(CONFIG_MPQEMU) += core/
>> remote-pci-obj-$(CONFIG_MPQEMU) += block/
>> remote-pci-obj-$(CONFIG_MPQEMU) += pci/
>> diff --git a/hw/proxy/Makefile.objs b/hw/proxy/Makefile.objs
>> new file mode 100644
>> index 0000000000..eb81624cf8
>> --- /dev/null
>> +++ b/hw/proxy/Makefile.objs
>> @@ -0,0 +1 @@
>> +common-obj-$(CONFIG_MPQEMU) += qemu-proxy.o
>> diff --git a/hw/proxy/qemu-proxy.c b/hw/proxy/qemu-proxy.c
>> new file mode 100644
>> index 0000000000..bf6c4117ef
>> --- /dev/null
>> +++ b/hw/proxy/qemu-proxy.c
>> @@ -0,0 +1,56 @@
>> +/*
>> + * 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 "qapi/error.h"
>> +#include "io/mpqemu-link.h"
>> +#include "hw/proxy/qemu-proxy.h"
>> +#include "hw/pci/pci.h"
>> +
>> +static void pci_proxy_dev_realize(PCIDevice *device, Error **errp)
>> +{
>> +    PCIProxyDev *dev = PCI_PROXY_DEV(device);
>> +    PCIProxyDevClass *k = PCI_PROXY_DEV_GET_CLASS(dev);
>> +    Error *local_err = NULL;
>> +
>> +    if (k->realize) {
> 
> Will anything inherit from this class? I thought this is the remote PCI
> client that can acts as a stand-in for all remote PCI devices, so it's
> not clear why it's acting as a base class here.

No one is inheriting from this class anymore. This is code from before
when that was the case. We could remove this.

> 
>> diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
>> index d46cb81058..73cc59b874 100644
>> --- a/include/io/mpqemu-link.h
>> +++ b/include/io/mpqemu-link.h
>> @@ -14,6 +14,7 @@
>> #include "qemu/osdep.h"
>> #include "qemu-common.h"
>> 
>> +#include "sys/eventfd.h"
> 
> Why? Nothing in this patch uses this header.

OK, got it.

Thanks!
—
Jag