[PATCH v4 0/7] unsafe big.LITTLE support

[PATCH v4 0/7] unsafe big.LITTLE support

[Stefano Stabellini]

Hi all,

This series changes the initialization of two virtual registers to make
sure they match the value of the underlying physical cpu.

It also disables cpus different from the boot cpu, unless a newly
introduced command line option is specified. In that case, it explains
how to setup the system to avoid corruptions, which involves manually
specifying the cpu affinity of all domains, because the scheduler still
lacks big.LITTLE support.

In the uncommon case of a system where the cacheline sizes are different
across cores, it disables all cores that have a different dcache line size
from the boot cpu. In fact, it is not sufficient to use the dcache line
size of the current cpu, it would be necessary to use the minimum across
all dcache line sizes of all cores.  Given that it is actually uncommon
even in big.LITTLE systems, just disable cpus for now.

The first patch in the series is a fix for the way we read the dcache
line size.

Cheers,

Stefano


Julien Grall (1):
      xen/arm: Park CPUs with a MIDR different from the boot CPU.

Stefano Stabellini (6):
      xen/arm: Read the dcache line size from CTR register
      xen/arm: make processor a per cpu variable
      xen/arm: read ACTLR on the pcpu where the vcpu will run
      xen/arm: set VPIDR based on the MIDR value of the underlying pCPU
      xen/arm: update the docs about heterogeneous computing
      xen/arm: disable CPUs with different dcache line sizes

 docs/misc/arm/big.LITTLE.txt        | 46 +++++++++++++++++++++++++++++++++++++
 docs/misc/xen-command-line.markdown | 15 ++++++++++++
 xen/arch/arm/arm32/head.S           |  2 +-
 xen/arch/arm/arm64/head.S           |  2 +-
 xen/arch/arm/domain.c               | 15 ++++++------
 xen/arch/arm/processor.c            |  8 +++----
 xen/arch/arm/setup.c                | 17 ++------------
 xen/arch/arm/smpboot.c              | 39 +++++++++++++++++++++++++++++++
 xen/arch/arm/vcpreg.c               |  4 ++--
 xen/include/asm-arm/cpregs.h        |  2 ++
 xen/include/asm-arm/domain.h        |  3 ---
 xen/include/asm-arm/page.h          | 27 +++++++++++++++-------
 12 files changed, 138 insertions(+), 42 deletions(-)
 create mode 100644 docs/misc/arm/big.LITTLE.txt

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 1/7] xen/arm: Read the dcache line size from CTR register

[Stefano Stabellini]

See the corresponding Linux commit as reference:

  commit f91e2c3bd427239c198351f44814dd39db91afe0
  Author: Catalin Marinas <catalin.marinas@arm.com>
  Date:   Tue Dec 7 16:52:04 2010 +0100

      ARM: 6527/1: Use CTR instead of CCSIDR for the D-cache line size on ARMv7

      The current implementation of the dcache_line_size macro reads the L1
      cache size from the CCSIDR register. This, however, is not guaranteed to
      be the smallest cache line in the cache hierarchy. The patch changes to
      the macro to use the more architecturally correct CTR register.

      Reported-by: Kevin Sapp <ksapp@quicinc.com>
      Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
      Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Also rename cacheline_bytes to dcache_line_bytes to clarify that it is
the minimum D-Cache line size.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

---
Changes in v4:
- move patch to the beginning of the series
- rename cacheline_bytes to dcache_line_bytes
- improve commit message
---
 xen/arch/arm/arm32/head.S    |  2 +-
 xen/arch/arm/arm64/head.S    |  2 +-
 xen/arch/arm/setup.c         | 13 ++++++-------
 xen/include/asm-arm/cpregs.h |  2 ++
 xen/include/asm-arm/page.h   | 16 ++++++++--------
 5 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 43374e7..2b12908 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -504,7 +504,7 @@ ENTRY(relocate_xen)
         dsb        /* So the CPU issues all writes to the range */
 
         mov   r5, r4
-        ldr   r6, =cacheline_bytes /* r6 := step */
+        ldr   r6, =dcache_line_bytes /* r6 := step */
         ldr   r6, [r6]
         mov   r7, r3
 
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index fa0ef70..38899c7 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -631,7 +631,7 @@ ENTRY(relocate_xen)
         dsb   sy        /* So the CPU issues all writes to the range */
 
         mov   x9, x3
-        ldr   x10, =cacheline_bytes /* x10 := step */
+        ldr   x10, =dcache_line_bytes /* x10 := step */
         ldr   x10, [x10]
         mov   x11, x2
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 032a6a8..fced75a 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -680,19 +680,18 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
 }
 #endif
 
-size_t __read_mostly cacheline_bytes;
+size_t __read_mostly dcache_line_bytes;
 
 /* Very early check of the CPU cache properties */
 void __init setup_cache(void)
 {
-    uint32_t ccsid;
+    uint32_t ctr;
 
-    /* Read the cache size ID register for the level-0 data cache */
-    WRITE_SYSREG32(0, CSSELR_EL1);
-    ccsid = READ_SYSREG32(CCSIDR_EL1);
+    /* Read CTR */
+    ctr = READ_SYSREG32(CTR_EL0);
 
-    /* Low 3 bits are log2(cacheline size in words) - 2. */
-    cacheline_bytes = 1U << (4 + (ccsid & 0x7));
+    /* Bits 16-19 are the log2 number of words in the cacheline. */
+    dcache_line_bytes = (size_t) (4 << ((ctr >> 16) & 0xf));
 }
 
 /* C entry point for boot CPU */
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index 9e13848..8db65d5 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -106,6 +106,7 @@
 
 /* CP15 CR0: CPUID and Cache Type Registers */
 #define MIDR            p15,0,c0,c0,0   /* Main ID Register */
+#define CTR             p15,0,c0,c0,1   /* Cache Type Register */
 #define MPIDR           p15,0,c0,c0,5   /* Multiprocessor Affinity Register */
 #define ID_PFR0         p15,0,c0,c1,0   /* Processor Feature Register 0 */
 #define ID_PFR1         p15,0,c0,c1,1   /* Processor Feature Register 1 */
@@ -303,6 +304,7 @@
 #define CPACR_EL1               CPACR
 #define CPTR_EL2                HCPTR
 #define CSSELR_EL1              CSSELR
+#define CTR_EL0                 CTR
 #define DACR32_EL2              DACR
 #define ESR_EL1                 DFSR
 #define ESR_EL2                 HSR
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index d948250..ce18f0c 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -134,7 +134,7 @@
 /* Architectural minimum cacheline size is 4 32-bit words. */
 #define MIN_CACHELINE_BYTES 16
 /* Actual cacheline size on the boot CPU. */
-extern size_t cacheline_bytes;
+extern size_t dcache_line_bytes;
 
 #define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
 
@@ -145,7 +145,7 @@ extern size_t cacheline_bytes;
 static inline int invalidate_dcache_va_range(const void *p, unsigned long size)
 {
     const void *end = p + size;
-    size_t cacheline_mask = cacheline_bytes - 1;
+    size_t cacheline_mask = dcache_line_bytes - 1;
 
     dsb(sy);           /* So the CPU issues all writes to the range */
 
@@ -153,7 +153,7 @@ static inline int invalidate_dcache_va_range(const void *p, unsigned long size)
     {
         p = (void *)((uintptr_t)p & ~cacheline_mask);
         asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
-        p += cacheline_bytes;
+        p += dcache_line_bytes;
     }
     if ( (uintptr_t)end & cacheline_mask )
     {
@@ -161,7 +161,7 @@ static inline int invalidate_dcache_va_range(const void *p, unsigned long size)
         asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (end));
     }
 
-    for ( ; p < end; p += cacheline_bytes )
+    for ( ; p < end; p += dcache_line_bytes )
         asm volatile (__invalidate_dcache_one(0) : : "r" (p));
 
     dsb(sy);           /* So we know the flushes happen before continuing */
@@ -173,8 +173,8 @@ static inline int clean_dcache_va_range(const void *p, unsigned long size)
 {
     const void *end = p + size;
     dsb(sy);           /* So the CPU issues all writes to the range */
-    p = (void *)((uintptr_t)p & ~(cacheline_bytes - 1));
-    for ( ; p < end; p += cacheline_bytes )
+    p = (void *)((uintptr_t)p & ~(dcache_line_bytes - 1));
+    for ( ; p < end; p += dcache_line_bytes )
         asm volatile (__clean_dcache_one(0) : : "r" (p));
     dsb(sy);           /* So we know the flushes happen before continuing */
     /* ARM callers assume that dcache_* functions cannot fail. */
@@ -186,8 +186,8 @@ static inline int clean_and_invalidate_dcache_va_range
 {
     const void *end = p + size;
     dsb(sy);         /* So the CPU issues all writes to the range */
-    p = (void *)((uintptr_t)p & ~(cacheline_bytes - 1));
-    for ( ; p < end; p += cacheline_bytes )
+    p = (void *)((uintptr_t)p & ~(dcache_line_bytes - 1));
+    for ( ; p < end; p += dcache_line_bytes )
         asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
     dsb(sy);         /* So we know the flushes happen before continuing */
     /* ARM callers assume that dcache_* functions cannot fail. */
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 2/7] xen/arm: Park CPUs with a MIDR different from the boot CPU.

[Stefano Stabellini]

From: Julien Grall <julien.grall@arm.com>

From: Julien Grall <julien.grall@arm.com>

Xen does not properly support big.LITTLE platform. All vCPUs of a guest
will always have the MIDR of the boot CPU (see arch_domain_create).
At best the guest may see unreliable performance (vCPU switching between
big and LITTLE), at worst the guest will become unreliable or insecure.

This is becoming more apparent with branch predictor hardening in Linux
because they target a specific kind of CPUs and may not work on other
CPUs.

For the time being, park any CPUs with a MDIR different from the boot
CPU. This will be revisited in the future once Xen gains understanding
of big.LITTLE.

[1] https://lists.xenproject.org/archives/html/xen-devel/2016-12/msg00826.html

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Oleksandr Tyshchenkko <oleksandr_tyshchenko@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 docs/misc/xen-command-line.markdown | 10 ++++++++++
 xen/arch/arm/smpboot.c              | 26 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index f73990f..7b80119 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -985,6 +985,16 @@ supported only when compiled with XSM on x86.
 
 Control Xens use of the APEI Hardware Error Source Table, should one be found.
 
+### hmp-unsafe (arm)
+> `= <boolean>`
+
+> Default : `false`
+
+Say yes at your own risk if you want to enable heterogenous computing
+(such as big.LITTLE). This may result to an unstable and insecure
+platform. When the option is disabled (default), CPUs that are not
+identical to the boot CPU will be parked and not used by Xen.
+
 ### hpetbroadcast
 > `= <boolean>`
 
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 1255185..7ea4e41 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -27,6 +27,7 @@
 #include <xen/smp.h>
 #include <xen/softirq.h>
 #include <xen/timer.h>
+#include <xen/warning.h>
 #include <xen/irq.h>
 #include <xen/console.h>
 #include <asm/cpuerrata.h>
@@ -69,6 +70,13 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_mask);
 /* representing HT and core siblings of each logical CPU */
 DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
 
+/*
+ * By default non-boot CPUs not identical to the boot CPU will be
+ * parked.
+ */
+static bool __read_mostly opt_hmp_unsafe = false;
+boolean_param("hmp-unsafe", opt_hmp_unsafe);
+
 static void setup_cpu_sibling_map(int cpu)
 {
     if ( !zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, cpu)) ||
@@ -255,6 +263,9 @@ void __init smp_init_cpus(void)
     else
         acpi_smp_init_cpus();
 
+    if ( opt_hmp_unsafe )
+        warning_add("WARNING: HMP COMPUTING HAS BEEN ENABLED.\n"
+                    "It has implications on the security and stability of the system.\n");
 }
 
 int __init
