Re: [PATCH 1/2] xen/arm: Add support of PSCI v1.0 for the host

[Julien Grall <julien.grall@citrix.com>]

Hi Ian,

On 09/10/15 16:08, Ian Campbell wrote:
> On Thu, 2015-10-08 at 19:45 +0100, Julien Grall wrote:
>> From Xen point of view, PSCI v0.2 and PSCI v1.0 are very similar. All
>> the PSCI calls used within Xen (PSCI_VERSION, CPU_ON, SYSTEM_OFF and
>> SYSTEM_RESET) behaves exactly the same.
>>
>> While there is no compatible string to represent PSCI v1.0 in the DT,
>> it's possible to detect it using the function PSCI_VERSION.
>>
>> The compatible string is now used to detect if the platform may support
>> PSCI v0.2 or higher.
> 
> The actual implementation here looks for precisely 0.2 or 1.0, not >= 0.2
> as suggested by this statement.

The first implementation I did was based on the Linux one which is
working checking if the PSCI version if >= 0.2.

Although I changed my mind before sending the patch because I was worry
to see Xen breaking badly when booting on another version of PSCI.

> 
> The PSCI 1.0 spec says (section 5.3.1, intended use of PSCI_VERSION) that
> for any 1.y version must be compatible with 1.x when y>x (for those
> functions which existed in 1.x, y might have more).
> IOW an OS supporting 1.0 should work with any 1.x.

Right, I will update the check.

> (which begs the question why there is not a "arm,psci-1.x" compat string,
> Mark/Andre?)
> 
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>>
>> ---
>>
>> Cc: Andre Przywara <andre.przywara@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> ---
>>  xen/arch/arm/psci.c        |  9 +++++----
>>  xen/include/asm-arm/psci.h | 13 +++++++++++++
>>  2 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> index 172c6e7..53ee2e4 100644
>> --- a/xen/arch/arm/psci.c
>> +++ b/xen/arch/arm/psci.c
>> @@ -122,15 +122,16 @@ int __init psci_init_0_2(void)
>>  
>>      psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
>>  
>> -    if ( psci_ver != XEN_PSCI_V_0_2 )
>> +    if ( psci_ver != PSCI_VERSION(0, 2) && psci_ver != PSCI_VERSION(1, 0) )
> 
> Based on the above I think this should read:
> 
>     if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_MAJOR_VERSION(psci_ver) != 1 )
> 
>>      {
>> -        printk("Error: PSCI version %#x is not supported.\n", psci_ver);
>> -        return -EOPNOTSUPP;
>> +        printk("Error: Conflicting PSCI version detected (%#x)\n", psci_ver);
> 
> Conflicting with what?
> I think perhaps you meant "Unrecognised" or "Unsupported"?

It's just a mistake. When I first wrote the patch the check was:
PSCI_MAJOR_VERSION(psci_vers) == 0 && PSCI_MINOR_VERSION(psci_vers) < 2

Although, I was worry about allowing to many version of PSCI. I will use
your suggestion.

> Also please format the version like you did below with %u.%u.

I will do.

> 
>>      }
>>  
>>      psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(CPU_ON);
>>  
>> -    printk(XENLOG_INFO "Using PSCI-0.2 for SMP bringup\n");
>> +    printk(XENLOG_INFO "Using PSCI-%u.%u for SMP bringup\n",
>> +           PSCI_VERSION_MAJOR(psci_ver), PSCI_VERSION_MINOR(psci_ver));
>>  
>>      return 0;
>>  }
> 

Regards,

-- 
Julien Grall