Re: [PATCH v6 10/21] gunyah: rsc_mgr: Add resource manager RPC core

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, Nov 01, 2022 at 05:12:58PM -0700, Elliot Berman wrote:
> 
> 
> On 11/1/2022 11:02 AM, Greg Kroah-Hartman wrote:
> > On Wed, Oct 26, 2022 at 11:58:35AM -0700, 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@quicinc.com>
> > > ---
> > >   MAINTAINERS                    |   2 +-
> > >   drivers/virt/gunyah/Kconfig    |  15 +
> > >   drivers/virt/gunyah/Makefile   |   3 +
> > >   drivers/virt/gunyah/rsc_mgr.c  | 602 +++++++++++++++++++++++++++++++++
> > >   drivers/virt/gunyah/rsc_mgr.h  |  34 ++
> > >   include/linux/gunyah_rsc_mgr.h |  26 ++
> > >   6 files changed, 681 insertions(+), 1 deletion(-)
> > >   create mode 100644 drivers/virt/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 586539eadd3b..e072a0d2e553 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -8945,7 +8945,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@kernel.org>
> > > diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig
> > > index 127156a678a6..4de88d80aa7b 100644
> > > --- a/drivers/virt/gunyah/Kconfig
> > > +++ b/drivers/virt/gunyah/Kconfig
> > > @@ -10,3 +10,18 @@ config GUNYAH
> > >   	  Say Y/M here to enable the drivers needed to interact in a Gunyah
> > >   	  virtual environment.
> > > +
> > > +config GUNYAH_RESORUCE_MANAGER
> > > +	tristate "Gunyah Resource Manager"
> > > +	select MAILBOX
> > > +	select GUNYAH_MESSAGE_QUEUES
> > > +	depends on GUNYAH
> > > +	default y
> > 
> > You only have "default y" if your machine can not boot without it.
> > Please do not add that here.
> > 
> 
> There's a guideline in Documentation/kbuild/kconfig-language.rst to provide
> some sane defaults for subdriver behavior. Here, CONFIG_GUNYAH is default n.
> It's unlikely for someone to want to have Linux with base Gunyah support
> (hypercalls and hypervisor detection) without also having the Resource
> Manager driver. If it's better, I could change to default m?

Why is this a separate build option at all anyway?  If you want
CONFIG_GUNYAH why would you ever turn this off?  So why even allow it to
be an option?  Just always built it depending on the main option.

> > > +/* Resource Manager Header */
> > > +struct gh_rm_rpc_hdr {
> > > +	u8 version : 4, hdr_words : 4;
> > > +	u8 type : 2, fragments : 6;
> > 
> > Ick, that's hard to read.  One variable per line please?
> 
> Ack.
> 
> > And why the bit packed stuff?  Are you sure this is the way to do this?
> > Why not use a bitmask instead?
> > 
> 
> I felt bit packed implementation is cleaner and easier to map to
> understanding what the fields are used for.

Ah, so this isn't what is on the "wire", then don't use a bitfield like
this, use a real variable and that will be faster and simpler to
understand.

> > > +static struct gh_rsc_mgr *__rsc_mgr;
> > 
> > Sorry, no, you don't get to just limit yourself to one of these.  Please
> > make this properly handle any number of "resource managers", static
> > variables like this is not ok.
> > 
> 
> There will only ever be one resource manager. optee, psci, and qcom_scm use
> a similar approach.

And all of those are also wrong.

There is no need for this variable at all, you are doing extra work to
make this a "single" device.  Just always work off of the device that
the driver core gave you and all is good and you will have no limits on
how many different ones you eventually get.  It will be less code
overall, so it's the right thing to do.

> > > +SRCU_NOTIFIER_HEAD_STATIC(gh_rm_notifier);
> > 
> > Why do you need a notifier list?
> > 
> > Who will register for this?  For what?  Why?
> > 
> 
> The majority of notifications that RM sends to Linux will be related to VM
> state, e.g. "VM crashed." I've not added the handling in VM manager yet to
> reduce the number of patches in this series. It was used in the previous
> series for the console driver. I can remove for now and re-introduce it once
> VM manager makes use?

Please remove if you are not using it.  Notifier lists are almost always
wrong when it comes to the driver model, so please don't add them now,
we can discuss it later if you feel it really needs to be introduced
then.

> > > +static struct platform_driver gh_rm_driver = {
> > > +	.probe = gh_rm_drv_probe,
> > > +	.remove = gh_rm_drv_remove,
> > > +	.driver = {
> > > +		.name = "gh_rsc_mgr",
> > > +		.of_match_table = gh_rm_of_match,
> > > +	},
> > 
> > Wait, why is this a platform driver?  This is binding to a real device
> > on a real bus, not a random platform description in DT, right?
> 
> This a binding for a real device and not a "random platform description" in
> DT to get the driver probed.
> 
> > Or is it controlled by your DT?  I can't figure that out here, sorry.
> 
> There is some info in Patch 2 about why the DT node exists and how it looks.
> Essentially, The DT node is provided by Gunyah during boot and describes how
> Linux can communicate with resource manager.

Ick, ok, for now let's leave this alone but for dynamic devices, you
should never use a platform device.  All devices that hang off of this
controller better not be platform devices, but belong to the bus type of
your new bus, right?

thanks,

greg k-h