[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC v1 3/5] xen/arm: introduce SCMI-SMC mediator driver
On Wed, Dec 22, 2021 at 06:23:24PM -0800, Stefano Stabellini wrote: > On Wed, 22 Dec 2021, Oleksii Moisieiev wrote: > > On Tue, Dec 21, 2021 at 01:22:50PM -0800, Stefano Stabellini wrote: > > > On Tue, 21 Dec 2021, Oleksii Moisieiev wrote: > > > > Hi Stefano, > > > > > > > > On Mon, Dec 20, 2021 at 04:52:01PM -0800, Stefano Stabellini wrote: > > > > > On Mon, 20 Dec 2021, Oleksii Moisieiev wrote: > > > > > > Hi Stefano, > > > > > > > > > > > > On Fri, Dec 17, 2021 at 06:14:55PM -0800, Stefano Stabellini wrote: > > > > > > > On Tue, 14 Dec 2021, Oleksii Moisieiev wrote: > > > > > > > > This is the implementation of SCI interface, called SCMI-SMC > > > > > > > > driver, > > > > > > > > which works as the mediator between XEN Domains and Firmware > > > > > > > > (SCP, ATF etc). > > > > > > > > This allows devices from the Domains to work with clocks, > > > > > > > > resets and > > > > > > > > power-domains without access to CPG. > > > > > > > > > > > > > > > > The following features are implemented: > > > > > > > > - request SCMI channels from ATF and pass channels to Domains; > > > > > > > > - set device permissions for Domains based on the Domain partial > > > > > > > > device-tree. Devices with permissions are able to work with > > > > > > > > clocks, > > > > > > > > resets and power-domains via SCMI; > > > > > > > > - redirect scmi messages from Domains to ATF. > > > > > > > > > > > > > > > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx> > > > > > > > > --- > > > > > > > > xen/arch/arm/Kconfig | 2 + > > > > > > > > xen/arch/arm/sci/Kconfig | 10 + > > > > > > > > xen/arch/arm/sci/Makefile | 1 + > > > > > > > > xen/arch/arm/sci/scmi_smc.c | 795 > > > > > > > > ++++++++++++++++++++++++++++++++++ > > > > > > > > xen/include/public/arch-arm.h | 1 + > > > > > > > > 5 files changed, 809 insertions(+) > > > > > > > > create mode 100644 xen/arch/arm/sci/Kconfig > > > > > > > > create mode 100644 xen/arch/arm/sci/scmi_smc.c > > > > > > > > > > > > > > > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > > > > > > > > index 186e1db389..02d96c6cfc 100644 > > > > > > > > --- a/xen/arch/arm/Kconfig > > > > > > > > +++ b/xen/arch/arm/Kconfig > > > > > > > > @@ -114,6 +114,8 @@ config SCI > > > > > > > > support. It allows guests to control system > > > > > > > > resourcess via one of > > > > > > > > SCI mediators implemented in XEN. > > > > > > > > > > > > > > > > +source "arch/arm/sci/Kconfig" > > > > > > > > + > > > > > > > > endmenu > > > > > > > > > > > > > > > > menu "ARM errata workaround via the alternative framework" > > > > > > > > diff --git a/xen/arch/arm/sci/Kconfig b/xen/arch/arm/sci/Kconfig > > > > > > > > new file mode 100644 > > > > > > > > index 0000000000..9563067ddc > > > > > > > > --- /dev/null > > > > > > > > +++ b/xen/arch/arm/sci/Kconfig > > > > > > > > @@ -0,0 +1,10 @@ > > > > > > > > +config SCMI_SMC > > > > > > > > + bool "Enable SCMI-SMC mediator driver" > > > > > > > > + default n > > > > > > > > + depends on SCI > > > > > > > > + ---help--- > > > > > > > > + > > > > > > > > + Enables mediator in XEN to pass SCMI requests from > > > > > > > > Domains to ATF. > > > > > > > > + This feature allows drivers from Domains to work with > > > > > > > > System > > > > > > > > + Controllers (such as power,resets,clock etc.). SCP is > > > > > > > > used as transport > > > > > > > > + for communication. > > > > > > > > diff --git a/xen/arch/arm/sci/Makefile > > > > > > > > b/xen/arch/arm/sci/Makefile > > > > > > > > index 837dc7492b..67f2611872 100644 > > > > > > > > --- a/xen/arch/arm/sci/Makefile > > > > > > > > +++ b/xen/arch/arm/sci/Makefile > > > > > > > > @@ -1 +1,2 @@ > > > > > > > > obj-y += sci.o > > > > > > > > +obj-$(CONFIG_SCMI_SMC) += scmi_smc.o > > > > > > > > diff --git a/xen/arch/arm/sci/scmi_smc.c > > > > > > > > b/xen/arch/arm/sci/scmi_smc.c > > > > > > > > new file mode 100644 > > > > > > > > index 0000000000..2eb01ea82d > > > > > > > > --- /dev/null > > > > > > > > +++ b/xen/arch/arm/sci/scmi_smc.c > > > > > > > > @@ -0,0 +1,795 @@ > > > > > > > > +/* > > > > > > > > + * xen/arch/arm/sci/scmi_smc.c > > > > > > > > + * > > > > > > > > + * SCMI mediator driver, using SCP as transport. > > > > > > > > + * > > > > > > > > + * Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx> > > > > > > > > + * Copyright (C) 2021, EPAM Systems. > > > > > > > > + * > > > > > > > > + * This program is free software; you can redistribute it > > > > > > > > and/or modify > > > > > > > > + * it under the terms of the GNU General Public License as > > > > > > > > published by > > > > > > > > + * the Free Software Foundation; either version 2 of the > > > > > > > > License, or > > > > > > > > + * (at your option) any later version. > > > > > > > > + * > > > > > > > > + * This program is distributed in the hope that it will be > > > > > > > > useful, > > > > > > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty > > > > > > > > of > > > > > > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See > > > > > > > > the > > > > > > > > + * GNU General Public License for more details. > > > > > > > > + */ > > > > > > > > + > > > > > > > > +#include <asm/sci/sci.h> > > > > > > > > +#include <asm/smccc.h> > > > > > > > > +#include <asm/io.h> > > > > > > > > +#include <xen/bitops.h> > > > > > > > > +#include <xen/config.h> > > > > > > > > +#include <xen/sched.h> > > > > > > > > +#include <xen/device_tree.h> > > > > > > > > +#include <xen/iocap.h> > > > > > > > > +#include <xen/init.h> > > > > > > > > +#include <xen/err.h> > > > > > > > > +#include <xen/lib.h> > > > > > > > > +#include <xen/list.h> > > > > > > > > +#include <xen/mm.h> > > > > > > > > +#include <xen/string.h> > > > > > > > > +#include <xen/time.h> > > > > > > > > +#include <xen/vmap.h> > > > > > > > > + > > > > > > > > +#define SCMI_BASE_PROTOCOL 0x10 > > > > > > > > +#define SCMI_BASE_PROTOCOL_ATTIBUTES 0x1 > > > > > > > > +#define SCMI_BASE_SET_DEVICE_PERMISSIONS 0x9 > > > > > > > > +#define SCMI_BASE_RESET_AGENT_CONFIGURATION 0xB > > > > > > > > +#define SCMI_BASE_DISCOVER_AGENT 0x7 > > > > > > > > + > > > > > > > > +/* SCMI return codes. See section 4.1.4 of SCMI spec > > > > > > > > (DEN0056C) */ > > > > > > > > +#define SCMI_SUCCESS 0 > > > > > > > > +#define SCMI_NOT_SUPPORTED (-1) > > > > > > > > +#define SCMI_INVALID_PARAMETERS (-2) > > > > > > > > +#define SCMI_DENIED (-3) > > > > > > > > +#define SCMI_NOT_FOUND (-4) > > > > > > > > +#define SCMI_OUT_OF_RANGE (-5) > > > > > > > > +#define SCMI_BUSY (-6) > > > > > > > > +#define SCMI_COMMS_ERROR (-7) > > > > > > > > +#define SCMI_GENERIC_ERROR (-8) > > > > > > > > +#define SCMI_HARDWARE_ERROR (-9) > > > > > > > > +#define SCMI_PROTOCOL_ERROR (-10) > > > > > > > > + > > > > > > > > +#define DT_MATCH_SCMI_SMC DT_MATCH_COMPATIBLE("arm,scmi-smc") > > > > > > > > + > > > > > > > > +#define SCMI_SMC_ID "arm,smc-id" > > > > > > > > +#define SCMI_SHARED_MEMORY "linux,scmi_mem" > > > > > > > > > > > > > > I could find the following SCMI binding in Linux, which describes > > > > > > > the arm,scmi-smc compatible and the arm,smc-id property: > > > > > > > > > > > > > > Documentation/devicetree/bindings/firmware/arm,scmi.yaml > > > > > > > > > > > > > > However, linux,scmi_mem is not described. Aren't you supposed to > > > > > > > read > > > > > > > the "shmem" property instead? And the compatible string used for > > > > > > > this > > > > > > > seems to be "arm,scmi-shmem". > > > > > > > > > > > > > > > > > > > We use linux,scmi_mem node to reserve memory, needed for all > > > > > > channels: > > > > > > > > > > > > reserved-memory { > > > > > > /* reserved region for scmi channels*/ > > > > > > scmi_memory: linux,scmi_mem@53FF0000 { > > > > > > no-map; > > > > > > reg = <0x0 0x53FF0000 0x0 0x10000>; > > > > > > }; > > > > > > }; > > > > > > > > > > > > arm,scmi-shmem node used in shmem property defines only 1 page > > > > > > needed to > > > > > > the current scmi channel: > > > > > > > > > > > > cpu_scp_shm: scp-shmem@0x53FF0000 { > > > > > > compatible = "arm,scmi-shmem"; > > > > > > reg = <0x0 0x53FF0000 0x0 0x1000>; > > > > > > }; > > > > > > > > > > > > For each Domain reg points to unigue page from linux,scmi_mem > > > > > > region, > > > > > > assigned to this agent. > > > > > > > > > > If we were to use "linux,scmi_mem" we would have to introduce it as a > > > > > compatible string, not as a node name, and it would need to be > > > > > described > > > > > in Documentation/devicetree/bindings/firmware/arm,scmi.yaml. > > > > > > > > > > But from your description I don't think it is necessary. We can just > > > > > use > > > > > "arm,scmi-shmem" to describe all the required regions: > > > > > > > > > > reserved-memory { > > > > > scp-shmem@0x53FF0000 { > > > > > compatible = "arm,scmi-shmem"; > > > > > reg = <0x0 0x53FF0000 0x0 0x1000>; > > > > > }; > > > > > scp-shmem@0x53FF1000 { > > > > > compatible = "arm,scmi-shmem"; > > > > > reg = <0x0 0x53FF1000 0x0 0x1000>; > > > > > }; > > > > > scp-shmem@0x53FF2000 { > > > > > compatible = "arm,scmi-shmem"; > > > > > reg = <0x0 0x53FF2000 0x0 0x1000>; > > > > > }; > > > > > ... > > > > > > > > > > In other words, if all the individual channel pages are described as > > > > > "arm,scmi-shmem", why do we also need a single larger region as > > > > > "linux,scmi_mem"? > > > > > > > > > > > > > That was my first implementation. But I've met a problem with > > > > scmi driver in kernel. I don't remember the exact place, but I remember > > > > there were some if, checking if memory weren't reserved. > > > > That's why I ended up splitting nodes reserved memory region and actual > > > > shmem page. > > > > For linux,scmi_mem node I took format from > > > > /reserved-memory/linux,lossy_decompress@54000000, > > > > which has no compatible string and provides no-map property. > > > > linux,scmi_shmem node is needed to prevent xen from allocating this > > > > space for the domain. > > > > > > > > Very interesting question about should I introduce linux,scmi_mem node > > > > and scmi_devid property to the > > > > Documentation/devicetree/bindings/firmware/arm,scmi.yaml? > > > > Those node and property are needed only for Xen and useless for > > > > non-virtualized systems. I can add this node and property description to > > > > arm,scmi.yaml, but leave a note that this is Xen specific params. > > > > What do you think about it? > > > > > > Reply below > > > > > > [...] > > > > > > > > > > > In general we can't use properties that are not part of the device > > > > > tree > > > > > spec, either > > > > > https://urldefense.com/v3/__https://www.devicetree.org/specifications/__;!!GF_29dbcQIUBPA!kNodtgmOQBc1iO76_6vTK-O1SoLxee_ChowYQiQYC595rMOsrnmof2zmk7BnhXCSnJPN$ > > > > > [devicetree[.]org] or > > > > > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings__;!!GF_29dbcQIUBPA!kNodtgmOQBc1iO76_6vTK-O1SoLxee_ChowYQiQYC595rMOsrnmof2zmk7BnhXloYUaj$ > > > > > [git[.]kernel[.]org] > > > > > > > > > > "linux,scmi_mem" is currently absent. Are you aware of any upstreaming > > > > > activities to get "linux,scmi_mem" upstream under > > > > > Documentation/devicetree/bindings in Linux? > > > > > > > > > > If "linux,scmi_mem" is going upstream in Linux, then we could use it. > > > > > Otherwise, first "linux,scmi_mem" needs to be added somewhere under > > > > > Documentation/devicetree/bindings (probably > > > > > Documentation/devicetree/bindings/firmware/arm,scmi.yaml), then we can > > > > > work on the Xen code that makes use of it. > > > > > > > > > > Does it make sense? > > > > > > > > > > > > > Yes I agree. I think linux,scmi_mem and scmi_devid should be upstreamed. > > > > I will add those properties to arm,scmi.yaml, mark them as related to > > > > XEN and send patch. > > > > > > I didn't realize that linux,scmi_mem and scmi_devid are supposed to be > > > Xen specific. In general, it would be best not to introduce Xen specific > > > properties into generic bindings. It is a problem both from a > > > specification perspective (because it has hard to handle Xen specific > > > cases in fully generic bindings, especially as those bindings are > > > maintained as part of the Linux kernel) and from a user perspective > > > (because now the user has to deal with a Xen-specific dtb, or has to > > > modify the host dtb to add Xen-specific information by hand.) > > > > > > > > > Let me start from scmi_devid. Why would scmi_devid be Xen-specific? It > > > looks like a generic property that should be needed for the Linux SCMI > > > driver too. Why the Linux driver doesn't need it? > > > > > > > scmi_devid used during domain build. It passed as input parameter for > > SCMI_BASE_SET_DEVICE_PERMISSIONS message. > > On non-virtualized systems - there is no need of this call, because OS > > is the only one entity, running on the system. > > OK. Even if it is only required for virtualized systems, I think that > scmi_devid is important enough that should be part of the upstream > binding. I think it is worth starting an email thread on the LKML with > Rob Herring and the SCMI maintainers to discuss the addition of > scmi_devid to the binding. > > > > I've chatted with Volodymyr_Babchuk and he gave a great idea to add a > > list of device_ids to dom.cfg, such as: > > sci_devs = [ 0, 1, 15, 35 ]; > > > > Using this approach, we can remove scmi_devid from the device tree and > > just pass a list of scmi_devids to XEN using additional hypercall. > > We can probably make hypercall taking devid list as input parameter. > > This will take only 1 hypercall to setup sci permissions. > > But how would a user know which are the right SCMI IDs to add to the > sci_devs list? Would the user have to go and read the reference manual > of the platform to find the SCMI IDs and then write sci_devs by hand? > If that is the case, then I think that it would be better to add > scmi_devid to device tree. > > In general, I think this configuration should happen automatically > without user intervention. The user should just specify "enable SCMI" > and it should work. > > > > > In regards to linux,scmi_mem, I think it would be best to do without it > > > and fix the Linux SCMI driver if we need to do so. Xen should be able to > > > parse the native "arm,scmi-shmem" nodes and Linux (dom0 or domU) should > > > be able to parse the "arm,scmi-shmem" nodes generated by Xen. Either > > > way, I don't think we should need linux,scmi_mem. > > > > This requires further investigation. I will try to make implementation > > without linux,scmi_mem, using only arm,scmi-shmem nodes and share > > reuslts with you. > > OK, thanks. Hi Stefano, As I did some investigation about using reserved-memory area linux,scmi_mem and now I need your advice. I see 2 possible implementations for now: 1) Add memory-region parameter to cpu_scp_shm node which points to the reserved memory region. So device-tree will look like this: reserved-memory { /* reserved region for scmi channels*/ scmi_memory: region@53FF0000{ no-map; reg = <0x0 0x53FF0000 0x0 0x10000>; }; }; cpu_scp_shm: scp-shmem@0x53FF0000 { compatible = "arm,scmi-shmem"; reg = <0x0 0x53FF0000 0x0 0x1000>; memory-region = <&scmi_memory>; }; So cpu_scp_shm node has a reference to scmi_memory region. This mean that xen can find reserved memory region without adding additional names to the device-tree bindings. memory-region parameter as a reference to reserved memory and region creation described in: https://github.com/torvalds/linux/blob/v5.15/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt This approach I've implemented already and it works. 2) The second approach is the format you suggested: > > > > > reserved-memory { > > > > > scp-shmem@0x53FF0000 { > > > > > compatible = "arm,scmi-shmem"; > > > > > reg = <0x0 0x53FF0000 0x0 0x1000>; > > > > > }; > > > > > scp-shmem@0x53FF1000 { > > > > > compatible = "arm,scmi-shmem"; > > > > > reg = <0x0 0x53FF1000 0x0 0x1000>; > > > > > }; > > > > > scp-shmem@0x53FF2000 { > > > > > compatible = "arm,scmi-shmem"; > > > > > reg = <0x0 0x53FF2000 0x0 0x1000>; > > > > > }; > > > > > ... This approach has an advantage that xen ARM_SCI driver do not know about how channels are placed in the reserved memory, but introduces some disadvantages: a) We provide extra 14 (in our case) arm,scmi-shmem nodes which are not used in the device-tree. In current implementation I have separate scmi.dtsi file which introduces scmi support for both XEN-based and non-virtualized systems. Having 14 extra channels in the device-tree may be confusing. b) In case if we have all 15 channels, described in partial device-tree, we should not copy any node to the domain device-tree. I think it will be better to generate arm,scmi-shmem node in the Domain device-tree. The problem is that arm,scmi-smc node, which is using arm,scmi-shmem node can't be generated. I prefer it to be copied from the partial device-tree because it includes some platform specific configuration, such as func-id and list of the protocols (for example different platforms may require different list of the protocols). So in this case we will have 1 node copied and 1 node generated. I think even for dom0less we should use arm,scmi-smc node from the device-tree because protocol configuration and funcid is related to the platform. I prefer the second approach and will try to make it if it's OK to copy arm,scmi-smc node from partial Device-tree and generate arm,scmi-shmem node. What do you think about that? Also I wanted to mention that I'm not planning to make ARM_SCI support for dom0less in terms of this patch series bacause I can't test dom0less configuration for now. So let me know if some of my functionality breaks dom0less. -- Best regards Oleksii.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |