[PATCH] [v2] x86_64/efi: Use all 64 bit of efi_memmap in setup_e820()

[PATCH] [v2] x86_64/efi: Use all 64 bit of efi_memmap in setup_e820()

[Dmitry Skorodumov]

The efi_info structure stores low 32 bits of memory map
in efi_memmap and high 32 bits in efi_memmap_hi.

While constructing pointer in the setup_e820(), need
to take into account all 64 bits of the pointer.

It is because on 64bit machine the function
efi_get_memory_map() may return full 64bit pointer and before
the patch that pointer was truncated.

Signed-off-by: Dmitry Skorodumov <sdmitry-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

tested in Parallels virtual machine with more then 3GB of memory
---
 arch/x86/boot/compressed/eboot.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 2c82bd1..9108fe5 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1184,6 +1184,12 @@ static efi_status_t setup_e820(struct boot_params *params,
 	u32 nr_entries;
 	u32 nr_desc;
 	int i;
+	unsigned long m;
+
+	m = efi->efi_memmap;
+#ifdef CONFIG_X86_64
+	m |= (u64)efi->efi_memmap_hi << 32;
+#endif
 
 	nr_entries = 0;
 	nr_desc = efi->efi_memmap_size / efi->efi_memdesc_size;
@@ -1191,7 +1197,6 @@ static efi_status_t setup_e820(struct boot_params *params,
 	for (i = 0; i < nr_desc; i++) {
 		efi_memory_desc_t *d;
 		unsigned int e820_type = 0;
-		unsigned long m = efi->efi_memmap;
 
 		d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
 		switch (d->type) {
-- 
1.7.9.1

Re: [PATCH] [v2] x86_64/efi: Use all 64 bit of efi_memmap in setup_e820()

[Matt Fleming]

On Wed, 22 Jul, at 07:31:08PM, Dmitry Skorodumov wrote:
> The efi_info structure stores low 32 bits of memory map
> in efi_memmap and high 32 bits in efi_memmap_hi.
> 
> While constructing pointer in the setup_e820(), need
> to take into account all 64 bits of the pointer.
> 
> It is because on 64bit machine the function
> efi_get_memory_map() may return full 64bit pointer and before
> the patch that pointer was truncated.
> 
> Signed-off-by: Dmitry Skorodumov <sdmitry-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> 
> tested in Parallels virtual machine with more then 3GB of memory

When you say "tested", you mean that it doesn't boot correctly without
your patch but does boot correctly with it applied? What I'm really
asking is: are we sure that your setup triggered this new code?

> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 2c82bd1..9108fe5 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -1184,6 +1184,12 @@ static efi_status_t setup_e820(struct boot_params *params,
>  	u32 nr_entries;
>  	u32 nr_desc;
>  	int i;
> +	unsigned long m;
> +
> +	m = efi->efi_memmap;
> +#ifdef CONFIG_X86_64
> +	m |= (u64)efi->efi_memmap_hi << 32;
> +#endif
>  
>  	nr_entries = 0;
>  	nr_desc = efi->efi_memmap_size / efi->efi_memdesc_size;
> @@ -1191,7 +1197,6 @@ static efi_status_t setup_e820(struct boot_params *params,
>  	for (i = 0; i < nr_desc; i++) {
>  		efi_memory_desc_t *d;
>  		unsigned int e820_type = 0;
> -		unsigned long m = efi->efi_memmap;
>  
>  		d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
>  		switch (d->type) {

Is there a reason that adding efi->efi_memmap_hi can't be done inside
this for loop? Gcc should be smart enough to hoist this calculation out
of the loop.

-- 
Matt Fleming, Intel Open Source Technology Center

RE:[PATCH] [v2] x86_64/efi: Use all 64 bit of efi_memmap in setup_e820()

[Dmitry Skorodumov]

Matt Fleming <matt@...> writes:

> On Wed, 22 Jul, at 07:31:08PM, Dmitry Skorodumov wrote:
> > The efi_info structure stores low 32 bits of memory map
> > in efi_memmap and high 32 bits in efi_memmap_hi.
> > 
> > While constructing pointer in the setup_e820(), need
> > to take into account all 64 bits of the pointer.

> > tested in Parallels virtual machine with more then 3GB of memory

> When you say "tested", you mean that it doesn't boot correctly without
> your patch but does boot correctly with it applied? What I'm really
> asking is: are we sure that your setup triggered this new code?

Yes, while debugging I added a lot of logging to trace the problem.
Parallels VM doesn't boot correctly without the patch.

I believe that any  EDK2-based efi-bios will not work also,
though I heard that OVMF uses own linux boot loader..
It is because the memory for memmap was allocated from EFI_LOADER_DATA pool -
efi_call_early(allocate_pool, EFI_LOADER_DATA, map_size... );

It is possible to kludge the problem by patching EFI-bios of the machine
to allocate  EFI_LOADER_DATA-memory below the 4GB space,
but I think that fixing setup_e820() is the right thing.

> > +	m = efi->efi_memmap;
> > +#ifdef CONFIG_X86_64
> > +	m |= (u64)efi->efi_memmap_hi << 32;
> > +#endif
..
> >  	for (i = 0; i < nr_desc; i++) {
> > -		unsigned long m = efi->efi_memmap;
> >  		d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));

> Is there a reason that adding efi->efi_memmap_hi can't be done inside
> this for loop? Gcc should be smart enough to hoist this calculation out
> of the loop.

Smart from its side.. I'll correct this and resend the patch.--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [v2] x86_64/efi: Use all 64 bit of efi_memmap in setup_e820()

[Matt Fleming]

On Tue, 28 Jul, at 01:18:37PM, Dmitry Skorodumov wrote:
> Matt Fleming <matt@...> writes:
> 
> > On Wed, 22 Jul, at 07:31:08PM, Dmitry Skorodumov wrote:
> > > The efi_info structure stores low 32 bits of memory map
> > > in efi_memmap and high 32 bits in efi_memmap_hi.
> > > 
> > > While constructing pointer in the setup_e820(), need
> > > to take into account all 64 bits of the pointer.
> 
> > > tested in Parallels virtual machine with more then 3GB of memory
> 
> > When you say "tested", you mean that it doesn't boot correctly without
> > your patch but does boot correctly with it applied? What I'm really
> > asking is: are we sure that your setup triggered this new code?
> 
> Yes, while debugging I added a lot of logging to trace the problem.
> Parallels VM doesn't boot correctly without the patch.
 
OK cool, I would definitely include this in the patch commit message.

> I believe that any  EDK2-based efi-bios will not work also,
> though I heard that OVMF uses own linux boot loader..
> It is because the memory for memmap was allocated from EFI_LOADER_DATA pool -
> efi_call_early(allocate_pool, EFI_LOADER_DATA, map_size... );
> 
> It is possible to kludge the problem by patching EFI-bios of the machine
> to allocate  EFI_LOADER_DATA-memory below the 4GB space,
> but I think that fixing setup_e820() is the right thing.
 
Yes your fix looks correct to me, I just wanted to know whether you hit
the bug with a real workload or via code inspection.

> > > +	m = efi->efi_memmap;
> > > +#ifdef CONFIG_X86_64
> > > +	m |= (u64)efi->efi_memmap_hi << 32;
> > > +#endif
> ..
> > >  	for (i = 0; i < nr_desc; i++) {
> > > -		unsigned long m = efi->efi_memmap;
> > >  		d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
> 
> > Is there a reason that adding efi->efi_memmap_hi can't be done inside
> > this for loop? Gcc should be smart enough to hoist this calculation out
> > of the loop.
> 
> Smart from its side.. I'll correct this and resend the patch.

Great, thanks.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] [v2] x86_64/efi: Use all 64 bit of efi_memmap in setup_e820()

[Dmitry Skorodumov]

> On Wed, 22 Jul, at 07:31:08PM, Dmitry Skorodumov wrote:
> > The efi_info structure stores low 32 bits of memory map
> > in efi_memmap and high 32 bits in efi_memmap_hi.
> > 
> > While constructing pointer in the setup_e820(), need
> > to take into account all 64 bits of the pointer.

> > tested in Parallels virtual machine with more then 3GB of memory

> When you say "tested", you mean that it doesn't boot correctly without
> your patch but does boot correctly with it applied? What I'm really
> asking is: are we sure that your setup triggered this new code?

Yes, while debugging I added a lot of logging to trace the problem.
Parallels VM doesn't boot correctly without the patch.

I believe that any  EDK2-based efi-bios will not work also,
though I heard that OVMF uses own linux boot loader..
It is because the memory for memmap was allocated from EFI_LOADER_DATA pool -
efi_call_early(allocate_pool, EFI_LOADER_DATA, map_size... );

It is possible to kludge the problem by patching EFI-bios of the machine
to allocate  EFI_LOADER_DATA-memory below the 4GB space,
but I think that fixing setup_e820() is the right thing.

> > +   m = efi->efi_memmap;
> > +#ifdef CONFIG_X86_64
> > +   m |= (u64)efi->efi_memmap_hi << 32;
> > +#endif
..
> >      for (i = 0; i < nr_desc; i++) {
> > -           unsigned long m = efi->efi_memmap;
> >              d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));

> Is there a reason that adding efi->efi_memmap_hi can't be done inside
> this for loop? Gcc should be smart enough to hoist this calculation out
> of the loop.

Smart from its side.. I'll correct this and resend the patch.

PS: Looks my mailer have not sent the message to the list.
Resending via gmane-web, sorry for potential duplicate