Re: [Xen-devel] [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Suriyan Ramasami <suriyan.r@xxxxxxxxx>]

On Thu, Sep 11, 2014 at 11:49 AM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hello Suriyan,
>
> My email address is @linaro.org not @linaro.com. I nearly miss this patch
> because of this.
>
Sorry about that. A slip on my part. Apologies.

> Depending if Ian apply his patch (see the conversation on "Odroid-XU..."), I
> would update the title and the message.
>
>
Would the title change back to the previous XU patch - "Add Odroid-XU
board ..etc" with patch version 7 and modified message?

> On 11/09/14 10:25, Suriyan Ramasami wrote:
>>
>> diff --git a/xen/arch/arm/platforms/exynos5.c
>> b/xen/arch/arm/platforms/exynos5.c
>> index bc9ae15..334650c 100644
>> --- a/xen/arch/arm/platforms/exynos5.c
>> +++ b/xen/arch/arm/platforms/exynos5.c
>> @@ -28,6 +28,9 @@
>>   #include <asm/platform.h>
>>   #include <asm/io.h>
>>
>> +/* This corresponds to CONFIG_NR_CPUS in linux config */
>> +#define EXYNOS_CONFIG_NR_CPUS       0x08
>> +
>>   #define EXYNOS_ARM_CORE0_CONFIG     0x2000
>>   #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr))
>>   #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) + 0x4)
>> @@ -42,8 +45,6 @@ static int exynos5_init_time(void)
>>       u64 mct_base_addr;
>>       u64 size;
>>
>> -    BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE);
>> -
>>       node = dt_find_compatible_node(NULL, NULL,
>> "samsung,exynos4210-mct");
>>       if ( !node )
>>       {
>> @@ -61,7 +62,14 @@ static int exynos5_init_time(void)
>>       dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
>>               mct_base_addr, size);
>>
>> -    mct = ioremap_attr(mct_base_addr, PAGE_SIZE,
>> PAGE_HYPERVISOR_NOCACHE);
>> +    if ( size <= EXYNOS5_MCT_G_TCON )
>
>
> Honestly, I don't think this check is very useful. The device tree should
> always contains valid size.
>
No doubt. But, wouldn't one have a wrong value for EXYNOS5_MCT_G_TCON?
I thought we are trying to catch wrong values of the offsets that we
use, to poke at the registers.

>> +    {
>> +        dprintk(XENLOG_ERR, "EXYNOS5_MCT_G_TCON: %d is >= size\n",
>> +                EXYNOS5_MCT_G_TCON);
>> +        return -EINVAL;
>> +    }
>> +
>> +    mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE);
>>       if ( !mct )
>>       {
>>           dprintk(XENLOG_ERR, "Unable to map MCT\n");
>> @@ -91,14 +99,36 @@ static int exynos5250_specific_mapping(struct domain
>> *d)
>>       return 0;
>>   }
>>
>> -static int __init exynos5_smp_init(void)
>> +static int exynos_smp_init_getbaseandoffset(u64 *sysram_addr,
>> +                                            u64 *sysram_offset)
>>   {
>
>
> This function contains nearly twice the same code. Except the compatible
> string and the offset which differs, everything is the same.
>
> I would do smth like:
>
> node = dt_find_compatible_node(NULL, NULL, "samsung,secure-firmare");
> if ( !node )
> {
>    compatible = "samsung,exynos4210-sysram";
>    *sysram_offset = 0;
> }
> else
> {
>    compatible "samsung,exynos4210-sysram-ns";
>    *sysram_offset = 0x1c;
> }
>
> node = dt_find_compatible_node(NULL, NULL, compatible);
> if ( !node )
> ....
>
> [..]
>
I shall make that change.

>> +static int __init exynos5_smp_init(void)
>> +{
>> +    void __iomem *sysram;
>> +    u64 sysram_addr;
>> +    u64 sysram_offset;
>> +    int rc;
>> +
>> +    rc = exynos_smp_init_getbaseandoffset(&sysram_addr, &sysram_offset);
>> +    if ( rc )
>> +        return rc;
>>
>> -    sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE);
>> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n",
>> +            sysram_addr, sysram_offset);
>> +
>> +    if ( sysram_offset >= PAGE_SIZE )
>> +    {
>> +        dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n");
>> +        return -EINVAL;
>> +    }
>
>
> As the previous check for the MCT. I don't think it's useful. Also why don't
> do get the size from the device tree?
>
In this case as we are poking only offset 0 or 0x1c which is always
less than PAGE_SIZE, I didn't bother with the size. I could if you
insist.
I can get rid of this check as sysram_offset is always less that PAGE_SIZE.

>> +
>> +    sysram = ioremap_nocache(sysram_addr, PAGE_SIZE);
>>       if ( !sysram )
>>       {
>>           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
>> @@ -125,7 +176,7 @@ static int __init exynos5_smp_init(void)
>>
>>       printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
>>              __pa(init_secondary), init_secondary);
>> -    writel(__pa(init_secondary), sysram + 0x1c);
>> +    writel(__pa(init_secondary), sysram + sysram_offset);
>>
>>       iounmap(sysram);
>>
>> @@ -134,14 +185,14 @@ static int __init exynos5_smp_init(void)
>>
>>   static int exynos_cpu_power_state(void __iomem *power, int cpu)
>>   {
>> -    return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
>> -                       S5P_CORE_LOCAL_PWR_EN;
>> +    return __raw_readl(power + EXYNOS_ARM_CORE0_CONFIG +
>> +                       EXYNOS_ARM_CORE_STATUS(cpu)) &
>> S5P_CORE_LOCAL_PWR_EN;
>
>
> Linux has the EXYNOS_ARM_CORE0_CONFIG included in the macro
> EXYNOS_CORE_CONFIGURATION. I would do the same here.
>
OK, I will make that change.

> [..]
>
>> -    power = ioremap_nocache(power_base_addr +
>> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
>> +    if ( size <= EXYNOS_ARM_CORE0_CONFIG +
>> +                 EXYNOS_ARM_CORE_STATUS(EXYNOS_CONFIG_NR_CPUS) )
>> +    {
>> +        dprintk(XENLOG_ERR, "CORE registers fall outside of pmu
>> range.\n");
>> +        return -EINVAL;
>> +    }
>> +
>
>
> Same remark as before for the check.
>
My same response, in the sense that the size from DT will be correct,
but don't we have to make sure that the offsets that we calculate with
the #defines, fall within size?

> [..]
>
>> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
>> +    if ( size <= EXYNOS5_SWRESET )
>> +    {
>> +        dprintk(XENLOG_ERR, "EXYNOS5_SWRESET: %d is >= size\n",
>> +                EXYNOS5_SWRESET);
>> +        return;
>> +    }
>> +
>
>
> Same here too.
My same response as before, that the EXYNOS5_SWRESET offset might be
miscalculated or quoted for a newer platform which falls beyond size
offset.

Please let me know if these checks are needed or not. I believe they
are good to keep, but if you insist I could leave them out.

Also, the next version of this patch, do I still maintain the same
subject and comments, or should this come up as something else?

Thanks as always
- Suriyan

>
> Regards,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel