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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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


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