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]
Date:   Mon, 19 Jun 2017 14:25:47 +0300
From:   Stanimir Varbanov <stanimir.varbanov@...aro.org>
To:     Olof Johansson <olof@...om.net>,
        Andy Gross <andy.gross@...aro.org>,
        Arnd Bergmann <arnd@...db.de>
Cc:     "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

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.

> 
> 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.

-- 
regards,
Stan

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