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

[Jan Beulich]

>>> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/