Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag

[Daniel Kiper <daniel.kiper@oracle.com>]

On Mon, May 19, 2014 at 04:58:32PM +0100, David Vrabel wrote:
> On 16/05/14 21:41, Daniel Kiper wrote:
> > Introduce EFI_DIRECT flag. If it is set this means that Linux
> > Kernel has direct access to EFI infrastructure. If not then
> > kernel runs on EFI platform but it has not direct control
> > on EFI stuff. This functionality is used in Xen dom0.
>
> This is backwards.  It should flag should indicate the weird platform
> not the standard, default one.

Do you wish EFI_NO_DIRECT? I understand your idea but I would like to avoid this
name because it is quite difficult to read e.g. !efi_enabled(EFI_NO_DIRECT).
You must think a bit to know what is going on. However, maybe you have a better idea?

> You also need to explain why this is needed rather than presenting a
> more complete EFI interface to PV guests.  And you should explain why
> each use is necessary.

OK.

> > +static void __init __iomem *efi_early_ioremap(resource_size_t phys_addr,
> > +							unsigned long size)
> > +{
> > +	if (efi_enabled(EFI_DIRECT))
> > +		return early_ioremap(phys_addr, size);
> > +
> > +	return (__force void __iomem *)phys_addr;
> > +}
>
> Er...  Perhaps you mean BUG_ON(!efi_enabled(EFI_DIRECT))? Or return NULL?

It is correct. As I said earlier: in case of !efi_enabled(EFI_DIRECT) some
structures are created artificially and they live in virtual address space.
So that is why they should not be mapped.

> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -893,12 +893,13 @@ extern int __init efi_setup_pcdp_console(char *);
> >   * possible, remove EFI-related code altogether.
> >   */
> >  #define EFI_BOOT		0	/* Were we booted from EFI? */
> > -#define EFI_SYSTEM_TABLES	1	/* Can we use EFI system tables? */
> > -#define EFI_CONFIG_TABLES	2	/* Can we use EFI config tables? */
> > -#define EFI_RUNTIME_SERVICES	3	/* Can we use runtime services? */
> > -#define EFI_MEMMAP		4	/* Can we use EFI memory map? */
> > -#define EFI_64BIT		5	/* Is the firmware 64-bit? */
> > -#define EFI_ARCH_1		6	/* First arch-specific bit */
> > +#define EFI_DIRECT		1	/* Can we access EFI directly? */
> > +#define EFI_SYSTEM_TABLES	2	/* Can we use EFI system tables? */
> > +#define EFI_CONFIG_TABLES	3	/* Can we use EFI config tables? */
> > +#define EFI_RUNTIME_SERVICES	4	/* Can we use runtime services? */
> > +#define EFI_MEMMAP		5	/* Can we use EFI memory map? */
> > +#define EFI_64BIT		6	/* Is the firmware 64-bit? */
> > +#define EFI_ARCH_1		7	/* First arch-specific bit */
>
> Unnecessary shuffling of these values.  Why didn't you stick it after
> EFI_64BIT?

I was going to have EFI_DIRECT close to EFI_BOOT which is quite generic
and platform independent name like EFI_BOOT. However, I do not insist
on having it in that place.

Daniel