@@ -292,6 +303,21 @@ void start_secondary(unsigned long boot_phys_offset,
 
     init_traps();
 
+    /*
+     * Currently Xen assumes the platform has only one kind of CPUs.
+     * This assumption does not hold on big.LITTLE platform and may
+     * result to instability and insecure platform. Better to park them
+     * for now.
+     */
+    if ( !opt_hmp_unsafe &&
+         current_cpu_data.midr.bits != boot_cpu_data.midr.bits )
+    {
+        printk(XENLOG_ERR "CPU%u MIDR (0x%x) does not match boot CPU MIDR (0x%x).\n",
+               smp_processor_id(), current_cpu_data.midr.bits,
+               boot_cpu_data.midr.bits);
+        stop_cpu();
+    }
+
     mmu_init_secondary_cpu();
 
     gic_init_secondary_cpu();
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 3/7] xen/arm: make processor a per cpu variable

[Stefano Stabellini]

There can be processors of different kinds on a single system. Make
processor a per_cpu variable pointing to the right processor type for
each core.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Julien Grall <julien.grall@arm.com>
---
Changes in v2:
- add patch
---
 xen/arch/arm/processor.c | 8 ++++----
 xen/arch/arm/smpboot.c   | 2 ++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/processor.c b/xen/arch/arm/processor.c
index 8c425ce..ce43850 100644
--- a/xen/arch/arm/processor.c
+++ b/xen/arch/arm/processor.c
@@ -18,7 +18,7 @@
  */
 #include <asm/procinfo.h>
 
-static const struct processor *processor = NULL;
+static DEFINE_PER_CPU(struct processor *, processor);
 
 void __init processor_setup(void)
 {
@@ -28,15 +28,15 @@ void __init processor_setup(void)
     if ( !procinfo )
         return;
 
-    processor = procinfo->processor;
+    this_cpu(processor) = procinfo->processor;
 }
 
 void processor_vcpu_initialise(struct vcpu *v)
 {
-    if ( !processor || !processor->vcpu_initialise )
+    if ( !this_cpu(processor) || !this_cpu(processor)->vcpu_initialise )
         return;
 
-    processor->vcpu_initialise(v);
+    this_cpu(processor)->vcpu_initialise(v);
 }
 
 /*
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 7ea4e41..122c0b5 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -32,6 +32,7 @@
 #include <xen/console.h>
 #include <asm/cpuerrata.h>
 #include <asm/gic.h>
+#include <asm/procinfo.h>
 #include <asm/psci.h>
 #include <asm/acpi.h>
 
@@ -300,6 +301,7 @@ void start_secondary(unsigned long boot_phys_offset,
     set_processor_id(cpuid);
 
     identify_cpu(&current_cpu_data);
+    processor_setup();
 
     init_traps();
 
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 4/7] xen/arm: read ACTLR on the pcpu where the vcpu will run

[Stefano Stabellini]

On big.LITTLE systems not all cores have the same ACTLR. Instead of
reading ACTLR and setting v->arch.actlr in vcpu_initialise, do it later
on the same pcpu where the vcpu will run.

This way, assuming that the vcpu has been created with the right pcpu
affinity, the guest will be able to read the right ACTLR value, matching
the one of the physical cpu.

Also move processor_vcpu_initialise(v) to continue_new_vcpu as it
can modify v->arch.actlr.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Julien Grall <julien.grall@arm.com>
---

Changes in v2:
- move processor_vcpu_initialise to continue_new_vcpu
- remove inaccurate sentence from commit message
---
 xen/arch/arm/domain.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index a74ff1c..5e76809 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -314,6 +314,9 @@ static void schedule_tail(struct vcpu *prev)
 
 static void continue_new_vcpu(struct vcpu *prev)
 {
+    current->arch.actlr = READ_SYSREG32(ACTLR_EL1);
+    processor_vcpu_initialise(current);
+
     schedule_tail(prev);
 
     if ( is_idle_vcpu(current) )
@@ -540,12 +543,8 @@ int vcpu_initialise(struct vcpu *v)
 
     v->arch.vmpidr = MPIDR_SMP | vcpuid_to_vaffinity(v->vcpu_id);
 
-    v->arch.actlr = READ_SYSREG32(ACTLR_EL1);
-
     v->arch.hcr_el2 = get_default_hcr_flags();
 
-    processor_vcpu_initialise(v);
-
     if ( (rc = vcpu_vgic_init(v)) != 0 )
         goto fail;
 
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 5/7] xen/arm: set VPIDR based on the MIDR value of the underlying pCPU

[Stefano Stabellini]

On big.LITTLE systems not all cores have the same MIDR. Instead of
storing only one VPIDR per domain, initialize it to the value of the
MIDR of the pCPU where the vCPU will run.

This way, assuming that the vCPU has been created with the right pCPU
affinity, the guest will be able to read the right VPIDR value, matching
the one of the physical cpu.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Julien Grall <julien.grall@arm.com>

---

Changes in v3:
- improve commit message
- do not store vpidr in struct vcpu

Changes in v2:
- remove warning message
- make vpidr per vcpu
---
 xen/arch/arm/domain.c        | 8 ++++----
 xen/arch/arm/vcpreg.c        | 4 ++--
 xen/include/asm-arm/domain.h | 3 ---
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 5e76809..545bbf6 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -172,6 +172,8 @@ static void ctxt_switch_from(struct vcpu *p)
 
 static void ctxt_switch_to(struct vcpu *n)
 {
+    uint32_t vpidr;
+
     /* When the idle VCPU is running, Xen will always stay in hypervisor
      * mode. Therefore we don't need to restore the context of an idle VCPU.
      */
@@ -180,7 +182,8 @@ static void ctxt_switch_to(struct vcpu *n)
 
     p2m_restore_state(n);
 
-    WRITE_SYSREG32(n->domain->arch.vpidr, VPIDR_EL2);
+    vpidr = READ_SYSREG32(MIDR_EL1);
+    WRITE_SYSREG32(vpidr, VPIDR_EL2);
     WRITE_SYSREG(n->arch.vmpidr, VMPIDR_EL2);
 
     /* VGIC */
@@ -595,9 +598,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( (d->shared_info = alloc_xenheap_pages(0, 0)) == NULL )
         goto fail;
 
-    /* Default the virtual ID to match the physical */
-    d->arch.vpidr = boot_cpu_data.midr.bits;
-
     clear_page(d->shared_info);
     share_xen_page_with_guest(
         virt_to_page(d->shared_info), d, XENSHARE_writable);
diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index e363183..b04d996 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -230,7 +230,6 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
 {
     const struct hsr_cp32 cp32 = hsr.cp32;
     int regidx = cp32.reg;
-    struct domain *d = current->domain;
 
     if ( !check_conditional_instr(regs, hsr) )
     {
@@ -295,7 +294,8 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
          *  - Variant and Revision bits match MDIR
          */
         val = (1 << 24) | (5 << 16);
-        val |= ((d->arch.vpidr >> 20) & 0xf) | (d->arch.vpidr & 0xf);
+        val |= ((current_cpu_data.midr.bits >> 20) & 0xf) |
+                (current_cpu_data.midr.bits & 0xf);
         set_user_reg(regs, regidx, val);
 
         break;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 4fe189b..0dd8c95 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -63,9 +63,6 @@ struct arch_domain
         RELMEM_done,
     } relmem;
 
-    /* Virtual CPUID */
-    uint32_t vpidr;
-
     struct {
         uint64_t offset;
     } phys_timer_base;
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 6/7] xen/arm: update the docs about heterogeneous computing

[Stefano Stabellini]

Introduce a new document about big.LITTLE and update the documentation
of hmp-unsafe.

Also update the warning messages to point users to the docs.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Julien Grall <julien.grall@arm.com>
CC: jbeulich@suse.com
CC: konrad.wilk@oracle.com
CC: tim@xen.org
CC: wei.liu2@citrix.com
CC: andrew.cooper3@citrix.com
CC: George.Dunlap@eu.citrix.com
CC: ian.jackson@eu.citrix.com

---

Changes in v3:
- split warning messages to be under 72 chars

Changes in v2:
- add a separate doc for big.LITTLE
- improve the warning message
---
 docs/misc/arm/big.LITTLE.txt        | 46 +++++++++++++++++++++++++++++++++++++
 docs/misc/xen-command-line.markdown |  7 +++++-
 xen/arch/arm/smpboot.c              | 11 +++++----
 3 files changed, 59 insertions(+), 5 deletions(-)
 create mode 100644 docs/misc/arm/big.LITTLE.txt

