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

[Elliot Berman <quic_eberman@quicinc.com>]

On 11/1/2022 7:53 PM, Greg Kroah-Hartman wrote:
> 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.
> 

Ack.

I'll also end up removing CONFIG_GUNYAH_MESSAGE_QUEUES from patch 9 as well.

>>>> +/* 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.
> 

This is what's on the "wire". Whether I use bitfield or bit packed would 
be functionally the same and is just a cosmetic change IMO.

>>>> +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.
> 

Ack

>>>> +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.
> 

Ack

>>>> +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?
> 

That's correct.