Re: [PATCH v2 1/2] efi/capsule: Add 'capsule' lookup support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 1 March 2017 at 17:59, Qiuxu Zhuo <qiuxu.zhuo@xxxxxxxxx> wrote:
> Add the interface to get the array of EFI capsules when
> parsing the configuration tables.
>
> This code is based on Matt Fleming previous work from:
> https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-pstore&id=99c5f047133555aa0442f64064e85b7da2d4a45f
>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@xxxxxxxxx>
> ---
>  drivers/firmware/efi/capsule.c | 107 +++++++++++++++++++++++++++++++++++++++++
>  drivers/firmware/efi/efi.c     |   4 ++
>  include/linux/efi.h            |   4 ++
>  3 files changed, 115 insertions(+)
>
> diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> index 6eedff4..7bf79fd 100644
> --- a/drivers/firmware/efi/capsule.c
> +++ b/drivers/firmware/efi/capsule.c
> @@ -24,6 +24,11 @@ typedef struct {
>  static bool capsule_pending;
>  static bool stop_capsules;
>  static int efi_reset_type = -1;
> +/*
> + * Information about capsules we pulled from the EFI System Table.
> + */
> +static efi_capsule_header_t **prev_capsules;
> +static u32 efi_capsule_num;
>
>  /*
>   * capsule_mutex serialises access to both capsule_pending and
> @@ -288,6 +293,108 @@ int efi_capsule_update(efi_capsule_header_t *capsule, struct page **pages)
>  }
>  EXPORT_SYMBOL_GPL(efi_capsule_update);
>
> +static int extract_capsules(void)
> +{

I would like to understand something: this routine is called
'extract_capsules', but the way it is wired up, we can only use if for
capsules with GUID set to LINUX_CRASH_EFI_GUID. That seems incorrect
to me: the generic capsule code should be able to extract capsules
with any GUID (which the prototype of efi_capsule_lookup suggests).

I think it comes down to removing the global state in prev_capsules
and efi_capsule_num, and folding extract_capsules() into
efi_capsule_lookup().

You could add a routine that iterates over the config table array to
look for your GUID: that way, you don't need to put it in generic EFI
code.

> +       void *capsule;
> +       size_t size;
> +
> +       if (efi.capsule == EFI_INVALID_TABLE_ADDR)
> +               return 0;
> +
> +       capsule = memremap(efi.capsule, sizeof(efi_capsule_num), MEMREMAP_WB);
> +       if (!capsule)
> +               return -ENOMEM;
> +
> +       /*
> +        * The array of capsules is prefixed with the number of
> +        * capsule entries in the array.
> +        */
> +       efi_capsule_num = *(uint32_t *)capsule;
> +       iounmap(capsule);
> +

Please use memunmap here

> +       if (!efi_capsule_num) {
> +               pr_info("No capsules on extraction\n");
> +               return 0;
> +       }
> +
> +       size = efi_capsule_num * sizeof(*capsule);
> +       capsule = memremap(efi.capsule, size, MEMREMAP_WB);

Is 'capsule' ever unmapped? If not, why not?

> +       if (!capsule)
> +               return -ENOMEM;
> +
> +       capsule += sizeof(uint32_t);

This is incorrect for 64-bit. You need to increment by the size of
unsigned long here, regardless of the size of efi_capsule_num.

> +       prev_capsules = (efi_capsule_header_t **)capsule;
> +       if (!*prev_capsules)
> +               pr_err("Capsule array has no entries\n");
> +
> +       return 0;
> +}
> +
> +/**
> + * efi_capsule_lookup - search capsule array for entries.
> + * @guid: the guid to search for.
> + * @nr_caps: the number of entries found.
> + *
> + * Map each capsule header into the kernel's virtual address space and
> + * inspect the guid. Build an array of capsule headers with every
> + * capsule that is found with @guid. If a match is found the capsule
> + * remains mapped, otherwise it is unmapped.
> + *
> + * Returns an array of capsule headers, each element of which has the
> + * guid @guid. The number of elements in the array is stored in
> + * @nr_caps. Returns %NULL if no capsules were found and stores zero
> + * in @nr_caps.
> + */
> +efi_capsule_header_t **efi_capsule_lookup(efi_guid_t guid, uint32_t *nr_caps)
> +{
> +       efi_capsule_header_t **capsules = NULL, **tmp;
> +       size_t capsule_size = 0;
> +       int i, rv;
> +
> +       rv = extract_capsules();
> +       if (rv)
> +               return ERR_PTR(rv);
> +
> +       *nr_caps = 0;
> +       for (i = 0; i < efi_capsule_num; i++) {
> +               efi_capsule_header_t *c;
> +               size_t size;
> +
> +               c = memremap((resource_size_t)prev_capsules[i], sizeof(*c), MEMREMAP_WB);
> +               if (!c) {
> +                       pr_err("Failed to memremap capsule\n");
> +                       continue;
> +               }
> +
> +               size = c->imagesize;
> +               iounmap(c);
> +

memunmap

> +               c = memremap((resource_size_t)prev_capsules[i], size, MEMREMAP_WB);
> +               if (!c) {
> +                       pr_err("Failed to memremap header + data\n");
> +                       continue;
> +               }
> +
> +               if (!efi_guidcmp(c->guid, guid)) {

When can we expect the capsule GUID to deviate from the GUID we looked for?

> +                       capsule_size += sizeof(*capsules);
> +                       tmp = krealloc(capsules, capsule_size, GFP_KERNEL);
> +                       if (!tmp) {
> +                               kfree(capsules);
> +                               return ERR_PTR(-ENOMEM);
> +                       }
> +
> +                       capsules = tmp;
> +                       capsules[(*nr_caps)++] = c;
> +                       continue;
> +               }
> +
> +               iounmap(c);

memunmap

> +       }
> +
> +       return capsules;
> +}
> +EXPORT_SYMBOL_GPL(efi_capsule_lookup);
> +
>  static int capsule_reboot_notify(struct notifier_block *nb, unsigned long event, void *cmd)
>  {
>         mutex_lock(&capsule_mutex);
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 9291480..03be9b8 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -52,6 +52,7 @@ struct efi __read_mostly efi = {
>         .properties_table       = EFI_INVALID_TABLE_ADDR,
>         .mem_attr_table         = EFI_INVALID_TABLE_ADDR,
>         .rng_seed               = EFI_INVALID_TABLE_ADDR,
> +       .capsule                = EFI_INVALID_TABLE_ADDR,
>  };
>  EXPORT_SYMBOL(efi);
>
> @@ -120,6 +121,8 @@ static ssize_t systab_show(struct kobject *kobj,
>                 str += sprintf(str, "BOOTINFO=0x%lx\n", efi.boot_info);
>         if (efi.uga != EFI_INVALID_TABLE_ADDR)
>                 str += sprintf(str, "UGA=0x%lx\n", efi.uga);
> +       if (efi.capsule != EFI_INVALID_TABLE_ADDR)
> +               str += sprintf(str, "CAPSULE=0x%lx\n", efi.capsule);
>
>         return str - buf;
>  }
> @@ -445,6 +448,7 @@ static __initdata efi_config_table_type_t common_tables[] = {
>         {EFI_PROPERTIES_TABLE_GUID, "PROP", &efi.properties_table},
>         {EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", &efi.mem_attr_table},
>         {LINUX_EFI_RANDOM_SEED_TABLE_GUID, "RNG", &efi.rng_seed},
> +       {LINUX_EFI_CRASH_GUID, "CAPSULE", &efi.capsule},
>         {NULL_GUID, NULL, NULL},
>  };
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 5b1af30..3296724e 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -893,6 +893,7 @@ extern struct efi {
>         unsigned long properties_table; /* properties table */
>         unsigned long mem_attr_table;   /* memory attributes table */
>         unsigned long rng_seed;         /* UEFI firmware random seed */
> +       unsigned long capsule;          /* EFI capsule table */

Can we get rid of all this global stuff? If it is too tedious to go
over the config table array, we could keep it I guess (but please
don't call capsule-pstore specific things 'capsule', this is confusing
the hell out of me)

>         efi_get_time_t *get_time;
>         efi_set_time_t *set_time;
>         efi_get_wakeup_time_t *get_wakeup_time;
> @@ -1401,6 +1402,9 @@ extern int efi_capsule_supported(efi_guid_t guid, u32 flags,
>  extern int efi_capsule_update(efi_capsule_header_t *capsule,
>                               struct page **pages);
>
> +extern efi_capsule_header_t **efi_capsule_lookup(efi_guid_t guid,
> +                             uint32_t *nr_caps);
> +
>  #ifdef CONFIG_EFI_RUNTIME_MAP
>  int efi_runtime_map_init(struct kobject *);
>  int efi_get_runtime_map_size(void);
> --
> 2.9.0.GIT
>
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux