Re: [Xen-devel] [PATCH v4 3/5] xen: Put EFI machinery in place

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.05.14 at 22:41, <daniel.kiper@xxxxxxxxxx> wrote:
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -240,4 +240,7 @@ config XEN_MCE_LOG
>  config XEN_HAVE_PVMMU
>         bool
>  
> +config XEN_EFI
> +     def_bool X86_64 && EFI

Constructs like this are bogus - they needlessly add a line to .config
when the expression evaluates to false.

config XEN_EFI
        def_bool y
        depends on X86_64 && EFI

is what avoids this.

> +static efi_status_t xen_efi_get_variable(efi_char16_t *name,
> +                                      efi_guid_t *vendor,
> +                                      u32 *attr,
> +                                      unsigned long *data_size,
> +                                      void *data)
> +{
> +     int err;
> +     DECLARE_CALL(get_variable);
> +
> +     set_xen_guest_handle(call.u.get_variable.name, name);
> +     BUILD_BUG_ON(sizeof(*vendor) !=
> +                  sizeof(call.u.get_variable.vendor_guid));
> +     memcpy(&call.u.get_variable.vendor_guid, vendor, sizeof(*vendor));
> +     call.u.get_variable.size = *data_size;
> +     set_xen_guest_handle(call.u.get_variable.data, data);
> +     err = HYPERVISOR_dom0_op(&op);
> +     if (err)
> +             return EFI_UNSUPPORTED;
> +
> +     *data_size = call.u.get_variable.size;
> +     *attr = call.misc; /* misc in struction is U32 variable*/

Iirc attr is an optional output, i.e. can be NULL, which hence needs
to be checked for here. I remember that this was broken in the
original EFI patch in our trees, so perhaps it would be good for you
to re-check against recent sources of ours for eventual other bug
fixes.

> +static efi_status_t xen_efi_query_variable_info(u32 attr,
> +                                             u64 *storage_space,
> +                                             u64 *remaining_space,
> +                                             u64 *max_variable_size)
> +{
> +     int err;
> +     DECLARE_CALL(query_variable_info);
> +
> +     if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
> +             return EFI_UNSUPPORTED;
> +

Here's a similar case - there is

        call.u.query_variable_info.attr = attr;

missing.

> +static efi_char16_t vendor[100] __initdata;
> +static const efi_char16_t unknown[] __initconst =
> +                     {'U', 'N', 'K', 'N', 'O', 'W', 'N', '\0'};

If you enforced -fshort-wchar for the file's compilation, this could be
written in a more legible manner (and probably you wouldn't need a
named variable at all).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel