[PATCH v1 0/2] Clean and invalidate dcache for boot pagetables during setup

[PATCH v1 0/2] Clean and invalidate dcache for boot pagetables during setup

[Oleksandr Tyshchenko]

Hello, all.

This small patch series are needed to avoid possible hangs during bringing up non-boot CPU.
This solution was suggested by Julien Grall and Ian Campbell.   

Discussion in Xen project Mailing List:
http://lists.xen.org/archives/html/xen-devel/2014-02/msg01575.html

The first patch introduces a new macro clean_and_invalidate_xen_dcache().
Unlike existing macro clean_xen_dcache() this macro performs clean and invalidate dcache.
The second patch replaces clean_xen_dcache() macro by a clean_and_invalidate_xen_dcache()
for boot pagetables after cleaning them.

This patch series was tested on DRA7 platform manufactured by TI.

Oleksandr Tyshchenko (2):
  xen/arm: Introduce clean_and_invalidate_xen_dcache() macro
  xen/arm: Clean and invalidate dcache for boot pagetables

 xen/arch/arm/mm.c          |    6 +++---
 xen/include/asm-arm/page.h |   26 ++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 5 deletions(-)

-- 
1.7.9.5

[PATCH v1 1/2] xen/arm: Introduce clean_and_invalidate_xen_dcache() macro

[Oleksandr Tyshchenko]

This macro is very similar to clean_xen_dcache(), but it performs
clean and invalidate dcache.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
---
 xen/include/asm-arm/page.h |   26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index e00be9e..34effa1 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -226,7 +226,7 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn)
 /* Actual cacheline size on the boot CPU. */
 extern size_t cacheline_bytes;
 
-/* Function for flushing medium-sized areas.
+/* Functions for flushing medium-sized areas.
  * if 'range' is large enough we might want to use model-specific
  * full-cache flushes. */
 static inline void clean_xen_dcache_va_range(void *p, unsigned long size)
@@ -238,7 +238,17 @@ static inline void clean_xen_dcache_va_range(void *p, unsigned long size)
     dsb();           /* So we know the flushes happen before continuing */
 }
 
-/* Macro for flushing a single small item.  The predicate is always
+static inline void clean_and_invalidate_xen_dcache_va_range(void *p,
+		unsigned long size)
+{
+    void *end;
+    dsb();           /* So the CPU issues all writes to the range */
+    for ( end = p + size; p < end; p += cacheline_bytes )
+        asm volatile (__clean_and_invalidate_xen_dcache_one(0) : : "r" (p));
+    dsb();           /* So we know the flushes happen before continuing */
+}
+
+/* Macros for flushing a single small item.  The predicate is always
  * compile-time constant so this will compile down to 3 instructions in
  * the common case. */
 #define clean_xen_dcache(x) do {                                        \
@@ -253,6 +263,18 @@ static inline void clean_xen_dcache_va_range(void *p, unsigned long size)
             : : "r" (_p), "m" (*_p));                                   \
 } while (0)
 
+#define clean_and_invalidate_xen_dcache(x) do {                         \
+    typeof(x) *_p = &(x);                                               \
+    if ( sizeof(x) > MIN_CACHELINE_BYTES || sizeof(x) > alignof(x) )    \
+        clean_and_invalidate_xen_dcache_va_range(_p, sizeof(x));        \
+    else                                                                \
+        asm volatile (                                                  \
+            "dsb sy;"   /* Finish all earlier writes */                 \
+            __clean_and_invalidate_xen_dcache_one(0)                    \
+            "dsb sy;"   /* Finish flush before continuing */            \
+            : : "r" (_p), "m" (*_p));                                   \
+} while (0)
+
 /* Flush the dcache for an entire page. */
 void flush_page_to_ram(unsigned long mfn);
 
-- 
1.7.9.5

[PATCH v1 2/2] xen/arm: Clean and invalidate dcache for boot pagetables

[Oleksandr Tyshchenko]

We need to invalidate dcache too after zeroing boot pagetables
to avoid unpredictable behavior which may take place after
non-boot CPUs enable their caches.

So, replace clean_xen_dcache() macro by a clean_and_invalidate_xen_dcache()
for boot pagetables.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
---
 xen/arch/arm/mm.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 308a798..4422b67 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -484,13 +484,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
     /* Clear the copy of the boot pagetables. Each secondary CPU
      * rebuilds these itself (see head.S) */
     memset(boot_pgtable, 0x0, PAGE_SIZE);