diff --git a/docs/misc/arm/big.LITTLE.txt b/docs/misc/arm/big.LITTLE.txt
new file mode 100644
index 0000000..b6ea1c9
--- /dev/null
+++ b/docs/misc/arm/big.LITTLE.txt
@@ -0,0 +1,46 @@
+big.LITTLE is a form of heterogeneous computing that comes with two
+types of general purpose cpu cores: big cores, more powerful and with a
+higher power consumption rate, and LITTLE cores, less powerful and
+cheaper to run. For example, Cortex A53 and Cortex A57 cpus. Typically,
+big cores are only recommended for burst activity, especially in
+battery powered environments. Please note that Xen doesn't not use any
+board specific power management techniques at the moment, it only uses
+WFI. It is recommended to check the vendor's big.LITTLE and power
+management documentation before using it in a Xen environment.
+
+
+big and LITTLE cores are fully compatible in terms of instruction sets,
+but can differ in many subtle ways. For example, their cacheline sizes
+might differ. For this reason, vcpu migration between big and LITTLE
+cores can lead to data corruptions.
+
+Today, the Xen scheduler does not have support for big.LITTLE,
+therefore, it might unknowingly move any vcpus between big and LITTLE
+cores, potentially leading to breakages. To avoid this kind of issues,
+at boot time Xen disables all cpus that differ from the boot cpu.
+
+
+Expert users can enable all big.LITTLE cores by passing hmp-unsafe=true
+to the Xen command line [1]. Given the lack of big.LITTLE support in the
+scheduler, it is only safe if the cpu affinity of all domains is
+manually specified, so that the scheduler is not allowed to switch a
+vcpu from big to LITTLE or vice versa.
+
+In the case of dom0, dom0_vcpus_pin needs to be added to the Xen command
+line options [1]. For DomUs, the `cpus' option should be added to all VM
+config files [2].
+
+For example, if the first 4 cpus are big and the last 4 are LITTLE, the
+following options run all domain vcpus on either big or LITTLE cores
+(not both):
+
+  cpus = "0-3"
+  cpus = "4-7"
+
+The following option runs one domain vcpu as big and one as LITTLE:
+
+  cpus = ["0-3", "4-7"]
+
+
+[1] docs/misc/xen-command-line.markdown
+[2] docs/man/xl.cfg.pod.5
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 7b80119..4391b75 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -992,7 +992,12 @@ Control Xens use of the APEI Hardware Error Source Table, should one be found.
 
 Say yes at your own risk if you want to enable heterogenous computing
 (such as big.LITTLE). This may result to an unstable and insecure
-platform. When the option is disabled (default), CPUs that are not
+platform, unless you manually specify the cpu affinity of all domains so
+that all vcpus are scheduled on the same class of pcpus (big or LITTLE
+but not both). vcpu migration between big cores and LITTLE cores is not
+supported. See docs/misc/arm/big.LITTLE.txt for more information.
+
+When the hmp-unsafe option is disabled (default), CPUs that are not
 identical to the boot CPU will be parked and not used by Xen.
 
 ### hpetbroadcast
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 122c0b5..04efb33 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -266,7 +266,8 @@ void __init smp_init_cpus(void)
 
     if ( opt_hmp_unsafe )
         warning_add("WARNING: HMP COMPUTING HAS BEEN ENABLED.\n"
-                    "It has implications on the security and stability of the system.\n");
+                    "It has implications on the security and stability of the system,\n"
+                    "unless the cpu affinity of all domains is specified.\n");
 }
 
 int __init
@@ -308,13 +309,15 @@ void start_secondary(unsigned long boot_phys_offset,
     /*
      * Currently Xen assumes the platform has only one kind of CPUs.
      * This assumption does not hold on big.LITTLE platform and may
-     * result to instability and insecure platform. Better to park them
-     * for now.
+     * result to instability and insecure platform (unless cpu affinity
+     * is manually specified for all domains). Better to park them for
+     * now.
      */
     if ( !opt_hmp_unsafe &&
          current_cpu_data.midr.bits != boot_cpu_data.midr.bits )
     {
-        printk(XENLOG_ERR "CPU%u MIDR (0x%x) does not match boot CPU MIDR (0x%x).\n",
+        printk(XENLOG_ERR "CPU%u MIDR (0x%x) does not match boot CPU MIDR (0x%x),\n"
+               "disable cpu (see big.LITTLE.txt under docs/).\n",
                smp_processor_id(), current_cpu_data.midr.bits,
                boot_cpu_data.midr.bits);
         stop_cpu();
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 7/7] xen/arm: disable CPUs with different dcache line sizes

[Stefano Stabellini]

Even different cpus in big.LITTLE systems are expected to have the same
dcache line size. Unless the minimum of all dcache line sizes is used
across all cpu cores, cache coherency protocols can go wrong. Instead,
for now, just disable any cpu with a different dcache line size.

This check is not covered by the hmp-unsafe option, because even with
the correct scheduling and vcpu pinning in place, the system breaks if
dcache line sizes differ across cores. We don't believe it is a problem
for most big.LITTLE systems.

This patch moves the implementation of setup_cache to a static inline,
still setting dcache_line_bytes at the beginning of start_xen as
before.

In start_secondary we check that the dcache level 1 line sizes match,
otherwise we disable the cpu.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

---
Changes in v4:
- improve commit message
- use %zu
---
 xen/arch/arm/setup.c       | 14 +-------------
 xen/arch/arm/smpboot.c     |  8 ++++++++
 xen/include/asm-arm/page.h | 11 +++++++++++
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index fced75a..6ccfdab 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -682,18 +682,6 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
 
 size_t __read_mostly dcache_line_bytes;
 
-/* Very early check of the CPU cache properties */
-void __init setup_cache(void)
-{
-    uint32_t ctr;
-
-    /* Read CTR */
-    ctr = READ_SYSREG32(CTR_EL0);
-
-    /* Bits 16-19 are the log2 number of words in the cacheline. */
-    dcache_line_bytes = (size_t) (4 << ((ctr >> 16) & 0xf));
-}
-
 /* C entry point for boot CPU */
 void __init start_xen(unsigned long boot_phys_offset,
                       unsigned long fdt_paddr,
@@ -707,7 +695,7 @@ void __init start_xen(unsigned long boot_phys_offset,
     struct domain *dom0;
     struct xen_arch_domainconfig config;
 
-    setup_cache();
+    dcache_line_bytes = read_dcache_line_size();
 
     percpu_init_areas();
     set_processor_id(0); /* needed early, for smp_processor_id() */
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 04efb33..d15230b 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -323,6 +323,14 @@ void start_secondary(unsigned long boot_phys_offset,
         stop_cpu();
     }
 
+    if ( dcache_line_bytes != read_dcache_line_size() )
+    {
+        printk(XENLOG_ERR "CPU%u dcache line size (%zu) does not match the boot CPU (%zu)\n",
+               smp_processor_id(), read_dcache_line_size(),
+               dcache_line_bytes);
+        stop_cpu();
+    }
+
     mmu_init_secondary_cpu();
 
     gic_init_secondary_cpu();
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index ce18f0c..e539f83 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -138,6 +138,17 @@ extern size_t dcache_line_bytes;
 
 #define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
 
+static inline size_t read_dcache_line_size(void)
+{
+    uint32_t ctr;
+
+    /* Read CTR */
+    ctr = READ_SYSREG32(CTR_EL0);
+
+    /* Bits 16-19 are the log2 number of words in the cacheline. */
+    return (size_t) (4 << ((ctr >> 16) & 0xf));
+}
+
 /* Functions for flushing medium-sized areas.
  * if 'range' is large enough we might want to use model-specific
  * full-cache flushes. */
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 1/7] xen/arm: Read the dcache line size from CTR register

[Julien Grall]

Hi Stefano,

On 02/03/18 19:06, Stefano Stabellini wrote:
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index d948250..ce18f0c 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -134,7 +134,7 @@
>   /* Architectural minimum cacheline size is 4 32-bit words. */
>   #define MIN_CACHELINE_BYTES 16
>   /* Actual cacheline size on the boot CPU. */

You probably want to update that comment either in this patch or #7.

With that:

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 7/7] xen/arm: disable CPUs with different dcache line sizes

[Julien Grall]

Hi Stefano,

Something is wrong with your threading again. Patch #2-7 have 
"In-Reply-To" matching patch #1 and not the cover letter.

On 02/03/18 19:06, Stefano Stabellini wrote:
> Even different cpus in big.LITTLE systems are expected to have the same
> dcache line size. Unless the minimum of all dcache line sizes is used
> across all cpu cores, cache coherency protocols can go wrong. Instead,
> for now, just disable any cpu with a different dcache line size.
> 
> This check is not covered by the hmp-unsafe option, because even with
> the correct scheduling and vcpu pinning in place, the system breaks if
> dcache line sizes differ across cores. We don't believe it is a problem
> for most big.LITTLE systems.
> 
> This patch moves the implementation of setup_cache to a static inline,
> still setting dcache_line_bytes at the beginning of start_xen as
> before.
> 
> In start_secondary we check that the dcache level 1 line sizes match,
> otherwise we disable the cpu.
> 
> Suggested-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> ---
> Changes in v4:
> - improve commit message
> - use %zu
> ---
>   xen/arch/arm/setup.c       | 14 +-------------
>   xen/arch/arm/smpboot.c     |  8 ++++++++
>   xen/include/asm-arm/page.h | 11 +++++++++++
>   3 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index fced75a..6ccfdab 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -682,18 +682,6 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>   
>   size_t __read_mostly dcache_line_bytes;
>   
> -/* Very early check of the CPU cache properties */
> -void __init setup_cache(void)
> -{
> -    uint32_t ctr;
> -
> -    /* Read CTR */
> -    ctr = READ_SYSREG32(CTR_EL0);
> -
> -    /* Bits 16-19 are the log2 number of words in the cacheline. */
> -    dcache_line_bytes = (size_t) (4 << ((ctr >> 16) & 0xf));
> -}
> -
>   /* C entry point for boot CPU */
>   void __init start_xen(unsigned long boot_phys_offset,
>                         unsigned long fdt_paddr,
> @@ -707,7 +695,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>       struct domain *dom0;
>       struct xen_arch_domainconfig config;
>   
> -    setup_cache();
> +    dcache_line_bytes = read_dcache_line_size();

It feels a bit odd to have one call dcache_line_bytes and the other call 
read_dcache_line_size. Would it be possible to uniformize the name?

With that:

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

>   
>       percpu_init_areas();
>       set_processor_id(0); /* needed early, for smp_processor_id() */
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 04efb33..d15230b 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -323,6 +323,14 @@ void start_secondary(unsigned long boot_phys_offset,
>           stop_cpu();
>       }
>   
> +    if ( dcache_line_bytes != read_dcache_line_size() )
> +    {
> +        printk(XENLOG_ERR "CPU%u dcache line size (%zu) does not match the boot CPU (%zu)\n",
> +               smp_processor_id(), read_dcache_line_size(),
> +               dcache_line_bytes);
> +        stop_cpu();
> +    }
> +
>       mmu_init_secondary_cpu();
>   
>       gic_init_secondary_cpu();
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index ce18f0c..e539f83 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -138,6 +138,17 @@ extern size_t dcache_line_bytes;
>   
>   #define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
>   
> +static inline size_t read_dcache_line_size(void)
> +{
> +    uint32_t ctr;
> +
> +    /* Read CTR */
> +    ctr = READ_SYSREG32(CTR_EL0);
> +
> +    /* Bits 16-19 are the log2 number of words in the cacheline. */
> +    return (size_t) (4 << ((ctr >> 16) & 0xf));
> +}
> +
>   /* Functions for flushing medium-sized areas.
>    * if 'range' is large enough we might want to use model-specific
>    * full-cache flushes. */
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 7/7] xen/arm: disable CPUs with different dcache line sizes

[Stefano Stabellini]

On Tue, 6 Mar 2018, Julien Grall wrote:
> Hi Stefano,
> 
> Something is wrong with your threading again. Patch #2-7 have "In-Reply-To"
> matching patch #1 and not the cover letter.

Thanks, I lost my git configuration.


> On 02/03/18 19:06, Stefano Stabellini wrote:
> > Even different cpus in big.LITTLE systems are expected to have the same
> > dcache line size. Unless the minimum of all dcache line sizes is used
> > across all cpu cores, cache coherency protocols can go wrong. Instead,
> > for now, just disable any cpu with a different dcache line size.
> > 
> > This check is not covered by the hmp-unsafe option, because even with
> > the correct scheduling and vcpu pinning in place, the system breaks if
> > dcache line sizes differ across cores. We don't believe it is a problem
> > for most big.LITTLE systems.
> > 
> > This patch moves the implementation of setup_cache to a static inline,
> > still setting dcache_line_bytes at the beginning of start_xen as
> > before.
> > 
> > In start_secondary we check that the dcache level 1 line sizes match,
> > otherwise we disable the cpu.
> > 
> > Suggested-by: Julien Grall <julien.grall@arm.com>
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > ---
> > Changes in v4:
> > - improve commit message
> > - use %zu
> > ---
> >   xen/arch/arm/setup.c       | 14 +-------------
> >   xen/arch/arm/smpboot.c     |  8 ++++++++
> >   xen/include/asm-arm/page.h | 11 +++++++++++
> >   3 files changed, 20 insertions(+), 13 deletions(-)
> > 
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index fced75a..6ccfdab 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -682,18 +682,6 @@ static void __init setup_mm(unsigned long dtb_paddr,
> > size_t dtb_size)
> >     size_t __read_mostly dcache_line_bytes;
> >   -/* Very early check of the CPU cache properties */
> > -void __init setup_cache(void)
> > -{
> > -    uint32_t ctr;
> > -
> > -    /* Read CTR */
> > -    ctr = READ_SYSREG32(CTR_EL0);
> > -
> > -    /* Bits 16-19 are the log2 number of words in the cacheline. */
> > -    dcache_line_bytes = (size_t) (4 << ((ctr >> 16) & 0xf));
> > -}
> > -
> >   /* C entry point for boot CPU */
> >   void __init start_xen(unsigned long boot_phys_offset,
> >                         unsigned long fdt_paddr,
> > @@ -707,7 +695,7 @@ void __init start_xen(unsigned long boot_phys_offset,
> >       struct domain *dom0;
> >       struct xen_arch_domainconfig config;
> >   -    setup_cache();
> > +    dcache_line_bytes = read_dcache_line_size();
> 
> It feels a bit odd to have one call dcache_line_bytes and the other call
> read_dcache_line_size. Would it be possible to uniformize the name?
> 
> With that:
> 
> Reviewed-by: Julien Grall <julien.grall@arm.com>

I fixed that and the other in-code comment, and committed the whole
series, thanks!


> >         percpu_init_areas();
> >       set_processor_id(0); /* needed early, for smp_processor_id() */
> > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > index 04efb33..d15230b 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -323,6 +323,14 @@ void start_secondary(unsigned long boot_phys_offset,
> >           stop_cpu();
> >       }
> >   +    if ( dcache_line_bytes != read_dcache_line_size() )
> > +    {
> > +        printk(XENLOG_ERR "CPU%u dcache line size (%zu) does not match the
> > boot CPU (%zu)\n",
> > +               smp_processor_id(), read_dcache_line_size(),
> > +               dcache_line_bytes);
> > +        stop_cpu();
> > +    }
> > +
> >       mmu_init_secondary_cpu();
> >         gic_init_secondary_cpu();
> > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > index ce18f0c..e539f83 100644
> > --- a/xen/include/asm-arm/page.h
> > +++ b/xen/include/asm-arm/page.h
> > @@ -138,6 +138,17 @@ extern size_t dcache_line_bytes;
> >     #define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
> >   +static inline size_t read_dcache_line_size(void)
> > +{
> > +    uint32_t ctr;
> > +
> > +    /* Read CTR */
> > +    ctr = READ_SYSREG32(CTR_EL0);
> > +
> > +    /* Bits 16-19 are the log2 number of words in the cacheline. */
> > +    return (size_t) (4 << ((ctr >> 16) & 0xf));
> > +}
> > +
> >   /* Functions for flushing medium-sized areas.
> >    * if 'range' is large enough we might want to use model-specific
> >    * full-cache flushes. */
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Peng Fan]

Hi Stefano,
On Fri, Mar 02, 2018 at 11:05:54AM -0800, Stefano Stabellini wrote:
>Hi all,
>
>This series changes the initialization of two virtual registers to make
>sure they match the value of the underlying physical cpu.
>
>It also disables cpus different from the boot cpu, unless a newly
>introduced command line option is specified. In that case, it explains
>how to setup the system to avoid corruptions, which involves manually
>specifying the cpu affinity of all domains, because the scheduler still
>lacks big.LITTLE support.
>
>In the uncommon case of a system where the cacheline sizes are different
>across cores, it disables all cores that have a different dcache line size
>from the boot cpu. In fact, it is not sufficient to use the dcache line
>size of the current cpu, it would be necessary to use the minimum across
>all dcache line sizes of all cores.  Given that it is actually uncommon
>even in big.LITTLE systems, just disable cpus for now.
>
>The first patch in the series is a fix for the way we read the dcache
>line size.

I am trying the patchset, but I meet issue that Guest Big/Little with
vcpu not working properly. As my current hardware has an issue
which has fix in Kernel, https://source.codeaurora.org/external/imx/linux-imx/commit/?h=imx_4.9.51_imx8_beta2&id=917cc3a8db2f3609ef8e2f59e7bcd31aa2cd4e59

I am not sure whether this issue cause DomU big/Little not work.
So wonder has this patchset been tested on Big/Little Hardware?

Thanks,
Peng.

>
>Cheers,
>
>Stefano
>
>
>Julien Grall (1):
>      xen/arm: Park CPUs with a MIDR different from the boot CPU.
>
>Stefano Stabellini (6):
>      xen/arm: Read the dcache line size from CTR register
>      xen/arm: make processor a per cpu variable
>      xen/arm: read ACTLR on the pcpu where the vcpu will run
>      xen/arm: set VPIDR based on the MIDR value of the underlying pCPU
>      xen/arm: update the docs about heterogeneous computing
>      xen/arm: disable CPUs with different dcache line sizes
>
> docs/misc/arm/big.LITTLE.txt        | 46 +++++++++++++++++++++++++++++++++++++
> docs/misc/xen-command-line.markdown | 15 ++++++++++++
> xen/arch/arm/arm32/head.S           |  2 +-
> xen/arch/arm/arm64/head.S           |  2 +-
> xen/arch/arm/domain.c               | 15 ++++++------
> xen/arch/arm/processor.c            |  8 +++----
> xen/arch/arm/setup.c                | 17 ++------------
> xen/arch/arm/smpboot.c              | 39 +++++++++++++++++++++++++++++++
> xen/arch/arm/vcpreg.c               |  4 ++--
> xen/include/asm-arm/cpregs.h        |  2 ++
> xen/include/asm-arm/domain.h        |  3 ---
> xen/include/asm-arm/page.h          | 27 +++++++++++++++-------
> 12 files changed, 138 insertions(+), 42 deletions(-)
> create mode 100644 docs/misc/arm/big.LITTLE.txt
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xenproject.org
>https://lists.xenproject.org/mailman/listinfo/xen-devel

-- 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Julien Grall]

Hello,

On 08/03/18 06:15, Peng Fan wrote:
> Hi Stefano,
> On Fri, Mar 02, 2018 at 11:05:54AM -0800, Stefano Stabellini wrote:
>> Hi all,
>>
>> This series changes the initialization of two virtual registers to make
>> sure they match the value of the underlying physical cpu.
>>
>> It also disables cpus different from the boot cpu, unless a newly
>> introduced command line option is specified. In that case, it explains
>> how to setup the system to avoid corruptions, which involves manually
>> specifying the cpu affinity of all domains, because the scheduler still
>> lacks big.LITTLE support.
>>
>> In the uncommon case of a system where the cacheline sizes are different
>> across cores, it disables all cores that have a different dcache line size
>>from the boot cpu. In fact, it is not sufficient to use the dcache line
>> size of the current cpu, it would be necessary to use the minimum across
>> all dcache line sizes of all cores.  Given that it is actually uncommon
>> even in big.LITTLE systems, just disable cpus for now.
>>
>> The first patch in the series is a fix for the way we read the dcache
>> line size.
> 
> I am trying the patchset, but I meet issue that Guest Big/Little with
> vcpu not working properly. As my current hardware has an issue
> which has fix in Kernel, https://source.codeaurora.org/external/imx/linux-imx/commit/?h=imx_4.9.51_imx8_beta2&id=917cc3a8db2f3609ef8e2f59e7bcd31aa2cd4e59

Can you describe what you mean by not working properly? Also what is 
your setup? Did you pin the different vCPUs as requested by the 
documentation.

> 
> I am not sure whether this issue cause DomU big/Little not work.

Well, I would recommend to speak with NXP whether this errata affects 
TLB flush for Hypervisor Page-Table or Stage-2 Page-Table.

> So wonder has this patchset been tested on Big/Little Hardware?

This series only adds facility to report the correct MIDR to the guest. 
If your platform requires more, then it would be necessary send a patch 
for Xen.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Peng Fan]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of
> Julien Grall
> Sent: 2018年3月8日 19:04
> To: Peng Fan <van.freenix@gmail.com>; Stefano Stabellini
> <sstabellini@kernel.org>
> Cc: xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH v4 0/7] unsafe big.LITTLE support
> 
> Hello,
> 
> On 08/03/18 06:15, Peng Fan wrote:
> > Hi Stefano,
> > On Fri, Mar 02, 2018 at 11:05:54AM -0800, Stefano Stabellini wrote:
> >> Hi all,
> >>
> >> This series changes the initialization of two virtual registers to
> >> make sure they match the value of the underlying physical cpu.
> >>
> >> It also disables cpus different from the boot cpu, unless a newly
> >> introduced command line option is specified. In that case, it
> >> explains how to setup the system to avoid corruptions, which involves
> >> manually specifying the cpu affinity of all domains, because the
> >> scheduler still lacks big.LITTLE support.
> >>
> >> In the uncommon case of a system where the cacheline sizes are
> >>different  across cores, it disables all cores that have a different
> >>dcache line size from the boot cpu. In fact, it is not sufficient to
> >>use the dcache line  size of the current cpu, it would be necessary to
> >>use the minimum across  all dcache line sizes of all cores.  Given
> >>that it is actually uncommon  even in big.LITTLE systems, just disable cpus
> for now.
> >>
> >> The first patch in the series is a fix for the way we read the dcache
> >> line size.
> >
> > I am trying the patchset, but I meet issue that Guest Big/Little with
> > vcpu not working properly. As my current hardware has an issue which
> > has fix in Kernel,
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsou
> >
> rce.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2F%3Fh%3Di
> mx_
> >
> 4.9.51_imx8_beta2%26id%3D917cc3a8db2f3609ef8e2f59e7bcd31aa2cd4e59&
> data
> >
> =02%7C01%7Cpeng.fan%40nxp.com%7Cc7f074c6708647441f2b08d584e45fec
> %7C686
> >
> ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636561038755176475&sdata
> =S%2BI
> > 7g1BwUDgAnXGP8%2FFc1bVZZTIimd3J7%2FkTIdeWL4o%3D&reserved=0
> 
> Can you describe what you mean by not working properly? Also what is your
> setup? Did you pin the different vCPUs as requested by the documentation.
> 

I may not describe clearly. It is domu with big/little guest could not bootup correctly.
For dom0, the args are 
dom0_mem=2048M dom0_max_vcpus=6 dom0_vcpus_pin=true hmp-unsafe=true

For domu
vcpus = 4

#vcpu pin
cpus = ['2-3', '2-3', '4-5', '4-5']

The hardware is cpu0-3 is A53, cpu4-5 is A72.

I do not met issue for dom0.

> >
> > I am not sure whether this issue cause DomU big/Little not work.
> 
> Well, I would recommend to speak with NXP whether this errata affects TLB
> flush for Hypervisor Page-Table or Stage-2 Page-Table.

I tried the following, but no help. Not sure my patch is correct. I think it
affects stage2 TLB.

--- a/xen/include/asm-arm/arm64/flushtlb.h
+++ b/xen/include/asm-arm/arm64/flushtlb.h
@@ -6,7 +6,7 @@ static inline void flush_tlb_local(void)
 {
     asm volatile(
         "dsb sy;"
-        "tlbi vmalls12e1;"
+        "tlbi alle1;"
         "dsb sy;"
         "isb;"
         : : : "memory");
@@ -17,7 +17,7 @@ static inline void flush_tlb(void)
 {
     asm volatile(
         "dsb sy;"
-        "tlbi vmalls12e1is;"
+        "tlbi alle1;"
         "dsb sy;"
         "isb;"
         : : : "memory");
@@ -39,7 +39,7 @@ static inline void flush_tlb_all(void)
 {
     asm volatile(
         "dsb sy;"
-        "tlbi alle1is;"
+        "tlbi alle1;"
         "dsb sy;"
         "isb;"
         : : : "memory");
--- a/xen/include/asm-arm/arm64/page.h
+++ b/xen/include/asm-arm/arm64/page.h
@@ -74,14 +74,16 @@ static inline void flush_xen_data_tlb_local(void)
 /* Flush TLB of local processor for address va. */
 static inline void  __flush_xen_data_tlb_one_local(vaddr_t va)
 {
-    asm volatile("tlbi vae2, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
+       flush_xen_data_tlb_local();
+    //asm volatile("tlbi vae2, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
 }

 /* Flush TLB of all processors in the inner-shareable domain for
  * address va. */
 static inline void __flush_xen_data_tlb_one(vaddr_t va)
 {
-    asm volatile("tlbi vae2is, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
+       flush_xen_data_tlb_local();
+    //asm volatile("tlbi vae2is, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
 }

> 
> > So wonder has this patchset been tested on Big/Little Hardware?
> 
> This series only adds facility to report the correct MIDR to the guest.
> If your platform requires more, then it would be necessary send a patch for Xen.

Do you have any suggestions? Besides MIDR/ACTLR/Cacheline, are there more needed?

Thanks,
Peng.

> 
> Cheers,
> 
> --
> Julien Grall
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.xe
> nproject.org%2Fmailman%2Flistinfo%2Fxen-devel&data=02%7C01%7Cpeng.fan
> %40nxp.com%7Cc7f074c6708647441f2b08d584e45fec%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C636561038755176475&sdata=hJr8K6RXCjkDT
> AMuM84nvzUn3qUtrHdo2e6qFn1%2Fdzg%3D&reserved=0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Julien Grall]

Hi,

On 08/03/18 12:23, Peng Fan wrote:
> 
> 
>> -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of
>> Julien Grall
>> Sent: 2018年3月8日 19:04
>> To: Peng Fan <van.freenix@gmail.com>; Stefano Stabellini
>> <sstabellini@kernel.org>
>> Cc: xen-devel@lists.xen.org
>> Subject: Re: [Xen-devel] [PATCH v4 0/7] unsafe big.LITTLE support
>>
>> Hello,
>>
>> On 08/03/18 06:15, Peng Fan wrote:
>>> Hi Stefano,
>>> On Fri, Mar 02, 2018 at 11:05:54AM -0800, Stefano Stabellini wrote:
>>>> Hi all,
>>>>
>>>> This series changes the initialization of two virtual registers to
>>>> make sure they match the value of the underlying physical cpu.
>>>>
>>>> It also disables cpus different from the boot cpu, unless a newly
>>>> introduced command line option is specified. In that case, it
>>>> explains how to setup the system to avoid corruptions, which involves
>>>> manually specifying the cpu affinity of all domains, because the
>>>> scheduler still lacks big.LITTLE support.
>>>>
>>>> In the uncommon case of a system where the cacheline sizes are
>>>> different  across cores, it disables all cores that have a different
>>>> dcache line size from the boot cpu. In fact, it is not sufficient to
>>>> use the dcache line  size of the current cpu, it would be necessary to
>>>> use the minimum across  all dcache line sizes of all cores.  Given
>>>> that it is actually uncommon  even in big.LITTLE systems, just disable cpus
>> for now.
>>>>
>>>> The first patch in the series is a fix for the way we read the dcache
>>>> line size.
>>>
>>> I am trying the patchset, but I meet issue that Guest Big/Little with
>>> vcpu not working properly. As my current hardware has an issue which
>>> has fix in Kernel,
>>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsou
>>>
>> rce.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2F%3Fh%3Di
>> mx_
>>>
>> 4.9.51_imx8_beta2%26id%3D917cc3a8db2f3609ef8e2f59e7bcd31aa2cd4e59&
>> data
>>>
>> =02%7C01%7Cpeng.fan%40nxp.com%7Cc7f074c6708647441f2b08d584e45fec
>> %7C686
>>>
>> ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636561038755176475&sdata
>> =S%2BI
>>> 7g1BwUDgAnXGP8%2FFc1bVZZTIimd3J7%2FkTIdeWL4o%3D&reserved=0
>>
>> Can you describe what you mean by not working properly? Also what is your
>> setup? Did you pin the different vCPUs as requested by the documentation.
>>
> 
> I may not describe clearly. It is domu with big/little guest could not bootup correctly.

What do you mean by "could not bootup correctly"? Can you please provide 
logs or a bit more feedback. Without them, it is nearly impossible to me 
to help to debugging the problem.

> For dom0, the args are
> dom0_mem=2048M dom0_max_vcpus=6 dom0_vcpus_pin=true hmp-unsafe=true
> 
> For domu
> vcpus = 4
> 
> #vcpu pin
> cpus = ['2-3', '2-3', '4-5', '4-5']
> 
> The hardware is cpu0-3 is A53, cpu4-5 is A72.

What does "xl vcpu-list" give you?

> 
> I do not met issue for dom0.
> 
>>>
>>> I am not sure whether this issue cause DomU big/Little not work.
>>
>> Well, I would recommend to speak with NXP whether this errata affects TLB
>> flush for Hypervisor Page-Table or Stage-2 Page-Table.
> 
> I tried the following, but no help. Not sure my patch is correct. I think it
> affects stage2 TLB.
> 
> --- a/xen/include/asm-arm/arm64/flushtlb.h
> +++ b/xen/include/asm-arm/arm64/flushtlb.h
> @@ -6,7 +6,7 @@ static inline void flush_tlb_local(void)
>   {
>       asm volatile(
>           "dsb sy;"
> -        "tlbi vmalls12e1;"
> +        "tlbi alle1;"
>           "dsb sy;"
>           "isb;"
>           : : : "memory");
> @@ -17,7 +17,7 @@ static inline void flush_tlb(void)
>   {
>       asm volatile(
>           "dsb sy;"
> -        "tlbi vmalls12e1is;"
> +        "tlbi alle1;"

I am not sure why you drop the innershareable here?

>           "dsb sy;"
>           "isb;"
>           : : : "memory");
> @@ -39,7 +39,7 @@ static inline void flush_tlb_all(void)
>   {
>       asm volatile(
>           "dsb sy;"
> -        "tlbi alle1is;"
> +        "tlbi alle1;"

Ditto.

>           "dsb sy;"
>           "isb;"
>           : : : "memory");
> --- a/xen/include/asm-arm/arm64/page.h
> +++ b/xen/include/asm-arm/arm64/page.h
> @@ -74,14 +74,16 @@ static inline void flush_xen_data_tlb_local(void)
>   /* Flush TLB of local processor for address va. */
>   static inline void  __flush_xen_data_tlb_one_local(vaddr_t va)
>   {
> -    asm volatile("tlbi vae2, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
> +       flush_xen_data_tlb_local();
> +    //asm volatile("tlbi vae2, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
>   }
> 
>   /* Flush TLB of all processors in the inner-shareable domain for
>    * address va. */
>   static inline void __flush_xen_data_tlb_one(vaddr_t va)
>   {
> -    asm volatile("tlbi vae2is, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
> +       flush_xen_data_tlb_local();

Why do you replace an innershareable call to a local call? Is it part of 
the errata?

> +    //asm volatile("tlbi vae2is, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
>   }
> 
>>
>>> So wonder has this patchset been tested on Big/Little Hardware?
>>
>> This series only adds facility to report the correct MIDR to the guest.
>> If your platform requires more, then it would be necessary send a patch for Xen.
> 
> Do you have any suggestions? Besides MIDR/ACTLR/Cacheline, are there more needed?

Having a bit more details from your side would be helpful. At the 
moment, I have no clue what's going on.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Peng Fan]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 2018年3月8日 20:30
> To: Peng Fan <peng.fan@nxp.com>; Peng Fan <van.freenix@gmail.com>;
> Stefano Stabellini <sstabellini@kernel.org>
> Cc: xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH v4 0/7] unsafe big.LITTLE support
> 
> Hi,
> 
> On 08/03/18 12:23, Peng Fan wrote:
> >
> >
> >> -----Original Message-----
> >> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On
> >> Behalf Of Julien Grall
> >> Sent: 2018年3月8日 19:04
> >> To: Peng Fan <van.freenix@gmail.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>
> >> Cc: xen-devel@lists.xen.org
> >> Subject: Re: [Xen-devel] [PATCH v4 0/7] unsafe big.LITTLE support
> >>
> >> Hello,
> >>
> >> On 08/03/18 06:15, Peng Fan wrote:
> >>> Hi Stefano,
> >>> On Fri, Mar 02, 2018 at 11:05:54AM -0800, Stefano Stabellini wrote:
> >>>> Hi all,
> >>>>
> >>>> This series changes the initialization of two virtual registers to
> >>>> make sure they match the value of the underlying physical cpu.
> >>>>
> >>>> It also disables cpus different from the boot cpu, unless a newly
> >>>> introduced command line option is specified. In that case, it
> >>>> explains how to setup the system to avoid corruptions, which
> >>>> involves manually specifying the cpu affinity of all domains,
> >>>> because the scheduler still lacks big.LITTLE support.
> >>>>
> >>>> In the uncommon case of a system where the cacheline sizes are
> >>>> different  across cores, it disables all cores that have a
> >>>> different dcache line size from the boot cpu. In fact, it is not
> >>>> sufficient to use the dcache line  size of the current cpu, it
> >>>> would be necessary to use the minimum across  all dcache line sizes
> >>>> of all cores.  Given that it is actually uncommon  even in
> >>>> big.LITTLE systems, just disable cpus
> >> for now.
> >>>>
> >>>> The first patch in the series is a fix for the way we read the
> >>>> dcache line size.
> >>>
> >>> I am trying the patchset, but I meet issue that Guest Big/Little
> >>> with vcpu not working properly. As my current hardware has an issue
> >>> which has fix in Kernel,
> >>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fs
> >>> ou
> >>>
> >>
> rce.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2F%3Fh%3Di
> >> mx_
> >>>
> >>
> 4.9.51_imx8_beta2%26id%3D917cc3a8db2f3609ef8e2f59e7bcd31aa2cd4e59&
> >> data
> >>>
> >>
> =02%7C01%7Cpeng.fan%40nxp.com%7Cc7f074c6708647441f2b08d584e45fec
> >> %7C686
> >>>
> >>
> ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636561038755176475&sdata
> >> =S%2BI
> >>> 7g1BwUDgAnXGP8%2FFc1bVZZTIimd3J7%2FkTIdeWL4o%3D&reserved=0
> >>
> >> Can you describe what you mean by not working properly? Also what is
> >> your setup? Did you pin the different vCPUs as requested by the
> documentation.
> >>
> >
> > I may not describe clearly. It is domu with big/little guest could not bootup
> correctly.
> 
> What do you mean by "could not bootup correctly"? Can you please provide logs
> or a bit more feedback. Without them, it is nearly impossible to me to help to
> debugging the problem.
> 
> > For dom0, the args are
> > dom0_mem=2048M dom0_max_vcpus=6 dom0_vcpus_pin=true
> hmp-unsafe=true
> >
> > For domu
> > vcpus = 4
> >
> > #vcpu pin
> > cpus = ['2-3', '2-3', '4-5', '4-5']
> >
> > The hardware is cpu0-3 is A53, cpu4-5 is A72.
> 
> What does "xl vcpu-list" give you?

# xl vcpu-list
Name                                ID  VCPU   CPU State   Time(s) Affinity (Hard / Soft)
Domain-0                             0     0    0   r--       7.4  0 / all
Domain-0                             0     1    1   -b-       9.4  1 / all
Domain-0                             0     2    2   -b-       2.2  2 / all
Domain-0                             0     3    3   -b-       2.6  3 / all
Domain-0                             0     4    4   -b-       2.1  4 / all
Domain-0                             0     5    5   -b-       3.3  5 / all
DomU                                 1     0    2   -b-       9.6  2-3 / all
DomU                                 1     1    3   r--       7.5  2-3 / all
DomU                                 1     2    5   -b-       5.5  4-5 / all
DomU                                 1     3    4   -b-       6.3  4-5 / all
> 
> >
> > I do not met issue for dom0.
> >
> >>>
> >>> I am not sure whether this issue cause DomU big/Little not work.
> >>
> >> Well, I would recommend to speak with NXP whether this errata affects
> >> TLB flush for Hypervisor Page-Table or Stage-2 Page-Table.
> >
> > I tried the following, but no help. Not sure my patch is correct. I
> > think it affects stage2 TLB.
> >
> > --- a/xen/include/asm-arm/arm64/flushtlb.h
> > +++ b/xen/include/asm-arm/arm64/flushtlb.h
> > @@ -6,7 +6,7 @@ static inline void flush_tlb_local(void)
> >   {
> >       asm volatile(
> >           "dsb sy;"
> > -        "tlbi vmalls12e1;"
> > +        "tlbi alle1;"
> >           "dsb sy;"
> >           "isb;"
> >           : : : "memory");
> > @@ -17,7 +17,7 @@ static inline void flush_tlb(void)
> >   {
> >       asm volatile(
> >           "dsb sy;"
> > -        "tlbi vmalls12e1is;"
> > +        "tlbi alle1;"
> 
> I am not sure why you drop the innershareable here?
Just want to invalid all the tlb, innershareable could be kept.
This is not a formal patch, just my trying to narrow the issue.
> 
> >           "dsb sy;"
> >           "isb;"
> >           : : : "memory");
> > @@ -39,7 +39,7 @@ static inline void flush_tlb_all(void)
> >   {
> >       asm volatile(
> >           "dsb sy;"
> > -        "tlbi alle1is;"
> > +        "tlbi alle1;"
> 
> Ditto.
> 
> >           "dsb sy;"
> >           "isb;"
> >           : : : "memory");
> > --- a/xen/include/asm-arm/arm64/page.h
> > +++ b/xen/include/asm-arm/arm64/page.h
> > @@ -74,14 +74,16 @@ static inline void flush_xen_data_tlb_local(void)
> >   /* Flush TLB of local processor for address va. */
> >   static inline void  __flush_xen_data_tlb_one_local(vaddr_t va)
> >   {
> > -    asm volatile("tlbi vae2, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
> > +       flush_xen_data_tlb_local();
> > +    //asm volatile("tlbi vae2, %0;" : : "r" (va>>PAGE_SHIFT) :
> > + "memory");
> >   }
> >
> >   /* Flush TLB of all processors in the inner-shareable domain for
> >    * address va. */
> >   static inline void __flush_xen_data_tlb_one(vaddr_t va)
> >   {
> > -    asm volatile("tlbi vae2is, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
> > +       flush_xen_data_tlb_local();
> 
> Why do you replace an innershareable call to a local call? Is it part of the errata?

No. Just my trying to narrow down.
> 
> > +    //asm volatile("tlbi vae2is, %0;" : : "r" (va>>PAGE_SHIFT) :
> > + "memory");
> >   }
> >
> >>
> >>> So wonder has this patchset been tested on Big/Little Hardware?
> >>
> >> This series only adds facility to report the correct MIDR to the guest.
> >> If your platform requires more, then it would be necessary send a patch for
> Xen.
> >
> > Do you have any suggestions? Besides MIDR/ACTLR/Cacheline, are there more
> needed?
> 
> Having a bit more details from your side would be helpful. At the moment, I have
> no clue what's going on.

As from the linux kernel commit:
    on i.MX8QM TO1.0, there is an issue: the bus width between A53-CCI-A72
    is limited to 36bits.TLB maintenance through DVM messages over AR channel,
    some bits will be forced(truncated) to zero as the followings:

    ASID[15:12] is forced to 0
    VA[48:45] is forced to 0
    VA[44:41] is forced to 0
    VA[39:36] is forced to 0

    This issue will result in the TLB aintenance across the clusters not working
    as expected due to some VA and ASID bits get truncated and forced to be zero.

    The SW workaround is: use the vmalle1is if VA larger than 36bits or
    ASID[15:12] is not zero, otherwise, we use original TLB maintenance path.

When doing tlb maintenance through DVM from A53 to A72, some bits are forced
to 0, this means TLB may not be really invalidated from A72 perspective.

Currently I am trying a domu with big/little capability, but not allowing big/little vcpu
migration.

I am not sure whether this hardware issue impacts DomU or not. Or it is software issue.
As you could see dom0 has 6 vcpus, I did a stress test and not found issue on dom0.

Thanks,
Peng
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Julien Grall]

Hi,

On 08/03/18 12:43, Peng Fan wrote:
>>>>> I am not sure whether this issue cause DomU big/Little not work.
>>>>
>>>> Well, I would recommend to speak with NXP whether this errata affects
>>>> TLB flush for Hypervisor Page-Table or Stage-2 Page-Table.
>>>
>>> I tried the following, but no help. Not sure my patch is correct. I
>>> think it affects stage2 TLB.
>>>
>>> --- a/xen/include/asm-arm/arm64/flushtlb.h
>>> +++ b/xen/include/asm-arm/arm64/flushtlb.h
>>> @@ -6,7 +6,7 @@ static inline void flush_tlb_local(void)
>>>    {
>>>        asm volatile(
>>>            "dsb sy;"
>>> -        "tlbi vmalls12e1;"
>>> +        "tlbi alle1;"
>>>            "dsb sy;"
>>>            "isb;"
>>>            : : : "memory");
>>> @@ -17,7 +17,7 @@ static inline void flush_tlb(void)
>>>    {
>>>        asm volatile(
>>>            "dsb sy;"
>>> -        "tlbi vmalls12e1is;"
>>> +        "tlbi alle1;"
>>
>> I am not sure why you drop the innershareable here?
> Just want to invalid all the tlb, innershareable could be kept.
> This is not a formal patch, just my trying to narrow the issue.

alle1 will only flush the TLBs of the local processor. The flush will 
not get propagated to the other CPUs of the system. So you definitely 
want this to be innershareable to avoid the other processors containing 
stale TLBs.

>>
>>>            "dsb sy;"
>>>            "isb;"
>>>            : : : "memory");
>>> @@ -39,7 +39,7 @@ static inline void flush_tlb_all(void)
>>>    {
>>>        asm volatile(
>>>            "dsb sy;"
>>> -        "tlbi alle1is;"
>>> +        "tlbi alle1;"
>>
>> Ditto.
>>
>>>            "dsb sy;"
>>>            "isb;"
>>>            : : : "memory");
>>> --- a/xen/include/asm-arm/arm64/page.h
>>> +++ b/xen/include/asm-arm/arm64/page.h
>>> @@ -74,14 +74,16 @@ static inline void flush_xen_data_tlb_local(void)
>>>    /* Flush TLB of local processor for address va. */
>>>    static inline void  __flush_xen_data_tlb_one_local(vaddr_t va)
>>>    {
>>> -    asm volatile("tlbi vae2, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
>>> +       flush_xen_data_tlb_local();
>>> +    //asm volatile("tlbi vae2, %0;" : : "r" (va>>PAGE_SHIFT) :
>>> + "memory");
>>>    }
>>>
>>>    /* Flush TLB of all processors in the inner-shareable domain for
>>>     * address va. */
>>>    static inline void __flush_xen_data_tlb_one(vaddr_t va)
>>>    {
>>> -    asm volatile("tlbi vae2is, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
>>> +       flush_xen_data_tlb_local();
>>
>> Why do you replace an innershareable call to a local call? Is it part of the errata?
> 
> No. Just my trying to narrow down.

Then you should keep the innershareable. See above.

>>
>>> +    //asm volatile("tlbi vae2is, %0;" : : "r" (va>>PAGE_SHIFT) :
>>> + "memory");
>>>    }
>>>
>>>>
>>>>> So wonder has this patchset been tested on Big/Little Hardware?
>>>>
>>>> This series only adds facility to report the correct MIDR to the guest.
>>>> If your platform requires more, then it would be necessary send a patch for
>> Xen.
>>>
>>> Do you have any suggestions? Besides MIDR/ACTLR/Cacheline, are there more
>> needed?
>>
>> Having a bit more details from your side would be helpful. At the moment, I have
>> no clue what's going on.
> 
> As from the linux kernel commit:
>      on i.MX8QM TO1.0, there is an issue: the bus width between A53-CCI-A72
>      is limited to 36bits.TLB maintenance through DVM messages over AR channel,
>      some bits will be forced(truncated) to zero as the followings:
> 
>      ASID[15:12] is forced to 0
>      VA[48:45] is forced to 0
>      VA[44:41] is forced to 0
>      VA[39:36] is forced to 0
> 
>      This issue will result in the TLB aintenance across the clusters not working
>      as expected due to some VA and ASID bits get truncated and forced to be zero.
> 
>      The SW workaround is: use the vmalle1is if VA larger than 36bits or
>      ASID[15:12] is not zero, otherwise, we use original TLB maintenance path.
> 
> When doing tlb maintenance through DVM from A53 to A72, some bits are forced
> to 0, this means TLB may not be really invalidated from A72 perspective.
> 
> Currently I am trying a domu with big/little capability, but not allowing big/little vcpu
> migration.
> 
> I am not sure whether this hardware issue impacts DomU or not. Or it is software issue.
> As you could see dom0 has 6 vcpus, I did a stress test and not found issue on dom0.

There are a major difference between Dom0 and DomU in your setup. Dom0 
vCPUs are pinned to a specific pCPU, so they can't move around. For 
DomU, each vCPU are pinned to a set of pCPUs, so they can move around.

But, did you check the DomU has the workaround enabled? I am asking that 
because it looks like to me the way to detect the workaround is based on 
a device (scu) and not processor. So I am not convinced that DomU is 
actually using your workaround.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Peng Fan]

On Thu, Mar 08, 2018 at 03:13:50PM +0000, Julien Grall wrote:
>Hi,
>
>On 08/03/18 12:43, Peng Fan wrote:
>>>>>>I am not sure whether this issue cause DomU big/Little not work.
>>>>>
>>>>>Well, I would recommend to speak with NXP whether this errata affects
>>>>>TLB flush for Hypervisor Page-Table or Stage-2 Page-Table.
>>>>
>>>>I tried the following, but no help. Not sure my patch is correct. I
>>>>think it affects stage2 TLB.
>>>>
>>>>--- a/xen/include/asm-arm/arm64/flushtlb.h
>>>>+++ b/xen/include/asm-arm/arm64/flushtlb.h
>>>>@@ -6,7 +6,7 @@ static inline void flush_tlb_local(void)
>>>>   {
>>>>       asm volatile(
>>>>           "dsb sy;"
>>>>-        "tlbi vmalls12e1;"
>>>>+        "tlbi alle1;"
>>>>           "dsb sy;"
>>>>           "isb;"
>>>>           : : : "memory");
>>>>@@ -17,7 +17,7 @@ static inline void flush_tlb(void)
>>>>   {
>>>>       asm volatile(
>>>>           "dsb sy;"
>>>>-        "tlbi vmalls12e1is;"
>>>>+        "tlbi alle1;"
>>>
>>>I am not sure why you drop the innershareable here?
>>Just want to invalid all the tlb, innershareable could be kept.
>>This is not a formal patch, just my trying to narrow the issue.
>
>alle1 will only flush the TLBs of the local processor. The flush will
>not get propagated to the other CPUs of the system. So you definitely
>want this to be innershareable to avoid the other processors
>containing stale TLBs.
>
>>>
>>>>           "dsb sy;"
>>>>           "isb;"
>>>>           : : : "memory");
>>>>@@ -39,7 +39,7 @@ static inline void flush_tlb_all(void)
>>>>   {
>>>>       asm volatile(
>>>>           "dsb sy;"
>>>>-        "tlbi alle1is;"
>>>>+        "tlbi alle1;"
>>>
>>>Ditto.
>>>
>>>>           "dsb sy;"
>>>>           "isb;"
>>>>           : : : "memory");
>>>>--- a/xen/include/asm-arm/arm64/page.h
>>>>+++ b/xen/include/asm-arm/arm64/page.h
>>>>@@ -74,14 +74,16 @@ static inline void flush_xen_data_tlb_local(void)
>>>>   /* Flush TLB of local processor for address va. */
>>>>   static inline void  __flush_xen_data_tlb_one_local(vaddr_t va)
>>>>   {
>>>>-    asm volatile("tlbi vae2, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
>>>>+       flush_xen_data_tlb_local();
>>>>+    //asm volatile("tlbi vae2, %0;" : : "r" (va>>PAGE_SHIFT) :
>>>>+ "memory");
>>>>   }
>>>>
>>>>   /* Flush TLB of all processors in the inner-shareable domain for
>>>>    * address va. */
>>>>   static inline void __flush_xen_data_tlb_one(vaddr_t va)
>>>>   {
>>>>-    asm volatile("tlbi vae2is, %0;" : : "r" (va>>PAGE_SHIFT) : "memory");
>>>>+       flush_xen_data_tlb_local();
>>>
>>>Why do you replace an innershareable call to a local call? Is it part of the errata?
>>
>>No. Just my trying to narrow down.
>
>Then you should keep the innershareable. See above.
>
>>>
>>>>+    //asm volatile("tlbi vae2is, %0;" : : "r" (va>>PAGE_SHIFT) :
>>>>+ "memory");
>>>>   }
>>>>
>>>>>
>>>>>>So wonder has this patchset been tested on Big/Little Hardware?
>>>>>
>>>>>This series only adds facility to report the correct MIDR to the guest.
>>>>>If your platform requires more, then it would be necessary send a patch for
>>>Xen.
>>>>
>>>>Do you have any suggestions? Besides MIDR/ACTLR/Cacheline, are there more
>>>needed?
>>>
>>>Having a bit more details from your side would be helpful. At the moment, I have
>>>no clue what's going on.
>>
>>As from the linux kernel commit:
>>     on i.MX8QM TO1.0, there is an issue: the bus width between A53-CCI-A72
>>     is limited to 36bits.TLB maintenance through DVM messages over AR channel,
>>     some bits will be forced(truncated) to zero as the followings:
>>
>>     ASID[15:12] is forced to 0
>>     VA[48:45] is forced to 0
>>     VA[44:41] is forced to 0
>>     VA[39:36] is forced to 0
>>
>>     This issue will result in the TLB aintenance across the clusters not working
>>     as expected due to some VA and ASID bits get truncated and forced to be zero.
>>
>>     The SW workaround is: use the vmalle1is if VA larger than 36bits or
>>     ASID[15:12] is not zero, otherwise, we use original TLB maintenance path.
>>
>>When doing tlb maintenance through DVM from A53 to A72, some bits are forced
>>to 0, this means TLB may not be really invalidated from A72 perspective.
>>
>>Currently I am trying a domu with big/little capability, but not allowing big/little vcpu
>>migration.
>>
>>I am not sure whether this hardware issue impacts DomU or not. Or it is software issue.
>>As you could see dom0 has 6 vcpus, I did a stress test and not found issue on dom0.
>
>There are a major difference between Dom0 and DomU in your setup.
>Dom0 vCPUs are pinned to a specific pCPU, so they can't move around.
>For DomU, each vCPU are pinned to a set of pCPUs, so they can move
>around.
>
>But, did you check the DomU has the workaround enabled? I am asking
>that because it looks like to me the way to detect the workaround is
>based on a device (scu) and not processor. So I am not convinced that
>DomU is actually using your workaround.

Just checked this. Because xen toolstack create device tree
with compatible "compatible = "xen,xenvm-4.10", "xen,xenvm";",
but the linux code use "fsl,imx8qm" to detect soc, then call scu
to get revision of chip.

After add an entry in linux side "{ .compatible = "xen,xenvm", .data = &imx8qm_soc_data, },"
It seems works. Passed a map/unmap stress test which easily fail without
the tlb workaround.

Wonder is it ok to specific machine compatible in domu.cfg and let xen stack
use this machine compatible other than "xen,xenvm"? Is this acceptable by community?
Also in domu kernel booting, there is waring.
[    0.201323] Invalid sched_group_energy for CPU3
[    0.201341] Invalid sched_group_energy for Cluster3
[    0.201353] Invalid sched_group_energy for CPU2
[    0.201365] Invalid sched_group_energy for Cluster2
[    0.201376] Invalid sched_group_energy for CPU1
[    0.201387] Invalid sched_group_energy for Cluster1
[    0.201398] Invalid sched_group_energy for CPU0
[    0.201409] Invalid sched_group_energy for Cluster0

This is because no cpu0/1/2/3 is not under cluster node in dts. As
I am using big/little guest, I think need create two cluster nodes
,one for vcpu0-1, the other for vcpu2-3. But this also needs xen toolstack
change (:

Thanks,
Peng.

>
>Cheers,
>
>-- 
>Julien Grall

-- 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Julien Grall]

Hi Peng,

On 09/03/18 09:05, Peng Fan wrote:
> On Thu, Mar 08, 2018 at 03:13:50PM +0000, Julien Grall wrote:
>> On 08/03/18 12:43, Peng Fan wrote:
>> There are a major difference between Dom0 and DomU in your setup.
>> Dom0 vCPUs are pinned to a specific pCPU, so they can't move around.
>> For DomU, each vCPU are pinned to a set of pCPUs, so they can move
>> around.
>>
>> But, did you check the DomU has the workaround enabled? I am asking
>> that because it looks like to me the way to detect the workaround is
>> based on a device (scu) and not processor. So I am not convinced that
>> DomU is actually using your workaround.
> 
> Just checked this. Because xen toolstack create device tree
> with compatible "compatible = "xen,xenvm-4.10", "xen,xenvm";",
> but the linux code use "fsl,imx8qm" to detect soc, then call scu
> to get revision of chip.

But how does the guest call the scu?

> 
> After add an entry in linux side "{ .compatible = "xen,xenvm", .data = &imx8qm_soc_data, },"
> It seems works. Passed a map/unmap stress test which easily fail without
> the tlb workaround.
> 
> Wonder is it ok to specific machine compatible in domu.cfg and let xen stack
> use this machine compatible other than "xen,xenvm"? Is this acceptable by community?

A user should be able to boot a guest safely on any machine without 
having to hack the configuration file. He should also be able to boot a 
guest with both ACPI and DT as this is independent from the real 
machine. So for me the way to find the workaround at the moment is not 
acceptable for a Xen guest upstream.

What could be acceptable for the community is one of the 3 solutions below:
	1) Only use one of the 2 clusters
	2) Restrict both Xen and Guest to use only 36-bit VA.
	3) Trap all TLBs access from the guest and convert them to TLB 
alle1s/vmalls12e1is

I would be happy to consider any other.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Peng Fan]

Hi Julien,
On Fri, Mar 09, 2018 at 10:22:09AM +0000, Julien Grall wrote:
>Hi Peng,
>
>On 09/03/18 09:05, Peng Fan wrote:
>>On Thu, Mar 08, 2018 at 03:13:50PM +0000, Julien Grall wrote:
>>>On 08/03/18 12:43, Peng Fan wrote:
>>>There are a major difference between Dom0 and DomU in your setup.
>>>Dom0 vCPUs are pinned to a specific pCPU, so they can't move around.
>>>For DomU, each vCPU are pinned to a set of pCPUs, so they can move
>>>around.
>>>
>>>But, did you check the DomU has the workaround enabled? I am asking
>>>that because it looks like to me the way to detect the workaround is
>>>based on a device (scu) and not processor. So I am not convinced that
>>>DomU is actually using your workaround.
>>
>>Just checked this. Because xen toolstack create device tree
>>with compatible "compatible = "xen,xenvm-4.10", "xen,xenvm";",
>>but the linux code use "fsl,imx8qm" to detect soc, then call scu
>>to get revision of chip.
>
>But how does the guest call the scu?

We are doing GPU and display passthrough, also some other IPs passthrough.
we could not totally rely on Dom0 to configure the pinmux, gpio, clk,
relying on dom0 to do that would bring much hack code to our kernel, also
runtime clk set rate in domu could not be done.

So we expose an interface to domu to directly communicate with SCU(system
control unit). 

>
>>
>>After add an entry in linux side "{ .compatible = "xen,xenvm", .data = &imx8qm_soc_data, },"
>>It seems works. Passed a map/unmap stress test which easily fail without
>>the tlb workaround.
>>
>>Wonder is it ok to specific machine compatible in domu.cfg and let xen stack
>>use this machine compatible other than "xen,xenvm"? Is this acceptable by community?
>
>A user should be able to boot a guest safely on any machine without
>having to hack the configuration file. He should also be able to boot
>a guest with both ACPI and DT as this is independent from the real
>machine. So for me the way to find the workaround at the moment is
>not acceptable for a Xen guest upstream.

I have no idea about ACPI (:
we are mainly working on embedded case, and mostly we are partitioning
our IPs. So our kernel normally only work with the dedicated DTB.
I am not asking to replace "xen,xenvm", just would like to add a option
that if user specific a machine compatible in cfg or else, xen toolstack
could add that in the final device tree.

Anyway new silicon will have issue fixed.
>
>What could be acceptable for the community is one of the 3 solutions below:
>	1) Only use one of the 2 clusters
The easiest way:)
>	2) Restrict both Xen and Guest to use only 36-bit VA.
I have no idea, need to check
>	3) Trap all TLBs access from the guest and convert them to TLB
>alle1s/vmalls12e1is
This will introduce performance penalty. I would not do this.

Thanks,
Peng.
>
>I would be happy to consider any other.
>
>Cheers,
>
>-- 
>Julien Grall

-- 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Julien Grall]

Hi,

On 09/03/18 13:30, Peng Fan wrote:
> Hi Julien,
> On Fri, Mar 09, 2018 at 10:22:09AM +0000, Julien Grall wrote:
>> Hi Peng,
>>
>> On 09/03/18 09:05, Peng Fan wrote:
>>> On Thu, Mar 08, 2018 at 03:13:50PM +0000, Julien Grall wrote:
>>>> On 08/03/18 12:43, Peng Fan wrote:
>>>> There are a major difference between Dom0 and DomU in your setup.
>>>> Dom0 vCPUs are pinned to a specific pCPU, so they can't move around.
>>>> For DomU, each vCPU are pinned to a set of pCPUs, so they can move
>>>> around.
>>>>
>>>> But, did you check the DomU has the workaround enabled? I am asking
>>>> that because it looks like to me the way to detect the workaround is
>>>> based on a device (scu) and not processor. So I am not convinced that
>>>> DomU is actually using your workaround.
>>>
>>> Just checked this. Because xen toolstack create device tree
>>> with compatible "compatible = "xen,xenvm-4.10", "xen,xenvm";",
>>> but the linux code use "fsl,imx8qm" to detect soc, then call scu
>>> to get revision of chip.
>>
>> But how does the guest call the scu?
> 
> We are doing GPU and display passthrough, also some other IPs passthrough.
> we could not totally rely on Dom0 to configure the pinmux, gpio, clk,
> relying on dom0 to do that would bring much hack code to our kernel, also
> runtime clk set rate in domu could not be done.
> 
> So we expose an interface to domu to directly communicate with SCU(system
> control unit).

Do you always expect a domain to access the SCU? Even with no 
passthrough involved?

> 
>>
>>>
>>> After add an entry in linux side "{ .compatible = "xen,xenvm", .data = &imx8qm_soc_data, },"
>>> It seems works. Passed a map/unmap stress test which easily fail without
>>> the tlb workaround.
>>>
>>> Wonder is it ok to specific machine compatible in domu.cfg and let xen stack
>>> use this machine compatible other than "xen,xenvm"? Is this acceptable by community?
>>
>> A user should be able to boot a guest safely on any machine without
>> having to hack the configuration file. He should also be able to boot
>> a guest with both ACPI and DT as this is independent from the real
>> machine. So for me the way to find the workaround at the moment is
>> not acceptable for a Xen guest upstream.
> 
> I have no idea about ACPI (:
> we are mainly working on embedded case, and mostly we are partitioning
> our IPs. So our kernel normally only work with the dedicated DTB.
> I am not asking to replace "xen,xenvm", just would like to add a option
> that if user specific a machine compatible in cfg or else, xen toolstack
> could add that in the final device tree.

I know you were suggesting that and my point stands. Xen VM are not 
compatible with IMX8 platform.

And again, a user should not have to tweak his configuration file, have 
to passthrough some device to an untrusted guest in order to have a 
guest booting normally on your platform. That is breaking the whole 
purpose of virtualization.

Furthermore, the workaround is not in Linux upstream and I doubt this 
will be accepted as it is. So I am not convinced that we should modify 
Xen interface for that.

Anyway, given that your silicon is going to be respined, then you 
probably want to restrict to run on the same cluster.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Stefano Stabellini]

On Fri, 9 Mar 2018, Julien Grall wrote:
> Furthermore, the workaround is not in Linux upstream and I doubt this will be
> accepted as it is. So I am not convinced that we should modify Xen interface
> for that.
> 
> Anyway, given that your silicon is going to be respined, then you probably
> want to restrict to run on the same cluster.

Hi Pen,

I think that i.MX8 is a critical platform for the future of embedded
virtualization and I really want to support it in Xen out of the box.

However, I agree with Julien that if there will be a new version of the
silicon with this issue properly fixed in hardware, then it might not
make sense to add workarounds in Xen for this. Unless you think the
version of the hardware with the errata will be commercialized?

Do you plan to upstream your workaround in Linux? If not, then it might
be best for you to carry the workaround for Xen in your Xen tree, the
same way you'll do for Linux. For workarounds that affect/involve both
Linux and Xen, we tend to follow the same policy as the Linux kernel,
unless we have good reasons not to. On the other end, if you intend to
upstream the Linux workaround, then we can discuss what to do for Xen.


Also let me expand on one of Julien's suggestions that actually I think
is quite good. Assuming that we have the tlb maintenance workaround in
the hypervisor, it would be safe to start guests only in the big or only
in the little cluster, right? In other words, you could still use both
clusters but only starting guests in one or the other, not both. This is
a good compromise because it allows full usage of the hardware, a
relatively small patch in Xen, and no guest visible changes (such as
toolstack changes to modify the compatible line). Guest visible and user
visible changes are particularly troublesome to maintain in the long
term and this is reason why we are reticent in introducing them. The tlb
maintenance workaround in the hypervisor is something much easier to
manage and we could consider taking it in if hardware with the errata
will become available to customers.

Cheers,

Stefano

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Peng Fan]

On Fri, Mar 09, 2018 at 02:40:25PM +0000, Julien Grall wrote:
>Hi,
>
>On 09/03/18 13:30, Peng Fan wrote:
>>Hi Julien,
>>On Fri, Mar 09, 2018 at 10:22:09AM +0000, Julien Grall wrote:
>>>Hi Peng,
>>>
>>>On 09/03/18 09:05, Peng Fan wrote:
>>>>On Thu, Mar 08, 2018 at 03:13:50PM +0000, Julien Grall wrote:
>>>>>On 08/03/18 12:43, Peng Fan wrote:
>>>>>There are a major difference between Dom0 and DomU in your setup.
>>>>>Dom0 vCPUs are pinned to a specific pCPU, so they can't move around.
>>>>>For DomU, each vCPU are pinned to a set of pCPUs, so they can move
>>>>>around.
>>>>>
>>>>>But, did you check the DomU has the workaround enabled? I am asking
>>>>>that because it looks like to me the way to detect the workaround is
>>>>>based on a device (scu) and not processor. So I am not convinced that
>>>>>DomU is actually using your workaround.
>>>>
>>>>Just checked this. Because xen toolstack create device tree
>>>>with compatible "compatible = "xen,xenvm-4.10", "xen,xenvm";",
>>>>but the linux code use "fsl,imx8qm" to detect soc, then call scu
>>>>to get revision of chip.
>>>
>>>But how does the guest call the scu?
>>
>>We are doing GPU and display passthrough, also some other IPs passthrough.
>>we could not totally rely on Dom0 to configure the pinmux, gpio, clk,
>>relying on dom0 to do that would bring much hack code to our kernel, also
>>runtime clk set rate in domu could not be done.
>>
>>So we expose an interface to domu to directly communicate with SCU(system
>>control unit).
>
>Do you always expect a domain to access the SCU? Even with no
>passthrough involved?

only needed when a domain only needs to directly access hardware.

>
>>
>>>
>>>>
>>>>After add an entry in linux side "{ .compatible = "xen,xenvm", .data = &imx8qm_soc_data, },"
>>>>It seems works. Passed a map/unmap stress test which easily fail without
>>>>the tlb workaround.
>>>>
>>>>Wonder is it ok to specific machine compatible in domu.cfg and let xen stack
>>>>use this machine compatible other than "xen,xenvm"? Is this acceptable by community?
>>>
>>>A user should be able to boot a guest safely on any machine without
>>>having to hack the configuration file. He should also be able to boot
>>>a guest with both ACPI and DT as this is independent from the real
>>>machine. So for me the way to find the workaround at the moment is
>>>not acceptable for a Xen guest upstream.
>>
>>I have no idea about ACPI (:
>>we are mainly working on embedded case, and mostly we are partitioning
>>our IPs. So our kernel normally only work with the dedicated DTB.
>>I am not asking to replace "xen,xenvm", just would like to add a option
>>that if user specific a machine compatible in cfg or else, xen toolstack
>>could add that in the final device tree.
>
>I know you were suggesting that and my point stands. Xen VM are not
>compatible with IMX8 platform.
>
>And again, a user should not have to tweak his configuration file,
>have to passthrough some device to an untrusted guest in order to
>have a guest booting normally on your platform. That is breaking the
>whole purpose of virtualization.
>
>Furthermore, the workaround is not in Linux upstream and I doubt this
>will be accepted as it is. So I am not convinced that we should
>modify Xen interface for that.
>
>Anyway, given that your silicon is going to be respined, then you
>probably want to restrict to run on the same cluster.

Understand.

Thanks,
Peng.

>
>Cheers,
>
>-- 
>Julien Grall

-- 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Peng Fan]

Hi Stefano,
On Fri, Mar 09, 2018 at 05:09:20PM -0800, Stefano Stabellini wrote:
>On Fri, 9 Mar 2018, Julien Grall wrote:
>> Furthermore, the workaround is not in Linux upstream and I doubt this will be
>> accepted as it is. So I am not convinced that we should modify Xen interface
>> for that.
>> 
>> Anyway, given that your silicon is going to be respined, then you probably
>> want to restrict to run on the same cluster.
>
>Hi Pen,
>
>I think that i.MX8 is a critical platform for the future of embedded
>virtualization and I really want to support it in Xen out of the box.
>
>However, I agree with Julien that if there will be a new version of the
>silicon with this issue properly fixed in hardware, then it might not
>make sense to add workarounds in Xen for this. Unless you think the
>version of the hardware with the errata will be commercialized?

Understand. I just thought some kernel code use machine
compatible string to do some check for passthrough case.

Some early customers might use the 1.0 chip to do their development,
but I think all will switch to use new Silicon in the end.

>
>Do you plan to upstream your workaround in Linux? If not, then it might

No plan. This workaround might not be accepted by Linux community.

>be best for you to carry the workaround for Xen in your Xen tree, the
>same way you'll do for Linux. For workarounds that affect/involve both
>Linux and Xen, we tend to follow the same policy as the Linux kernel,
>unless we have good reasons not to. On the other end, if you intend to
>upstream the Linux workaround, then we can discuss what to do for Xen.
>
>
>Also let me expand on one of Julien's suggestions that actually I think
>is quite good. Assuming that we have the tlb maintenance workaround in
>the hypervisor, it would be safe to start guests only in the big or only
>in the little cluster, right? In other words, you could still use both

I am a bit lost here. Are you refering Julien's suggestion 3?
"3) Trap all TLBs access from the guest and convert them to TLB alle1s/vmalls12e1is"

Currently, only use one the of the 2 clusters, I do not meet issue.
No change to xen and domu not aware of linux workaround.

Do you mean it is not safe without tlb maintenance workaround on my current
hardware, even if restricting Guest OS only have one kind of cpu?

A naive question, what case would require tlb broadcast from A53 to A72 in XEN? page balloon?

Thanks
Peng.

>clusters but only starting guests in one or the other, not both. This is
>a good compromise because it allows full usage of the hardware, a
>relatively small patch in Xen, and no guest visible changes (such as
>toolstack changes to modify the compatible line). Guest visible and user
>visible changes are particularly troublesome to maintain in the long
>term and this is reason why we are reticent in introducing them. The tlb
>maintenance workaround in the hypervisor is something much easier to
>manage and we could consider taking it in if hardware with the errata
>will become available to customers.
>
>Cheers,
>
>Stefano

-- 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Julien Grall]

On 12/03/18 02:32, Peng Fan wrote:
> On Fri, Mar 09, 2018 at 02:40:25PM +0000, Julien Grall wrote:
>> Hi,
>>
>> On 09/03/18 13:30, Peng Fan wrote:
>>> Hi Julien,
>>> On Fri, Mar 09, 2018 at 10:22:09AM +0000, Julien Grall wrote:
>>>> Hi Peng,
>>>>
>>>> On 09/03/18 09:05, Peng Fan wrote:
>>>>> On Thu, Mar 08, 2018 at 03:13:50PM +0000, Julien Grall wrote:
>>>>>> On 08/03/18 12:43, Peng Fan wrote:
>>>>>> There are a major difference between Dom0 and DomU in your setup.
>>>>>> Dom0 vCPUs are pinned to a specific pCPU, so they can't move around.
>>>>>> For DomU, each vCPU are pinned to a set of pCPUs, so they can move
>>>>>> around.
>>>>>>
>>>>>> But, did you check the DomU has the workaround enabled? I am asking
>>>>>> that because it looks like to me the way to detect the workaround is
>>>>>> based on a device (scu) and not processor. So I am not convinced that
>>>>>> DomU is actually using your workaround.
>>>>>
>>>>> Just checked this. Because xen toolstack create device tree
>>>>> with compatible "compatible = "xen,xenvm-4.10", "xen,xenvm";",
>>>>> but the linux code use "fsl,imx8qm" to detect soc, then call scu
>>>>> to get revision of chip.
>>>>
>>>> But how does the guest call the scu?
>>>
>>> We are doing GPU and display passthrough, also some other IPs passthrough.
>>> we could not totally rely on Dom0 to configure the pinmux, gpio, clk,
>>> relying on dom0 to do that would bring much hack code to our kernel, also
>>> runtime clk set rate in domu could not be done.
>>>
>>> So we expose an interface to domu to directly communicate with SCU(system
>>> control unit).
>>
>> Do you always expect a domain to access the SCU? Even with no
>> passthrough involved?
> 
> only needed when a domain only needs to directly access hardware.

Then your suggested workaround can't work for guest with not device 
passthrough. I would recommend to find a workaround that works for every 
guests.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 0/7] unsafe big.LITTLE support

[Julien Grall]

Hi,

On 12/03/18 02:57, Peng Fan wrote:
> Hi Stefano,
> On Fri, Mar 09, 2018 at 05:09:20PM -0800, Stefano Stabellini wrote:
>> On Fri, 9 Mar 2018, Julien Grall wrote:
>>> Furthermore, the workaround is not in Linux upstream and I doubt this will be
>>> accepted as it is. So I am not convinced that we should modify Xen interface
>>> for that.
>>>
>>> Anyway, given that your silicon is going to be respined, then you probably
>>> want to restrict to run on the same cluster.
>>
>> Hi Pen,
>>
>> I think that i.MX8 is a critical platform for the future of embedded
>> virtualization and I really want to support it in Xen out of the box.
>>
>> However, I agree with Julien that if there will be a new version of the
>> silicon with this issue properly fixed in hardware, then it might not
>> make sense to add workarounds in Xen for this. Unless you think the
>> version of the hardware with the errata will be commercialized?
> 
> Understand. I just thought some kernel code use machine
> compatible string to do some check for passthrough case.

Usually you give access to a device and describe it in the device-tree. 
The kernel will then use the appropriate driver for that device.

You should never need to know what the machine is. Can you describe why 
this would be necessary in your use case?

> 
> Some early customers might use the 1.0 chip to do their development,
> but I think all will switch to use new Silicon in the end.
> 
>>
>> Do you plan to upstream your workaround in Linux? If not, then it might
> 
> No plan. This workaround might not be accepted by Linux community.

Based on what you said above, I would not even consider to upstream 
workaround for Xen.

> 
>> be best for you to carry the workaround for Xen in your Xen tree, the
>> same way you'll do for Linux. For workarounds that affect/involve both
>> Linux and Xen, we tend to follow the same policy as the Linux kernel,
>> unless we have good reasons not to. On the other end, if you intend to
>> upstream the Linux workaround, then we can discuss what to do for Xen.
>>
>>
>> Also let me expand on one of Julien's suggestions that actually I think
>> is quite good. Assuming that we have the tlb maintenance workaround in
>> the hypervisor, it would be safe to start guests only in the big or only
>> in the little cluster, right? In other words, you could still use both
> 
> I am a bit lost here. Are you refering Julien's suggestion 3?
> "3) Trap all TLBs access from the guest and convert them to TLB alle1s/vmalls12e1is"
> 
> Currently, only use one the of the 2 clusters, I do not meet issue.
> No change to xen and domu not aware of linux workaround.
> 
> Do you mean it is not safe without tlb maintenance workaround on my current
> hardware, even if restricting Guest OS only have one kind of cpu?

I think so. But this is only based on how I understood the description 
provided in the commit message you pointed early on.

I have no insight whether TLB flush in hypervisor are going to be 
affected by the workaround. I would recommend to speak with your 
hardware team for that.

> 
> A naive question, what case would require tlb broadcast from A53 to A72 in XEN? page balloon?
Xen is sharing page-table between all the pCPU. As the hypervisor will 
run on the both cluster, you would need to convert all innershareable
TLBs flush by VA to a full innershareable.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel