| 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
| ||
|
Message-ID: <55fd0c34-c52c-95c5-5cd0-16fd66a4baa2@quicinc.com>
Date: Thu, 14 Nov 2024 10:02:40 +0530
From: Sibi Sankar <quic_sibis@...cinc.com>
To: Cristian Marussi <cristian.marussi@....com>
CC: <sudeep.holla@....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>, <quic_kshivnan@...cinc.com>,
<conor+dt@...nel.org>, <arm-scmi@...r.kernel.org>
Subject: Re: [PATCH V4 2/5] firmware: arm_scmi: Add QCOM Generic Vendor
Protocol documentation
On 10/22/24 15:52, Cristian Marussi wrote:
> On Mon, Oct 07, 2024 at 11:40:20AM +0530, Sibi Sankar wrote:
>> Add QCOM System Control Management Interface (SCMI) Generic Vendor
>> Extensions Protocol documentation.
>>
>
> Hi Sibi,
>
> a few remarks down below.
>
>> Signed-off-by: Sibi Sankar <quic_sibis@...cinc.com>
>> ---
>> .../arm_scmi/vendors/qcom/qcom_generic.rst | 210 ++++++++++++++++++
>> 1 file changed, 210 insertions(+)
>> create mode 100644 drivers/firmware/arm_scmi/vendors/qcom/qcom_generic.rst
>>
>> diff --git a/drivers/firmware/arm_scmi/vendors/qcom/qcom_generic.rst b/drivers/firmware/arm_scmi/vendors/qcom/qcom_generic.rst
>> new file mode 100644
>> index 000000000000..1ee6dabaac23
>> --- /dev/null
>> +++ b/drivers/firmware/arm_scmi/vendors/qcom/qcom_generic.rst
>> @@ -0,0 +1,210 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +.. include:: <isonum.txt>
>> +
>> +===============================================================================
>> +QCOM System Control and Management Interface(SCMI) Vendor Protocols Extension
>> +===============================================================================
>> +
>> +:Copyright: |copy| 2024, Qualcomm Innovation Center, Inc. All rights reserved.
>> +
>> +:Author: Sibi Sankar <quic_sibis@...cinc.com>
>> +
>> +SCMI_GENERIC: System Control and Management Interface QCOM Generic Vendor Protocol
>> +==================================================================================
>> +
>> +This protocol is intended as a generic way of exposing a number of Qualcomm
>> +SoC specific features through a mixture of pre-determined algorithm string and
>> +param_id pairs hosted on the SCMI controller. It implements an interface compliant
>> +with the Arm SCMI Specification with additional vendor specific commands as
>> +detailed below.
>> +
>> +Commands:
>> +_________
>> +
>> +PROTOCOL_VERSION
>> +~~~~~~~~~~~~~~~~
>> +
>> +message_id: 0x0
>> +protocol_id: 0x80
>> +
>> ++---------------+--------------------------------------------------------------+
>> +|Return values |
>> ++---------------+--------------------------------------------------------------+
>> +|Name |Description |
>> ++---------------+--------------------------------------------------------------+
>> +|int32 status |See ARM SCMI Specification for status code definitions. |
>> ++---------------+--------------------------------------------------------------+
>> +|uint32 version |For this revision of the specification, this value must be |
>> +| |0x10000. |
>> ++---------------+--------------------------------------------------------------+
>> +
>> +PROTOCOL_ATTRIBUTES
>> +~~~~~~~~~~~~~~~~~~~
>> +
>> +message_id: 0x1
>> +protocol_id: 0x80
>> +
>> ++---------------+--------------------------------------------------------------+
>> +|Return values |
>> ++------------------+-----------------------------------------------------------+
>> +|Name |Description |
>> ++------------------+-----------------------------------------------------------+
>> +|int32 status |See ARM SCMI Specification for status code definitions. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 attributes |Bits[8] Set to 1. |
>> +| |Bits[0] Set to 1. |
>> ++------------------+-----------------------------------------------------------+
>
> Mmmm, this does not explain so much what are those bits and what values
> they can indeed assume :P ...
lol, after a lot of rooting around figured out they
return the number of vendor protocols available in
the system :/
Not really sure if it's adding any useful info.
>
>> +
>> +PROTOCOL_MESSAGE_ATTRIBUTES
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +message_id: 0x2
>> +protocol_id: 0x80
>> +
>> ++---------------+--------------------------------------------------------------+
>> +|Return values |
>> ++------------------+-----------------------------------------------------------+
>> +|Name |Description |
>> ++------------------+-----------------------------------------------------------+
>> +|int32 status |See ARM SCMI Specification for status code definitions. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 attributes |For all message id's the parameter has a value of 0. |
>> ++------------------+-----------------------------------------------------------+
>> +
>> +QCOM_SCMI_SET_PARAM
>> +~~~~~~~~~~~~~~~~~~~
>> +
>> +message_id: 0x10
>> +protocol_id: 0x80
>> +
>> ++------------------+-----------------------------------------------------------+
>> +|Parameters |
>> ++------------------+-----------------------------------------------------------+
>> +|Name |Description |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 ext_id |Reserved, must be zero. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 algo_low |Lower 32-bit value of the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 algo_high |Upper 32-bit value of the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 param_id |Serves as the token message id for the algorithm string |
>> +| |and is used to set various parameters supported by it. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 buf[] |Serves as the payload for the specified param_id and |
>> +| |algorithm string pair. |
>> ++------------------+-----------------------------------------------------------+
>
> And what abot the size of this payload ? .. so you are relying on the
> fact that the transport will add the total message length at that layer
> and so the server can determine where the valid payload ends...
>
> ...it means the server will have some expectations about the payload length
> based on the param_id and will check against the received transport-advertised
> message-length, am I right ?
>
> ...BUT what if you end up with multiple versions of this protocol in the future,
> with varying payload lengths for the same param_id...REMEMEBER that the server
> cannot know which version of a protocol the client is running (while the client
> can see what the server runs) UNLESS you implement also NEGOTIATE_PROTOCOL_VERSION
> for this protocol...
>
> ...so without an explicit length nor the NEGOTIATE_PROTOCOL_VERSION you wont be
> able in the future, server-side, to be sure if you are assumnig the right payload
> length for the right version that the client is speaking...so at the end you
> wont be able to support multiple versions of the protocol even if the Kernel
> can support all of those versions...do you see what I mean ?
>
> I think that would be advisable to implement NEGOTIATE_PROTOCOL_VERSION
> if you dont want to carry an explicit size in the message for this payload...
>
> ...or am I missing something ?
ack makes sense but we planned to make sure that the sub-strings
used maintain abi for all their messages but like you said having
way to negotiate protocol version will be nice to have. Will make
sure something along the lines get implemented with the next major
version upgrade.
-Sibi
>
>> +|Return values |
>> ++------------------+-----------------------------------------------------------+
>> +|Name |Description |
>> ++------------------+-----------------------------------------------------------+
>> +|int32 status |SUCCESS: if the param_id and buf[] is parsed successfully |
>> +| |by the chosen algorithm string. |
>> +| |NOT_SUPPORTED: if the algorithm string does not have any |
>> +| |matches. |
>> +| |INVALID_PARAMETERS: if the param_id and the buf[] passed |
>> +| |is rejected by the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>> +
>> +QCOM_SCMI_GET_PARAM
>> +~~~~~~~~~~~~~~~~~~~
>> +
>> +message_id: 0x11
>> +protocol_id: 0x80
>> +
>> ++------------------+-----------------------------------------------------------+
>> +|Parameters |
>> ++------------------+-----------------------------------------------------------+
>> +|Name |Description |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 ext_id |Reserved, must be zero. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 algo_low |Lower 32-bit value of the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 algo_high |Upper 32-bit value of the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 param_id |Serves as the token message id for the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 buf[] |Serves as the payload and store of value for the specified |
>> +| |param_id and algorithm string pair. |
>> ++------------------+-----------------------------------------------------------+
>> +|Return values |
>> ++------------------+-----------------------------------------------------------+
>> +|Name |Description |
>> ++------------------+-----------------------------------------------------------+
>> +|int32 status |SUCCESS: if the param_id and buf[] is parsed successfully |
>> +| |by the chosen algorithm string and the result is copied |
>> +| |into buf[]. |
>> +| |NOT_SUPPORTED: if the algorithm string does not have any |
>> +| |matches. |
>> +| |INVALID_PARAMETERS: if the param_id and the buf[] passed |
>> +| |is rejected by the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>
> Similarly, no payload length means you will have to code some builtin
> check to verify the length of the message that you have received against
> the specific version that the server is running...this is NOT so
> problematic here as in the _SET above since the client/agent DOES know which
> protocol version the server is running...
>
> ...it is a bit odd, but indeed similar to other variable sized SCMI messages in
> standard protocols that sports optional fields in the reply, for which, similarly
> you have to check the version of the protocol to desume the size of the message
> based on the presence or not of some fields...
>
>> +
>> +QCOM_SCMI_START_ACTIVITY
>> +~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +message_id: 0x12
>> +protocol_id: 0x80
>> +
>> ++------------------+-----------------------------------------------------------+
>> +|Parameters |
>> ++------------------+-----------------------------------------------------------+
>> +|Name |Description |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 ext_id |Reserved, must be zero. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 algo_low |Lower 32-bit value of the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 algo_high |Upper 32-bit value of the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 param_id |Serves as the token message id for the algorithm string |
>> +| |and is generally used to start the activity performed by |
>> +| |the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 buf[] |Serves as the payload for the specified param_id and |
>> +| |algorithm string pair. |
>> ++------------------+-----------------------------------------------------------+
>> +|Return values |
>> ++------------------+-----------------------------------------------------------+
>> +|Name |Description |
>> ++------------------+-----------------------------------------------------------+
>> +|int32 status |SUCCESS: if the activity performed by the algorithm string |
>> +| |starts successfully. |
>> +| |NOT_SUPPORTED: if the algorithm string does not have any. |
>> +| |matches or if the activity is already running. |
>> ++------------------+-----------------------------------------------------------+
>> +
>
> Same consideration as above...being a SET-like operation with a variable
> sized field in the request AND no explicit payload length, you will have
> to derive the size from the message length BUT since you doint even have
> implemented NEGOTIATE_PROTOCOL_VERSION in the future any kind of check
> will become impossibe server side if you will have multiple protocols
> with varying sizes for buf depending on the protocol version
>
>> +QCOM_SCMI_STOP_ACTIVITY
>> +~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +message_id: 0x13
>> +protocol_id: 0x80
>> +
>> ++------------------+-----------------------------------------------------------+
>> +|Parameters |
>> ++------------------+-----------------------------------------------------------+
>> +|Name |Description |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 ext_id |Reserved, must be zero. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 algo_low |Lower 32-bit value of the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 algo_high |Upper 32-bit value of the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 param_id |Serves as the token message id for the algorithm string |
>> +| |and is generally used to stop the activity performed by |
>> +| |the algorithm string. |
>> ++------------------+-----------------------------------------------------------+
>> +|uint32 buf[] |Serves as the payload for the specified param_id and |
>> +| |algorithm string pair. |
>> ++------------------+-----------------------------------------------------------+
>
> Same.
>
>> +|Return values |
>> ++------------------+-----------------------------------------------------------+
>> +|Name |Description |
>> ++------------------+-----------------------------------------------------------+
>> +|int32 status |SUCCESS: if the activity performed by the algorithm string |
>> +| |stops successfully. |
>> +| |NOT_SUPPORTED: if the algorithm string does not have any |
>> +| |matches or if the activity isn't running. |
>> ++------------------+-----------------------------------------------------------+
>
> Thanks,
> Cristian
Powered by blists - more mailing lists