|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot
On Fri, Sep 12, 2014 at 12:34 PM, Suriyan Ramasami <suriyan.r@xxxxxxxxx> wrote:
> On Fri, Sep 12, 2014 at 11:58 AM, Julien Grall <julien.grall@xxxxxxxxxx>
> wrote:
>> Hi Suriyan,
>>
>> A couple of remarks about the Samsung secure firmware and your patch.
>>
>> When secure firmware is present, the CPU bring up is done by SMC (I don't
>> see this code in your patch).
>>
> Yes you are right. A call to call_firmware_op(cpu_boot, phys_cpu)
> translating to exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0) is missing
> right before the call to waking up the cpu.
> This was intentional on my part as the trustzone blob that comes with
> the odroid xu does not have code for that smc call and just returns
> -1.
>
> I am guessing as we want this as generic as posisble, I should add
> that part in.
>
>> Also, for now, the secure firmware MMIO range is mapped to DOM0. I'm not
>> sure we should do that because it contains way to idle CPU...
>> I think we should blacklist it for now. And see how DOM0 behave without.
>>
> I shall try this on the Odroid XU.
>
Hello Julien,
I blacklisted "samsung,secure-firmware" and I can't perceive any
change in DOM0's behavior in the Odroid XU. Do you recommend, I send
the next patch with this blacklisted?
- Suriyan
> Thanks!
> - Suriyan
>
>> Regards,
>>
>>
>> On 11/09/14 14:01, Suriyan Ramasami wrote:
>>>
>>> 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
>>
>>
>> --
>> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |