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

2014-05-22 Thread Matt Fleming 
On Mon, 19 May, at 11:02:55PM, Daniel Kiper wrote:
> 
> 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.
 
So, exploring Jan's idea, is it not possible to store the physical
address and have early_ioremap() just work? Even if they're mapping in
virtual address space they must have a corresponding physical address.

We really need to be keeping these kinds of special code paths to a
minimum. Unless absolutely necessary there should be just one way to do
things.

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

Right, please don't shuffle these bits.

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-05-19 Thread Jan Beulich 
>>> On 19.05.14 at 22:46,  wrote:
> On Mon, May 19, 2014 at 02:30:45PM +0100, Jan Beulich wrote:
>> >>> On 16.05.14 at 22:41,  wrote:
>> > @@ -457,6 +460,21 @@ void __init efi_free_boot_services(void)
>> >efi_unmap_memmap();
>> >  }
>> >
>> > +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;
>>
>> Now that surely needs some explanation: I can't see how this can
>> ever be correct, Xen or not being completely irrelevant.
> 
> I hope that efi_enabled(EFI_DIRECT) is obvious. However, 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. If you wish I could add relevant comment here.

That would be the very minimum I suppose. But I wonder whether
you wouldn't be better off storing their physical addresses in the
first place (and then decide whether you can stay with early_ioremap()
or want/need to use early_memremap() if !EFI_DIRECT).

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-05-19 Thread Daniel Kiper 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-05-19 Thread Daniel Kiper 
On Mon, May 19, 2014 at 02:30:45PM +0100, Jan Beulich wrote:
> >>> On 16.05.14 at 22:41,  wrote:
> > @@ -457,6 +460,21 @@ void __init efi_free_boot_services(void)
> > efi_unmap_memmap();
> >  }
> >
> > +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;
>
> Now that surely needs some explanation: I can't see how this can
> ever be correct, Xen or not being completely irrelevant.

I hope that efi_enabled(EFI_DIRECT) is obvious. However, 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. If you wish I could add relevant comment here.

Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-05-19 Thread David Vrabel 
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.

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.

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

> --- 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_TABLES1   /* Can we use EFI system tables? */
> -#define EFI_CONFIG_TABLES2   /* 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_64BIT5   /* 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_TABLES2   /* Can we use EFI system tables? */
> +#define EFI_CONFIG_TABLES3   /* 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_64BIT6   /* 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?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-05-19 Thread Jan Beulich 
>>> On 16.05.14 at 22:41,  wrote:
> @@ -457,6 +460,21 @@ void __init efi_free_boot_services(void)
>   efi_unmap_memmap();
>  }
>  
> +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;

Now that surely needs some explanation: I can't see how this can
ever be correct, Xen or not being completely irrelevant.

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-05-16 Thread Daniel Kiper 
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.

Signed-off-by: Daniel Kiper 
---
 arch/x86/kernel/setup.c |2 ++
 arch/x86/platform/efi/efi.c |   58 ---
 include/linux/efi.h |   13 +-
 3 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 09c76d2..f41f648 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -926,9 +926,11 @@ void __init setup_arch(char **cmdline_p)
if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
 "EL32", 4)) {
set_bit(EFI_BOOT, &efi.flags);
+   set_bit(EFI_DIRECT, &efi.flags);
} else if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
 "EL64", 4)) {
set_bit(EFI_BOOT, &efi.flags);
+   set_bit(EFI_DIRECT, &efi.flags);
set_bit(EFI_64BIT, &efi.flags);
}
 
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 3781dd3..7fcef06 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -352,6 +352,9 @@ int __init efi_memblock_x86_reserve_range(void)
struct efi_info *e = &boot_params.efi_info;
unsigned long pmap;
 
+   if (!efi_enabled(EFI_DIRECT))
+   return 0;
+
 #ifdef CONFIG_X86_32
/* Can't handle data above 4GB at this time */
if (e->efi_memmap_hi) {
@@ -457,6 +460,21 @@ void __init efi_free_boot_services(void)
efi_unmap_memmap();
 }
 
+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;
+}
+
+static void __init efi_early_iounmap(void __iomem *addr, unsigned long size)
+{
+   if (efi_enabled(EFI_DIRECT))
+   early_iounmap(addr, size);
+}
+
 static int __init efi_systab_init(void *phys)
 {
if (efi_enabled(EFI_64BIT)) {
@@ -469,8 +487,8 @@ static int __init efi_systab_init(void *phys)
if (!data)
return -ENOMEM;
}
-   systab64 = early_ioremap((unsigned long)phys,
-sizeof(*systab64));
+   systab64 = efi_early_ioremap((unsigned long)phys,
+   sizeof(*systab64));
if (systab64 == NULL) {
pr_err("Couldn't map the system table!\n");
if (data)
@@ -506,7 +524,7 @@ static int __init efi_systab_init(void *phys)
   systab64->tables;
tmp |= data ? data->tables : systab64->tables;
 
-   early_iounmap(systab64, sizeof(*systab64));
+   efi_early_iounmap(systab64, sizeof(*systab64));
if (data)
early_iounmap(data, sizeof(*data));
 #ifdef CONFIG_X86_32
@@ -518,8 +536,8 @@ static int __init efi_systab_init(void *phys)
} else {
efi_system_table_32_t *systab32;
 
-   systab32 = early_ioremap((unsigned long)phys,
-sizeof(*systab32));
+   systab32 = efi_early_ioremap((unsigned long)phys,
+   sizeof(*systab32));
if (systab32 == NULL) {
pr_err("Couldn't map the system table!\n");
return -ENOMEM;
@@ -539,7 +557,7 @@ static int __init efi_systab_init(void *phys)
efi_systab.nr_tables = systab32->nr_tables;
efi_systab.tables = systab32->tables;
 
-   early_iounmap(systab32, sizeof(*systab32));
+   efi_early_iounmap(systab32, sizeof(*systab32));
}
 
efi.systab = &efi_systab;
@@ -619,13 +637,16 @@ static int __init efi_runtime_init(void)
 * address of several of the EFI runtime functions, needed to
 * set the firmware into virtual mode.
 */
-   if (efi_enabled(EFI_64BIT))
-   rv = efi_runtime_init64();
-   else
-   rv = efi_runtime_init32();
 
-   if (rv)
-   return rv;
+   if (efi_enabled(EFI_DIRECT)) {
+   if (efi_enabled(EFI_64BIT))
+   rv = efi_runtime_init64();
+   else
+   rv = efi_runtime_init32();
+
+   if (rv)
+   return rv;
+   }
 
set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 
@@ -634,6 +655,9 @@ static int __init efi_runtime_init(void)
 
 static int _