[PATCH v2 0/2] xen: arm: introduce a function and flush dcache while preparing the device tree for Dom0

[PATCH v2 0/2] xen: arm: introduce a function and flush dcache while preparing the device tree for Dom0

[Oleksandr Dmytryshyn]

Currently we use OMAP5 ES2.0 Panda5 board to work with the hypervisor.

Without flushing dcache the hypervisor couldn't copy the device tree
correctly when booting the kernel dom0 Image (memory with device tree
is corrupted). As the result - when we try to load the kernel dom0
Image - dom0 hungs frequently. This issue is not reproduced with the
kernel dom0 zImage because the zImage decompressor code flushes all
dcache before starting the decompressed kernel Image. When the
hypervisor loads the kernel image or initrd, this memory region
isn't corrupted because the hypervisor code flushes the dcache.

Oleksandr Dmytryshyn (2):
  xen: arm: introduce raw_copy_to_guest_flush_dcache() function
  xen: arm: flush dcache while preparing the device tree for Dom0

 xen/arch/arm/domain_build.c        |  3 ++-
 xen/arch/arm/guestcopy.c           | 16 +++++++++++++++-
 xen/include/asm-arm/guest_access.h |  2 ++
 3 files changed, 19 insertions(+), 2 deletions(-)

-- 
1.8.2.rc2

[PATCH v2 1/2] xen: arm: introduce raw_copy_to_guest_flush_dcache() function

[Oleksandr Dmytryshyn]

This function flushes the dcache while copying the data.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 xen/arch/arm/guestcopy.c           | 16 +++++++++++++++-
 xen/include/asm-arm/guest_access.h |  2 ++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index d146cd6..4875b1d 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -5,7 +5,8 @@
 #include <asm/mm.h>
 #include <asm/guest_access.h>
 
-unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
+unsigned long raw_copy_to_guest_helper(void *to, const void *from, unsigned len,
+                                       unsigned flush_dcache)
 {
     /* XXX needs to handle faults */
     unsigned offset = (vaddr_t)to & ~PAGE_MASK;
@@ -24,6 +25,8 @@ unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
         p = map_domain_page(g>>PAGE_SHIFT);
         p += offset;
         memcpy(p, from, size);
+        if (flush_dcache)
+            flush_xen_dcache_va_range(p, size);
 
         unmap_domain_page(p - offset);
         len -= size;
@@ -35,6 +38,17 @@ unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
     return 0;
 }
 
+unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
+{
+    return raw_copy_to_guest_helper(to, from, len, 0);
+}
+
+unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
+                                             unsigned len)
+{
+    return raw_copy_to_guest_helper(to, from, len, 1);
+}
+
 unsigned long raw_clear_guest(void *to, unsigned len)
 {
     /* XXX needs to handle faults */
diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
index 8ff088f..5876988 100644
--- a/xen/include/asm-arm/guest_access.h
+++ b/xen/include/asm-arm/guest_access.h
@@ -11,6 +11,8 @@
     (likely(count < (~0UL/size)) && access_ok(addr,count*size))
 
 unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len);
+unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
+                                             unsigned len);
 unsigned long raw_copy_from_guest(void *to, const void *from, unsigned len);
 unsigned long raw_clear_guest(void *to, unsigned len);
 
-- 
1.8.2.rc2

Re: [PATCH v2 1/2] xen: arm: introduce raw_copy_to_guest_flush_dcache() function

[Julien Grall]

On 11/26/2013 10:54 AM, Oleksandr Dmytryshyn wrote:
> This function flushes the dcache while copying the data.
> 
> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> ---
>  xen/arch/arm/guestcopy.c           | 16 +++++++++++++++-
>  xen/include/asm-arm/guest_access.h |  2 ++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index d146cd6..4875b1d 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -5,7 +5,8 @@
>  #include <asm/mm.h>
>  #include <asm/guest_access.h>
>  
> -unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
> +unsigned long raw_copy_to_guest_helper(void *to, const void *from, unsigned len,
This function should be static.

Except that:

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

> +                                       unsigned flush_dcache)
>  {
>      /* XXX needs to handle faults */
>      unsigned offset = (vaddr_t)to & ~PAGE_MASK;
> @@ -24,6 +25,8 @@ unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
>          p = map_domain_page(g>>PAGE_SHIFT);
>          p += offset;
>          memcpy(p, from, size);
> +        if (flush_dcache)
> +            flush_xen_dcache_va_range(p, size);
>  
>          unmap_domain_page(p - offset);
>          len -= size;
> @@ -35,6 +38,17 @@ unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
>      return 0;
>  }
>  
> +unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
> +{
> +    return raw_copy_to_guest_helper(to, from, len, 0);
> +}
> +
> +unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
> +                                             unsigned len)
> +{
> +    return raw_copy_to_guest_helper(to, from, len, 1);
> +}
> +
>  unsigned long raw_clear_guest(void *to, unsigned len)
>  {
>      /* XXX needs to handle faults */
> diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
> index 8ff088f..5876988 100644
> --- a/xen/include/asm-arm/guest_access.h
> +++ b/xen/include/asm-arm/guest_access.h
> @@ -11,6 +11,8 @@
>      (likely(count < (~0UL/size)) && access_ok(addr,count*size))
>  
>  unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len);
> +unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
> +                                             unsigned len);
>  unsigned long raw_copy_from_guest(void *to, const void *from, unsigned len);
>  unsigned long raw_clear_guest(void *to, unsigned len);
>  
> 


-- 
Julien Grall

Re: [PATCH v2 1/2] xen: arm: introduce raw_copy_to_guest_flush_dcache() function

[Oleksandr Dmytryshyn]

Hi, Julien.

I'll fix this in the next patch-set.

Oleksandr Dmytryshyn | Product Engineering and Development
GlobalLogic
P x3657  M +38.067.382.2525
www.globallogic.com

http://www.globallogic.com/email_disclaimer.txt


On Wed, Nov 27, 2013 at 4:49 PM, Julien Grall <julien.grall@linaro.org> wrote:
> On 11/26/2013 10:54 AM, Oleksandr Dmytryshyn wrote:
>> This function flushes the dcache while copying the data.
>>
>> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
>> ---
>>  xen/arch/arm/guestcopy.c           | 16 +++++++++++++++-
>>  xen/include/asm-arm/guest_access.h |  2 ++
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
>> index d146cd6..4875b1d 100644
>> --- a/xen/arch/arm/guestcopy.c
>> +++ b/xen/arch/arm/guestcopy.c
>> @@ -5,7 +5,8 @@
>>  #include <asm/mm.h>
>>  #include <asm/guest_access.h>
>>
>> -unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
>> +unsigned long raw_copy_to_guest_helper(void *to, const void *from, unsigned len,
> This function should be static.
>
> Except that:
>
> Acked-by: Julien Grall<julien.grall@linaro.org>
>
>> +                                       unsigned flush_dcache)
>>  {
>>      /* XXX needs to handle faults */
>>      unsigned offset = (vaddr_t)to & ~PAGE_MASK;
>> @@ -24,6 +25,8 @@ unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
>>          p = map_domain_page(g>>PAGE_SHIFT);
>>          p += offset;
>>          memcpy(p, from, size);
>> +        if (flush_dcache)
>> +            flush_xen_dcache_va_range(p, size);
>>
>>          unmap_domain_page(p - offset);
>>          len -= size;
>> @@ -35,6 +38,17 @@ unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
>>      return 0;
>>  }
>>
>> +unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
>> +{
>> +    return raw_copy_to_guest_helper(to, from, len, 0);
>> +}
>> +
>> +unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>> +                                             unsigned len)
>> +{
>> +    return raw_copy_to_guest_helper(to, from, len, 1);
>> +}
>> +
>>  unsigned long raw_clear_guest(void *to, unsigned len)
>>  {
>>      /* XXX needs to handle faults */
>> diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
>> index 8ff088f..5876988 100644
>> --- a/xen/include/asm-arm/guest_access.h
>> +++ b/xen/include/asm-arm/guest_access.h
>> @@ -11,6 +11,8 @@
>>      (likely(count < (~0UL/size)) && access_ok(addr,count*size))
>>
>>  unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len);
>> +unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>> +                                             unsigned len);
>>  unsigned long raw_copy_from_guest(void *to, const void *from, unsigned len);
>>  unsigned long raw_clear_guest(void *to, unsigned len);
>>
>>
>
>
> --
> Julien Grall

[PATCH v2 2/2] xen: arm: flush dcache while preparing the device tree for Dom0

[Oleksandr Dmytryshyn]

