Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5048416b-ed93-30d3-05e3-5797a894dfd0@linaro.org>
Date:   Fri, 14 Apr 2023 13:22:44 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v2 6/6] remoterproc: qcom: refactor to leverage exported
 minidump symbol
Content-Language: en-US
To:     Mukesh Ojha <quic_mojha@quicinc.com>, agross@kernel.org,
        andersson@kernel.org, konrad.dybcio@linaro.org, corbet@lwn.net,
        keescook@chromium.org, tony.luck@intel.com, gpiccoli@igalia.com,
        catalin.marinas@arm.com, will@kernel.org
Cc:     linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org
References: <1679491817-2498-1-git-send-email-quic_mojha@quicinc.com>
 <1679491817-2498-7-git-send-email-quic_mojha@quicinc.com>
 <6c01d2fc-3155-0dcd-f473-9cbd75dd69ec@linaro.org>
 <af23d650-3dec-9a51-566c-1eec2fa4b84b@quicinc.com>
 <5a872001-dfcb-5c00-3b13-3c29e6be213b@linaro.org>
 <93f5dc05-a91b-0fe6-c7e2-d3457cba8a1f@quicinc.com>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
In-Reply-To: <93f5dc05-a91b-0fe6-c7e2-d3457cba8a1f@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 14/04/2023 12:49, Mukesh Ojha wrote:
> 
> 
> On 4/14/2023 5:10 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 14/04/2023 12:14, Mukesh Ojha wrote:
>>>
>>>
>>> On 4/14/2023 4:14 PM, Srinivas Kandagatla wrote:
>>>>
>>>>
>>>> On 22/03/2023 13:30, Mukesh Ojha wrote:
>>>>> qcom_minidump driver provides qcom_minidump_subsystem_desc()
>>>>> exported API which other driver can use it query subsystem
>>>>> descriptor. Refactor qcom_minidump() to use this symbol.
>>>>>
>>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>>> ---
>>>>>   drivers/remoteproc/qcom_common.c | 13 ++-----------
>>>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/remoteproc/qcom_common.c 
>>>>> b/drivers/remoteproc/qcom_common.c
>>>>> index 88fc984..240e9f7 100644
>>>>> --- a/drivers/remoteproc/qcom_common.c
>>>>> +++ b/drivers/remoteproc/qcom_common.c
>>>>> @@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, 
>>>>> unsigned int minidump_id,
>>>>>   {
>>>>>       int ret;
>>>>>       struct minidump_subsystem *subsystem;
>>>>> -    struct minidump_global_toc *toc;
>>>>> -    /* Get Global minidump ToC*/
>>>>> -    toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, 
>>>>> NULL);
>>>>> -
>>>>> -    /* check if global table pointer exists and init is set */
>>>>> -    if (IS_ERR(toc) || !toc->status) {
>>>>> -        dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
>>>>> +    subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>>>> +    if (IS_ERR(subsystem))
>>>>>           return;
>>>>
>>>> Sorry If I am missing something but I got lost looking at the below 
>>>> code snippet in drivers/remoteproc/qcom_common.c
>>>>
>>>>
>>>> -------------------->cut<-----------------------------
>>>>      subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>>>      if (IS_ERR(subsystem))
>>>>          return;
>>>>
>>>>      /**
>>>>       * Collect minidump if SS ToC is valid and segment table
>>>>       * is initialized in memory and encryption status is set.
>>>>       */
>>>>      if (subsystem->regions_baseptr == 0 ||
>>>>          le32_to_cpu(subsystem->status) != 1 ||
>>>>          le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
>>>>          le32_to_cpu(subsystem->encryption_status) != 
>>>> MINIDUMP_SS_ENCR_DONE) {
>>>>          dev_err(&rproc->dev, "Minidump not ready, skipping\n");
>>>>          return;
>>>>      }
>>>> -------------------->cut<-----------------------------
>>>>
>>>> where does "subsystem->regions_baseptr" for this ADSP minidump 
>>>> descriptor get set?
>>>
>>> Other co-processor such as adsp/cdsp/Mpss has their own way of 
>>> registering their region/segment (mostly they are static known
>>> regions) with minidump global infra and which could be happening
>>> from firmware side .
>> If its happening from firmware side, then that ram phys address range 
>> should be reserved from kernel usage I guess.
>>
>> Do you have more details on where exactly is this reserved from within 
>> linux kernel?
> 
> These regions are inside remoteproc memory carve-out.
> like.
> 
> adsp_mem: memory@85e00000 {
>      reg = <0x0 0x85e00000 0x0 0x2100000>;
>      no-map;
> };

thanks for explaining.

--srini
> 
> 
> 
> remoteproc_adsp: remoteproc@30000000 {
>      compatible = "qcom,sm8450-adsp-pas";
>      reg = <0 0x30000000 0 0x100>;
>             ...
>             ...
>      memory-region = <&adsp_mem>; <==
> 
> -Mukesh
> 
>>
>>
>> --srini
>>
>>>
>>>
>>> -Mukesh
>>>
>>>>
>>>>
>>>> --srini
>>>>
>>>>> -    }
>>>>> -
>>>>> -    /* Get subsystem table of contents using the minidump id */
>>>>> -    subsystem = &toc->subsystems[minidump_id];
>>>>>       /**
>>>>>        * Collect minidump if SS ToC is valid and segment table