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

[Stefano Stabellini <sstabellini@kernel.org>]

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@epam.com>
> > > > > ---
> > > > >  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@epam.com>
> > > > > + * 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?


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.