Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag
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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag
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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag
>>> 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag
On 19.05.14 at 22:46, daniel.ki...@oracle.com wrote:
On Mon, May 19, 2014 at 02:30:45PM +0100, Jan Beulich wrote:
On 16.05.14 at 22:41, daniel.ki...@oracle.com 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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag
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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag
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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag
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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag
>>> 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag
On 16.05.14 at 22:41, daniel.ki...@oracle.com 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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag
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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag
On Mon, May 19, 2014 at 02:30:45PM +0100, Jan Beulich wrote:
On 16.05.14 at 22:41, daniel.ki...@oracle.com 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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag
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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
