Re: [Linaro-uefi] EFI stub patches for FVP model

Hi Supreeth,

On Tue, Dec 08, 2015 at 08:07:47PM +0000, Supreeth Venkatesh wrote:

All,

Please find the attached patch with EFI stub changes to make it work for FVP.

First of all - please don't send patches as attachments. For situations like this, when there is a substantial amount of background information needed, it can make sense to provide a cover letter even for a one-patch series. But regardless, the commit message for the patch should contain the information for what

Secondly, this patch is lacking context (I know why you're doing this, but others may not) - EFI stub works just fine with FVP today.

This patch contains two fixes

1.  FVP model not booting to Linux Shell with -C css.cache_state_modelled=1 flag.  (arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S)
   

Without testing right now, I believe this is not the case unless one is making use of the new features you are adding with this patch.

2.  Pre-load RAM Location support for dtb and initrd. (Format: dtb=RAM Address,Size, initrd= RAM Address,Size). (drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c)
   

2 is a new feature not a fix.

This patch was based off of git clone http://git.linaro.org/landing-teams/working/arm/kernel.git . git checkout v4.3

Unless there is a strong reason (which should be specified in the cover letter), patches should be based on a plain upstream kernel.

diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S index 8ce9b05..0024d02 100644 --- a/arch/arm64/kernel/efi-entry.S +++ b/arch/arm64/kernel/efi-entry.S @@ -91,16 +91,6 @@ ENTRY(efi_stub_entry) sub x1, x1, x0 bl __flush_dcache_area

• /* Turn off Dcache and MMU */
• mrs x0, CurrentEL
• cmp x0, #CurrentEL_EL2
• b.ne 1f
• mrs x0, sctlr_el2
• bic x0, x0, #1 << 0 // clear SCTLR.M
• bic x0, x0, #1 << 2 // clear SCTLR.C
• msr sctlr_el2, x0
• isb
• b 2f

This is not a valid thing to do, and potentially breaks all other platforms, apart from violating the kernel boot protocol. Why do you believe it necessary?

The issue you've encountered probably stems from the images magically appearing in RAM at reset rather than being allocated, written to cacheable locations and cleaned like they would be if loaded from a filesystem. We need to resolve that in an architecturally correct fashion.

1: mrs x0, sctlr_el1 bic x0, x0, #1 << 0 // clear SCTLR.M diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index f07d4a6..61757ef 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -369,6 +369,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, efi_status_t status; int nr_files; char *str;

• char Message[256] = {0};

So, I get why you're doing this, and having better debug print output in the stub would be useful, but: - this is an arbitrarily sized buffer. - variable names do not start with upper-case - this style violation is not highlighted by scripts/checkpatch.pl, but several other issues are - those need to be addressed.

If we can indeed always rely on having sprintf available to us in the stub, this would be worth implementing as a proper output function rather than doing it inline in one location.

int i, j, k; file_addr = 0; @@ -413,10 +414,14 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, } str = cmd_line;

No need to add new empty lines in existing code that isn't being modified.

for (i = 0; i < nr_files; i++) { struct file_info *file; efi_char16_t filename_16[256]; efi_char16_t *p;

• char pram[256];
  

Another arbitrary buffer size. Yes, there is already one for the filename, but the address pair is actually 100% predictable in string length.

• char *token = NULL;
  

• char *ppram;
  

In general, interleaving the address parsing completely into the (already a bit messy) path scanning code makes this quite difficult to read.

I think it is worth considering splitting the two out into separate functions.

str = strstr(str, option_string); if (!str) @@ -426,6 +431,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, file = &files[i]; p = filename_16;

• ppram = pram;
  

/* Skip any leading slashes */ while (*str == '/' || *str == '\') @@ -437,13 +443,41 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, if (*str == '/') { *p++ = '\';

• 		*ppram++ = '\\';
  	str++;
  } else {
  

• 		*p++ = *str++;
  

• 		*p = *str;
  

• 		*ppram = *str;
  

• 		p++;
  

• 		ppram++;
  

• 		str++;
  }
  

  }

*p = '\0';

• *ppram = '\0';
  

• /* Check Whether Pre-load RAM Location. Format dtb=0x83000000,0x83200000 */
  

• ppram = pram;
  

• token = strsep(&ppram, ",");
  

• if(token != NULL)
  

• {
  

• 	sscanf( (const char *)token, "%lx", &file_addr);
  

• 	memset(Message, 0, sizeof(Message));
  

Why memset the string to 0?

• 	sprintf(Message, "Preloaded dtb or initrd found. Address = 0x%lx\n", file_addr);
  

• 	efi_printk(sys_table_arg, Message);
  

• 	token = strsep(&ppram, ",");
  

• 	if(token != NULL)
  

• 	{
  

• 		sscanf( (const char *)token, "%llx", &file_size_total );
  

• 		memset(Message, 0, sizeof(Message));
  

• 		sprintf(Message, "Preloaded dtb or initrd File Size = 0x%llx\n", file_size_total);
  

• 		efi_printk(sys_table_arg, Message);
  

• 		break;
  

• 	}
  

• }
  

/* Only open the volume once. */ if (!i) { @@ -460,8 +494,8 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, file_size_total += file->size; }

• if (file_size_total) {

• if (file_size_total && !file_addr) { unsigned long addr;

/*