[PATCH] firmware: stratix10-svc: build only on 64-bit ARM

[PATCH] firmware: stratix10-svc: build only on 64-bit ARM

[Dinh Nguyen]

From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

The Stratix10 service layer and RCU drivers are useful only on
Stratix10, so on ARMv8.  Compile testing the RCU driver on 32-bit ARM
fails:

  drivers/firmware/stratix10-rsu.c: In function 'rsu_status_callback':
  include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_179'
    declared with attribute error: FIELD_GET: type of reg too small for mask
    _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
  ...
  drivers/firmware/stratix10-rsu.c:96:26: note: in expansion of macro 'FIELD_GET'
    priv->status.version = FIELD_GET(RSU_VERSION_MASK,

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Reported-by: kernel test robot <lkp@intel.com>
Acked-by: Richard Gong <richard.gong@linux.intel.com>
Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
 drivers/firmware/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 6a4e882e448d..08bd4d01fb04 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -206,7 +206,7 @@ config FW_CFG_SYSFS_CMDLINE
 
 config INTEL_STRATIX10_SERVICE
 	tristate "Intel Stratix10 Service Layer"
-	depends on ARCH_INTEL_SOCFPGA && HAVE_ARM_SMCCC
+	depends on ARCH_INTEL_SOCFPGA && ARM64 && HAVE_ARM_SMCCC
 	default n
 	help
 	  Intel Stratix10 service layer runs at privileged exception level,
-- 
2.25.1

Re: [PATCH] firmware: stratix10-svc: build only on 64-bit ARM

[Greg KH]

On Thu, Apr 01, 2021 at 10:13:29AM -0500, Dinh Nguyen wrote:
> From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> 
> The Stratix10 service layer and RCU drivers are useful only on
> Stratix10, so on ARMv8.  Compile testing the RCU driver on 32-bit ARM
> fails:
> 
>   drivers/firmware/stratix10-rsu.c: In function 'rsu_status_callback':
>   include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_179'
>     declared with attribute error: FIELD_GET: type of reg too small for mask
>     _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>   ...
>   drivers/firmware/stratix10-rsu.c:96:26: note: in expansion of macro 'FIELD_GET'
>     priv->status.version = FIELD_GET(RSU_VERSION_MASK,
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Acked-by: Richard Gong <richard.gong@linux.intel.com>
> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
> ---
>  drivers/firmware/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

What commit caused this error to happen?  Can you resend this with a
"Fixes:" tag with that information in it?

thanks,

greg k-h

[PATCH] firmware: stratix10-svc: build only on 64-bit ARM

[Krzysztof Kozlowski]

The Stratix10 service layer and RCU drivers are useful only on
Stratix10, so on ARMv8.  Compile testing the RCU driver on 32-bit ARM
fails:

  drivers/firmware/stratix10-rsu.c: In function 'rsu_status_callback':
  include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_179'
    declared with attribute error: FIELD_GET: type of reg too small for mask
    _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
  ...
  drivers/firmware/stratix10-rsu.c:96:26: note: in expansion of macro 'FIELD_GET'
    priv->status.version = FIELD_GET(RSU_VERSION_MASK,

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Reported-by: kernel test robot <lkp@intel.com>

---

Fix for commit in:
https://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux.git
---
 drivers/firmware/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 6a4e882e448d..08bd4d01fb04 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -206,7 +206,7 @@ config FW_CFG_SYSFS_CMDLINE
 
 config INTEL_STRATIX10_SERVICE
 	tristate "Intel Stratix10 Service Layer"
-	depends on ARCH_INTEL_SOCFPGA && HAVE_ARM_SMCCC
+	depends on ARCH_INTEL_SOCFPGA && ARM64 && HAVE_ARM_SMCCC
 	default n
 	help
 	  Intel Stratix10 service layer runs at privileged exception level,
-- 
2.25.1

Re: [PATCH] firmware: stratix10-svc: build only on 64-bit ARM

[Arnd Bergmann]

On Sun, Mar 21, 2021 at 7:46 PM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> The Stratix10 service layer and RCU drivers are useful only on
> Stratix10, so on ARMv8.  Compile testing the RCU driver on 32-bit ARM
> fails:
>
>   drivers/firmware/stratix10-rsu.c: In function 'rsu_status_callback':
>   include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_179'
>     declared with attribute error: FIELD_GET: type of reg too small for mask
>     _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>   ...
>   drivers/firmware/stratix10-rsu.c:96:26: note: in expansion of macro 'FIELD_GET'
>     priv->status.version = FIELD_GET(RSU_VERSION_MASK,
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Reported-by: kernel test robot <lkp@intel.com>

While I agree that one shouldn't run 32-bit kernels on this, we should also try
to write drivers portably, and in theory any SoC that can run a 64-bit
Arm kernel
should also be able to run a 32-bit kernel if you include the same drivers.

It seems that the problem here is in the smccc definition

struct arm_smccc_res {
        unsigned long a0;
        unsigned long a1;
        unsigned long a2;
        unsigned long a3;
};

so the result of

#define RSU_VERSION_MASK                GENMASK_ULL(63, 32)
          priv->status.version = FIELD_GET(RSU_VERSION_MASK, res->a2);

tries to access bits that are just not returned by the firmware here,
which indicates that it probably won't work in this case.

What I'm not entirely sure about is whether this is a problem in
the Intel firmware implementation requiring the smccc caller to
run in a 64-bit context, or if it's a mistake in the way the driver
extracts the information if the firmware can actually pass it down
correctly.

        Arnd

Re: [PATCH] firmware: stratix10-svc: build only on 64-bit ARM

[Krzysztof Kozlowski]

On 21/03/2021 22:09, Arnd Bergmann wrote:
> On Sun, Mar 21, 2021 at 7:46 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> The Stratix10 service layer and RCU drivers are useful only on
>> Stratix10, so on ARMv8.  Compile testing the RCU driver on 32-bit ARM
>> fails:
>>
>>   drivers/firmware/stratix10-rsu.c: In function 'rsu_status_callback':
>>   include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_179'
>>     declared with attribute error: FIELD_GET: type of reg too small for mask
>>     _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>   ...
>>   drivers/firmware/stratix10-rsu.c:96:26: note: in expansion of macro 'FIELD_GET'
>>     priv->status.version = FIELD_GET(RSU_VERSION_MASK,
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>> Reported-by: kernel test robot <lkp@intel.com>
> 
> While I agree that one shouldn't run 32-bit kernels on this, we should also try
> to write drivers portably, and in theory any SoC that can run a 64-bit
> Arm kernel
> should also be able to run a 32-bit kernel if you include the same drivers.
> 
> It seems that the problem here is in the smccc definition
> 
> struct arm_smccc_res {
>         unsigned long a0;
>         unsigned long a1;
>         unsigned long a2;
>         unsigned long a3;
> };
> 
> so the result of
> 
> #define RSU_VERSION_MASK                GENMASK_ULL(63, 32)
>           priv->status.version = FIELD_GET(RSU_VERSION_MASK, res->a2);
> 
> tries to access bits that are just not returned by the firmware here,
> which indicates that it probably won't work in this case.
> 
> What I'm not entirely sure about is whether this is a problem in
> the Intel firmware implementation requiring the smccc caller to
> run in a 64-bit context, or if it's a mistake in the way the driver
> extracts the information if the firmware can actually pass it down
> correctly.

The SMC has two calling conventions - SMC32/HVC32 and SMC64/HVC64. The
Stratix 10 driver uses the 64-bit calling convention (see
INTEL_SIP_SMC_FAST_CALL_VAL in
include/linux/firmware/intel/stratix10-smc.h), so it should not run in
aarch32 (regardless of type of hardware).

I think that my patch limiting the support to 64-bit makes sense.

Best regards,
Krzysztof

Re: [PATCH] firmware: stratix10-svc: build only on 64-bit ARM

[Arnd Bergmann]

On Mon, Mar 22, 2021 at 9:26 AM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
> On 21/03/2021 22:09, Arnd Bergmann wrote:
>
> The SMC has two calling conventions - SMC32/HVC32 and SMC64/HVC64. The
> Stratix 10 driver uses the 64-bit calling convention (see
> INTEL_SIP_SMC_FAST_CALL_VAL in
> include/linux/firmware/intel/stratix10-smc.h), so it should not run in
> aarch32 (regardless of type of hardware).
>
> I think that my patch limiting the support to 64-bit makes sense.

I see that this is the only driver in the kernel that doesn't support the
32-bit calling conventions though, everything else either uses the the 32-bit
calling conventions unconditionally, or picks the ones matching the
kernel execution state.

If the firmware supports both, it would seem best to change the driver
to work like the other ones and pick the appropriate interface based
on what kernel it's running on.

If the firmware is fundamentally limited to the 64-bit interface, your
patch does seem correct, but I'd suggest explaining that in the
changelog.

         Arnd

Re: [PATCH] firmware: stratix10-svc: build only on 64-bit ARM

[Richard Gong]

On 3/22/21 3:26 AM, Krzysztof Kozlowski wrote:
> 
> On 21/03/2021 22:09, Arnd Bergmann wrote:
>> On Sun, Mar 21, 2021 at 7:46 PM Krzysztof Kozlowski
>> <krzysztof.kozlowski@canonical.com> wrote:
>>>
>>> The Stratix10 service layer and RCU drivers are useful only on
>>> Stratix10, so on ARMv8.  Compile testing the RCU driver on 32-bit ARM
>>> fails:
>>>
>>>    drivers/firmware/stratix10-rsu.c: In function 'rsu_status_callback':
>>>    include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_179'
>>>      declared with attribute error: FIELD_GET: type of reg too small for mask
>>>      _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>>    ...
>>>    drivers/firmware/stratix10-rsu.c:96:26: note: in expansion of macro 'FIELD_GET'
>>>      priv->status.version = FIELD_GET(RSU_VERSION_MASK,
>>>
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> While I agree that one shouldn't run 32-bit kernels on this, we should also try
>> to write drivers portably, and in theory any SoC that can run a 64-bit
>> Arm kernel
>> should also be able to run a 32-bit kernel if you include the same drivers.
>>
>> It seems that the problem here is in the smccc definition
>>
>> struct arm_smccc_res {
>>          unsigned long a0;
>>          unsigned long a1;
>>          unsigned long a2;
>>          unsigned long a3;
>> };
>>
>> so the result of
>>
>> #define RSU_VERSION_MASK                GENMASK_ULL(63, 32)
>>            priv->status.version = FIELD_GET(RSU_VERSION_MASK, res->a2);
>>
>> tries to access bits that are just not returned by the firmware here,
>> which indicates that it probably won't work in this case.
>>
>> What I'm not entirely sure about is whether this is a problem in
>> the Intel firmware implementation requiring the smccc caller to
>> run in a 64-bit context, or if it's a mistake in the way the driver
>> extracts the information if the firmware can actually pass it down
>> correctly.
> 
> The SMC has two calling conventions - SMC32/HVC32 and SMC64/HVC64. The
> Stratix 10 driver uses the 64-bit calling convention (see
> INTEL_SIP_SMC_FAST_CALL_VAL in
> include/linux/firmware/intel/stratix10-smc.h), so it should not run in
> aarch32 (regardless of type of hardware).
> 
> I think that my patch limiting the support to 64-bit makes sense.
> 

The stratix10 service layer and RSU driver are only used in Intel 64-bit 
SoCFPGA platforms.

> Best regards,
> Krzysztof
> 

Re: [PATCH] firmware: stratix10-svc: build only on 64-bit ARM

[Krzysztof Kozlowski]

On 22/03/2021 13:58, Richard Gong wrote:
> 
> 
> On 3/22/21 3:26 AM, Krzysztof Kozlowski wrote:
>>
>> On 21/03/2021 22:09, Arnd Bergmann wrote:
>>> On Sun, Mar 21, 2021 at 7:46 PM Krzysztof Kozlowski
>>> <krzysztof.kozlowski@canonical.com> wrote:
>>>>
>>>> The Stratix10 service layer and RCU drivers are useful only on
>>>> Stratix10, so on ARMv8.  Compile testing the RCU driver on 32-bit ARM
>>>> fails:
>>>>
>>>>    drivers/firmware/stratix10-rsu.c: In function 'rsu_status_callback':
>>>>    include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_179'
>>>>      declared with attribute error: FIELD_GET: type of reg too small for mask
>>>>      _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>>>    ...
>>>>    drivers/firmware/stratix10-rsu.c:96:26: note: in expansion of macro 'FIELD_GET'
>>>>      priv->status.version = FIELD_GET(RSU_VERSION_MASK,
>>>>
>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>
>>> While I agree that one shouldn't run 32-bit kernels on this, we should also try
>>> to write drivers portably, and in theory any SoC that can run a 64-bit
>>> Arm kernel
>>> should also be able to run a 32-bit kernel if you include the same drivers.
>>>
>>> It seems that the problem here is in the smccc definition
>>>
>>> struct arm_smccc_res {
>>>          unsigned long a0;
>>>          unsigned long a1;
>>>          unsigned long a2;
>>>          unsigned long a3;
>>> };
>>>
>>> so the result of
>>>
>>> #define RSU_VERSION_MASK                GENMASK_ULL(63, 32)
>>>            priv->status.version = FIELD_GET(RSU_VERSION_MASK, res->a2);
>>>
>>> tries to access bits that are just not returned by the firmware here,
>>> which indicates that it probably won't work in this case.
>>>
>>> What I'm not entirely sure about is whether this is a problem in
>>> the Intel firmware implementation requiring the smccc caller to
>>> run in a 64-bit context, or if it's a mistake in the way the driver
>>> extracts the information if the firmware can actually pass it down
>>> correctly.
>>
>> The SMC has two calling conventions - SMC32/HVC32 and SMC64/HVC64. The
>> Stratix 10 driver uses the 64-bit calling convention (see
>> INTEL_SIP_SMC_FAST_CALL_VAL in
>> include/linux/firmware/intel/stratix10-smc.h), so it should not run in
>> aarch32 (regardless of type of hardware).
>>
>> I think that my patch limiting the support to 64-bit makes sense.
>>
> 
> The stratix10 service layer and RSU driver are only used in Intel 64-bit 
> SoCFPGA platforms.

This we know, however the questions were:
1. Why the driver cannot be made portable? Why it cannot be developed in
a way it allows building on different platforms?
2. Does the actual firmware support 32-bit SMC convention call?

Best regards,
Krzysztof

Re: [PATCH] firmware: stratix10-svc: build only on 64-bit ARM

[Richard Gong]

On 3/22/21 7:41 AM, Krzysztof Kozlowski wrote:
> On 22/03/2021 13:58, Richard Gong wrote:
>>
>>
>> On 3/22/21 3:26 AM, Krzysztof Kozlowski wrote:
>>>
>>> On 21/03/2021 22:09, Arnd Bergmann wrote:
>>>> On Sun, Mar 21, 2021 at 7:46 PM Krzysztof Kozlowski
>>>> <krzysztof.kozlowski@canonical.com> wrote:
>>>>>
>>>>> The Stratix10 service layer and RCU drivers are useful only on
>>>>> Stratix10, so on ARMv8.  Compile testing the RCU driver on 32-bit ARM
>>>>> fails:
>>>>>
>>>>>     drivers/firmware/stratix10-rsu.c: In function 'rsu_status_callback':
>>>>>     include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_179'
>>>>>       declared with attribute error: FIELD_GET: type of reg too small for mask
>>>>>       _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>>>>     ...
>>>>>     drivers/firmware/stratix10-rsu.c:96:26: note: in expansion of macro 'FIELD_GET'
>>>>>       priv->status.version = FIELD_GET(RSU_VERSION_MASK,
>>>>>
>>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>>
>>>> While I agree that one shouldn't run 32-bit kernels on this, we should also try
>>>> to write drivers portably, and in theory any SoC that can run a 64-bit
>>>> Arm kernel
>>>> should also be able to run a 32-bit kernel if you include the same drivers.
>>>>
>>>> It seems that the problem here is in the smccc definition
>>>>
>>>> struct arm_smccc_res {
>>>>           unsigned long a0;
>>>>           unsigned long a1;
>>>>           unsigned long a2;
>>>>           unsigned long a3;
>>>> };
>>>>
>>>> so the result of
>>>>
>>>> #define RSU_VERSION_MASK                GENMASK_ULL(63, 32)
>>>>             priv->status.version = FIELD_GET(RSU_VERSION_MASK, res->a2);
>>>>
>>>> tries to access bits that are just not returned by the firmware here,
>>>> which indicates that it probably won't work in this case.
>>>>
>>>> What I'm not entirely sure about is whether this is a problem in
>>>> the Intel firmware implementation requiring the smccc caller to
>>>> run in a 64-bit context, or if it's a mistake in the way the driver
>>>> extracts the information if the firmware can actually pass it down
>>>> correctly.
>>>
>>> The SMC has two calling conventions - SMC32/HVC32 and SMC64/HVC64. The
>>> Stratix 10 driver uses the 64-bit calling convention (see
>>> INTEL_SIP_SMC_FAST_CALL_VAL in
>>> include/linux/firmware/intel/stratix10-smc.h), so it should not run in
>>> aarch32 (regardless of type of hardware).
>>>
>>> I think that my patch limiting the support to 64-bit makes sense.
>>>
>>
>> The stratix10 service layer and RSU driver are only used in Intel 64-bit
>> SoCFPGA platforms.
> 
> This we know, however the questions were:
> 1. Why the driver cannot be made portable? Why it cannot be developed in
> a way it allows building on different platforms?

The drivers was originally developed for Intel Stratix10 SoCFPGA 
platform, which is ARM 64-bit architecture. The same drivers can be used 
for other Intel ARM 64-bit SoCFPGA platforms (Agilex, eASIC N5X as 
example), which have the same SDM architecture as Stratix10 has.

SDM = Secure Device Manager

So far Intel 32-bit SoCFPGA platform doesn't support SDM architecture.

> 2. Does the actual firmware support 32-bit SMC convention call?

No.

> 
> Best regards,
> Krzysztof
> 

Regards,
Richard

Re: [PATCH] firmware: stratix10-svc: build only on 64-bit ARM

[Richard Gong]

On 3/21/21 1:46 PM, Krzysztof Kozlowski wrote:
> The Stratix10 service layer and RCU drivers are useful only on
> Stratix10, so on ARMv8.  Compile testing the RCU driver on 32-bit ARM
> fails:
> 
>    drivers/firmware/stratix10-rsu.c: In function 'rsu_status_callback':
>    include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_179'
>      declared with attribute error: FIELD_GET: type of reg too small for mask
>      _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>    ...
>    drivers/firmware/stratix10-rsu.c:96:26: note: in expansion of macro 'FIELD_GET'
>      priv->status.version = FIELD_GET(RSU_VERSION_MASK,
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Reported-by: kernel test robot <lkp@intel.com>
> 
> ---
> 
> Fix for commit in:
> https://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux.git
> ---
>   drivers/firmware/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 6a4e882e448d..08bd4d01fb04 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -206,7 +206,7 @@ config FW_CFG_SYSFS_CMDLINE
>   
>   config INTEL_STRATIX10_SERVICE
>   	tristate "Intel Stratix10 Service Layer"
> -	depends on ARCH_INTEL_SOCFPGA && HAVE_ARM_SMCCC
> +	depends on ARCH_INTEL_SOCFPGA && ARM64 && HAVE_ARM_SMCCC
>   	default n
>   	help
>   	  Intel Stratix10 service layer runs at privileged exception level,
> 
Acked-by: Richard Gong <richard.gong@linux.intel.com>

Regards,
Richard