Without flushing dcache the hypervisor couldn't copy the device tree
correctly when booting the kernel dom0 Image (memory with device tree
is corrupted). As the result - when we try to load the kernel dom0
Image - dom0 hungs frequently. This issue is not reproduced with the
kernel dom0 zImage because the zImage decompressor code flushes all
dcache before starting the decompressed kernel Image. When the
hypervisor loads the kernel image or initrd, this memory region
isn't corrupted because the hypervisor code flushes the dcache.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 xen/arch/arm/domain_build.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e9bb01f..99e785a 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -906,7 +906,8 @@ static void dtb_load(struct kernel_info *kinfo)
     printk("Loading dom0 DTB to 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
            kinfo->dtb_paddr, kinfo->dtb_paddr + fdt_totalsize(kinfo->fdt));
 
-    rc = raw_copy_to_guest(dtb_virt, kinfo->fdt, fdt_totalsize(kinfo->fdt));
+    rc = raw_copy_to_guest_flush_dcache(dtb_virt, kinfo->fdt,
+                                        fdt_totalsize(kinfo->fdt));
     if ( rc != 0 )
         panic("Unable to copy the DTB to dom0 memory (rc = %lu)\n", rc);
     xfree(kinfo->fdt);
-- 
1.8.2.rc2

Re: [PATCH v2 0/2] xen: arm: introduce a function and flush dcache while preparing the device tree for Dom0

[Ian Campbell]

On Tue, 2013-11-26 at 12:54 +0200, Oleksandr Dmytryshyn wrote:
> Currently we use OMAP5 ES2.0 Panda5 board to work with the hypervisor.
> 
> Without flushing dcache the hypervisor couldn't copy the device tree
> correctly when booting the kernel dom0 Image (memory with device tree
> is corrupted). As the result - when we try to load the kernel dom0
> Image - dom0 hungs frequently. This issue is not reproduced with the
> kernel dom0 zImage because the zImage decompressor code flushes all
> dcache before starting the decompressed kernel Image. When the
> hypervisor loads the kernel image or initrd, this memory region
> isn't corrupted because the hypervisor code flushes the dcache.
> 
> Oleksandr Dmytryshyn (2):
>   xen: arm: introduce raw_copy_to_guest_flush_dcache() function
>   xen: arm: flush dcache while preparing the device tree for Dom0

Both of these look good to me as a fix for the dom0 case:
Acked-by: Ian Campbell <ian.campbell@citrix.com>


> 
>  xen/arch/arm/domain_build.c        |  3 ++-
>  xen/arch/arm/guestcopy.c           | 16 +++++++++++++++-
>  xen/include/asm-arm/guest_access.h |  2 ++
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 

Re: [PATCH v2 0/2] xen: arm: introduce a function and flush dcache while preparing the device tree for Dom0

[Julien Grall]

On 11/27/2013 01:30 PM, Ian Campbell wrote:
> On Tue, 2013-11-26 at 12:54 +0200, Oleksandr Dmytryshyn wrote:
>> Currently we use OMAP5 ES2.0 Panda5 board to work with the hypervisor.
>>
>> Without flushing dcache the hypervisor couldn't copy the device tree
>> correctly when booting the kernel dom0 Image (memory with device tree
>> is corrupted). As the result - when we try to load the kernel dom0
>> Image - dom0 hungs frequently. This issue is not reproduced with the
>> kernel dom0 zImage because the zImage decompressor code flushes all
>> dcache before starting the decompressed kernel Image. When the
>> hypervisor loads the kernel image or initrd, this memory region
>> isn't corrupted because the hypervisor code flushes the dcache.
>>
>> Oleksandr Dmytryshyn (2):
>>    xen: arm: introduce raw_copy_to_guest_flush_dcache() function
>>    xen: arm: flush dcache while preparing the device tree for Dom0
>
> Both of these look good to me as a fix for the dom0 case:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

I'm not convince that is enough to fix dom0 data cache issue. What about 
the initrd?
For the zImage, I took a look to the decompressor code 
(arch/arm/boot/compressed/head.S) and I didn't see any data cache flush 
on the first instruction. So the issue can also unlikely happen with the 
zImage.

Instead of introduce a new helper I would prefer modify the existing 
raw_copy_* helpers to check if we are currently building the domain.

-- 
Julien Grall

