linux-kernel - Re: [PATCH V4 4/5] soc: qcom: Introduce SCMI based Memlat (Memory Latency) governor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z1GfGk0yQAVQKEVL@pluto>
Date: Thu, 5 Dec 2024 12:39:54 +0000
From: Cristian Marussi <cristian.marussi@....com>
To: Sibi Sankar <quic_sibis@...cinc.com>
Cc: Shivnandan Kumar <quic_kshivnan@...cinc.com>, sudeep.holla@....com,
	cristian.marussi@....com, andersson@...nel.org,
	konrad.dybcio@...aro.org, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, linux-kernel@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, quic_rgottimu@...cinc.com,
	conor+dt@...nel.org, arm-scmi@...r.kernel.org,
	Amir Vajid <avajid@...cinc.com>
Subject: Re: [PATCH V4 4/5] soc: qcom: Introduce SCMI based Memlat (Memory
 Latency) governor

On Thu, Dec 05, 2024 at 04:33:05PM +0530, Sibi Sankar wrote:
> 
> 
> On 11/29/24 15:27, Shivnandan Kumar wrote:
> > 
> >

Hi Sibi,

some rants down below :P
 
> > On 10/7/2024 11:40 AM, Sibi Sankar wrote:
> > > Introduce a client driver that uses the memlat algorithm string
> > > hosted on QCOM SCMI Generic Extension Protocol to detect memory
> > > latency workloads and control frequency/level of the various
> > > memory buses (DDR/LLCC/DDR_QOS).
> > > 

[snip]

> > > +    /* Set the effective cpu frequency calculation method */
> > > +    ret = ops->set_param(ph, &cpucp_freq_method,
> > > sizeof(cpucp_freq_method), MEMLAT_ALGO_STR,
> > > +                 MEMLAT_SET_EFFECTIVE_FREQ_METHOD);
> > > +    if (ret < 0)
> > > +        return dev_err_probe(&sdev->dev, ret,
> > > +                     "failed to set effective frequency calc method\n");
> > > +
> > 
> > Hi Sibi,
> 
> Hey Shiv,
> 
> Thanks for taking time to review the series!
> 
> > Since the MEMLAT_SET_EFFECTIVE_FREQ_METHOD command is not supported in
> > the legacy CPUCP firmware, it should be kept optional. This way, if the
> > legacy firmware is used, the driver will not return an error when the
> > CPUCP firmware returns -EOPNOTSUPP.
> 
> The vendor protocol with the current major/minor version is
> expected to work as is on x1e platforms. What legacy firmware
> are you referring to? All future SoCs that plan to adhere to
> it are expected to maintain this abi and can decide to make
> use of alternate mechanisms to calculating frequency based
> on additional dt properties set.
> 

Normally in the SCMI world you could leverage protocol versioning and
the standard PROTOCOL_MESSAGE_ATTRIBUTES(0x2) command to let the agent
investigate if the SCMI server it is speaking to implements or NOT a
specific command and which specific version of that command is understood
(with possibly varying size and fields)...

...BUT since your vendor protocol is 'Generic' and, as it stands, it
basically piggybacks any kind of binary payload (i.e. subcommands of
some kind of subprotocols of yours) into the same 4 get/set/start/stop
'Generic' ops, without even specifying the transmitted/received payload
sizes into the message itself....all of the possible SCMI versioning
autodiscovery and backward-compatibility capabilities happily go out of
the window because:

- your versioning refers to the generic protocol and you cannot possibly
  describe all the possible future subcommands (opaque payloads) variations
  and/or subcommands addition/removals purely on the major/minor version, AND
  even if you did that, NONE of such future variations will be documented
  anywhere since you are hiding all of this inside a bunch of binary blobs

- you dont even specify the payload sizes of the tx/rx 'Generic' payload
  subcommands so it becomes even difficult for both the server and the
  client to safely handle your 'Generic' subcommand message payloads

- you cannot issue a PROTOCOL_MESSAGE_ATTRIBUTE() to see if a specific
  subcommand is supported, because your subcommand is NOT really a protocol
  command but it is just one of the payloads of one of the 'Generic' protocol:
  you commmands are only set/get/start/stop (maybe some sort of hack
  could be doen around these...bit all will be even more flaky...)

- you dont implement NEGOTIATE_PROTOCOL_VERSION and so you cannot even
  check if the SCMI server that you are speaking to will agree to
  downgrade and 'speak' your Kernel SCMI agent (possibly older) 'Generic'
  protocol version

All of this basically defeats all of the SCMI general capabilities
around versioning and auto-discovery when it comes to your 'Generic' vendor
protocol, with the end result that you will have to be EXTREMELY confident
to perfectly match and keep in sync at all times your SCMI Server(FW) and
Client(Kernel) sides, without any possibility of coping with a such a mismatch
on the field by using some of the fallback/downgrade mechanism that you
threw out of the window...
(...and sorry but looking at the above xchange about 'legacy firmware' I am
 a bit skeptic about this as of now :D)

...as I said initially when reviewing this series, you can do whatever
you want within your Vendor protocol, but abusing the SCMI Vendor extensions
capabilities in such a way could end up bringing to the table more cons
(the above) than pros (some supposed 'simplification')

Thanks,
Cristian