-    clean_xen_dcache(boot_pgtable);
+    clean_and_invalidate_xen_dcache(boot_pgtable);
 #ifdef CONFIG_ARM_64
     memset(boot_first, 0x0, PAGE_SIZE);
-    clean_xen_dcache(boot_first);
+    clean_and_invalidate_xen_dcache(boot_first);
 #endif
     memset(boot_second, 0x0, PAGE_SIZE);
-    clean_xen_dcache(boot_second);
+    clean_and_invalidate_xen_dcache(boot_second);
 
     /* Break up the Xen mapping into 4k pages and protect them separately. */
     for ( i = 0; i < LPAE_ENTRIES; i++ )
-- 
1.7.9.5

Re: [PATCH v1 1/2] xen/arm: Introduce clean_and_invalidate_xen_dcache() macro

[Andrew Cooper]

On 06/03/14 13:50, Oleksandr Tyshchenko wrote:
> This macro is very similar to clean_xen_dcache(), but it performs
> clean and invalidate dcache.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
> ---
>  xen/include/asm-arm/page.h |   26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index e00be9e..34effa1 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -226,7 +226,7 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn)
>  /* Actual cacheline size on the boot CPU. */
>  extern size_t cacheline_bytes;
>  
> -/* Function for flushing medium-sized areas.
> +/* Functions for flushing medium-sized areas.
>   * if 'range' is large enough we might want to use model-specific
>   * full-cache flushes. */
>  static inline void clean_xen_dcache_va_range(void *p, unsigned long size)
> @@ -238,7 +238,17 @@ static inline void clean_xen_dcache_va_range(void *p, unsigned long size)
>      dsb();           /* So we know the flushes happen before continuing */
>  }
>  
> -/* Macro for flushing a single small item.  The predicate is always
> +static inline void clean_and_invalidate_xen_dcache_va_range(void *p,

>From a style point of view, this would  be better if void *p was on the
next line and indented once

> +		unsigned long size)
> +{
> +    void *end;

Newline here, as per CODING_STYLE in the Xen root.

~Andrew

> +    dsb();           /* So the CPU issues all writes to the range */
> +    for ( end = p + size; p < end; p += cacheline_bytes )
> +        asm volatile (__clean_and_invalidate_xen_dcache_one(0) : : "r" (p));
> +    dsb();           /* So we know the flushes happen before continuing */
> +}
> +
> +/* Macros for flushing a single small item.  The predicate is always
>   * compile-time constant so this will compile down to 3 instructions in
>   * the common case. */
>  #define clean_xen_dcache(x) do {                                        \
> @@ -253,6 +263,18 @@ static inline void clean_xen_dcache_va_range(void *p, unsigned long size)
>              : : "r" (_p), "m" (*_p));                                   \
>  } while (0)
>  
> +#define clean_and_invalidate_xen_dcache(x) do {                         \
> +    typeof(x) *_p = &(x);                                               \
> +    if ( sizeof(x) > MIN_CACHELINE_BYTES || sizeof(x) > alignof(x) )    \
> +        clean_and_invalidate_xen_dcache_va_range(_p, sizeof(x));        \
> +    else                                                                \
> +        asm volatile (                                                  \
> +            "dsb sy;"   /* Finish all earlier writes */                 \
> +            __clean_and_invalidate_xen_dcache_one(0)                    \
> +            "dsb sy;"   /* Finish flush before continuing */            \
> +            : : "r" (_p), "m" (*_p));                                   \
> +} while (0)
> +
>  /* Flush the dcache for an entire page. */
>  void flush_page_to_ram(unsigned long mfn);
>  

Re: [PATCH v1 1/2] xen/arm: Introduce clean_and_invalidate_xen_dcache() macro

[Ian Campbell]

On Thu, 2014-03-06 at 15:50 +0200, Oleksandr Tyshchenko wrote:
> This macro is very similar to clean_xen_dcache(), but it performs
> clean and invalidate dcache.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Please don't add signed-off-by from other people unless they have
explicitly given you one. Adding an S-o-b conveys a specific meaning,
which is to certify the change under the DCO[0] which at least I haven't
done.

If someone has contributed either ideas or code then the appropriate way
to credit them is Suggested-by or similar or a mention in the prose part
("Based on a suggestion by..."). Of course it would also be appropriate
to include a CC to that person and they may then reply with an (Acked|
Reviewed|Signed-off)-by if they think that is appropriate. If someone
contributed code then you could also ask if they are happy/want you to
include their S-o-b.

Sorry to be picky but as I say there is a legal angle to the use of
S-o-b.

(I've not got time to properly review the actual change today, I'll do
that next week, but at first glance it looks good, thanks)

Thanks,
Ian.

[0] http://wiki.xen.org/wiki/Submitting_Xen_Patches#Signing_off_a_patch

> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
> ---
>  xen/include/asm-arm/page.h |   26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index e00be9e..34effa1 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -226,7 +226,7 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn)
>  /* Actual cacheline size on the boot CPU. */
>  extern size_t cacheline_bytes;
>  
> -/* Function for flushing medium-sized areas.
> +/* Functions for flushing medium-sized areas.
>   * if 'range' is large enough we might want to use model-specific
>   * full-cache flushes. */
>  static inline void clean_xen_dcache_va_range(void *p, unsigned long size)
> @@ -238,7 +238,17 @@ static inline void clean_xen_dcache_va_range(void *p, unsigned long size)
>      dsb();           /* So we know the flushes happen before continuing */
>  }
>  
> -/* Macro for flushing a single small item.  The predicate is always
> +static inline void clean_and_invalidate_xen_dcache_va_range(void *p,
> +		unsigned long size)
> +{
> +    void *end;
> +    dsb();           /* So the CPU issues all writes to the range */
> +    for ( end = p + size; p < end; p += cacheline_bytes )
> +        asm volatile (__clean_and_invalidate_xen_dcache_one(0) : : "r" (p));
> +    dsb();           /* So we know the flushes happen before continuing */
> +}
> +
> +/* Macros for flushing a single small item.  The predicate is always
>   * compile-time constant so this will compile down to 3 instructions in
>   * the common case. */
>  #define clean_xen_dcache(x) do {                                        \
> @@ -253,6 +263,18 @@ static inline void clean_xen_dcache_va_range(void *p, unsigned long size)
>              : : "r" (_p), "m" (*_p));                                   \
>  } while (0)
>  
> +#define clean_and_invalidate_xen_dcache(x) do {                         \
> +    typeof(x) *_p = &(x);                                               \
> +    if ( sizeof(x) > MIN_CACHELINE_BYTES || sizeof(x) > alignof(x) )    \
> +        clean_and_invalidate_xen_dcache_va_range(_p, sizeof(x));        \
> +    else                                                                \
> +        asm volatile (                                                  \
> +            "dsb sy;"   /* Finish all earlier writes */                 \
> +            __clean_and_invalidate_xen_dcache_one(0)                    \
> +            "dsb sy;"   /* Finish flush before continuing */            \
> +            : : "r" (_p), "m" (*_p));                                   \
> +} while (0)
> +
>  /* Flush the dcache for an entire page. */
>  void flush_page_to_ram(unsigned long mfn);
>  

Re: [PATCH v1 0/2] Clean and invalidate dcache for boot pagetables during setup

[Julien Grall]

On 06/03/14 21:50, Oleksandr Tyshchenko wrote:
> Hello, all.

Hello Oleksandr,

> This small patch series are needed to avoid possible hangs during bringing up non-boot CPU.
> This solution was suggested by Julien Grall and Ian Campbell.
>
> Discussion in Xen project Mailing List:
> http://lists.xen.org/archives/html/xen-devel/2014-02/msg01575.html
>
> The first patch introduces a new macro clean_and_invalidate_xen_dcache().
> Unlike existing macro clean_xen_dcache() this macro performs clean and invalidate dcache.
> The second patch replaces clean_xen_dcache() macro by a clean_and_invalidate_xen_dcache()
> for boot pagetables after cleaning them.

Thanks you for the patch series. While you introduce 
clean_and_invalidate_xen_dcache, can you modify flush_to_page_ram to 
call this function?

Regards,

-- 
Julien Grall

Re: [PATCH v1 2/2] xen/arm: Clean and invalidate dcache for boot pagetables

[Julien Grall]

Hello Oleksandr,

On 06/03/14 21:50, Oleksandr Tyshchenko wrote:
> We need to invalidate dcache too after zeroing boot pagetables
> to avoid unpredictable behavior which may take place after
> non-boot CPUs enable their caches.
>
> So, replace clean_xen_dcache() macro by a clean_and_invalidate_xen_dcache()
> for boot pagetables.

This patch, and the previous one, might be a good candidate to backport 
for Xen 4.4.1.

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>

With the comment about "signed-off-by" made by Ian:

Reviewed-by: Julien Grall <julien.grall@linaro.org>

> ---
>   xen/arch/arm/mm.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 308a798..4422b67 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -484,13 +484,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>       /* Clear the copy of the boot pagetables. Each secondary CPU
>        * rebuilds these itself (see head.S) */
>       memset(boot_pgtable, 0x0, PAGE_SIZE);
> -    clean_xen_dcache(boot_pgtable);
> +    clean_and_invalidate_xen_dcache(boot_pgtable);
>   #ifdef CONFIG_ARM_64
>       memset(boot_first, 0x0, PAGE_SIZE);
> -    clean_xen_dcache(boot_first);
> +    clean_and_invalidate_xen_dcache(boot_first);
>   #endif
>       memset(boot_second, 0x0, PAGE_SIZE);
> -    clean_xen_dcache(boot_second);
> +    clean_and_invalidate_xen_dcache(boot_second);
>
>       /* Break up the Xen mapping into 4k pages and protect them separately. */
>       for ( i = 0; i < LPAE_ENTRIES; i++ )
>

-- 
Julien Grall

Re: [PATCH v1 1/2] xen/arm: Introduce clean_and_invalidate_xen_dcache() macro

[Oleksandr Tyshchenko]

>>
>> -/* Macro for flushing a single small item.  The predicate is always
>> +static inline void clean_and_invalidate_xen_dcache_va_range(void *p,
>
> From a style point of view, this would  be better if void *p was on the
> next line and indented once
>
>> +             unsigned long size)
>> +{
>> +    void *end;
>
> Newline here, as per CODING_STYLE in the Xen root.
>
> ~Andrew

OK, I will correct.
Thank you.

-- 

Oleksandr Tyshchenko | Embedded Developer
GlobalLogic
www.globallogic.com

Re: [PATCH v1 1/2] xen/arm: Introduce clean_and_invalidate_xen_dcache() macro

[Oleksandr Tyshchenko]

> On Thu, 2014-03-06 at 15:50 +0200, Oleksandr Tyshchenko wrote:
>> This macro is very similar to clean_xen_dcache(), but it performs
>> clean and invalidate dcache.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> Please don't add signed-off-by from other people unless they have
> explicitly given you one. Adding an S-o-b conveys a specific meaning,
> which is to certify the change under the DCO[0] which at least I haven't
> done.
>
> If someone has contributed either ideas or code then the appropriate way
> to credit them is Suggested-by or similar or a mention in the prose part
> ("Based on a suggestion by..."). Of course it would also be appropriate
> to include a CC to that person and they may then reply with an (Acked|
> Reviewed|Signed-off)-by if they think that is appropriate. If someone
> contributed code then you could also ask if they are happy/want you to
> include their S-o-b.
>
> Sorry to be picky but as I say there is a legal angle to the use of
> S-o-b.
>
> (I've not got time to properly review the actual change today, I'll do
> that next week, but at first glance it looks good, thanks)
>
> Thanks,
> Ian.
>
> [0] http://wiki.xen.org/wiki/Submitting_Xen_Patches#Signing_off_a_patch

I will try to follow the rules.
Thank you.

-- 

Oleksandr Tyshchenko | Embedded Developer
GlobalLogic
www.globallogic.com

Re: [PATCH v1 0/2] Clean and invalidate dcache for boot pagetables during setup

[Oleksandr Tyshchenko]

>
> Hello Oleksandr,
Hello Julien.
>
>
>> This small patch series are needed to avoid possible hangs during bringing
>> up non-boot CPU.
>> This solution was suggested by Julien Grall and Ian Campbell.
>>
>> Discussion in Xen project Mailing List:
>> http://lists.xen.org/archives/html/xen-devel/2014-02/msg01575.html
>>
>> The first patch introduces a new macro clean_and_invalidate_xen_dcache().
>> Unlike existing macro clean_xen_dcache() this macro performs clean and
>> invalidate dcache.
>> The second patch replaces clean_xen_dcache() macro by a
>> clean_and_invalidate_xen_dcache()
>> for boot pagetables after cleaning them.
>
>
> Thanks you for the patch series. While you introduce
> clean_and_invalidate_xen_dcache, can you modify flush_to_page_ram to call
> this function?
Yes, I can. I will create a new patch series where I will take into
account all comments.
Thank you.
>
> Regards,
>
> --
> Julien Grall

-- 

Oleksandr Tyshchenko | Embedded Developer
GlobalLogic
www.globallogic.com