Re: [RFC v1 3/5] xen/arm: introduce SCMI-SMC mediator driver

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

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.

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.

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