Re: [Xen-devel] [PATCH v4 7/7] xen/arm: disable CPUs with different dcache line sizes
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
> > Signed-off-by: Stefano Stabellini
> >
> > ---
> > 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
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: [Xen-devel] [PATCH v4 7/7] xen/arm: disable CPUs with different dcache line sizes
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
Signed-off-by: Stefano Stabellini
---
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
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
[Xen-devel] [PATCH v4 7/7] xen/arm: disable CPUs with different dcache line sizes
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
Signed-off-by: Stefano Stabellini
---
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
