linux-kernel - Re: [PATCH v3 2/4] firmware: qcom

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: <1f96396a-558b-2272-3a71-04ed7878728a@quicinc.com>
Date:   Mon, 11 Sep 2023 18:52:46 +0530
From:   Mukesh Ojha <quic_mojha@...cinc.com>
To:     Sricharan Ramabadhran <quic_srichara@...cinc.com>,
        Robert Marko <robimarko@...il.com>
CC:     <agross@...nel.org>, <andersson@...nel.org>,
        <konrad.dybcio@...aro.org>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <quic_gurus@...cinc.com>, <linux-arm-msm@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <computersforpeace@...il.com>
Subject: Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required



On 9/7/2023 12:02 PM, Sricharan Ramabadhran wrote:
> 
> <snip ..>
> 
>>>>> +     int ret;
>>>>> +     struct qcom_scm_desc desc = {
>>>>> +             .svc = QCOM_SCM_SVC_BOOT,
>>>>> +             .cmd = QCOM_SCM_BOOT_SDI_CONFIG,
>>>>> +             .args[0] = 1, /* Disable watchdog debug */
>>>>> +             .args[1] = 0, /* Disable SDI */
>>>>> +             .arginfo = QCOM_SCM_ARGS(2),
>>>>> +             .owner = ARM_SMCCC_OWNER_SIP,
>>>>> +     };
>>>>> +     struct qcom_scm_res res;
>>>>> +
>>>>> +     ret = qcom_scm_clk_enable();
>>>>> +     if (ret)
>>>>> +             return ret;
>>>>> +     ret = qcom_scm_call(__scm->dev, &desc, &res);
>>>>
>>>> Would you not be wanting this call to be atomic ?
>>>
>>> This is implemented based off the downstream 5.4 kernel as I dont have
>>> the SCM docs
>>> so I dont know if its even supported in the atomic version.
>>
>> Ok,.
>>
>> Well, Kernel version does not guarantees us whether certain things
>> are supported or not in the firmware and it is not bound to any
>> particular firmware version;
>>
>> So, whatever firmware version it is running with, we should try to
>> support.
>>
>> Should we implement certain kind of call, if fastcall(atomic) is 
>> supported go-ahead otherwise fallback to slowcalls (interruptible)
>> calls, but this is completely out of the context of this patch.
>>
> 
>   I replied on older thread, was not in CC here, just saw this.
> 
>   Agree, atomic api is out of this context and we could take it up
>   separately.
> 
>>>>
>>>>> +
>>>>> +     qcom_scm_clk_disable();
>>>>> +
>>>>> +     return ret ? : res.result[0];
>>>>> +}
>>>>> +
>>>>>    static int __qcom_scm_set_dload_mode(struct device *dev, bool 
>>>>> enable)
>>>>>    {
>>>>>        struct qcom_scm_desc desc = {
>>>>> @@ -1468,6 +1491,12 @@ static int qcom_scm_probe(struct 
>>>>> platform_device *pdev)
>>>>>        if (download_mode)
>>>>>                qcom_scm_set_download_mode(true);
>>>>>
>>>>> +     /*
>>>>> +      * Disable SDI if indicated by DT that it is enabled by default.
>>>>> +      */
>>>>> +     if (of_property_read_bool(pdev->dev.of_node, 
>>>>> "qcom,sdi-enabled"))
>>>>> +             qcom_scm_disable_sdi();
>>>>
>>>> Why don't we do this call in qcom_scm_shutdown()
>>>> also does it not conflict with above download_mode
>>>> we have enabled download mode but disabling SDI
>>>> means (hard reset) and will not be collecting
>>>> crash dump?
>>>
>>> Because doing it in SCM removal is too late, what if we have a WDT
>>> assertion and not a
>>> regular reboot?
>>> It would mean that the board will get stuck in the debug mode which is
>>> not useful for users and
>>> requires the power to be pulled in order to boot normally again.
>>
>> Agree.
> 
>   For IPQ chipsets, SDI bit is used like below,
> 
>     For abnormal resets (like WDT), should be set '1' for valid dump
>     collection.
> 
>     For reboot, should be cleared to '0' to avoid dump collection which
>     is not required in this case.
> 
>     For HLOS panic, is a don't care, dumps always get collected and
>     firmware takes care of clearing the SDI bit.
> 
>     Mukesh,  Can you confirm if its same for msm also ?

Yes, it is same in MSM as well.

-Mukesh

>>
>> Just a wild guess..
>>
>> Can we check if this call __qcom_scm_is_call_available() helps
>> to determine, if the certain soc has this SCM calls supported
>> and if it is there it can be disabled.
>>
>> __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_BOOT, 
>> QCOM_SCM_BOOT_SDI_CONFIG)
>>
> 
>   Yes, as i mentioned in other thread, checking using
>   qcom_scm_is_call_available is better. That said, would require
>   testing on all IPQ/MSM socs to confirm if firmware supports it.
> 
>>>
>>> I am not sure about the download mode, this is where insight from QCA
>>> really help as I am
>>> doing this with very limited docs.
>>
>> Download mode would not be reflected unless it is debug
>> board, whatever you write will not be allowed if it is a
>> secure device.
>>
> 
>    Yes, 'download mode' bit is similar, but that is used by the firmware
>    to determining whether to collect dumps on non-secure boards.
>    Specifically, 'SDI bit' on some socs is used by firmware to determine
>    if boot is happening from a 'abnormal crash', hence put DDR to
>    self-refresh etc for valid dumps.
> 
> Regards,
>   Sricharan