[PATCH v2 3/4] efi_loader: add an EFI variable with the file contents

[Ilias Apalodimas]

Hi Heinrich,

[...]

> >   {
> >       int ret = 0;
> >
> > diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> > index c8f7a88ba8db..99ad1f35d8f1 100644
> > --- a/lib/efi_loader/efi_runtime.c
> > +++ b/lib/efi_loader/efi_runtime.c
> > @@ -130,6 +130,8 @@ efi_status_t efi_init_runtime_supported(void)
> >                               EFI_RT_SUPPORTED_CONVERT_POINTER;
> >
> >       if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> > +             int s = 0;
>
> u8 s = 0;
> should be enough.

Sure,

> >               }
> > +             ret = efi_set_variable_int(u"VarToFile",
> > +                                        &efi_guid_efi_rt_var_file,
> > +                                        EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                                        EFI_VARIABLE_RUNTIME_ACCESS,
>
> Why is the variable set here? A comment would be helpful.

We need to set it so it shows up in the efivarfs, fo linux, and the
variable list in general for other OSes. Otherwise, people won't be
able to read it, unless they specifically search for it. I'll add a
comment though

> If you set it for protection, shouldn't it be EFI_VARIABLE_READ_ONLY?
>
> An alternative would be to return EFI_INVALID_PARAMETER in
> efi_set_variable_int for any attempt to set a variable with GUID
> efi_guid_efi_rt_var_file.

It's not volatile, so it will be treated as RO at runtime. But I'll
add EFI_VARIABLE_READ_ONLY as well. I prefer using the existing EFI
APIs instead of treating GUIDs specially

[...]

> > +/**
> > + * efi_var_collect_mem() - Copy EFI variables matching attributes mask from
> > + *                         efi_var_buf
> > + *
> > + * @buf:     buffer containing variable collection
> > + * @lenp:    buffer length
> > + * @mask:    mask of matched attributes
> > + *
> > + * Return:   Status code
> > + */
> > +efi_status_t __efi_runtime
> > +efi_var_collect_mem(struct efi_var_file *buf, efi_uintn_t *lenp, u32 mask)
> > +{
> > +     static struct efi_var_file __efi_runtime_data hdr = {
> > +             .magic = EFI_VAR_FILE_MAGIC,
> > +     };
> > +     struct efi_var_entry *last, *var, *var_to;
> > +
> > +     hdr.length = sizeof(struct efi_var_file);
> > +
> > +     var = efi_var_buf->var;
> > +     last = (struct efi_var_entry *)
> > +            ((uintptr_t)efi_var_buf + efi_var_buf->length);
> > +     if (buf)
> > +             var_to = buf->var;
> > +
> > +     while (var < last) {
> > +             u32 len;
> > +             struct efi_var_entry *var_next;
> > +
> > +             efi_var_mem_compare(var, NULL, u"", &var_next);
>
> On IRC you suggested to make u16_strlen __efi_runtime_code to replace
> this efi_var_mem_compare() call and avoid the modifications of said
> function. That might be more readable.

Yes it will, I'll change that on v3. That function will also help us
clean various open-coded sequences of length calculations.

FWIW I just managed to run FWTS over this -- and I also fixed
QueryVariableInfo at runtime
We had 22 tests passing and 1 failing. I am still trying to figure out
what failed, but I'll fix that as well for v3

>
> Best regards
>
> Heinrich
>
> > +             len = (uintptr_t)var_next - (uintptr_t)var;
> > +
> > +             if ((var->attr & mask) != mask) {

[...]

Thanks
/Ilias