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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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@xxxxxxxxxxxxx> 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!

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel