[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <28eaa4bd-a9ee-c415-57c4-a9a56ffeef18@quicinc.com>
Date: Wed, 2 Nov 2022 11:04:45 -0700
From: Elliot Berman <quic_eberman@...cinc.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jassi Brar <jassisinghbrar@...il.com>
CC: Bjorn Andersson <quic_bjorande@...cinc.com>,
Murali Nalajala <quic_mnalajal@...cinc.com>,
Trilok Soni <quic_tsoni@...cinc.com>,
"Srivatsa Vaddagiri" <quic_svaddagi@...cinc.com>,
Carl van Schaik <quic_cvanscha@...cinc.com>,
Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>,
Andy Gross <agross@...nel.org>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Jassi Brar <jassisinghbrar@...il.com>,
<linux-arm-kernel@...ts.infradead.org>,
Mark Rutland <mark.rutland@....com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Sudeep Holla <sudeep.holla@....com>,
Marc Zyngier <maz@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Jonathan Corbet <corbet@....net>,
"Will Deacon" <will@...nel.org>,
Catalin Marinas <catalin.marinas@....com>,
"Arnd Bergmann" <arnd@...db.de>,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Amol Maheshwari <amahesh@....qualcomm.com>,
Kalle Valo <kvalo@...nel.org>, <devicetree@...r.kernel.org>,
<linux-doc@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 10/21] gunyah: rsc_mgr: Add resource manager RPC core
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@...cinc.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@...nel.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.
Powered by blists - more mailing lists