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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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


> > --- 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