Re: [PATCH v7 10/20] gunyah: rsc_mgr: Add resource manager RPC core

[Elliot Berman]

On 11/21/2022 7:23 AM, Greg Kroah-Hartman wrote:
> On Mon, Nov 21, 2022 at 05:59:59AM -0800, Elliot Berman wrote:
> > The resource manager is a special virtual machine which is always
> > running on a Gunyah system. It provides APIs for creating and destroying
> > VMs, secure memory management, sharing/lending of memory between VMs,
> > and setup of inter-VM communication. Calls to the resource manager are
> > made via message queues.
> >
> > This patch implements the basic probing and RPC mechanism to make those
> > API calls. Request/response calls can be made with gh_rm_call.
> > Drivers can also register to notifications pushed by RM via
> > gh_rm_register_notifier
> >
> > Specific API calls that resource manager supports will be implemented in
> > subsequent patches.
> >
> > Signed-off-by: Elliot Berman <quic_eberman@xxxxxxxxxxx>
> > ---
> >   MAINTAINERS                          |   2 +-
> >   drivers/virt/gunyah/Kconfig          |   7 +
> >   drivers/virt/gunyah/Makefile         |   2 +
> >   drivers/virt/gunyah/gunyah_rm_rpc.c  | 570 +++++++++++++++++++++++++++
> >   drivers/virt/gunyah/gunyah_rsc_mgr.c |  50 +++
> >   drivers/virt/gunyah/rsc_mgr.h        |  37 ++
> >   include/linux/gunyah_rsc_mgr.h       |  18 +
> >   7 files changed, 685 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/virt/gunyah/gunyah_rm_rpc.c
> >   create mode 100644 drivers/virt/gunyah/gunyah_rsc_mgr.c
> >   create mode 100644 drivers/virt/gunyah/rsc_mgr.h
> >   create mode 100644 include/linux/gunyah_rsc_mgr.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 502798197b80..b65f7ff444e5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8948,7 +8948,7 @@ F:	Documentation/virt/gunyah/
> >   F:	arch/arm64/gunyah/
> >   F:	drivers/mailbox/gunyah-msgq.c
> >   F:	drivers/virt/gunyah/
> > -F:	include/linux/gunyah.h
> > +F:	include/linux/gunyah*.h
> >   
> >   HABANALABS PCI DRIVER
> >   M:	Oded Gabbay <ogabbay@xxxxxxxxxx>
> > diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig
> > index 127156a678a6..0bb497372d4e 100644
> > --- a/drivers/virt/gunyah/Kconfig
> > +++ b/drivers/virt/gunyah/Kconfig
> > @@ -10,3 +10,10 @@ config GUNYAH
> >   
> >   	  Say Y/M here to enable the drivers needed to interact in a Gunyah
> >   	  virtual environment.
> > +
> > +if GUNYAH
> > +config GUNYAH_RESOURCE_MANAGER
> > +	tristate
> > +	depends on MAILBOX
> > +	select GUNYAH_MESSAGE_QUEUES
> > +endif
> > diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile
> > index 2ac4ee64b89d..b62ac4045621 100644
> > --- a/drivers/virt/gunyah/Makefile
> > +++ b/drivers/virt/gunyah/Makefile
> > @@ -1 +1,3 @@
> >   obj-$(CONFIG_GUNYAH) += gunyah.o
> > +
> > +obj-$(CONFIG_GUNYAH_RESOURCE_MANAGER) += gunyah_rsc_mgr.o gunyah_rm_rpc.o
> > diff --git a/drivers/virt/gunyah/gunyah_rm_rpc.c b/drivers/virt/gunyah/gunyah_rm_rpc.c
> > new file mode 100644
> > index 000000000000..45b1a8691982
> > --- /dev/null
> > +++ b/drivers/virt/gunyah/gunyah_rm_rpc.c
> > @@ -0,0 +1,570 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/slab.h>
> > +#include <linux/mutex.h>
> > +#include <linux/sched.h>
> > +#include <linux/gunyah.h>
> > +#include <linux/module.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/kthread.h>
> > +#include <linux/notifier.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/completion.h>
> > +#include <linux/gunyah_rsc_mgr.h>
> > +#include <linux/platform_device.h>
>
> This should not have anything to do with a platform device, please see
> below.
>
> > +#include <linux/gunyah_rsc_mgr.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "rsc_mgr.h"
> > +
> > +static int gh_rm_drv_probe(struct platform_device *pdev)
>
> Why is this tied to a platformm device?
>
> I don't understand the relationship here, sorry.
> >> +{
> > +	struct gh_rm_rpc *rsc_mgr;
> > +
> > +	rsc_mgr = gh_rm_rpc_init(pdev);
>
> Shouldn't this be creating a new one that is just a child passed in?
> Shouldn't this call just take a 'struct device *'?

I'm following the suggestion from Arnd to have small core module that 
calls into initialization routines for the other parts of the driver, 
rather than creating bus for a few (2) devices.

https://lore.kernel.org/all/a3754259-9989-495e-a6bd-5501daff06a2@xxxxxxxxxxxxxxxx/

Thanks,
Elliot