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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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

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