| 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: <4c03413b-34f4-44d3-8f12-786af265d59c@linaro.org>
Date: Wed, 5 Jun 2024 14:00:59 +0200
From: Caleb Connolly <caleb.connolly@...aro.org>
To: "Aiqun Yu (Maria)" <quic_aiquny@...cinc.com>,
Tengfei Fan <quic_tengfan@...cinc.com>, andersson@...nel.org,
konrad.dybcio@...aro.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, dmitry.baryshkov@...aro.org
Cc: linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, kernel@...cinc.com
Subject: Re: [PATCH v9 2/4] arm64: dts: qcom: qcs8550: introduce qcs8550 dtsi
Hi,
On 05/06/2024 06:51, Aiqun Yu (Maria) wrote:
>
>
> On 6/4/2024 7:20 PM, Caleb Connolly wrote:
>>
>>
>> On 04/06/2024 12:51, Aiqun Yu (Maria) wrote:
>>>
>>>
>>> On 6/3/2024 5:20 PM, Caleb Connolly wrote:
>>>> Hi Tengfei,
>>>>
>>>> On 29/05/2024 12:09, Tengfei Fan wrote:
>>>>> QCS8550 is derived from SM8550. The difference between SM8550 and
>>>>> QCS8550 is QCS8550 doesn't have modem RF system. QCS8550 is mainly used
>>>>> in IoT products.
>>>>> QCS8550 firmware has different memory map compared to SM8550.
>>>>> The memory map will be runtime added through bootloader.
>>>>> There are 3 types of reserved memory regions here:
>>>>> 1. Firmware related regions which aren't shared with kernel.
>>>>> The device tree source in kernel doesn't need to have node to
>>>>> indicate
>>>>> the firmware related reserved information. Bootloader converys the
>>>>> information by updating devicetree at runtime.
>>>>> This will be described as: UEFI saves the physical address of the
>>>>> UEFI System Table to dts file's chosen node. Kernel read this table and
>>>>> add reserved memory regions to efi config table. Current reserved
>>>>> memory
>>>>> region may have reserved region which was not yet used, release note of
>>>>> the firmware have such kind of information.
>>>>
>>>> Are you describing some particular quirk of the platform here, or just
>>>> standard UEFI booting?
>>>
>>> It's standard UEFI booting efi config table.
>>
>> Ok, thanks for confirming.
>>>>
>>>> When booting with UEFI, the memory map is passed via the ESRT, so having
>>>> memory that the kernel shouldn't use it pretty simple (and typical).
>>
>> woo! \o/
>>>
>>> yes. It is very simple. And the bootloader firmware config the
>>> "reserved" region in the efi config table from the uefi firmware.
>>>>> 2. Firmware related memory regions which are shared with Kernel
>>>>> The device tree source in the kernel needs to include nodes that
>>>>> indicate fimware-related shared information. A label name is suggested
>>>>> because this type of shared information needs to be referenced by
>>>>> specific drivers for handling purposes.
>>>>
>>>> Again, is there something non-standard here? If not I would suggest
>>>> dropping these detail comments as they might be misleading.
>>>
>>> Detailed comments is used to describe current device tree reserved
>>> memory regions.
>>>
>>> Current patch is not creating a new mechanism to have memory map
>>> described. But it is the first time qcom device trees use this design,
>>> and have a simplified(also more compatible) device tree reserved memory
>>> region(memory map). Previously, bootloader(apps bootloader) only pass
>>> the whole physical memory base and size, and use reserved memory nodes
>>> only in device tree(which is also a standard choose).
>>>
>>> So that's why it is detailed comments for other qcom platform reference.
>>
>> Doesn't the rb3gen2 also use this design?
>
> Checked current qcs6490-rb3gen2.dts still use the device tree to have
> all the reserved regions, even have detailed regions like "Firmware
> related regions which aren't shared with kernel."
Right,
>
> Not sure current qcs6490 firmware efi config table looks like, if it
> have all the reserved region marked carefully on efi config table, then
> device tree don't need to mention the reserved regions which is not
> shared to kernel.
That makes sense.
>
> The qcom memory map in device tree discussion was happened after qcs6490
> rb3gen2 time frame. efi config table is standard. But we still need to
> check what's the final config placed in the table for different
> platforms. I will suggest to have current qcs8550 as an example to
> config the current memory non-kernel needed to know region inside the
> efi config table in bootloader, and have kernel shared reserved region
> marked in the device tree.
Ok, thanks for explaining the context here. Using the ESRT for this
certainly makes more sense to me.
So regarding the comment in the reserved-memory node below, I think this
could be simplified to just a sentence or two explaining how this
platform is different. Maybe something like:
/* Unlike previous platforms, QCS8550 boots using EFI and describes most
reserved regions in the ESRT memory map. As a result, reserved memory
regions which aren't relevant to the kernel (like the hypervisor region)
don't need to be described in DT. */
A few more comments in-line.
>
>>>
>>>>
>>>> Thanks and regards,
>>>>> 3. Remoteproc regions.
>>>>> Remoteproc regions will be reserved and then assigned to
>>>>> subsystem
>>>>> firmware later.
>>>>> Here is a reserved memory map for this platform:
>>>>> 0x100000000 +-------------------+
>>>>> | |
>>>>> | Firmware Related |
>>>>> | |
>>>>> 0xd4d00000 +-------------------+
>>>>> | |
>>>>> | Kernel Available |
>>>>> | |
>>>>> 0xa7000000 +-------------------+
>>>>> | |
>>>>> | Remoteproc Region |
>>>>> | |
>>>>> 0x8a800000 +-------------------+
>>>>> | |
>>>>> | Firmware Related |
>>>>> | |
>>>>> 0x80000000 +-------------------+
>>>>>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>>>>> Signed-off-by: Tengfei Fan <quic_tengfan@...cinc.com>
>>>>> ---
>>>>> arch/arm64/boot/dts/qcom/qcs8550.dtsi | 167
>>>>> ++++++++++++++++++++++++++
>>>>> 1 file changed, 167 insertions(+)
>>>>> create mode 100644 arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>> b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>> new file mode 100644
>>>>> index 000000000000..685668c6ad14
>>>>> --- /dev/null
>>>>> +++ b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>> @@ -0,0 +1,167 @@
>>>>> +// SPDX-License-Identifier: BSD-3-Clause
>>>>> +/*
>>>>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All
>>>>> rights reserved.
>>>>> + */
>>>>> +
>>>>> +#include "sm8550.dtsi"
>>>>> +
>>>>> +/delete-node/ &reserved_memory;
>>>>> +
>>>>> +/ {
>>>>> + reserved_memory: reserved-memory {
>>>>> + #address-cells = <2>;
>>>>> + #size-cells = <2>;
>>>>> + ranges;
>>>>> +
>>>>> +
>>>>> + /* These are 3 types of reserved memory regions here:
>>>>> + * 1. Firmware related regions which aren't shared with
>>>>> kernel.
>>>>> + * The device tree source in kernel doesn't need to have
>>>>> node to
>>>>> + * indicate the firmware related reserved information.
>>>>> Bootloader
>>>>> + * conveys the information by updating devicetree at runtime.
>>>>> + * This will be described as: UEFI saves the physical
>>>>> address of
>>>>> + * the UEFI System Table to dts file's chosen node. Kernel
>>>>> read this
>>>>> + * table and add reserved memory regions to efi config table.
>>>>> Current
>>>>> + * reserved memory region may have reserved region which was
>>>>> not yet
>>>>> + * used, release note of the firmware have such kind of
>>>>> information.
>>>>> + * 2. Firmware related memory regions which are shared with
>>>>> Kernel
>>>>> + * The device tree source in the kernel needs to include
>>>>> nodes
>>>>> + * that indicate fimware-related shared information. A label
>>>>> name
>>>>> + * is suggested because this type of shared information
>>>>> needs to
>>>>> + * be referenced by specific drivers for handling purposes.
>>>>> + * 3. Remoteproc regions.
>>>>> + * Remoteproc regions will be reserved and then
>>>>> assigned to
>>>>> + * subsystem firmware later.
>>>>> + * Here is a reserved memory map for this platform:
>>>>> + * 0x100000000 +-------------------+
>>>>> + * | |
>>>>> + * | Firmware Related |
>>>>> + * | |
>>>>> + * 0xd4d00000 +-------------------+
>>>>> + * | |
>>>>> + * | Kernel Available |
>>>>> + * | |
>>>>> + * 0xa7000000 +-------------------+
>>>>> + * | |
>>>>> + * | Remoteproc Region |
>>>>> + * | |
>>>>> + * 0x8a800000 +-------------------+
>>>>> + * | |
>>>>> + * | Firmware Related |
>>>>> + * | |
>>>>> + * 0x80000000 +-------------------+
I guess this is quite subjective, but this diagram looks "upside down"
to me. I think it's generally more popular to have the lower addresses
at the top.
>>>>> + */
>>>>> +
>>>>> + /*
>>>>> + * Firmware related regions, bootloader will possible reserve
>>>>> parts of
>>>>> + * region from 0x80000000..0x8a800000.
This is just duplicating info from the table, please drop this comment
(it should be obvious from the above explanation).
>>>>> + */
>>>>> + aop_image_mem: aop-image-region@...00000 {
>>>>> + reg = <0x0 0x81c00000 0x0 0x60000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + aop_cmd_db_mem: aop-cmd-db-region@...60000 {
>>>>> + compatible = "qcom,cmd-db";
>>>>> + reg = <0x0 0x81c60000 0x0 0x20000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + aop_config_mem: aop-config-region@...80000 {
>>>>> + no-map;
>>>>> + reg = <0x0 0x81c80000 0x0 0x20000>;
>>>>> + };
>>>>> +
>>>>> + smem_mem: smem-region@...00000 {
>>>>> + compatible = "qcom,smem";
>>>>> + reg = <0x0 0x81d00000 0x0 0x200000>;
>>>>> + hwlocks = <&tcsr_mutex 3>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + adsp_mhi_mem: adsp-mhi-region@...00000 {
>>>>> + reg = <0x0 0x81f00000 0x0 0x20000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + /* PIL region */
Drop this comment
>>>>> + mpss_mem: mpss-region@...00000 {
>>>>> + reg = <0x0 0x8a800000 0x0 0x10800000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + q6_mpss_dtb_mem: q6-mpss-dtb-region@...00000 {
>>>>> + reg = <0x0 0x9b000000 0x0 0x80000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + ipa_fw_mem: ipa-fw-region@...80000 {
>>>>> + reg = <0x0 0x9b080000 0x0 0x10000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + ipa_gsi_mem: ipa-gsi-region@...90000 {
>>>>> + reg = <0x0 0x9b090000 0x0 0xa000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + gpu_micro_code_mem: gpu-micro-code-region@...9a000 {
>>>>> + reg = <0x0 0x9b09a000 0x0 0x2000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + spss_region_mem: spss-region@...00000 {
>>>>> + reg = <0x0 0x9b100000 0x0 0x180000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + spu_secure_shared_memory_mem:
>>>>> spu-secure-shared-memory-region@...80000 {
>>>>> + reg = <0x0 0x9b280000 0x0 0x80000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + camera_mem: camera-region@...00000 {
>>>>> + reg = <0x0 0x9b300000 0x0 0x800000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + video_mem: video-region@...00000 {
>>>>> + reg = <0x0 0x9bb00000 0x0 0x700000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + cvp_mem: cvp-region@...00000 {
>>>>> + reg = <0x0 0x9c200000 0x0 0x700000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + cdsp_mem: cdsp-region@...00000 {
>>>>> + reg = <0x0 0x9c900000 0x0 0x2000000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + q6_cdsp_dtb_mem: q6-cdsp-dtb-region@...00000 {
>>>>> + reg = <0x0 0x9e900000 0x0 0x80000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + q6_adsp_dtb_mem: q6-adsp-dtb-region@...80000 {
>>>>> + reg = <0x0 0x9e980000 0x0 0x80000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + adspslpi_mem: adspslpi-region@...00000 {
>>>>> + reg = <0x0 0x9ea00000 0x0 0x4080000>;
>>>>> + no-map;
>>>>> + };
>>>>> +
>>>>> + /*
>>>>> + * Firmware related regions, bootloader will possible reserve
>>>>> parts of
>>>>> + * region from 0xd8000000..0x100000000.
>>>>> + */
The address specified in this comment (0xd8000000) doesn't match the
mpss_dsm_mem region OR the diagram above. I would suggest dropping this
comment too.
>>>>> + mpss_dsm_mem: mpss_dsm_region@...00000 {
>>>>> + reg = <0x0 0xd4d00000 0x0 0x3300000>;
>>>>> + no-map;
>>>>> + };
>>>>> + };
>>>>> +};
>>>>
>>>
>>
>
Kind regards,
--
// Caleb (they/them)
Powered by blists - more mailing lists