| 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: <CAOesGMhJs9E9j5zjCo-ZP6VDFShAtc7tGz21w_oLTmPWrv0Hww@mail.gmail.com>
Date: Mon, 19 Jun 2017 14:05:24 -0700
From: Olof Johansson <olof@...om.net>
To: Stanimir Varbanov <stanimir.varbanov@...aro.org>
Cc: Andy Gross <andy.gross@...aro.org>, Arnd Bergmann <arnd@...db.de>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
linux-arm-msm <linux-arm-msm@...r.kernel.org>,
Stephen Boyd <sboyd@...eaurora.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>
Subject: Re: [PATCH v2] firmware: qcom_scm: Fix to allow COMPILE_TEST-ing
On Mon, Jun 19, 2017 at 4:25 AM, Stanimir Varbanov
<stanimir.varbanov@...aro.org> wrote:
> Hi Olof,
>
> On 06/19/2017 08:35 AM, Olof Johansson wrote:
>> Hi,
>>
>> On Thu, Jun 8, 2017 at 8:23 AM, Stanimir Varbanov
>> <stanimir.varbanov@...aro.org> wrote:
>>> Unfortunatly previous attempt to allow consumer drivers to
>>> use COMPILE_TEST option in Kconfig is not enough, because in the
>>> past the consumer drivers used 'depends on' Kconfig option but
>>> now they are using 'select' Kconfig option which means on non ARM
>>> arch'es compilation is triggered. Thus we need to move the ifdefery
>>> one level below by touching the private qcom_scm.h header.
>>>
>>> Cc: Stephen Boyd <sboyd@...eaurora.org>
>>> Cc: Bjorn Andersson <bjorn.andersson@...aro.org>
>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@...aro.org>
>>> ---
>>> This is second version of the patch with comments addressed:
>>> * proper identation for static inline functions
>>> * to avoid duplicating defines group them on top of the header
>>>
>>> The first version has been part of the venus driver patchset
>>> v8 and can be found at:
>>> https://patchwork.kernel.org/patch/9704275/
>>>
>>> drivers/firmware/Kconfig | 2 +-
>>> drivers/firmware/qcom_scm.h | 122 ++++++++++++++++++++++++++++++++++++--------
>>> include/linux/qcom_scm.h | 32 ------------
>>> 3 files changed, 101 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>>> index 6e4ed5a9c6fd..480578c3691a 100644
>>> --- a/drivers/firmware/Kconfig
>>> +++ b/drivers/firmware/Kconfig
>>> @@ -204,7 +204,7 @@ config FW_CFG_SYSFS_CMDLINE
>>>
>>> config QCOM_SCM
>>> bool
>>> - depends on ARM || ARM64
>>> + depends on ARM || ARM64 || COMPILE_TEST
>>> select RESET_CONTROLLER
>>>
>>> config QCOM_SCM_32
>>> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
>>> index 9bea691f30fb..60689fc8a567 100644
>>> --- a/drivers/firmware/qcom_scm.h
>>> +++ b/drivers/firmware/qcom_scm.h
>>> @@ -16,31 +16,20 @@
>>> #define QCOM_SCM_BOOT_ADDR 0x1
>>> #define QCOM_SCM_BOOT_ADDR_MC 0x11
>>> #define QCOM_SCM_SET_REMOTE_STATE 0xa
>>> -extern int __qcom_scm_set_remote_state(struct device *dev, u32 state, u32 id);
>>>
>>> #define QCOM_SCM_FLAG_HLOS 0x01
>>> #define QCOM_SCM_FLAG_COLDBOOT_MC 0x02
>>> #define QCOM_SCM_FLAG_WARMBOOT_MC 0x04
>>> -extern int __qcom_scm_set_warm_boot_addr(struct device *dev, void *entry,
>>> - const cpumask_t *cpus);
>>> -extern int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
>>>
>>> #define QCOM_SCM_CMD_TERMINATE_PC 0x2
>>> #define QCOM_SCM_FLUSH_FLAG_MASK 0x3
>>> #define QCOM_SCM_CMD_CORE_HOTPLUGGED 0x10
>>> -extern void __qcom_scm_cpu_power_down(u32 flags);
>>>
>>> #define QCOM_SCM_SVC_INFO 0x6
>>> #define QCOM_IS_CALL_AVAIL_CMD 0x1
>>> -extern int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
>>> - u32 cmd_id);
>>>
>>> #define QCOM_SCM_SVC_HDCP 0x11
>>> #define QCOM_SCM_CMD_HDCP 0x01
>>> -extern int __qcom_scm_hdcp_req(struct device *dev,
>>> - struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp);
>>> -
>>> -extern void __qcom_scm_init(void);
>>>
>>> #define QCOM_SCM_SVC_PIL 0x2
>>> #define QCOM_SCM_PAS_INIT_IMAGE_CMD 0x1
>>> @@ -49,6 +38,27 @@ extern void __qcom_scm_init(void);
>>> #define QCOM_SCM_PAS_SHUTDOWN_CMD 0x6
>>> #define QCOM_SCM_PAS_IS_SUPPORTED_CMD 0x7
>>> #define QCOM_SCM_PAS_MSS_RESET 0xa
>>> +
>>> +#define QCOM_SCM_SVC_MP 0xc
>>> +#define QCOM_SCM_RESTORE_SEC_CFG 2
>>> +
>>> +#define QCOM_SCM_IOMMU_SECURE_PTBL_SIZE 3
>>> +#define QCOM_SCM_IOMMU_SECURE_PTBL_INIT 4
>>> +
>>> +#define QCOM_SCM_SVC_HDCP 0x11
>>> +#define QCOM_SCM_CMD_HDCP 0x01
>>> +
>>> +#if IS_ENABLED(CONFIG_ARM) || IS_ENABLED(CONFIG_ARM64)
>>
>> This isn't right -- this should be IS_ENABLED(QCOM_SCM)
>
> The idea was to use the __qcom_scm_xxx functions only on ARM and ARM64
> and fallback to the empty stubs on every other architecture.
>
>>
>>> +extern int __qcom_scm_set_remote_state(struct device *dev, u32 state, u32 id);
>>> +extern int __qcom_scm_set_warm_boot_addr(struct device *dev, void *entry,
>>> + const cpumask_t *cpus);
>>> +extern int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
>>> +extern void __qcom_scm_cpu_power_down(u32 flags);
>>> +extern int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
>>> + u32 cmd_id);
>>> +extern int __qcom_scm_hdcp_req(struct device *dev,
>>> + struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp);
>>> +extern void __qcom_scm_init(void);
>>> extern bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral);
>>> extern int __qcom_scm_pas_init_image(struct device *dev, u32 peripheral,
>>> dma_addr_t metadata_phys);
>>> @@ -57,6 +67,85 @@ extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
>>> extern int __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
>>> extern int __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
>>> extern int __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
>>> +extern int __qcom_scm_restore_sec_cfg(struct device *dev, u32 device_id,
>>> + u32 spare);
>>> +extern int __qcom_scm_iommu_secure_ptbl_size(struct device *dev, u32 spare,
>>> + size_t *size);
>>> +extern int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr,
>>> + u32 size, u32 spare);
>>> +#else /* !ARM and !ARM64 */
>>> +static inline int __qcom_scm_set_remote_state(struct device *dev, u32 state,
>>> + u32 id)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_set_warm_boot_addr(struct device *dev, void *entry,
>>> + const cpumask_t *cpus)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_set_cold_boot_addr(void *entry,
>>> + const cpumask_t *cpus)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline void __qcom_scm_cpu_power_down(u32 flags) {}
>>> +static inline int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
>>> + u32 cmd_id)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_hdcp_req(struct device *dev,
>>> + struct qcom_scm_hdcp_req *req,
>>> + u32 req_cnt, u32 *resp)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline void __qcom_scm_init(void) {}
>>> +static inline bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral)
>>> +{
>>> + return false;
>>> +}
>>> +static inline int __qcom_scm_pas_init_image(struct device *dev, u32 peripheral,
>>> + dma_addr_t metadata_phys)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
>>> + phys_addr_t addr, phys_addr_t size)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_pas_auth_and_reset(struct device *dev,
>>> + u32 peripheral)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_restore_sec_cfg(struct device *dev, u32 device_id,
>>> + u32 spare)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_iommu_secure_ptbl_size(struct device *dev,
>>> + u32 spare, size_t *size)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_iommu_secure_ptbl_init(struct device *dev,
>>> + u64 addr, u32 size,
>>> + u32 spare)
>>> +{
>>> + return -ENODEV;
>>> +}
>>
>> The amount of boilerplate random stubs here indiciates that the
>> abstraction point for this is wrong.
>
> This was the best solution I came to.
>
>>
>> Why are you looking to COMPILE_TEST a very platform-tied driver like
>> this on other architectures?
>
> In fact the aim is to make possible to COMPILE_TEST drivers which are
> using QCOM_SCM interface. So that qcom_scm driver itself doesn't need
> compile test coverage, and compile testing qcom_scm is impossible on
> non-ARM architectures.
Seems to me that the abstraction level is wrong then, and too much
details are leaking out of qcom_scm to the users.
>> It probably makes more sense to stub the driver->scm interface than
>> the internal scm interface if what you're looking for is driver
>> compile_test coverage.
>
> Actually, this is the state of qcom_scm if we don't apply the this
> patch, and it didn't help. Thinking more on that it looks like that
> adding COMPILE_TEST in 'config QCOM_SCM' is controversial.
>
> Arnd, Andy any ideas how to proceed. If this patch is not get merged
> (and I/we cannot find better solution) the video driver for qualcomm
> platforms will be rejected for 4.13.
Fixing the qcom_scm abstractions is probably the best way forward here.
-Olof
Powered by blists - more mailing lists