Re: [PATCH v2] efi/x86: Call efi_parse_options() from efi_main()

2018-09-12 Thread Ard Biesheuvel
On 12 September 2018 at 20:32, Hans de Goede  wrote:
> Before this commit we were only calling efi_parse_options() from
> make_boot_params(), but make_boot_params() only gets called if the
> kernel gets booted directly as an EFI executable. So when booted through
> e.g. grub we ended up not parsing the commandline in the boot code.
>
> This makes the drivers/firmware/efi/libstub code ignore the "quiet"
> commandline argument resulting in the following message being printed:
> "EFI stub: UEFI Secure Boot is enabled."
>
> Despite the quiet request. This commits adds an extra call to
> efi_parse_options() to efi_main() to make sure that the options are
> always processed. This fixes quiet not working.
>
> This also fixes the libstub code ignoring nokaslr and efi=nochunk.
>
> Reported-by: Peter Robinson 
> Signed-off-by: Hans de Goede 

Queued in efi/next - thanks.

> ---
> Changes in v2:
> -Fix a compiler warning on 32 bit builds about casting a non pointer
>  sized integer to a pointer
> ---
>  arch/x86/boot/compressed/eboot.c | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/x86/boot/compressed/eboot.c 
> b/arch/x86/boot/compressed/eboot.c
> index 1458b1700fc7..8b4c5e001157 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -738,6 +738,7 @@ efi_main(struct efi_config *c, struct boot_params 
> *boot_params)
> struct desc_struct *desc;
> void *handle;
> efi_system_table_t *_table;
> +   unsigned long cmdline_paddr;
>
> efi_early = c;
>
> @@ -755,6 +756,15 @@ efi_main(struct efi_config *c, struct boot_params 
> *boot_params)
> else
> setup_boot_services32(efi_early);
>
> +   /*
> +* make_boot_params() may have been called before efi_main(), in which
> +* case this is the second time we parse the cmdline. This is ok,
> +* parsing the cmdline multiple times does not have side-effects.
> +*/
> +   cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> +((u64)boot_params->ext_cmd_line_ptr << 32));
> +   efi_parse_options((char *)cmdline_paddr);
> +
> /*
>  * If the boot loader gave us a value for secure_boot then we use 
> that,
>  * otherwise we ask the BIOS.
> --
> 2.19.0.rc0
>


[PATCH v4] efi: take size of partition entry from GPT header

2018-09-12 Thread Eugene Korenevsky
Use gpt_header.sizeof_partition_entry instead of sizeof(gpt_entry)
for GPT entry size.
According to UEFI 2.7 spec 5.3.1 "GPT overview":, the size of a GUID Partition
Entry element is defined in the Size Of Partition Entry field of GPT header.
The GPT with entries sized more than sizeof(gpt_entry) is not illegal.
OVMF firmware from EDK2 perfectly works with it, see edk2-tianocore source
code.

Changes since v1: refactoring (extract get_gpt_entry function),
  fix ([i] -> pte)
Changes since v2: use le32_to_cpu, fix typo, sanity check for
  sizeof_partition_entry
Changes since v3: style fixes

Signed-off-by: Eugene Korenevsky 
---
 block/partitions/efi.c | 31 ---
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 39f70d968754..0d10a582b5bc 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -429,8 +429,8 @@ static int is_gpt_valid(struct parsed_partitions *state, 
u64 lba,
goto fail;
}
/* Check that sizeof_partition_entry has the correct value */
-   if (le32_to_cpu((*gpt)->sizeof_partition_entry) != sizeof(gpt_entry)) {
-   pr_debug("GUID Partition Entry Size check failed.\n");
+   if (le32_to_cpu((*gpt)->sizeof_partition_entry) < sizeof(gpt_entry)) {
+   pr_debug("GUID Partition Entry Size is too small.\n");
goto fail;
}
 
@@ -670,6 +670,12 @@ static int find_valid_gpt(struct parsed_partitions *state, 
gpt_header **gpt,
 return 0;
 }
 
+static gpt_entry *get_gpt_entry(gpt_header *gpt, gpt_entry *ptes, u32 index)
+{
+   return (gpt_entry *)((u8 *)ptes +
+   le32_to_cpu(gpt->sizeof_partition_entry) * index);
+}
+
 /**
  * efi_partition(struct parsed_partitions *state)
  * @state: disk parsed partitions
@@ -704,32 +710,35 @@ int efi_partition(struct parsed_partitions *state)
 
pr_debug("GUID Partition Table is valid!  Yea!\n");
 
-   for (i = 0; i < le32_to_cpu(gpt->num_partition_entries) && i < 
state->limit-1; i++) {
+   for (i = 0; i < le32_to_cpu(gpt->num_partition_entries) &&
+   i < state->limit-1; i++) {
+   gpt_entry *pte = get_gpt_entry(gpt, ptes, i);
struct partition_meta_info *info;
unsigned label_count = 0;
unsigned label_max;
-   u64 start = le64_to_cpu(ptes[i].starting_lba);
-   u64 size = le64_to_cpu(ptes[i].ending_lba) -
-  le64_to_cpu(ptes[i].starting_lba) + 1ULL;
+   u64 start = le64_to_cpu(pte->starting_lba);
+   u64 size = le64_to_cpu(pte->ending_lba) -
+  le64_to_cpu(pte->starting_lba) + 1ULL;
 
-   if (!is_pte_valid([i], last_lba(state->bdev)))
+   if (!is_pte_valid(pte, last_lba(state->bdev)))
continue;
 
put_partition(state, i+1, start * ssz, size * ssz);
 
/* If this is a RAID volume, tell md */
-   if (!efi_guidcmp(ptes[i].partition_type_guid, 
PARTITION_LINUX_RAID_GUID))
+   if (!efi_guidcmp(pte->partition_type_guid,
+PARTITION_LINUX_RAID_GUID))
state->parts[i + 1].flags = ADDPART_FLAG_RAID;
 
info = >parts[i + 1].info;
-   efi_guid_to_str([i].unique_partition_guid, info->uuid);
+   efi_guid_to_str(>unique_partition_guid, info->uuid);
 
/* Naively convert UTF16-LE to 7 bits. */
label_max = min(ARRAY_SIZE(info->volname) - 1,
-   ARRAY_SIZE(ptes[i].partition_name));
+   ARRAY_SIZE(pte->partition_name));
info->volname[label_max] = 0;
while (label_count < label_max) {
-   u8 c = ptes[i].partition_name[label_count] & 0xff;
+   u8 c = pte->partition_name[label_count] & 0xff;
if (c && !isprint(c))
c = '!';
info->volname[label_count] = c;
-- 
2.18.0



Re: [PATCH] efi/efi_test: add exporting ResetSystem runtime service

2018-09-12 Thread Ard Biesheuvel
On 22 August 2018 at 11:15, Ivan Hu  wrote:
> Add exporting the UEFI runtime service ResetSystem for upper application or 
> test
> tools to use.
>
> Signed-off-by: Ivan Hu 

Queued in efi/next - thanks.

> ---
>  drivers/firmware/efi/test/efi_test.c | 27 +++
>  drivers/firmware/efi/test/efi_test.h | 10 ++
>  2 files changed, 37 insertions(+)
>
> diff --git a/drivers/firmware/efi/test/efi_test.c 
> b/drivers/firmware/efi/test/efi_test.c
> index 08129b7..a00cb67 100644
> --- a/drivers/firmware/efi/test/efi_test.c
> +++ b/drivers/firmware/efi/test/efi_test.c
> @@ -542,6 +542,30 @@ static long efi_runtime_get_nexthighmonocount(unsigned 
> long arg)
> return 0;
>  }
>
> +static long efi_runtime_reset_system(unsigned long arg)
> +{
> +   struct efi_resetsystem __user *resetsystem_user;
> +   struct efi_resetsystem resetsystem;
> +   void *data = NULL;
> +
> +   resetsystem_user = (struct efi_resetsystem __user *)arg;
> +   if (copy_from_user(, resetsystem_user,
> +   sizeof(resetsystem)))
> +   return -EFAULT;
> +   if (resetsystem.data_size != 0) {
> +   data = memdup_user((void *)resetsystem.data,
> +   resetsystem.data_size);
> +   if (IS_ERR(data))
> +   return PTR_ERR(data);
> +   }
> +
> +   efi.reset_system(resetsystem.reset_type, resetsystem.status,
> +   resetsystem.data_size, (efi_char16_t *)data);
> +
> +   kfree(data);
> +   return 0;
> +}
> +
>  static long efi_runtime_query_variableinfo(unsigned long arg)
>  {
> struct efi_queryvariableinfo __user *queryvariableinfo_user;
> @@ -679,6 +703,9 @@ static long efi_test_ioctl(struct file *file, unsigned 
> int cmd,
>
> case EFI_RUNTIME_QUERY_CAPSULECAPABILITIES:
> return efi_runtime_query_capsulecaps(arg);
> +
> +   case EFI_RUNTIME_RESET_SYSTEM:
> +   return efi_runtime_reset_system(arg);
> }
>
> return -ENOTTY;
> diff --git a/drivers/firmware/efi/test/efi_test.h 
> b/drivers/firmware/efi/test/efi_test.h
> index a33a6c6..1cc421a 100644
> --- a/drivers/firmware/efi/test/efi_test.h
> +++ b/drivers/firmware/efi/test/efi_test.h
> @@ -80,6 +80,13 @@ struct efi_querycapsulecapabilities {
> efi_status_t*status;
>  } __packed;
>
> +struct efi_resetsystem {
> +   int reset_type;
> +   efi_status_tstatus;
> +   unsigned long   data_size;
> +   efi_char16_t*data;
> +} __packed;
> +
>  #define EFI_RUNTIME_GET_VARIABLE \
> _IOWR('p', 0x01, struct efi_getvariable)
>  #define EFI_RUNTIME_SET_VARIABLE \
> @@ -107,4 +114,7 @@ struct efi_querycapsulecapabilities {
>  #define EFI_RUNTIME_QUERY_CAPSULECAPABILITIES \
> _IOR('p', 0x0A, struct efi_querycapsulecapabilities)
>
> +#define EFI_RUNTIME_RESET_SYSTEM \
> +   _IOW('p', 0x0B, struct efi_resetsystem)
> +
>  #endif /* _DRIVERS_FIRMWARE_EFI_TEST_H_ */
> --
> 2.7.4
>


RE: [PATCH V6 2/2] x86/efi: Add efi page fault handler to recover from page faults caused by the firmware

2018-09-12 Thread Prakhya, Sai Praneeth
> On 12 September 2018 at 20:56, Thomas Gleixner  wrote:
> > On Tue, 11 Sep 2018, Sai Praneeth Prakhya wrote:
> >> diff --git a/drivers/firmware/efi/runtime-wrappers.c
> b/drivers/firmware/efi/runtime-wrappers.c
> >> index b18b2d864c2c..7455277a3b65 100644
> >> --- a/drivers/firmware/efi/runtime-wrappers.c
> >> +++ b/drivers/firmware/efi/runtime-wrappers.c
> >> @@ -61,6 +61,11 @@ struct efi_runtime_work efi_rts_work;
> >>  ({   \
> >>   efi_rts_work.status = EFI_ABORTED;  \
> >>   \
> >> + if (!efi_enabled(EFI_RUNTIME_SERVICES)) {   \
> >> + pr_info("Aborting! EFI Runtime Services disabled\n");   \
> >
> > This probaby wants to be pr_info_ince().
> >

Yes.. that makes sense.

> 
> I can fix that up when applying.
>

Thanks a lot! Ard.
 
> > Other than that:
> >
> >   Reviewed-by: Thomas Gleixner 
> 
> Thanks! I will queue this up in efi/next


Re: [PATCH V6 2/2] x86/efi: Add efi page fault handler to recover from page faults caused by the firmware

2018-09-12 Thread Ard Biesheuvel
On 12 September 2018 at 20:56, Thomas Gleixner  wrote:
> On Tue, 11 Sep 2018, Sai Praneeth Prakhya wrote:
>> diff --git a/drivers/firmware/efi/runtime-wrappers.c 
>> b/drivers/firmware/efi/runtime-wrappers.c
>> index b18b2d864c2c..7455277a3b65 100644
>> --- a/drivers/firmware/efi/runtime-wrappers.c
>> +++ b/drivers/firmware/efi/runtime-wrappers.c
>> @@ -61,6 +61,11 @@ struct efi_runtime_work efi_rts_work;
>>  ({   \
>>   efi_rts_work.status = EFI_ABORTED;  \
>>   \
>> + if (!efi_enabled(EFI_RUNTIME_SERVICES)) {   \
>> + pr_info("Aborting! EFI Runtime Services disabled\n");   \
>
> This probaby wants to be pr_info_ince().
>

I can fix that up when applying.

> Other than that:
>
>   Reviewed-by: Thomas Gleixner 

Thanks! I will queue this up in efi/next


Re: [PATCH V6 2/2] x86/efi: Add efi page fault handler to recover from page faults caused by the firmware

2018-09-12 Thread Thomas Gleixner
On Tue, 11 Sep 2018, Sai Praneeth Prakhya wrote:
> diff --git a/drivers/firmware/efi/runtime-wrappers.c 
> b/drivers/firmware/efi/runtime-wrappers.c
> index b18b2d864c2c..7455277a3b65 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -61,6 +61,11 @@ struct efi_runtime_work efi_rts_work;
>  ({   \
>   efi_rts_work.status = EFI_ABORTED;  \
>   \
> + if (!efi_enabled(EFI_RUNTIME_SERVICES)) {   \
> + pr_info("Aborting! EFI Runtime Services disabled\n");   \

This probaby wants to be pr_info_ince().

Other than that:

  Reviewed-by: Thomas Gleixner 


Re: [PATCH v3] efi: take size of partition entry from GPT header

2018-09-12 Thread Davidlohr Bueso

On Wed, 12 Sep 2018, Eugene Korenevsky wrote:

/**
 * efi_partition(struct parsed_partitions *state)
 * @state: disk parsed partitions
@@ -704,32 +710,36 @@ int efi_partition(struct parsed_partitions *state)

pr_debug("GUID Partition Table is valid!  Yea!\n");

-   for (i = 0; i < le32_to_cpu(gpt->num_partition_entries) && i < 
state->limit-1; i++) {
+   for (i = 0;
+i < le32_to_cpu(gpt->num_partition_entries) && i < state->limit-1;
+i++) {


Please rearrange this like:

  for (i = 0;  < le32_to_cpu(gpt->num_partition_entries) &&
  i < state->limit-1; i++)


+   gpt_entry *pte = get_gpt_entry(gpt, ptes, i);
struct partition_meta_info *info;
unsigned label_count = 0;
unsigned label_max;
-   u64 start = le64_to_cpu(ptes[i].starting_lba);
-   u64 size = le64_to_cpu(ptes[i].ending_lba) -
-  le64_to_cpu(ptes[i].starting_lba) + 1ULL;
+   u64 start = le64_to_cpu(pte->starting_lba);
+   u64 size = le64_to_cpu(pte->ending_lba) -
+  le64_to_cpu(pte->starting_lba) + 1ULL;

-   if (!is_pte_valid([i], last_lba(state->bdev)))
+   if (!is_pte_valid(pte, last_lba(state->bdev)))
continue;

put_partition(state, i+1, start * ssz, size * ssz);

/* If this is a RAID volume, tell md */
-   if (!efi_guidcmp(ptes[i].partition_type_guid, 
PARTITION_LINUX_RAID_GUID))
+   if (!efi_guidcmp(
+   pte->partition_type_guid, PARTITION_LINUX_RAID_GUID))


This is ugly. If you are worried about 80-chars, please do:

 if (!efi_guidcmp(pte->partition_type_guid,
PARTITION_LINUX_RAID_GUID))

Thanks,
Davidlohr


Re: [PATCH v2] x86/efi: earlyprintk - Add 64bit efi fb address support

2018-09-12 Thread Ard Biesheuvel
On 12 September 2018 at 20:00, Aaron Ma  wrote:
> EFI GOP uses 64-bit frame buffer address in some BIOS.
> Add 64bit address support in efi earlyprintk.
>
> V2:
> Fix type of address.
>
> Signed-off-by: Aaron Ma 

Thanks. Queued in efi/next.

> ---
>  arch/x86/platform/efi/early_printk.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/platform/efi/early_printk.c 
> b/arch/x86/platform/efi/early_printk.c
> index 5fdacb322ceb..7476b3b097e1 100644
> --- a/arch/x86/platform/efi/early_printk.c
> +++ b/arch/x86/platform/efi/early_printk.c
> @@ -26,12 +26,14 @@ static bool early_efi_keep;
>   */
>  static __init int early_efi_map_fb(void)
>  {
> -   unsigned long base, size;
> +   u64 base, size;
>
> if (!early_efi_keep)
> return 0;
>
> base = boot_params.screen_info.lfb_base;
> +   if (boot_params.screen_info.capabilities & 
> VIDEO_CAPABILITY_64BIT_BASE)
> +   base |= (u64)boot_params.screen_info.ext_lfb_base << 32;
> size = boot_params.screen_info.lfb_size;
> efi_fb = ioremap(base, size);
>
> @@ -46,9 +48,11 @@ early_initcall(early_efi_map_fb);
>   */
>  static __ref void *early_efi_map(unsigned long start, unsigned long len)
>  {
> -   unsigned long base;
> +   u64 base;
>
> base = boot_params.screen_info.lfb_base;
> +   if (boot_params.screen_info.capabilities & 
> VIDEO_CAPABILITY_64BIT_BASE)
> +   base |= (u64)boot_params.screen_info.ext_lfb_base << 32;
>
> if (efi_fb)
> return (efi_fb + start);
> --
> 2.17.1
>


[PATCH v3] efi: take size of partition entry from GPT header

2018-09-12 Thread Eugene Korenevsky
Use gpt_header.sizeof_partition_entry instead of sizeof(gpt_entry)
for GPT entry size.
According to UEFI 2.7 spec 5.3.1 "GPT overview":, the size of a GUID Partition
Entry element is defined in the Size Of Partition Entry field of GPT header.
The GPT with entries sized more than sizeof(gpt_entry) is not illegal.
OVMF firmware from EDK2 perfectly works with it, see edk2-tianocore source
code.

Changes since v1: refactoring (extract get_gpt_entry function),
  fix ([i] -> pte)
Changes since v2: use le32_to_cpu, fix typo, sanity check for
  sizeof_partition_entry

Signed-off-by: Eugene Korenevsky 
---
 block/partitions/efi.c | 32 +---
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 39f70d968754..c8ff7860973d 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -429,8 +429,8 @@ static int is_gpt_valid(struct parsed_partitions *state, 
u64 lba,
goto fail;
}
/* Check that sizeof_partition_entry has the correct value */
-   if (le32_to_cpu((*gpt)->sizeof_partition_entry) != sizeof(gpt_entry)) {
-   pr_debug("GUID Partition Entry Size check failed.\n");
+   if (le32_to_cpu((*gpt)->sizeof_partition_entry) < sizeof(gpt_entry)) {
+   pr_debug("GUID Partition Entry Size is too small.\n");
goto fail;
}
 
@@ -670,6 +670,12 @@ static int find_valid_gpt(struct parsed_partitions *state, 
gpt_header **gpt,
 return 0;
 }
 
+static gpt_entry *get_gpt_entry(gpt_header *gpt, gpt_entry *ptes, u32 index)
+{
+   return (gpt_entry *)((u8 *)ptes +
+   le32_to_cpu(gpt->sizeof_partition_entry) * index);
+}
+
 /**
  * efi_partition(struct parsed_partitions *state)
  * @state: disk parsed partitions
@@ -704,32 +710,36 @@ int efi_partition(struct parsed_partitions *state)
 
pr_debug("GUID Partition Table is valid!  Yea!\n");
 
-   for (i = 0; i < le32_to_cpu(gpt->num_partition_entries) && i < 
state->limit-1; i++) {
+   for (i = 0;
+i < le32_to_cpu(gpt->num_partition_entries) && i < state->limit-1;
+i++) {
+   gpt_entry *pte = get_gpt_entry(gpt, ptes, i);
struct partition_meta_info *info;
unsigned label_count = 0;
unsigned label_max;
-   u64 start = le64_to_cpu(ptes[i].starting_lba);
-   u64 size = le64_to_cpu(ptes[i].ending_lba) -
-  le64_to_cpu(ptes[i].starting_lba) + 1ULL;
+   u64 start = le64_to_cpu(pte->starting_lba);
+   u64 size = le64_to_cpu(pte->ending_lba) -
+  le64_to_cpu(pte->starting_lba) + 1ULL;
 
-   if (!is_pte_valid([i], last_lba(state->bdev)))
+   if (!is_pte_valid(pte, last_lba(state->bdev)))
continue;
 
put_partition(state, i+1, start * ssz, size * ssz);
 
/* If this is a RAID volume, tell md */
-   if (!efi_guidcmp(ptes[i].partition_type_guid, 
PARTITION_LINUX_RAID_GUID))
+   if (!efi_guidcmp(
+   pte->partition_type_guid, PARTITION_LINUX_RAID_GUID))
state->parts[i + 1].flags = ADDPART_FLAG_RAID;
 
info = >parts[i + 1].info;
-   efi_guid_to_str([i].unique_partition_guid, info->uuid);
+   efi_guid_to_str(>unique_partition_guid, info->uuid);
 
/* Naively convert UTF16-LE to 7 bits. */
label_max = min(ARRAY_SIZE(info->volname) - 1,
-   ARRAY_SIZE(ptes[i].partition_name));
+   ARRAY_SIZE(pte->partition_name));
info->volname[label_max] = 0;
while (label_count < label_max) {
-   u8 c = ptes[i].partition_name[label_count] & 0xff;
+   u8 c = pte->partition_name[label_count] & 0xff;
if (c && !isprint(c))
c = '!';
info->volname[label_count] = c;
-- 
2.18.0



[PATCH v2] efi/x86: Call efi_parse_options() from efi_main()

2018-09-12 Thread Hans de Goede
Before this commit we were only calling efi_parse_options() from
make_boot_params(), but make_boot_params() only gets called if the
kernel gets booted directly as an EFI executable. So when booted through
e.g. grub we ended up not parsing the commandline in the boot code.

This makes the drivers/firmware/efi/libstub code ignore the "quiet"
commandline argument resulting in the following message being printed:
"EFI stub: UEFI Secure Boot is enabled."

Despite the quiet request. This commits adds an extra call to
efi_parse_options() to efi_main() to make sure that the options are
always processed. This fixes quiet not working.

This also fixes the libstub code ignoring nokaslr and efi=nochunk.

Reported-by: Peter Robinson 
Signed-off-by: Hans de Goede 
---
Changes in v2:
-Fix a compiler warning on 32 bit builds about casting a non pointer
 sized integer to a pointer
---
 arch/x86/boot/compressed/eboot.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 1458b1700fc7..8b4c5e001157 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -738,6 +738,7 @@ efi_main(struct efi_config *c, struct boot_params 
*boot_params)
struct desc_struct *desc;
void *handle;
efi_system_table_t *_table;
+   unsigned long cmdline_paddr;
 
efi_early = c;
 
@@ -755,6 +756,15 @@ efi_main(struct efi_config *c, struct boot_params 
*boot_params)
else
setup_boot_services32(efi_early);
 
+   /*
+* make_boot_params() may have been called before efi_main(), in which
+* case this is the second time we parse the cmdline. This is ok,
+* parsing the cmdline multiple times does not have side-effects.
+*/
+   cmdline_paddr = ((u64)hdr->cmd_line_ptr |
+((u64)boot_params->ext_cmd_line_ptr << 32));
+   efi_parse_options((char *)cmdline_paddr);
+
/*
 * If the boot loader gave us a value for secure_boot then we use that,
 * otherwise we ask the BIOS.
-- 
2.19.0.rc0



Re: [PATCH] efi/x86: Call efi_parse_options() from efi_main()

2018-09-12 Thread Ard Biesheuvel
On 12 September 2018 at 17:28, Hans de Goede  wrote:
> Hi,
>
>
> On 12-09-18 17:06, Ard Biesheuvel wrote:
>>
>> On 12 September 2018 at 15:18, Hans de Goede  wrote:
>>>
>>> Before this commit we were only calling efi_parse_options() from
>>> make_boot_params(), but make_boot_params() only gets called if the
>>> kernel gets booted directly as an EFI executable. So when booted through
>>> e.g. grub we ended up not parsing the commandline in the boot code.
>>>
>>> This makes the drivers/firmware/efi/libstub code ignore the "quiet"
>>> commandline argument resulting in the following message being printed:
>>> "EFI stub: UEFI Secure Boot is enabled."
>>>
>>> Despite the quiet request. This commits adds an extra call to
>>> efi_parse_options() to efi_main() to make sure that the options are
>>> always processed. This fixes quiet not working.
>>>
>>> This also fixes the libstub code ignoring nokaslr and efi=nochunk.
>>>
>>> Reported-by: Peter Robinson 
>>> Signed-off-by: Hans de Goede 
>>> ---
>>>   arch/x86/boot/compressed/eboot.c | 10 ++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/x86/boot/compressed/eboot.c
>>> b/arch/x86/boot/compressed/eboot.c
>>> index aaabf979cbd9..a4c85d37adc9 100644
>>> --- a/arch/x86/boot/compressed/eboot.c
>>> +++ b/arch/x86/boot/compressed/eboot.c
>>> @@ -738,6 +738,7 @@ efi_main(struct efi_config *c, struct boot_params
>>> *boot_params)
>>>  struct desc_struct *desc;
>>>  void *handle;
>>>  efi_system_table_t *_table;
>>> +   const char *cmdline_ptr;
>>>
>>>  efi_early = c;
>>>
>>> @@ -755,6 +756,15 @@ efi_main(struct efi_config *c, struct boot_params
>>> *boot_params)
>>>  else
>>>  setup_boot_services32(efi_early);
>>>
>>> +   /*
>>> +* make_boot_params() may have been called before efi_main(), in
>>> which
>>> +* case this is the second time we parse the cmdline. This is ok,
>>> +* parsing the cmdline multiple times does not have side-effects.
>>> +*/
>>> +   cmdline_ptr = (const char *)((u64)hdr->cmd_line_ptr |
>>> +   ((u64)boot_params->ext_cmd_line_ptr
>>> << 32));
>>> +   efi_parse_options(cmdline_ptr);
>>> +
>>
>>
>> This triggers a warning on 32-bit because sizeof(void*) != sizeof(u64)
>
>
> Grumble, ok so looking at what other code accessing  hdr->cmd_line_ptr +
> boot_params->ext_cmd_line_ptr is most code seems to store it in an
> unsigned long. So here is what I suggest for v2:
>
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -738,6 +738,7 @@ efi_main(struct efi_config *c, struct boot_params
> *boot_params)
> struct desc_struct *desc;
> void *handle;
> efi_system_table_t *_table;
> +   unsigned long cmdline_paddr;
>
> efi_early = c;
>
> @@ -755,6 +756,15 @@ efi_main(struct efi_config *c, struct boot_params
> *boot_params)
> else
> setup_boot_services32(efi_early);
>
> +   /*
> +* make_boot_params() may have been called before efi_main(), in
> which
> +* case this is the second time we parse the cmdline. This is ok,
> +* parsing the cmdline multiple times does not have side-effects.
> +*/
> +   cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> +((u64)boot_params->ext_cmd_line_ptr << 32));
> +   efi_parse_options((char *)cmdline_paddr);
> +
> /*
>  * If the boot loader gave us a value for secure_boot then we use
> that,
>  * otherwise we ask the BIOS.
>
> If that looks ok to you I will send out a proper v2 with this.
>

Using a temp variable is fine. Using (const char *)(unsigned
long)((u64)xxx ...) is also fine, whichever works best for you.


[PATCH v2] x86/efi: earlyprintk - Add 64bit efi fb address support

2018-09-12 Thread Aaron Ma
EFI GOP uses 64-bit frame buffer address in some BIOS.
Add 64bit address support in efi earlyprintk.

V2:
Fix type of address.

Signed-off-by: Aaron Ma 
---
 arch/x86/platform/efi/early_printk.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/early_printk.c 
b/arch/x86/platform/efi/early_printk.c
index 5fdacb322ceb..7476b3b097e1 100644
--- a/arch/x86/platform/efi/early_printk.c
+++ b/arch/x86/platform/efi/early_printk.c
@@ -26,12 +26,14 @@ static bool early_efi_keep;
  */
 static __init int early_efi_map_fb(void)
 {
-   unsigned long base, size;
+   u64 base, size;
 
if (!early_efi_keep)
return 0;
 
base = boot_params.screen_info.lfb_base;
+   if (boot_params.screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
+   base |= (u64)boot_params.screen_info.ext_lfb_base << 32;
size = boot_params.screen_info.lfb_size;
efi_fb = ioremap(base, size);
 
@@ -46,9 +48,11 @@ early_initcall(early_efi_map_fb);
  */
 static __ref void *early_efi_map(unsigned long start, unsigned long len)
 {
-   unsigned long base;
+   u64 base;
 
base = boot_params.screen_info.lfb_base;
+   if (boot_params.screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
+   base |= (u64)boot_params.screen_info.ext_lfb_base << 32;
 
if (efi_fb)
return (efi_fb + start);
-- 
2.17.1



RE: [PATCH V6 0/2] Add efi page fault handler to recover from page

2018-09-12 Thread Prakhya, Sai Praneeth
> > This issue was reported by Al Stone when he saw that reboot via EFI
> > hangs the machine. Upon debugging, I found that it's
> > efi_reset_system() that's touching memory regions which it shouldn't.
> > To reproduce the same behavior, I have hacked OVMF and made
> > efi_reset_system() buggy. Along with efi_reset_system(), I have also
> > modified get_next_high_mono_count() and set_virtual_address_map().
> > They illegally access both boot time and other efi regions.
> >
> > Testing the patch set:
> > --
> > 1. Download buggy firmware from here [1].
> > 2. Run a qemu instance with this buggy BIOS and boot mainline kernel.
> > Add reboot=efi to the kernel command line arguments and after the
> > kernel is up and running, type "reboot". The kernel should hang while
> rebooting.
> > 3. With the same setup, boot kernel after applying patches and the
> > reboot should work fine. Also please notice warning/error messages
> > printed by kernel.
> >
> 
> Did you test these patches with other buggy runtime services?

Yes, I did. I have modified efi runtime service GetNextHighMonotonicCount 
and made it buggy, when invoked from FWTS test suites the efi page fault 
handler works as expected (i.e. freezing efi_rts_wq and disabling efi runtime 
services forever).

Regards,
Sai


Re: [PATCH] efi/x86: Call efi_parse_options() from efi_main()

2018-09-12 Thread Hans de Goede

Hi,

On 12-09-18 17:06, Ard Biesheuvel wrote:

On 12 September 2018 at 15:18, Hans de Goede  wrote:

Before this commit we were only calling efi_parse_options() from
make_boot_params(), but make_boot_params() only gets called if the
kernel gets booted directly as an EFI executable. So when booted through
e.g. grub we ended up not parsing the commandline in the boot code.

This makes the drivers/firmware/efi/libstub code ignore the "quiet"
commandline argument resulting in the following message being printed:
"EFI stub: UEFI Secure Boot is enabled."

Despite the quiet request. This commits adds an extra call to
efi_parse_options() to efi_main() to make sure that the options are
always processed. This fixes quiet not working.

This also fixes the libstub code ignoring nokaslr and efi=nochunk.

Reported-by: Peter Robinson 
Signed-off-by: Hans de Goede 
---
  arch/x86/boot/compressed/eboot.c | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index aaabf979cbd9..a4c85d37adc9 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -738,6 +738,7 @@ efi_main(struct efi_config *c, struct boot_params 
*boot_params)
 struct desc_struct *desc;
 void *handle;
 efi_system_table_t *_table;
+   const char *cmdline_ptr;

 efi_early = c;

@@ -755,6 +756,15 @@ efi_main(struct efi_config *c, struct boot_params 
*boot_params)
 else
 setup_boot_services32(efi_early);

+   /*
+* make_boot_params() may have been called before efi_main(), in which
+* case this is the second time we parse the cmdline. This is ok,
+* parsing the cmdline multiple times does not have side-effects.
+*/
+   cmdline_ptr = (const char *)((u64)hdr->cmd_line_ptr |
+   ((u64)boot_params->ext_cmd_line_ptr << 32));
+   efi_parse_options(cmdline_ptr);
+


This triggers a warning on 32-bit because sizeof(void*) != sizeof(u64)


Grumble, ok so looking at what other code accessing  hdr->cmd_line_ptr +
boot_params->ext_cmd_line_ptr is most code seems to store it in an
unsigned long. So here is what I suggest for v2:

--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -738,6 +738,7 @@ efi_main(struct efi_config *c, struct boot_params 
*boot_params)
struct desc_struct *desc;
void *handle;
efi_system_table_t *_table;
+   unsigned long cmdline_paddr;

efi_early = c;

@@ -755,6 +756,15 @@ efi_main(struct efi_config *c, struct boot_params 
*boot_params)
else
setup_boot_services32(efi_early);

+   /*
+* make_boot_params() may have been called before efi_main(), in which
+* case this is the second time we parse the cmdline. This is ok,
+* parsing the cmdline multiple times does not have side-effects.
+*/
+   cmdline_paddr = ((u64)hdr->cmd_line_ptr |
+((u64)boot_params->ext_cmd_line_ptr << 32));
+   efi_parse_options((char *)cmdline_paddr);
+
/*
 * If the boot loader gave us a value for secure_boot then we use that,
 * otherwise we ask the BIOS.

If that looks ok to you I will send out a proper v2 with this.

Regards,

Hans




Re: [PATCH] x86/efi: earlyprintk - Add 64bit efi fb address support

2018-09-12 Thread Aaron Ma
On 09/12/2018 11:00 PM, Ard Biesheuvel wrote:
> On 8 September 2018 at 18:23, Aaron Ma  wrote:
>> EFI GOP uses 64-bit frame buffer address in some BIOS.
>> Add 64bit address support in efi earlyprintk.
>>
>> Signed-off-by: Aaron Ma 
>> ---
>>  arch/x86/platform/efi/early_printk.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/platform/efi/early_printk.c 
>> b/arch/x86/platform/efi/early_printk.c
>> index 5fdacb322ceb..c0ae25f59acd 100644
>> --- a/arch/x86/platform/efi/early_printk.c
>> +++ b/arch/x86/platform/efi/early_printk.c
>> @@ -32,6 +32,8 @@ static __init int early_efi_map_fb(void)
>> return 0;
>>
>> base = boot_params.screen_info.lfb_base;
>> +   if (boot_params.screen_info.capabilities & 
>> VIDEO_CAPABILITY_64BIT_BASE)
>> +   base |= (u64)boot_params.screen_info.ext_lfb_base << 32;
>> size = boot_params.screen_info.lfb_size;
>> efi_fb = ioremap(base, size);
>>
>> @@ -49,6 +51,8 @@ static __ref void *early_efi_map(unsigned long start, 
>> unsigned long len)
>> unsigned long base;
>>
>> base = boot_params.screen_info.lfb_base;
>> +   if (boot_params.screen_info.capabilities & 
>> VIDEO_CAPABILITY_64BIT_BASE)
>> +   base |= (u64)boot_params.screen_info.ext_lfb_base << 32;
>>
>> if (efi_fb)
>> return (efi_fb + start);
> Please fix this in a way that works on 32-bit x86/PAE as well (i.e.,
> use a suitable type for base)
> 

Right, I will send a v2 patch.

Thanks,
Aaron


Re: [PATCH] efi/x86: Call efi_parse_options() from efi_main()

2018-09-12 Thread Ard Biesheuvel
On 12 September 2018 at 15:18, Hans de Goede  wrote:
> Before this commit we were only calling efi_parse_options() from
> make_boot_params(), but make_boot_params() only gets called if the
> kernel gets booted directly as an EFI executable. So when booted through
> e.g. grub we ended up not parsing the commandline in the boot code.
>
> This makes the drivers/firmware/efi/libstub code ignore the "quiet"
> commandline argument resulting in the following message being printed:
> "EFI stub: UEFI Secure Boot is enabled."
>
> Despite the quiet request. This commits adds an extra call to
> efi_parse_options() to efi_main() to make sure that the options are
> always processed. This fixes quiet not working.
>
> This also fixes the libstub code ignoring nokaslr and efi=nochunk.
>
> Reported-by: Peter Robinson 
> Signed-off-by: Hans de Goede 
> ---
>  arch/x86/boot/compressed/eboot.c | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/x86/boot/compressed/eboot.c 
> b/arch/x86/boot/compressed/eboot.c
> index aaabf979cbd9..a4c85d37adc9 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -738,6 +738,7 @@ efi_main(struct efi_config *c, struct boot_params 
> *boot_params)
> struct desc_struct *desc;
> void *handle;
> efi_system_table_t *_table;
> +   const char *cmdline_ptr;
>
> efi_early = c;
>
> @@ -755,6 +756,15 @@ efi_main(struct efi_config *c, struct boot_params 
> *boot_params)
> else
> setup_boot_services32(efi_early);
>
> +   /*
> +* make_boot_params() may have been called before efi_main(), in which
> +* case this is the second time we parse the cmdline. This is ok,
> +* parsing the cmdline multiple times does not have side-effects.
> +*/
> +   cmdline_ptr = (const char *)((u64)hdr->cmd_line_ptr |
> +   ((u64)boot_params->ext_cmd_line_ptr << 
> 32));
> +   efi_parse_options(cmdline_ptr);
> +

This triggers a warning on 32-bit because sizeof(void*) != sizeof(u64)

> /*
>  * If the boot loader gave us a value for secure_boot then we use 
> that,
>  * otherwise we ask the BIOS.
> --
> 2.19.0.rc0
>


Re: [PATCH] x86/efi: earlyprintk - Add 64bit efi fb address support

2018-09-12 Thread Ard Biesheuvel
On 8 September 2018 at 18:23, Aaron Ma  wrote:
> EFI GOP uses 64-bit frame buffer address in some BIOS.
> Add 64bit address support in efi earlyprintk.
>
> Signed-off-by: Aaron Ma 
> ---
>  arch/x86/platform/efi/early_printk.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/platform/efi/early_printk.c 
> b/arch/x86/platform/efi/early_printk.c
> index 5fdacb322ceb..c0ae25f59acd 100644
> --- a/arch/x86/platform/efi/early_printk.c
> +++ b/arch/x86/platform/efi/early_printk.c
> @@ -32,6 +32,8 @@ static __init int early_efi_map_fb(void)
> return 0;
>
> base = boot_params.screen_info.lfb_base;
> +   if (boot_params.screen_info.capabilities & 
> VIDEO_CAPABILITY_64BIT_BASE)
> +   base |= (u64)boot_params.screen_info.ext_lfb_base << 32;
> size = boot_params.screen_info.lfb_size;
> efi_fb = ioremap(base, size);
>
> @@ -49,6 +51,8 @@ static __ref void *early_efi_map(unsigned long start, 
> unsigned long len)
> unsigned long base;
>
> base = boot_params.screen_info.lfb_base;
> +   if (boot_params.screen_info.capabilities & 
> VIDEO_CAPABILITY_64BIT_BASE)
> +   base |= (u64)boot_params.screen_info.ext_lfb_base << 32;
>
> if (efi_fb)
> return (efi_fb + start);

Please fix this in a way that works on 32-bit x86/PAE as well (i.e.,
use a suitable type for base)


Re: [PATCH V6 0/2] Add efi page fault handler to recover from page

2018-09-12 Thread Ard Biesheuvel
On 11 September 2018 at 21:15, Sai Praneeth Prakhya
 wrote:
> From: Sai Praneeth 
>
> There may exist some buggy UEFI firmware implementations that access efi
> memory regions other than EFI_RUNTIME_SERVICES_ even after
> the kernel has assumed control of the platform. This violates UEFI
> specification. Hence, provide a efi specific page fault handler which
> recovers from page faults caused by buggy firmware.
>
> Page faults triggered by firmware happen at ring 0 and if unhandled,
> hangs the kernel. So, provide an efi specific page fault handler to:
> 1. Avoid panics/hangs caused by buggy firmware.
> 2. Shout loud that the firmware is buggy and hence is not a kernel bug.
>
> The efi page fault handler will check if the access is by
> efi_reset_system().
> 1. If so, then the efi page fault handler will reboot the machine
>through BIOS and not through efi_reset_system().
> 2. If not, then the efi page fault handler will freeze efi_rts_wq and
>schedules a new process.
>
> This issue was reported by Al Stone when he saw that reboot via EFI hangs
> the machine. Upon debugging, I found that it's efi_reset_system() that's
> touching memory regions which it shouldn't. To reproduce the same
> behavior, I have hacked OVMF and made efi_reset_system() buggy. Along
> with efi_reset_system(), I have also modified get_next_high_mono_count()
> and set_virtual_address_map(). They illegally access both boot time and
> other efi regions.
>
> Testing the patch set:
> --
> 1. Download buggy firmware from here [1].
> 2. Run a qemu instance with this buggy BIOS and boot mainline kernel.
> Add reboot=efi to the kernel command line arguments and after the kernel
> is up and running, type "reboot". The kernel should hang while rebooting.
> 3. With the same setup, boot kernel after applying patches and the
> reboot should work fine. Also please notice warning/error messages
> printed by kernel.
>

Did you test these patches with other buggy runtime services?

> Changes from RFC to V1:
> ---
> 1. Drop "long jump" technique of dealing with illegal access and instead
>use scheduling away from efi_rts_wq.
>
> Changes from V1 to V2:
> --
> 1. Shortened config name to CONFIG_EFI_WARN_ON_ILLEGAL_ACCESS from
>CONFIG_EFI_WARN_ON_ILLEGAL_ACCESSES.
> 2. Made the config option available only to expert users.
> 3. efi_free_boot_services() should be called only when
>CONFIG_EFI_WARN_ON_ILLEGAL_ACCESS is not enabled. Previously, this
>was part of init/main.c file. As it is an architecture agnostic code,
>moved the change to arch/x86/platform/efi/quirks.c file.
>
> Changes from V2 to V3:
> --
> 1. Drop treating illegal access to EFI_BOOT_SERVICES_ regions
>separately from illegal accesses to other regions like
>EFI_CONVENTIONAL_MEMORY or EFI_LOADER_.
>In previous versions, illegal access to EFI_BOOT_SERVICES_
>regions were handled by mapping requested region to efi_pgd but from
>V3 they are handled similar to illegal access to other regions i.e by
>freezing efi_rts_wq and scheduling new process.
> 2. Change __efi_init_fixup attribute to __efi_init.
>
> Changes from V3 to V4:
> --
> 1. Drop saving original memory map passed by kernel. It also means less
>checks in efi page fault handler.
> 2. Change the config name to EFI_PAGE_FAULT_HANDLER to reflect it's
>functionality more appropriately.
>
> Changes from V4 to V5:
> --
> 1. Drop config option that enables efi page fault handler, instead make
>it default.
> 2. Call schedule() in an infinite loop to account for spurious wake ups.
> 3. Introduce "NONE" as an efi runtime service function identifier so that
>it could be used in efi_recover_from_page_fault() to check if the page
>fault was indeed triggered by an efi runtime service.
>
> Changes from V5 to V6:
> --
> 1. Thanks to 0-day for reporting build error when CONFIG_EFI is not
>enabled. Fixed it by calling efi page fault handler only when
>CONFIG_EFI is enabled.
> 2. Change return type of efi page fault handler from int to void. void
>return type should do (and int is not needed) because the efi page
>fault handler returns only upon a failure to handle page fault.
>
> Note:
> -
> Patch set based on "next" branch in efi tree.
>
> [1] https://drive.google.com/drive/folders/1VozKTms92ifyVHAT0ZDQe55ZYL1UE5wt
>
> Sai Praneeth (2):
>   efi: Make efi_rts_work accessible to efi page fault handler
>   x86/efi: Add efi page fault handler to recover from page faults caused
> by the firmware
>
>  arch/x86/include/asm/efi.h  |  1 +
>  arch/x86/mm/fault.c |  9 
>  arch/x86/platform/efi/quirks.c  | 78 
> +
>  drivers/firmware/efi/runtime-wrappers.c | 61 +++---
>  include/linux/efi.h | 42 ++
> 

[PATCH] efi/x86: Call efi_parse_options() from efi_main()

2018-09-12 Thread Hans de Goede
Before this commit we were only calling efi_parse_options() from
make_boot_params(), but make_boot_params() only gets called if the
kernel gets booted directly as an EFI executable. So when booted through
e.g. grub we ended up not parsing the commandline in the boot code.

This makes the drivers/firmware/efi/libstub code ignore the "quiet"
commandline argument resulting in the following message being printed:
"EFI stub: UEFI Secure Boot is enabled."

Despite the quiet request. This commits adds an extra call to
efi_parse_options() to efi_main() to make sure that the options are
always processed. This fixes quiet not working.

This also fixes the libstub code ignoring nokaslr and efi=nochunk.

Reported-by: Peter Robinson 
Signed-off-by: Hans de Goede 
---
 arch/x86/boot/compressed/eboot.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index aaabf979cbd9..a4c85d37adc9 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -738,6 +738,7 @@ efi_main(struct efi_config *c, struct boot_params 
*boot_params)
struct desc_struct *desc;
void *handle;
efi_system_table_t *_table;
+   const char *cmdline_ptr;
 
efi_early = c;
 
@@ -755,6 +756,15 @@ efi_main(struct efi_config *c, struct boot_params 
*boot_params)
else
setup_boot_services32(efi_early);
 
+   /*
+* make_boot_params() may have been called before efi_main(), in which
+* case this is the second time we parse the cmdline. This is ok,
+* parsing the cmdline multiple times does not have side-effects.
+*/
+   cmdline_ptr = (const char *)((u64)hdr->cmd_line_ptr |
+   ((u64)boot_params->ext_cmd_line_ptr << 32));
+   efi_parse_options(cmdline_ptr);
+
/*
 * If the boot loader gave us a value for secure_boot then we use that,
 * otherwise we ask the BIOS.
-- 
2.19.0.rc0



Re: [REGRESSION] boot-screen override by "34db50e55656 efifb: Copy the ACPI BGRT"

2018-09-12 Thread Hans de Goede

Hi,

On 03-09-18 17:11, David Herrmann wrote:

Hey

On Mon, Sep 3, 2018 at 4:47 PM Hans de Goede  wrote:


Hi,

On 03-09-18 16:16, Bartlomiej Zolnierkiewicz wrote:


Hi,

On Monday, September 03, 2018 03:23:38 PM David Herrmann wrote:

Hey

Since this commit:

  34db50e55656 efifb: Copy the ACPI BGRT

the kernel will override boot-splashs unasked. This breaks the
graphical boot-process on our setups. In particular, we have a setup
where an efi-boot-entry draws the early boot-splash on-screen, then
hands-over to the linux-kernel + initrd. The boot-splash daemon in the
initrd then takes over control of possible animations.

With the mentioned commit compiled in, the kernel will redraw the
firmware logo on screen at a random time without any way to intervene.


You have CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER=y (the deferred
console takeover support introduced in v4,19-rc1). I assume that this
is intended?


What is the intention of this commit? Why is the kernel re-drawing the
firmware logo unasked? If someone during the boot-process draws
content on the screen, I would prefer if the kernel does not clear
that on driver load.


+/*
+ * If fbcon deffered console takeover is configured, the intent is for the
+ * framebuffer to show the boot graphics (e.g. vendor logo) until there is some
+ * (error) message to display. But the boot graphics may have been destroyed by
+ * e.g. option ROM output, detect this and restore the boot graphics.
+ */
+#if defined CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER && \
+defined CONFIG_ACPI_BGRT
...
+static void efifb_show_boot_graphics(struct fb_info *info)
...
+#else
+static inline void efifb_show_boot_graphics(struct fb_info *info) {}
+#endif


Can we either provide an option to disable this feature, or revert the commit?


Hans?


We use the ACPI bgrt extension to get the logo to draw and then
draw it since some devices rely on the OS to draw it and otherwise we
just end up with a blackscreen when doing silent / flickerfree boot.

Ideally you would be able to get your vendor to put the logo in firmware
in the ACPI bgrt extension, then you also do not need to play any tricks
with an EFI binary drawing your own logo. But I can understand if you don't
have control over this, so you need to do this with the EFI binary trick.

I can also understand that you want to use deferred console takeover
in a setup like yours to avoid fbcon messing up what is on the display
during boot, that is exactly what it is for. So I think a reasonable
approach here is to add a kernel commandline option, say:

video=efifb:nobgrt

David, would that work for you?


That would be perfect!


Ok, I just send out a patch for this with you in the Cc.

Sorry for taking a bit long to write the patch.

Regards,

Hans



Re: [PATCH v2] efi: take size of partition entry from GPT header

2018-09-12 Thread Karel Zak
On Tue, Sep 11, 2018 at 07:15:27PM +0300, Eugene Korenevsky wrote:
> +static gpt_entry *get_gpt_entry(gpt_header *gpt_hdr, gpt_entry *ptes, u32 
> index)
> +{
> + return (gpt_entry *)((u8 *)ptes + gpt->sizeof_partition_entry * index);

I guess header use LE, so you need:

   le32_to_cpu(gpt_hdr->sizeof_partition_entry)

Karel

-- 
 Karel Zak  
 http://karelzak.blogspot.com


Re: [PATCH v6 1/3] x86/boot: Add acpitb.c to parse acpi tables

2018-09-12 Thread kbuild test robot
Hi Chao,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/x86/core]
[also build test WARNING on v4.19-rc3 next-20180911]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Chao-Fan/x86-boot-KASLR-Parse-ACPI-table-and-limit-kaslr-in-immovable-memory/20180911-043350
config: i386-randconfig-x0-09121328 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

   arch/x86/boot/compressed/acpitb.c: In function 'bios_get_rsdp_addr':
>> arch/x86/boot/compressed/acpitb.c:181:14: warning: cast to pointer from 
>> integer of different size [-Wint-to-pointer-cast]
 table_ptr = (u8 *)(acpi_physical_address)address;
 ^
   arch/x86/boot/compressed/acpitb.c: In function 'get_acpi_rsdp':
   arch/x86/boot/compressed/acpitb.c:225:3: warning: implicit declaration of 
function 'error' [-Wimplicit-function-declaration]
  error("Failed to allocate space for tmp_cmdline");
  ^
>> arch/x86/boot/compressed/acpitb.c:237:4: warning: implicit declaration of 
>> function 'warn' [-Wimplicit-function-declaration]
   warn("Only '--' specified in cmdline");
   ^
   arch/x86/boot/compressed/acpitb.c: In function 'get_acpi_srat_table':
   arch/x86/boot/compressed/acpitb.c:296:9: warning: cast to pointer from 
integer of different size [-Wint-to-pointer-cast]
 rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
^
   arch/x86/boot/compressed/acpitb.c:311:11: warning: cast to pointer from 
integer of different size [-Wint-to-pointer-cast]
 header = (struct acpi_table_header *)root_table;
  ^
   arch/x86/boot/compressed/acpitb.c:328:13: warning: cast to pointer from 
integer of different size [-Wint-to-pointer-cast]
   header = (struct acpi_table_header *)acpi_table;
^

vim +181 arch/x86/boot/compressed/acpitb.c

   164  
   165  /*
   166   * Used to search RSDP physical address.
   167   * Based on acpi_find_root_pointer(). Since only use physical address
   168   * in this period, so there is no need to do the memory map jobs.
   169   */
   170  static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
   171  {
   172  struct acpi_table_rsdp *rsdp;
   173  u8 *table_ptr;
   174  u8 *mem_rover;
   175  u32 address;
   176  
   177  /* Get the location of the Extended BIOS Data Area (EBDA) */
   178  table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION;
   179  *(u32 *)(void *) = *(u16 *)(void *)table_ptr;
   180  address <<= 4;
 > 181  table_ptr = (u8 *)(acpi_physical_address)address;
   182  
   183  /*
   184   * Search EBDA paragraphs (EBDA is required to be a minimum of
   185   * 1K length)
   186   */
   187  if (address > 0x400) {
   188  mem_rover = scan_mem_for_rsdp(table_ptr, 
ACPI_EBDA_WINDOW_SIZE);
   189  
   190  if (mem_rover) {
   191  address += (u32)ACPI_PTR_DIFF(mem_rover, 
table_ptr);
   192  *rsdp_addr = (acpi_physical_address)address;
   193  return;
   194  }
   195  }
   196  
   197  table_ptr = (u8 *)ACPI_HI_RSDP_WINDOW_BASE;
   198  mem_rover = scan_mem_for_rsdp(table_ptr, 
ACPI_HI_RSDP_WINDOW_SIZE);
   199  
   200  /*
   201   * Search upper memory: 16-byte boundaries in Eh-Fh
   202   */
   203  if (mem_rover) {
   204  address = (u32)(ACPI_HI_RSDP_WINDOW_BASE +
   205  ACPI_PTR_DIFF(mem_rover, table_ptr));
   206  *rsdp_addr = (acpi_physical_address)address;
   207  return;
   208  }
   209  }
   210  
   211  #ifdef CONFIG_KEXEC
   212  static bool get_acpi_rsdp(acpi_physical_address *rsdp_addr)
   213  {
   214  char *args = (char *)get_cmd_line_ptr();
   215  size_t len = strlen((char *)args);
   216  char *tmp_cmdline, *param, *val;
   217  unsigned long long addr = 0;
   218  char *endptr;
   219  
   220  if (!strstr(args, "acpi_rsdp="))
   221  return false;
   222  
   223  tmp_cmdline = malloc(len+1);
   224  if (!tmp_cmdline)
   225  error("Failed to allocate space for tmp_cmdline");
   226  
   227  memcpy(tmp_cmdline, args, len);
   228  tmp_cmdline[len] = 0;
   229  args = tmp_cmdline;
   230  
   231  args = skip_spaces(args);
   232  
   233  while (*args) {
   234  args = next_arg(args, , );
   235  
   236  if (!val && strcmp(param, "--") == 0) {
 > 237  warn("Only