Message-ID: <CAKfTPtAcEiqBDr5BXBS8Q9HzBWg544YR0eZkkaF3u_9EowZJcQ@mail.gmail.com>
Date:   Wed, 2 Nov 2022 09:54:50 +0100
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Cristian Marussi <cristian.marussi@....com>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        sudeep.holla@....com, james.quinlan@...adcom.com,
        Jonathan.Cameron@...wei.com, f.fainelli@...il.com,
        etienne.carriere@...aro.org, souvik.chakravarty@....com,
        wleavitt@...vell.com, peter.hilber@...nsynergy.com,
        nicola.mazzucato@....com, tarek.el-sherbiny@....com,
        quic_kshivnan@...cinc.com
Subject: Re: [PATCH v4 0/11] Introduce a unified API for SCMI Server testing

On Fri, 28 Oct 2022 at 18:58, Cristian Marussi <cristian.marussi@....com> wrote:
>
> On Fri, Oct 28, 2022 at 06:18:52PM +0200, Vincent Guittot wrote:
> > On Fri, 28 Oct 2022 at 17:04, Cristian Marussi <cristian.marussi@....com> wrote:
> > >
> > > On Fri, Oct 28, 2022 at 04:40:02PM +0200, Vincent Guittot wrote:
> > > > On Wed, 19 Oct 2022 at 22:46, Cristian Marussi <cristian.marussi@....com> wrote:
> > > > >
> > > > > Hi all,
> > > > >
> > >
> > > Hi Vincent,
> > >
> > > > > This series aims to introduce a new SCMI unified userspace interface meant
> > > > > to ease testing an SCMI Server implementation for compliance, fuzzing etc.,
> > > > > from the perspective of the OSPM agent (non-secure world only ...)
> > > > >
> >
> > [ snip]
> >
> > > > Hi Cristian,
> > > >
> > > > I have tested your series with an optee message transport layer and
> > > > been able to send raw messages to the scmi server PTA
> > > >
> > > > FWIW
> > > >
> > > > Tested-by: Vincent Guittot <vincent.guittot@...aro.org>
> > > >
> > >
> > > Thanks a lot for your test and feedback !
> > >
> > > ... but I was going to reply to this saying that I spotted another issue
> > > with the current SCMI Raw implementation (NOT related to optee/smc) so
> > > that I'll post a V5 next week :P
> > >
> > > Anyway, the issue is much related to the debugfs root used by SCMI Raw,
> > > i.e.:
> > >
> > >         /sys/kernel/debug/scmi_raw/
> > >
> > > ..this works fine unless you run it on a system sporting multiple DT-defined
> > > server instances ...that is not officially supported but....ehm...a little
> > > bird told these system with multiple servers do exists :D
> >
> > ;-)
> >
> > >
> > > In such a case the SCMI core stack is probed multiuple times and so it
> > > will try to register multiple debugfs Raw roots: there is no chanche to
> > > root the SCMI Raw entries at the same point clearly ... and then anyway
> > > there is the issue of recognizing which server is rooted where ... with
> > > the additional pain that a server CANNOT be recognized by querying...cause
> > > there is only one by teh spec with agentID ZERO ... in theory :D...
> > >
> > > So my tentaive solution for V5 would be:
> > >
> > > - change the Raw root debugfs as:
> > >
> > >         /sys/kernel/debug/scmi_raw/0/... (first server defined)
> > >
> > >         /sys/kernel/debug/scmi_raw/1/... (possible additional server(s)..)
> > >
> > > - expose the DT scmi-server root-node name of the server somewhere under
> > >   that debugfs root like:
> > >
> > >         ..../scmi_raw/0/transport_name -> 'scmi-mbx'
> > >
> > >         ..../scmi_raw/1/transport_name -> 'scmi-virtio'
> >
> > I was about to say that you would display the server name but that
> > means that you have send a request to the server which probably
> > defeats the purpose of the raw mode
> >
> > >
> > >   so that if you know HOW you have configured your own system in the DT
> > >   (maybe multiple servers with different kind of transports ?), you can
> > >   easily select programmatically which one is which, and so decide
> > >   which Raw debugfs fs to use...
> > >
> > > ... I plan to leave the SCMI ACS suite use by default the first system
> > > rooted at /sys/kernel/debug/scmi_raw/0/...maybe adding a commandline
> > > option to choose an alternative path for SCMI Raw.
> > >
> > > Does all of this sound reasonable ?
> >
> > Yes, adding an index looks good to me.
>
> Ok, I'll rework accordingly.
>
> >
> > As we are there, should we consider adding a per channel entry in the
> > tree when there are several channels shared with the same server ?
> >
>
> So, I was thinking about this and, even though, it seems not strictly
> needed for Compliance testing (as discussed offline) I do think that
> could be a sensible option to have as an additional mean to stress the
> server transport implementation (as you wish).

Thanks

>
> Having said that, this week, I was reasoning about an alternative
> interface to do this, i.e. to avoid to add even more debugfs entries
> for this chosen-channel config or possibly in the future to ask for
> transport polling mode (if supported by the underlying transport)
>
> My idea (not thought fully through as of now eh..) would be as follows:
>
> since the current Raw implementation enforces a minimum size of 4 bytes
> for the injected message (more on this later down below in NOTE), I was
> thinking about using less-than-4-bytes-sized messages to sort of
> pre-configure the Raw stack.
>
> IOW, instead of having a number of new additional entries like
>
>         ../message_ch10
>         ../message_ch13
>         ../message_poll
>
> we could design a sort of API (in the API :D) that defines how
> 3-bytes message payload are to be interpreted, so that in normal usage
> everything will go on as it is now, while if a 3-bytes message is
> injected by a specially crafted testcase, it would be used to configure
> the behaviour stack for the subsequent set of Raw transactions
> (i.e. for the currently opened fd...) like:
>
> - open message fd
>
> - send a configure message:
>
>         | proto_chan_#  |     flags (polling..)  |
>         ------------------------------------------
>         0               7                       21
>
> - send/receive your test messages
>
> - repeat or close (then the config will vanish...)
>
> This would mean adding some sort entry under scmi_raw to expose the
> configured available channels on the system though.
>
> [maybe the flags above could also include an async flag and avoid
>  also to add the message_async entries...]
>
> I discarded the idea to run the above config process via IOCTLs since
> it seemed to me even more frowned upon to use IOCTLs on a debugfs entry
> :P...but I maybe wrong ah...
>
> All of this is still to be explored anyway, any thoughts ? or evident
> drawbacks ? (beside having to clearly define an API for these message
> config machinery)

TBH, I'm not a fan of adding a protocol on top of the SCMI one. This
interface aims to test the SCMI servers and their channels so we
should focus on this and make it simple to use. IMHO, adding some
special bytes before the real scmi message is prone to create
complexity and error in the use of this debug interface.

>
> Anyway, whatever direction we'll choose (additional entries vs 3-bytes
> config msg), I would prefer to add this per-channel (or polling)
> capabilities with separate series to post on top of this in teh next
> cycle.

Ok

>
> ..too many words even this time :P

Thanks
Vincent

>
> Thanks,
> Cristian
>
>
> P.S: NOTE min_injected_msg_size:
> --------------------------------
> Thinking about all of the above, at first, I was a bit dubious if
> instead I should not allow, in Raw mode, the injection of shorter than
> 4 bytes messages (i.e. shorter than a SCMI header) for the purpose of
> fuzzing: then I realized that even though I should allow the injection
> of smaller messages, the underlying transports, as they are defined, both
> sides (platform and agent) will anyway carry out a 4bytes transaction,
> it's just that all the other non-provided bytes will be zeroed in the
> memory layout; this is just how the transports itself (shmem or msg
> based) are designed to work both sides. (and again would be transport
> layer testing more than SCMI spec verification..)
>
> So at the end I thought this kind of less-than-4-bytes transmissions
> gave no benefit and I came up with the above trick to use such tiny
> message for configuration.
>

Powered by blists - more mailing lists