Re: [PATCH v2] xen:arm: use SMC64 version function ID on ARM64

[Brijesh Singh <brijeshkumar.singh@amd.com>]

On 10/05/2015 11:03 AM, Julien Grall wrote:
> Hi,
> 
> From the commit title, it's not clear that you are touching the PSCI code.
> 
> I would rewrite it like:
> 
> "xen/arm: psci: use SMC64 function ID when available on ARM64"
> 
> On 05/10/15 16:27, Brijesh Singh wrote:
>> As per PSCI 0.2 spec, if CPU_ON entry_point_address is 64-bit then SMC
>> call function ID parameter should be set to SMC64 version.
>>
>> Signed-off-by: Brijesh Singh <brijeshkumar.singh@amd.com>
>> ---
>>  xen/arch/arm/psci.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> index 7ad6a43..654b89f 100644
>> --- a/xen/arch/arm/psci.c
>> +++ b/xen/arch/arm/psci.c
>> @@ -23,6 +23,17 @@
>>  #include <xen/smp.h>
>>  #include <asm/psci.h>
>>  
>> +/* While a 64-bit OS can make SMC32 call convension but some calls which
> 
> s/convension/convention/
> 
>> + * requires passing function addresses (e.g CPU_ON) should set the
>> + * parameter ID to SMC64. The macro will be used to choose the appropriate
>> + * parameter ID.
> 
> This comment gives the impression that the only reason to use SMC64
> convention is to support 64bit address entry point.
> But this is not true. SMC64 also brings the support of 64bit target
> affinity, change the type of the return...
> 
> Please make a generic comment similar to the Linux one. I.e:
> 
> "While a 64-bit OS can make calls with SMC32 calling conventions, for
> some calls it is necessary to use SMC64 to pass or return 64-bit values.
> For such calls PSCI_0_2_FN_NATIVE(x) will choose the appropriate
> (native-width) function ID."
> 
Thanks for review. I will shortly send v3 with updated comment and patch title.

>> + */
>> +#ifdef CONFIG_ARM_64
>> +#define PSCI_0_2_FN_NATIVE(name)	PSCI_0_2_FN64_##name
>> +#else
>> +#define PSCI_0_2_FN_NATIVE(name)	PSCI_0_2_FN_##name
>> +#endif
>> +
>>  uint32_t psci_ver;
>>  
>>  static uint32_t psci_cpu_on_nr;
>> @@ -116,7 +127,7 @@ int __init psci_init_0_2(void)
>>          return -EOPNOTSUPP;
>>      }
>>  
>> -    psci_cpu_on_nr = PSCI_0_2_FN_CPU_ON;
>> +    psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(CPU_ON);
>>  
>>      printk(XENLOG_INFO "Using PSCI-0.2 for SMP bringup\n");
>>  
>>
> 
> Regards,
> 
> 
-Brijesh