[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
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).
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.
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
|