Re: [PATCH v2 0/2] xen: arm: introduce a function and flush dcache while preparing the device tree for Dom0

[Ian Campbell]

On Wed, 2013-11-27 at 13:50 +0000, Julien Grall wrote:
> 
> On 11/27/2013 01:30 PM, Ian Campbell wrote:
> > On Tue, 2013-11-26 at 12:54 +0200, Oleksandr Dmytryshyn wrote:
> >> Currently we use OMAP5 ES2.0 Panda5 board to work with the hypervisor.
> >>
> >> Without flushing dcache the hypervisor couldn't copy the device tree
> >> correctly when booting the kernel dom0 Image (memory with device tree
> >> is corrupted). As the result - when we try to load the kernel dom0
> >> Image - dom0 hungs frequently. This issue is not reproduced with the
> >> kernel dom0 zImage because the zImage decompressor code flushes all
> >> dcache before starting the decompressed kernel Image. When the
> >> hypervisor loads the kernel image or initrd, this memory region
> >> isn't corrupted because the hypervisor code flushes the dcache.
> >>
> >> Oleksandr Dmytryshyn (2):
> >>    xen: arm: introduce raw_copy_to_guest_flush_dcache() function
> >>    xen: arm: flush dcache while preparing the device tree for Dom0
> >
> > Both of these look good to me as a fix for the dom0 case:
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I'm not convince that is enough to fix dom0 data cache issue. What about 
> the initrd?
> For the zImage, I took a look to the decompressor code 
> (arch/arm/boot/compressed/head.S) and I didn't see any data cache flush 
> on the first instruction. So the issue can also unlikely happen with the 
> zImage.

The zImage and initrd are both loaded with copy_from_paddr, which does
the appropriate flushing.

> Instead of introduce a new helper I would prefer modify the existing 
> raw_copy_* helpers to check if we are currently building the domain.

Given the above I don't think that is necessary or correct.

Ian.

Re: [PATCH v2 0/2] xen: arm: introduce a function and flush dcache while preparing the device tree for Dom0

[Julien Grall]

On 11/27/2013 01:54 PM, Ian Campbell wrote:
> On Wed, 2013-11-27 at 13:50 +0000, Julien Grall wrote:
>>
>> On 11/27/2013 01:30 PM, Ian Campbell wrote:
>>> On Tue, 2013-11-26 at 12:54 +0200, Oleksandr Dmytryshyn wrote:
>>>> Currently we use OMAP5 ES2.0 Panda5 board to work with the hypervisor.
>>>>
>>>> Without flushing dcache the hypervisor couldn't copy the device tree
>>>> correctly when booting the kernel dom0 Image (memory with device tree
>>>> is corrupted). As the result - when we try to load the kernel dom0
>>>> Image - dom0 hungs frequently. This issue is not reproduced with the
>>>> kernel dom0 zImage because the zImage decompressor code flushes all
>>>> dcache before starting the decompressed kernel Image. When the
>>>> hypervisor loads the kernel image or initrd, this memory region
>>>> isn't corrupted because the hypervisor code flushes the dcache.
>>>>
>>>> Oleksandr Dmytryshyn (2):
>>>>    xen: arm: introduce raw_copy_to_guest_flush_dcache() function
>>>>    xen: arm: flush dcache while preparing the device tree for Dom0
>>>
>>> Both of these look good to me as a fix for the dom0 case:
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>
>> I'm not convince that is enough to fix dom0 data cache issue. What about 
>> the initrd?
>> For the zImage, I took a look to the decompressor code 
>> (arch/arm/boot/compressed/head.S) and I didn't see any data cache flush 
>> on the first instruction. So the issue can also unlikely happen with the 
>> zImage.
> 
> The zImage and initrd are both loaded with copy_from_paddr, which does
> the appropriate flushing.

Oh right, I didn't pay attention that we are using different function
for initrd and zImage.

I have acked all the patch series.

-- 
Julien Grall

Re: [PATCH v2 0/2] xen: arm: introduce a function and flush dcache while preparing the device tree for Dom0

[Ian Campbell]

On Wed, 2013-11-27 at 14:51 +0000, Julien Grall wrote:
> I have acked all the patch series.

I couldn't find a mail with your ack to 2/2 but I've taken you at your
word and applied both patches from v4 of this series with both yours and
my acks.

Thanks Oleksandr.