On Thu, 20 Feb 2020 at 19:30, Leif Lindholm <leif@xxxxxxxxxxxx> wrote:
>
> On Wed, Feb 19, 2020 at 16:24:40 +0100, Ard Biesheuvel wrote:
> > The efi_get_fdt_params() routine uses the early OF device tree
> > traversal helpers, that iterate over each node in the DT and invoke
> > a caller provided callback that can inspect the node's contents and
> > look for the required data. This requires a special param struct to
> > be passed around, with pointers into param enumeration structs that
> > contain (and duplicate) property names and offsets into yet another
> > struct that carries the collected data.
> >
> > Since we know the data we look for is either under /hypervisor/uefi
> > or under /chosen, it is much simpler to use the libfdt routines
> > directly, and try to grab a reference to either node directly, and
> > read each property in sequence.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > ---
> > drivers/firmware/efi/fdtparams.c | 203 ++++++++------------
> > 1 file changed, 85 insertions(+), 118 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/fdtparams.c b/drivers/firmware/efi/fdtparams.c
> > index 7a384b307c56..23af4062e913 100644
> > --- a/drivers/firmware/efi/fdtparams.c
> > +++ b/drivers/firmware/efi/fdtparams.c
> > @@ -5,154 +5,121 @@
> > #include <linux/module.h>
> > #include <linux/init.h>
> > #include <linux/efi.h>
> > -#include <linux/of.h>
> > +#include <linux/libfdt.h>
> > #include <linux/of_fdt.h>
> >
> > -#include <asm/early_ioremap.h>
> > +#include <asm/unaligned.h>
> >
> > -#define UEFI_PARAM(name, prop, field) \
> > - { \
> > - { name }, \
> > - { prop }, \
> > - offsetof(struct efi_fdt_params, field), \
> > - sizeof_field(struct efi_fdt_params, field) \
> > - }
> > -
> > -struct efi_fdt_params {
> > - u64 system_table;
> > - u64 mmap;
> > - u32 mmap_size;
> > - u32 desc_size;
> > - u32 desc_ver;
> > -};
> > -
> > -struct params {
> > - const char name[32];
> > - const char propname[32];
> > - int offset;
> > - int size;
> > +enum {
> > + SYSTAB,
> > + MMBASE,
> > + MMSIZE,
> > + DCSIZE,
> > + DCVERS,
> > };
> >
> > -static __initdata struct params fdt_params[] = {
> > - UEFI_PARAM("System Table", "linux,uefi-system-table", system_table),
> > - UEFI_PARAM("MemMap Address", "linux,uefi-mmap-start", mmap),
> > - UEFI_PARAM("MemMap Size", "linux,uefi-mmap-size", mmap_size),
> > - UEFI_PARAM("MemMap Desc. Size", "linux,uefi-mmap-desc-size", desc_size),
> > - UEFI_PARAM("MemMap Desc. Version", "linux,uefi-mmap-desc-ver", desc_ver)
> > +static __initconst const char name[][22] = {
>
> I was going to complain about that 22, and I still think it looks a
> bit iffy, but I can't find a compiler that doesn't throw a warning if
> the value changes, so I guess I'm being over sensitive.
>
Yeah, 32 is just as arbitrary, and wastes more space :-)
> > + [SYSTAB] = "System Table ",
> > + [MMBASE] = "MemMap Address ",
> > + [MMSIZE] = "MemMap Size ",
> > + [DCSIZE] = "MemMap Desc. Size ",
> > + [DCVERS] = "MemMap Desc. Version ",
> > };
> >
> > -static __initdata struct params xen_fdt_params[] = {
> > - UEFI_PARAM("System Table", "xen,uefi-system-table", system_table),
> > - UEFI_PARAM("MemMap Address", "xen,uefi-mmap-start", mmap),
> > - UEFI_PARAM("MemMap Size", "xen,uefi-mmap-size", mmap_size),
> > - UEFI_PARAM("MemMap Desc. Size", "xen,uefi-mmap-desc-size", desc_size),
> > - UEFI_PARAM("MemMap Desc. Version", "xen,uefi-mmap-desc-ver", desc_ver)
> > -};
> > -
> > -#define EFI_FDT_PARAMS_SIZE ARRAY_SIZE(fdt_params)
> > -
> > -static __initdata struct {
> > - const char *uname;
> > - const char *subnode;
> > - struct params *params;
> > +static __initconst const struct {
> > + const char path[17];
> > + const char params[5][26];
>
> And I guess the same is true here, but the 5 *could* theoretically be
> replaced by a compile-time calculation of DCVERS - SYSTAB + 1. That
> may not be less error prone however. Finishing the enum on a
> somethingMAX entry might make it slightly better?
>
Yeah that make sense - I'll change that.
> But the error checking isn't really what I'm after, but getting rid of
> the opaqueness. Could we have comments about which specific string
> lengths we're matching against here for us bears of very little brain?
>
Sure.
> > } dt_params[] = {
> > - { "hypervisor", "uefi", xen_fdt_params },
> > - { "chosen", NULL, fdt_params },
> > -};
> > -
> > -struct param_info {
> > - int found;
> > - void *params;
> > - const char *missing;
> > +#ifdef CONFIG_XEN
> > + {
> > + "/hypervisor/uefi",
> > + {
> > + [SYSTAB] = "xen,uefi-system-table",
> > + [MMBASE] = "xen,uefi-mmap-start",
> > + [MMSIZE] = "xen,uefi-mmap-size",
> > + [DCSIZE] = "xen,uefi-mmap-desc-size",
> > + [DCVERS] = "xen,uefi-mmap-desc-ver",
> > + }
> > + },
> > +#endif
> > + {
> > + "/chosen",
> > + {
> > + [SYSTAB] = "linux,uefi-system-table",
> > + [MMBASE] = "linux,uefi-mmap-start",
> > + [MMSIZE] = "linux,uefi-mmap-size",
> > + [DCSIZE] = "linux,uefi-mmap-desc-size",
> > + [DCVERS] = "linux,uefi-mmap-desc-ver",
> > + }
> > + }
> > };
> >
> > -static int __init __find_uefi_params(unsigned long node,
> > - struct param_info *info,
> > - struct params *params)
> > +static int __init efi_get_fdt_prop(const void *fdt, int node, int dtp, int pp,
> > + void *var, int size)
> > {
> > const void *prop;
> > - void *dest;
> > + int len;
> > u64 val;
> > - int i, len;
> >
> > - for (i = 0; i < EFI_FDT_PARAMS_SIZE; i++) {
> > - prop = of_get_flat_dt_prop(node, params[i].propname, &len);
> > - if (!prop) {
> > - info->missing = params[i].name;
> > - return 0;
> > - }
> > -
> > - dest = info->params + params[i].offset;
> > - info->found++;
> > + prop = fdt_getprop(fdt, node, dt_params[dtp].params[pp], &len);
> > + if (!prop) {
> > + pr_err("Can't find property '%s' in device tree!\n",
> > + dt_params[dtp].params[pp]);
> > + return 1;
> > + }
> >
> > - val = of_read_number(prop, len / sizeof(u32));
> > + val = (len == 4) ? (u64)be32_to_cpup(prop) : get_unaligned_be64(prop);
> >
> > - if (params[i].size == sizeof(u32))
> > - *(u32 *)dest = val;
> > - else
> > - *(u64 *)dest = val;
> > + if (size == 8)
> > + *(u64 *)var = val;
> > + else
> > + *(u32 *)var = (val <= U32_MAX) ? val : U32_MAX; // saturate
> >
> > - if (efi_enabled(EFI_DBG))
> > - pr_info(" %s: 0x%0*llx\n", params[i].name,
> > - params[i].size * 2, val);
> > - }
> > + if (efi_enabled(EFI_DBG))
> > + pr_info(" %s: 0x%0*llx\n", name[pp], size * 2, val);
> >
> > - return 1;
> > + return 0;
> > }
> >
> > -static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
> > - int depth, void *data)
> > +u64 __init efi_get_fdt_params(struct efi_memory_map_data *mm)
> > {
> > - struct param_info *info = data;
> > + const void *fdt = initial_boot_params;
> > + unsigned long systab;
> > + struct {
> > + void *var;
> > + int size;
> > + } target[] = {
> > + [SYSTAB] = { &systab, sizeof(systab) },
> > + [MMBASE] = { &mm->phys_map, sizeof(mm->phys_map) },
> > + [MMSIZE] = { &mm->size, sizeof(mm->size) },
> > + [DCSIZE] = { &mm->desc_size, sizeof(mm->desc_size) },
> > + [DCVERS] = { &mm->desc_version, sizeof(mm->desc_version) },
> > + };
> > int i;
> >
> > + BUILD_BUG_ON(ARRAY_SIZE(target) != ARRAY_SIZE(name));
> > + BUILD_BUG_ON(ARRAY_SIZE(target) != ARRAY_SIZE(dt_params[0].params));
> > +
> > for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
> > - const char *subnode = dt_params[i].subnode;
> > + int node;
> > + int j;
> >
> > - if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
> > - info->missing = dt_params[i].params[0].name;
> > + node = fdt_path_offset(fdt, dt_params[i].path);
> > + if (node < 0)
> > continue;
> > - }
> >
> > - if (subnode) {
> > - int err = of_get_flat_dt_subnode_by_name(node, subnode);
> > + if (efi_enabled(EFI_DBG))
> > + pr_info("Getting UEFI parameters from %s in DT:\n",
> > + dt_params[i].path);
> >
> > - if (err < 0)
> > + for (j = 0; j < ARRAY_SIZE(target); j++) {
> > + if (efi_get_fdt_prop(fdt, node, i, j, target[j].var,
> > + target[j].size)) {
> > return 0;
> > -
> > - node = err;
> > + }
> > }
> > -
> > - return __find_uefi_params(node, info, dt_params[i].params);
> > + return systab;
> > }
> > -
> > + pr_info("UEFI not found.\n");
> > return 0;
> > }
> > -
> > -u64 __init efi_get_fdt_params(struct efi_memory_map_data *memmap)
> > -{
> > - struct efi_fdt_params params;
> > - struct param_info info;
> > - int ret;
> > -
> > - pr_info("Getting EFI parameters from FDT:\n");
> > -
> > - info.found = 0;
> > - info.params = ¶ms;
> > -
> > - ret = of_scan_flat_dt(fdt_find_uefi_params, &info);
> > - if (!info.found) {
> > - pr_info("UEFI not found.\n");
> > - return 0;
> > - } else if (!ret) {
> > - pr_err("Can't find '%s' in device tree!\n", info.missing);
> > - return 0;
> > - }
> > -
> > - memmap->desc_version = params.desc_ver;
> > - memmap->desc_size = params.desc_size;
> > - memmap->size = params.mmap_size;
> > - memmap->phys_map = params.mmap;
> > -
> > - return params.system_table;
> > -}
> > --
> > 2.17.1
> >
