[PATCH for-4.6] efi: introduce efi_arch_flush_dcache_area

[PATCH for-4.6] efi: introduce efi_arch_flush_dcache_area

[Stefano Stabellini]

Objects loaded by FileHandle->Read need to be flushed to dcache,
otherwise copy_from_paddr will read stale data when copying the kernel,
causing a failure to boot.

Introduce efi_arch_flush_dcache_area and call it from read_file.

This commit introduces no functional changes on x86.

Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: ian.campbell@citrix.com
CC: JBeulich@suse.com
CC: wei.liu2@citrix.com
---
 xen/arch/arm/efi/efi-boot.h |    7 +++++++
 xen/arch/x86/efi/efi-boot.h |    2 ++
 xen/common/efi/boot.c       |    3 ++-
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 6a12d91..fd79658 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -9,6 +9,7 @@
 #include <asm/smp.h>
 
 void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
+void __flush_dcache_area(const void * vaddr, unsigned long size);
 
 #define DEVICE_TREE_GUID \
 {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
@@ -571,6 +572,12 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
                                        EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info)
 {
 }
+
+static void efi_arch_flush_dcache_area(const void *vaddr, UINTN size)
+{
+    __flush_dcache_area(vaddr, size);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 2dd69f6..4c7f383 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -640,6 +640,8 @@ static bool_t __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
     return 1; /* x86 always uses a config file */
 }
 
+static void efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { }
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 75a939f..f0f26c1 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -526,7 +526,8 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
         PrintErr(what);
         PrintErr(L" failed for ");
         PrintErrMesg(name, ret);
-    }
+    } else
+        efi_arch_flush_dcache_area(file->ptr, file->size);
 
     return 1;
 }
-- 
1.7.10.4

Re: [PATCH for-4.6] efi: introduce efi_arch_flush_dcache_area

[Ian Campbell]

On Mon, 2015-09-07 at 15:13 +0100, Stefano Stabellini wrote:
> Objects loaded by FileHandle->Read need to be flushed to dcache,
> otherwise copy_from_paddr will read stale data when copying the kernel,
> causing a failure to boot.
> 
> Introduce efi_arch_flush_dcache_area and call it from read_file.
> 
> This commit introduces no functional changes on x86.
> 
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: ian.campbell@citrix.com
> CC: JBeulich@suse.com
> CC: wei.liu2@citrix.com

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH for-4.6] efi: introduce efi_arch_flush_dcache_area

[Jan Beulich]

>>> On 07.09.15 at 16:13, <stefano.stabellini@eu.citrix.com> wrote:
> Objects loaded by FileHandle->Read need to be flushed to dcache,
> otherwise copy_from_paddr will read stale data when copying the kernel,
> causing a failure to boot.

I have to admit that I'm a little puzzled by this description: If this
was a general requirement (which is how it reads to me), why does
->Read() not do the necessary flushing? Or if it's not a general
requirement, perhaps the description could be changed to make
clear what exact dependency exists that does not exist for all
users of ->Read()?

> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -9,6 +9,7 @@
>  #include <asm/smp.h>
>  
>  void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
> +void __flush_dcache_area(const void * vaddr, unsigned long size);

While I see Ian ack-ed this, I find it wrong to replicate a declaration
here that now can get out of sync with the canonical one at any
point in time, without anyone noticing at build time. Or wait - this
looks to be a boot time only interface that so far didn't even have a
C declaration. That's fine then except for the minor coding style issue
(stray blank between "*" and "vaddr").

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -526,7 +526,8 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>          PrintErr(what);
>          PrintErr(L" failed for ");
>          PrintErrMesg(name, ret);
> -    }
> +    } else
> +        efi_arch_flush_dcache_area(file->ptr, file->size);

No need for the (misplaced) "else" - PrintErrMesg() does not return.

Jan

Re: [PATCH for-4.6] efi: introduce efi_arch_flush_dcache_area

[Stefano Stabellini]

On Mon, 7 Sep 2015, Jan Beulich wrote:
> >>> On 07.09.15 at 16:13, <stefano.stabellini@eu.citrix.com> wrote:
> > Objects loaded by FileHandle->Read need to be flushed to dcache,
> > otherwise copy_from_paddr will read stale data when copying the kernel,
> > causing a failure to boot.
> 
> I have to admit that I'm a little puzzled by this description: If this
> was a general requirement (which is how it reads to me)

Yes, it is

    
> why does > ->Read() not do the necessary flushing? Or if it's not a
> general requirement, perhaps the description could be changed to make
> clear what exact dependency exists that does not exist for all users
> of ->Read()?

It is a general requirement: anything that could be accessed without a
cacheable mapping needs to be flushed. Unfortunately the UEFI spec is
not clear enough on who should do the flushing on what, and in fact the
Forum is working on improving it


> > --- a/xen/arch/arm/efi/efi-boot.h
> > +++ b/xen/arch/arm/efi/efi-boot.h
> > @@ -9,6 +9,7 @@
> >  #include <asm/smp.h>
> >  
> >  void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
> > +void __flush_dcache_area(const void * vaddr, unsigned long size);
> 
> While I see Ian ack-ed this, I find it wrong to replicate a declaration
> here that now can get out of sync with the canonical one at any
> point in time, without anyone noticing at build time. Or wait - this
> looks to be a boot time only interface that so far didn't even have a
> C declaration.

That's right


> That's fine then except for the minor coding style issue
> (stray blank between "*" and "vaddr").

Thanks for spotting that


> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -526,7 +526,8 @@ static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> >          PrintErr(what);
> >          PrintErr(L" failed for ");
> >          PrintErrMesg(name, ret);
> > -    }
> > +    } else
> > +        efi_arch_flush_dcache_area(file->ptr, file->size);
> 
> No need for the (misplaced) "else" - PrintErrMesg() does not return.

Ah! Thanks!

Re: [PATCH for-4.6] efi: introduce efi_arch_flush_dcache_area

[Ian Campbell]

On Mon, 2015-09-07 at 16:08 +0100, Stefano Stabellini wrote:
> On Mon, 7 Sep 2015, Jan Beulich wrote:
> > > > > On 07.09.15 at 16:13, <stefano.stabellini@eu.citrix.com> wrote:
> > > Objects loaded by FileHandle->Read need to be flushed to dcache,
> > > otherwise copy_from_paddr will read stale data when copying the 
> > > kernel,
> > > causing a failure to boot.
> > 
> > I have to admit that I'm a little puzzled by this description: If this
> > was a general requirement (which is how it reads to me)
> 
> Yes, it is
> 
>     
> > why does > ->Read() not do the necessary flushing? Or if it's not a
> > general requirement, perhaps the description could be changed to make
> > clear what exact dependency exists that does not exist for all users
> > of ->Read()?
> 
> It is a general requirement: anything that could be accessed without a
> cacheable mapping needs to be flushed. Unfortunately the UEFI spec is
> not clear enough on who should do the flushing on what, and in fact the
> Forum is working on improving it

I might be wrong and/or misremembering but I think this stems partly from
the fact that the ARM UEFI stub in Xen needs to turn off caches (and
paging?) before jumping to the usual Xen entry point.

Then when caches come back on you get inconsistencies because of stale
stuff in the cache which suddenly reappears etc.

Ideally we would flush the whole cache when we disable the cache, but the
mechanism for doing so for the whole cache (by set/way) is not
architecturally required to be snooped by external cache controllers. Only
flush by VA is required to be snooped but that would take a very long time
on a system of any size. At this level Xen is not yet aware of any external
cache controllers to flush them explicitly etc, so we are a bit stuck and
end up having to flush each thing we load by VA.

I'm not sure but an alternative could be to make ARM's head.S work
regardless of the cache being enabled or disabled. I don't know if that is
practical though.

Ian.

