[PATCH v2] fix setup_efi_pci()

[PATCH v2] fix setup_efi_pci()

[Jan Beulich]

This fixes two issues:
- wrong memory type used for allocation intended to persist post-boot
- four similar build warnings on 32-bit (casts between different size
  pointers and integers)

Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
---
v2: Drop the change that was already applied separately (commit
    886d751a2ea99a160f2d0a472231566d9cb0cf58 "x86, efi: correct
    precedence of operators in setup_efi_pci")

---
 arch/x86/boot/compressed/eboot.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

--- 3.8-rc4/arch/x86/boot/compressed/eboot.c
+++ 3.8-rc4-x86-EFI-PCI-ROMs/arch/x86/boot/compressed/eboot.c
@@ -256,10 +256,10 @@ static efi_status_t setup_efi_pci(struct
 	int i;
 	struct setup_data *data;
 
-	data = (struct setup_data *)params->hdr.setup_data;
+	data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
 
 	while (data && data->next)
-		data = (struct setup_data *)data->next;
+		data = (struct setup_data *)(unsigned long)data->next;
 
 	status = efi_call_phys5(sys_table->boottime->locate_handle,
 				EFI_LOCATE_BY_PROTOCOL, &pci_proto,
@@ -311,7 +311,7 @@ static efi_status_t setup_efi_pci(struct
 		size = pci->romsize + sizeof(*rom);
 
 		status = efi_call_phys3(sys_table->boottime->allocate_pool,
-				EFI_LOADER_DATA, size, &rom);
+					EFI_RUNTIME_SERVICES_DATA, size, &rom);
 
 		if (status != EFI_SUCCESS)
 			continue;
@@ -345,9 +345,9 @@ static efi_status_t setup_efi_pci(struct
 		memcpy(rom->romdata, pci->romimage, pci->romsize);
 
 		if (data)
-			data->next = (uint64_t)rom;
+			data->next = (unsigned long)rom;
 		else
-			params->hdr.setup_data = (uint64_t)rom;
+			params->hdr.setup_data = (unsigned long)rom;
 
 		data = (struct setup_data *)rom;
 

Re: [PATCH v2] fix setup_efi_pci()

[Matt Fleming]

On Fri, 2013-01-18 at 12:35 +0000, Jan Beulich wrote:
> This fixes two issues:
> - wrong memory type used for allocation intended to persist post-boot

[...]

> @@ -311,7 +311,7 @@ static efi_status_t setup_efi_pci(struct
>  		size = pci->romsize + sizeof(*rom);
>  
>  		status = efi_call_phys3(sys_table->boottime->allocate_pool,
> -				EFI_LOADER_DATA, size, &rom);
> +					EFI_RUNTIME_SERVICES_DATA, size, &rom);
>  
>  		if (status != EFI_SUCCESS)
>  			continue;

I'm curious why you made this change. No one should be stealing that
region of memory because that's all handled in parse_e820_ext() - it's
marked as off limits wrt memory for the kernel's use. And the firmware
certainly shouldn't start touching it.

Have you witnessed some case where things explode without your change?

Re: [PATCH v2] fix setup_efi_pci()

[Jan Beulich]

>>> On 24.01.13 at 19:20, Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, 2013-01-18 at 12:35 +0000, Jan Beulich wrote:
>> This fixes two issues:
>> - wrong memory type used for allocation intended to persist post-boot
> 
> [...]
> 
>> @@ -311,7 +311,7 @@ static efi_status_t setup_efi_pci(struct
>>  		size = pci->romsize + sizeof(*rom);
>>  
>>  		status = efi_call_phys3(sys_table->boottime->allocate_pool,
>> -				EFI_LOADER_DATA, size, &rom);
>> +					EFI_RUNTIME_SERVICES_DATA, size, &rom);
>>  
>>  		if (status != EFI_SUCCESS)
>>  			continue;
> 
> I'm curious why you made this change. No one should be stealing that
> region of memory because that's all handled in parse_e820_ext() - it's
> marked as off limits wrt memory for the kernel's use. And the firmware
> certainly shouldn't start touching it.

According to my reading of do_add_efi_memmap(), EFI_LOADER_DATA
gets treated the same as EFI_CONVENTIONAL_MEMORY, i.e. gets
marked as available for allocation. That's also how I'd expect things
to be. I don't think parse_e820_ext() matters here at all.

> Have you witnessed some case where things explode without your change?

No, I didn't. I merely spotted this when evaluating the code for
porting over to Xen.

Jan

Re: [PATCH v2] fix setup_efi_pci()

[Matt Fleming]

On Fri, 2013-01-25 at 07:41 +0000, Jan Beulich wrote:
> >>> On 24.01.13 at 19:20, Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> > On Fri, 2013-01-18 at 12:35 +0000, Jan Beulich wrote:
> >> This fixes two issues:
> >> - wrong memory type used for allocation intended to persist post-boot
> > 
> > [...]
> > 
> >> @@ -311,7 +311,7 @@ static efi_status_t setup_efi_pci(struct
> >>  		size = pci->romsize + sizeof(*rom);
> >>  
> >>  		status = efi_call_phys3(sys_table->boottime->allocate_pool,
> >> -				EFI_LOADER_DATA, size, &rom);
> >> +					EFI_RUNTIME_SERVICES_DATA, size, &rom);
> >>  
> >>  		if (status != EFI_SUCCESS)
> >>  			continue;
> > 
> > I'm curious why you made this change. No one should be stealing that
> > region of memory because that's all handled in parse_e820_ext() - it's
> > marked as off limits wrt memory for the kernel's use. And the firmware
> > certainly shouldn't start touching it.
> 
> According to my reading of do_add_efi_memmap(), EFI_LOADER_DATA
> gets treated the same as EFI_CONVENTIONAL_MEMORY, i.e. gets
> marked as available for allocation. That's also how I'd expect things
> to be. I don't think parse_e820_ext() matters here at all.

Oh, I totally misread the flow of execution here. parse_setup_data()
seems to be missing a SETUP_PCI case.

Matthew, what guarantees do we have that the memory we've allocated for
the PCI ROMs isn't going to be reused? I suspect what's needed is for
parse_setup_data() to reserve the SETUP_PCI regions, just like you would
for any other key data structure.

It's the kernel's responsibility to protect any EFI_LOADER_DATA regions
that it needs, we shouldn't have to trick ourselves by using the
EFI_RUNTIME_SERVICES_DATA pool for allocations.

Re: [PATCH v2] fix setup_efi_pci()

[Matt Fleming]

On Fri, 2013-01-25 at 08:45 +0000, Matt Fleming wrote:
> On Fri, 2013-01-25 at 07:41 +0000, Jan Beulich wrote:
> > >>> On 24.01.13 at 19:20, Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> > > On Fri, 2013-01-18 at 12:35 +0000, Jan Beulich wrote:
> > >> This fixes two issues:
> > >> - wrong memory type used for allocation intended to persist post-boot
> > > 
> > > [...]
> > > 
> > >> @@ -311,7 +311,7 @@ static efi_status_t setup_efi_pci(struct
> > >>  		size = pci->romsize + sizeof(*rom);
> > >>  
> > >>  		status = efi_call_phys3(sys_table->boottime->allocate_pool,
> > >> -				EFI_LOADER_DATA, size, &rom);
> > >> +					EFI_RUNTIME_SERVICES_DATA, size, &rom);
> > >>  
> > >>  		if (status != EFI_SUCCESS)
> > >>  			continue;
> > > 
> > > I'm curious why you made this change. No one should be stealing that
> > > region of memory because that's all handled in parse_e820_ext() - it's
> > > marked as off limits wrt memory for the kernel's use. And the firmware
> > > certainly shouldn't start touching it.
> > 
> > According to my reading of do_add_efi_memmap(), EFI_LOADER_DATA
> > gets treated the same as EFI_CONVENTIONAL_MEMORY, i.e. gets
> > marked as available for allocation. That's also how I'd expect things
> > to be. I don't think parse_e820_ext() matters here at all.
> 
> Oh, I totally misread the flow of execution here. parse_setup_data()
> seems to be missing a SETUP_PCI case.
> 
> Matthew, what guarantees do we have that the memory we've allocated for
> the PCI ROMs isn't going to be reused? I suspect what's needed is for
> parse_setup_data() to reserve the SETUP_PCI regions, just like you would
> for any other key data structure.
> 
> It's the kernel's responsibility to protect any EFI_LOADER_DATA regions
> that it needs, we shouldn't have to trick ourselves by using the
> EFI_RUNTIME_SERVICES_DATA pool for allocations.

Meanwhile I've queued up the following patch. Jan, please shout if
you're unhappy with me doing so,

---

>From bc754790f932f3466ec521ee792da2791e7003ae Mon Sep 17 00:00:00 2001
From: Jan Beulich <JBeulich-IBi9RG/b67k@public.gmane.org>
Date: Fri, 18 Jan 2013 12:35:14 +0000
Subject: [PATCH] x86, efi: fix 32-bit warnings in setup_efi_pci()

Fix four similar build warnings on 32-bit (casts between different
size pointers and integers).

Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
Cc: Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>
Cc: Stefan Hasko <hasko.stevo-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/boot/compressed/eboot.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 18e329c..3f3d36f 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -256,10 +256,10 @@ static efi_status_t setup_efi_pci(struct boot_params *params)
 	int i;
 	struct setup_data *data;
 
-	data = (struct setup_data *)params->hdr.setup_data;
+	data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
 
 	while (data && data->next)
-		data = (struct setup_data *)data->next;
+		data = (struct setup_data *)(unsigned long)data->next;
 
 	status = efi_call_phys5(sys_table->boottime->locate_handle,
 				EFI_LOCATE_BY_PROTOCOL, &pci_proto,
@@ -345,9 +345,9 @@ static efi_status_t setup_efi_pci(struct boot_params *params)
 		memcpy(rom->romdata, pci->romimage, pci->romsize);
 
 		if (data)
-			data->next = (uint64_t)rom;
+			data->next = (unsigned long)rom;
 		else
-			params->hdr.setup_data = (uint64_t)rom;
+			params->hdr.setup_data = (unsigned long)rom;
 
 		data = (struct setup_data *)rom;
 
-- 
1.7.11.7

Re: [PATCH v2] fix setup_efi_pci()

[Matthew Garrett]

On Fri, Jan 25, 2013 at 08:45:37AM +0000, Matt Fleming wrote:

> Matthew, what guarantees do we have that the memory we've allocated for
> the PCI ROMs isn't going to be reused? I suspect what's needed is for
> parse_setup_data() to reserve the SETUP_PCI regions, just like you would
> for any other key data structure.

Yeah, I think that originally I had a copy and then failed to update 
this to handle just re-using the original allocation.

> It's the kernel's responsibility to protect any EFI_LOADER_DATA regions
> that it needs, we shouldn't have to trick ourselves by using the
> EFI_RUNTIME_SERVICES_DATA pool for allocations.

I guess the concern is that non-EFI systems may end up passing 
SETUP_PCI? EFI_RUNTIME_DATA would solve one case without handling that, 
so doing it in the data parsing code probably does make sense.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH v2] fix setup_efi_pci()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

On Fri, 2013-01-25 at 08:45 +0000, Matt Fleming wrote:
> 
> Matthew, what guarantees do we have that the memory we've allocated for
> the PCI ROMs isn't going to be reused? I suspect what's needed is for
> parse_setup_data() to reserve the SETUP_PCI regions, just like you would
> for any other key data structure.
> 
> It's the kernel's responsibility to protect any EFI_LOADER_DATA regions
> that it needs, we shouldn't have to trick ourselves by using the
> EFI_RUNTIME_SERVICES_DATA pool for allocations.

When pondering this, please remember that it has to be coherently passed
off to the next kernel during kexec, too.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]