Message-ID: <20220222135134.GA1597587@EPUAKYIW015D>
Date:   Tue, 22 Feb 2022 13:51:35 +0000
From:   Oleksii Moisieiev <Oleksii_Moisieiev@...m.com>
To:     Cristian Marussi <cristian.marussi@....com>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Souvik Chakravarty <Souvik.Chakravarty@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 0/1] dt-bindings: arm: Add scmi_devid paramter for

Hi Cristian,

On Tue, Feb 22, 2022 at 11:00:03AM +0000, Cristian Marussi wrote:
> On Mon, Feb 21, 2022 at 05:26:46PM +0000, Oleksii Moisieiev wrote:
> > Introducing new parameter called scmi_devid to the device-tree bindings.
> > This parameter should be set for the device nodes, which has
> > clocks/power-domains/resets working through SCMI.
> > Given parameter should set the device_id, needed to set device
> > permissions in the Firmware. This feature will be extremely useful for
> > the virtualized systems, which has more that one Guests running on the
> > system at the same time or for the syestems, which require several
> > agents with different permissions. Trusted agent will use scmi_devid to
> > set the Device permissions for the Firmware (See Section 4.2.2.10 [0]
> > for details).
> > Agents concept is described in Section 4.2.1 [0].
> > 
> 
> Hi Oleksii,
> 
> I had a look at this patch and the related XEN series and I'd have a few
> questions/doubts. (adding to the loop in CC Souvik from ATG and Vincent
> from Linaro since he's working on similar SCMI virtualization stuff)
> 

I'm aware of what Vincent is working on.

> > scmi_devid in Device-tree node example:
> > usb@...90000
> > {
> >     scmi_devid = <19>;
> >     clocks = <&scmi_clock 3>, <&scmi_clock 2>;
> >     resets = <&scmi_reset 10>, <&scmi_reset 9>;
> >     power-domains = <&scmi_power 0>;
> > };
> > 
> 
> So this SCMI device ID is meant to identify an SCMI device, viewed as a
> grouping of SCMI resources (clock/power/...etc) so that a Trusted Agent
> can issue a BASE_SET_DEVICE_PERMISSIONS telling to the SCMI Server
> platform backend (SCP sw sitting somewhere) which SCMI agents on the
> system can access which (SCMI) devices (in the Normal nonSecure world):
> basically, if I got it right from the Xen series, your hypervisor acting
> as Trusted Agent (and recognized as trusted agent by by the SCP) tells
> the SCMI platform server SCP (via SMC in your case) how to configure the
> access to the devices for all the other (non trusted) agents in the system
> (other Guest OS/Domains instances).

That's correct.

> 
> The SCMI spec does not indeed cover the discovery of such devices and
> the related associated resources: it indeed delegates such description
> to FDT/ACPI as of now.
> 
> AFAIU in this scenario I imagine:
> 
> - SCMI Server platform (SCP) knows via its own methods (builtin_config
>   FDT...etc) the list of defined SCMI devices and related associated
>   resources like:
>   
>   deviceNNN -->> clock_X / power_Q
>   deviceYYY ---> clock_Z / power_W
>   ..etc
> 
> 
> - Trusted Agent (XEN hypervisor) in turn:
> 
>   + is configured/recognized by the SCMI Server as a Trusted Agent (based
>     on the channel it uses to talk to the server) and as such it is allowed
>     to issue BASE_SET_DEVICE_PERMISSIONS (by the SCMI server)
> 
>   + has knowledge of the same set of devices/resources allocations (via
>     its own FDT) as the SCMI server
> 
>   + can issue a proper set of BASE_SET_DEVICE_PERMISSIONS telling the
>     SCMI server backend which devices can be used by which non-trusted
>     agents (GuestOS) ... even dynamically I suppose when guests come and
>     go.
> 
>     Xen:
> 	    BASE_SET_DEVICE_PERMISSIONS(dev_NNN, agent_3)
> 	    BASE_SET_DEVICE_PERMISSIONS(dev_YYY, agent_2)
> 	    BASE_SET_DEVICE_PERMISSIONS(dev_NNN, agent_4)
> 
>     and in this scenario the same dev_NNN could be made accessible to
>     two different agents, it will be anyway up to the SCMI Server
>     backend to armonize or reject such requests from 2 different
>     agents around the same shared resources
> 
> 
> - Other non-trusted agents on the system (GuestOSes or other non
>   virtualized subsystems...e.g. WiFi/Modem...etc), described in their
>   DTs (for Linux GuestOS) as using SCMI resources as usual (without SCMI
>   device id) just issue SCMI request on the basic resource and those are
>   routed to the SCMI Server backend by the Hypervisor UNMODIFIED:
> 
>   example for a shared resource:
> 
>    - Agent_2 set power_Q ON --->>> SCMI Server - OK - powerQ TURNED ON
>     				  (allowed as configured by Trusted Agent,
> 				   powerQ hw was OFF turn it ON)
> 
>    - Agent_3 set power_Q OFF --->>> SCMI Server - DENIED
>     				  (disallowed as configured by Trusted Agent)
> 
>    - Agent_4 set power_Q ON --->>> SCMI Server - OK - powerQ ALREADY ON
>     				  (allowed as configured by Trusted Agent,
> 				   powerQ hw was ON nothing to be done)
> 
>    - Agent_2 set power_Q OFF --->>> SCMI Server - OK - powerQ UNCHANGED (SHARED with Agent_2)
>     				  (allowed as configured by Trusted Agent but shared with
> 				   another agent)
> 
>    - Agent_4 set power_Q OFF --->>> SCMI Server - OK - powerQ OFF
>     				  (allowed as configured by Trusted Agent but shared with
> 				   another agent
> 
> So in all of this, I don't get why you need this DT definition aggregating SCMI
> resources to SCMI device IDs in the Guest OS, which is an SCMI agent that does not
> need to now anything about SCMI device IDs (at least with the current spec): this
> would make sense only if the Linux Kernel was the TrustedAgent in charge of
> configuring the devices permissions via BASE_SET_DEVICE_PERMISSIONS.
> (in fact you said you won't provide any code to manage this scmi_devid
> in the kernel since those guests are not trusted agents and the won't be
> allowed to set device permissions...)

You are right. The idea is that only Xen in our case will use
scmi-devid. I thought that providing this parameter in DT-bindings will
be useful for the future usage, let's say for some sort of the scmi
backend, running in the different OS or for the different hypervisor.
That's why I've decided to open discussion here.

> 
> The only tricky part I can see in all of the above is agent identification, since
> the agents are assigned an ID by the SCMI platform (which can be queried) and they
> have a set of dedicated channels to use, so basically the platform really identifies
> the Agents looking at the channel from which a request is coming from and AgentID is
> not carried inside the message as a source and cannot be spoofed.

That's a very good question. I've raised it in Xen mailing list: 
https://xen.markmail.org/message/jofia74b2s4jprqa

The main question for me is how Firmware will know from which channel it
should read the message.

In current implementataion we use SMC Client_ID (reg7) of SMC message to
pass channel_id from Xen to Firmware.

I'm also thinking about the alternative way to generate unique Func_ID
for each channel, so Firmware can get information about channel_id from
the Func_ID. 
In any case - I will be happy if you join the discussion about agent_id
in Xen mailing list. I can CC you if you intend to take part.

> 
> > Given example shows the configuration of the hsusb node, which is using
> > scmi to contol clocks, resets and power-domains. scmi_devid is set
> > equals to 19, which should match defined id for usb in the Firmware.
> > 
> > Trusted agent will use scmi_devid to set the device permissions for
> > the Agents. Guest OS should not have an access to the permissions
> > settings, so no code to process scmi_devid was presented in Linux
> > kernel.
> > 
> > We are currently contributing changes to Xen, which are intended to
> > mediate SCMI access from Guests to the Firmware. Xen uses scmi_devid to set
> > the permissions for the devices. See [1] thread for details.
> > 
> > [0] https://urldefense.com/v3/__https://developer.arm.com/documentation/den0056/latest__;!!GF_29dbcQIUBPA!ipwkZidQn94BuaF7b6AopvUjQJGLSkgQm6QnAqOaevFBqCYEYEQZVOq8CGFd5j8_JWEz$ [developer[.]arm[.]com]
> > [1] https://urldefense.com/v3/__https://xen.markmail.org/message/mmi4fpb4qr6e3kad__;!!GF_29dbcQIUBPA!ipwkZidQn94BuaF7b6AopvUjQJGLSkgQm6QnAqOaevFBqCYEYEQZVOq8CGFd5seDaFDv$ [xen[.]markmail[.]org]
> 
> IMHO, but I could be wrong, looking at the current SCMI spec you cannot just
> gather messages from a set of GuestOs talking via different SCMI channels and
> then pipe/route them through a single channel to the backend server,
> attaching/spoofing some sort of Agent source ID to each message like you seem to
> be doing in the Xen series

I think you misunderstood my approach. We're not gathering messages from
GuestOs. Each GuestOs uses it's own channel_id to communicate with
firmware. What Xen does in current implementation is intercept SMC
messages and set channel_id to SMC Client_ID parameter (reg7) and
redirects it to the Firmware. Firmware parses reg7 and knows which
channed it should use to get the request.

In the new approach, which I suggest in Xen mailing list (I will be very
happy if you share your thoughts there) doesn't require SMC messages to
be intercepted by Xen, so each GuestOs will have it's own channel with
the Firmware.

> 
> " Also XEN is the mediator which redirects SCMI requests, adding agentID so
>   firmware should know the sender."
> 
> I may missing something though, not really a Xen expert here, or maybe this
> agentID identification trick could be considered something not covered
> by the spec and strictly part of the transport layer...not sure... the guys
> in CC may have different/opposite opinions so feel free to redirect my
> blabbing to /dev/null at the end :D
> 

SCMI spec describes how agent_id should be passed only for Message type
3 which is Notification (See 4.1.2 of DEN0056D). SMC spec say that
Client_ID is reg7. This Client_id is what I've used originally.
The new approach I suggest gives ability to get rid of Client_ID and use
Func_ID to determine channel.

Best regards,
Oleksii

Powered by blists - more mailing lists