> 
> 
> > > --- a/xen/arch/arm/efi/efi-boot.h
> > > +++ b/xen/arch/arm/efi/efi-boot.h
> > > @@ -9,6 +9,7 @@
> > >  #include <asm/smp.h>
> > >  
> > >  void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
> > > +void __flush_dcache_area(const void * vaddr, unsigned long size);
> > 
> > While I see Ian ack-ed this, I find it wrong to replicate a declaration
> > here that now can get out of sync with the canonical one at any
> > point in time, without anyone noticing at build time. Or wait - this
> > looks to be a boot time only interface that so far didn't even have a
> > C declaration.
> 
> That's right
> 
> 
> > That's fine then except for the minor coding style issue
> > (stray blank between "*" and "vaddr").
> 
> Thanks for spotting that
> 
> 
> > > --- a/xen/common/efi/boot.c
> > > +++ b/xen/common/efi/boot.c
> > > @@ -526,7 +526,8 @@ static bool_t __init read_file(EFI_FILE_HANDLE 
> > > dir_handle, CHAR16 *name,
> > >          PrintErr(what);
> > >          PrintErr(L" failed for ");
> > >          PrintErrMesg(name, ret);
> > > -    }
> > > +    } else
> > > +        efi_arch_flush_dcache_area(file->ptr, file->size);
> > 
> > No need for the (misplaced) "else" - PrintErrMesg() does not return.
> 
> Ah! Thanks!

Re: [PATCH for-4.6] efi: introduce efi_arch_flush_dcache_area

[Mark Rutland]

On Mon, Sep 07, 2015 at 05:06:36PM +0100, Ian Campbell wrote:
> On Mon, 2015-09-07 at 16:08 +0100, Stefano Stabellini wrote:
> > On Mon, 7 Sep 2015, Jan Beulich wrote:
> > > > > > On 07.09.15 at 16:13, <stefano.stabellini@eu.citrix.com> wrote:
> > > > Objects loaded by FileHandle->Read need to be flushed to dcache,
> > > > otherwise copy_from_paddr will read stale data when copying the 
> > > > kernel,
> > > > causing a failure to boot.
> > > 
> > > I have to admit that I'm a little puzzled by this description: If this
> > > was a general requirement (which is how it reads to me)
> > 
> > Yes, it is
> > 
> >     
> > > why does > ->Read() not do the necessary flushing? Or if it's not a
> > > general requirement, perhaps the description could be changed to make
> > > clear what exact dependency exists that does not exist for all users
> > > of ->Read()?
> > 
> > It is a general requirement: anything that could be accessed without a
> > cacheable mapping needs to be flushed. Unfortunately the UEFI spec is
> > not clear enough on who should do the flushing on what, and in fact the
> > Forum is working on improving it
> 
> I might be wrong and/or misremembering but I think this stems partly from
> the fact that the ARM UEFI stub in Xen needs to turn off caches (and
> paging?) before jumping to the usual Xen entry point.
> 
> Then when caches come back on you get inconsistencies because of stale
> stuff in the cache which suddenly reappears etc.

It's more to do with using inconsistent attributes than about whether
the caches are enabled -- at both points the kernel image is
manipulated, SCTLR_EL2.{C,M} == {1,1}.

EFI_FILE_PROTOCOL.Read() may place data into the caches without updating
memory because of coherent IO or simply because it copies to a cacheable
alias. If Xen were to modify the image in any way (e.g. decompressing
it), it would only update the cached copy.

Later kernel_zimage_load calls copy_from_paddr, which uses a
non-cacheable alias, bypassing the caches and going straight to memory.

Even if the two aliases were in use simultaneously, they wouldn't be
coherent with each other.

For more background, Marc Zyngier did a good talk at KVM Forum regarding
the usual behaviour of the caches [1,2].

Mark.

[1] http://events.linuxfoundation.org/sites/events/files/slides/slides_10.pdf
[2] https://www.youtube.com/watch?v=A_zCxzpxzmE

Re: [PATCH for-4.6] efi: introduce efi_arch_flush_dcache_area

[Wei Liu]

On Mon, Sep 07, 2015 at 03:13:50PM +0100, Stefano Stabellini wrote:
> Objects loaded by FileHandle->Read need to be flushed to dcache,
> otherwise copy_from_paddr will read stale data when copying the kernel,
> causing a failure to boot.
> 
> Introduce efi_arch_flush_dcache_area and call it from read_file.
> 
> This commit introduces no functional changes on x86.
> 
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: ian.campbell@citrix.com
> CC: JBeulich@suse.com
> CC: wei.liu2@citrix.com

I'm expecting a v2 with minor adjustments.

In any case this is a valid and straightforward bug fix:

  Release-acked-by: Wei Liu <wei.liu2@citrix.com>

I will wait for v2 and keep an eye on our testing progress. This will
probably go in after RC3 (i.e. after branching) unless OSSTest is wedged
again and we somehow need to retest staging.

Wei.