Re: Use PCI ROMs from EFI boot services

Re: Use PCI ROMs from EFI boot services

[Bjorn Helgaas]

On Thu, Aug 23, 2012 at 10:36 AM, Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> V3 just fixes all the casting issues and incorporates David's change in
> search ordering.

I think there's still a section mismatch issue with these patches, so
I haven't merged them yet.

I rebased my pci/mjg-pci-roms-from-efi branch to v3.7-rc2, and if we
get this issue fixed I'll put it in -next as v3.8 material.

Bjorn

Re: Use PCI ROMs from EFI boot services

[Seth Forshee]

On Thu, Oct 25, 2012 at 11:35:57AM -0600, Bjorn Helgaas wrote:
> On Thu, Aug 23, 2012 at 10:36 AM, Matthew Garrett <mjg@redhat.com> wrote:
> > V3 just fixes all the casting issues and incorporates David's change in
> > search ordering.
> 
> I think there's still a section mismatch issue with these patches, so
> I haven't merged them yet.
> 
> I rebased my pci/mjg-pci-roms-from-efi branch to v3.7-rc2, and if we
> get this issue fixed I'll put it in -next as v3.8 material.

I still don't see this series in -next, so I take it the section
mismatch was never fixed? How about the following?

Thanks,
Seth


>From ece31852159a6b2cf9a059031638354e9817a6a6 Mon Sep 17 00:00:00 2001
From: Seth Forshee <seth.forshee@canonical.com>
Date: Mon, 3 Dec 2012 13:55:50 -0600
Subject: [PATCH] x86: Don't discard boot_params

boot_params is now used at runtime on EFI systems to stash option ROMs
that aren't available after exiting boot services, so it can no longer
be marked __initdata.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 arch/x86/kernel/setup.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 468e98d..6e13035 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -143,11 +143,7 @@ int default_check_phys_apicid_present(int phys_apicid)
 }
 #endif
 
-#ifndef CONFIG_DEBUG_BOOT_PARAMS
-struct boot_params __initdata boot_params;
-#else
 struct boot_params boot_params;
-#endif
 
 /*
  * Machine setup..
-- 
1.7.9.5

Re: Use PCI ROMs from EFI boot services

[Bjorn Helgaas]

On Mon, Dec 3, 2012 at 1:02 PM, Seth Forshee <seth.forshee@canonical.com> wrote:
> On Thu, Oct 25, 2012 at 11:35:57AM -0600, Bjorn Helgaas wrote:
>> On Thu, Aug 23, 2012 at 10:36 AM, Matthew Garrett <mjg@redhat.com> wrote:
>> > V3 just fixes all the casting issues and incorporates David's change in
>> > search ordering.
>>
>> I think there's still a section mismatch issue with these patches, so
>> I haven't merged them yet.
>>
>> I rebased my pci/mjg-pci-roms-from-efi branch to v3.7-rc2, and if we
>> get this issue fixed I'll put it in -next as v3.8 material.
>
> I still don't see this series in -next, so I take it the section
> mismatch was never fixed? How about the following?

That's right; nobody stepped up to fix the section mismatch.  I'm
happy to fold in your fix, especially if Matthew acks it.

David, Eric, what about the kexec question?  It looks to me like this
wouldn't make things worse than they are today.  If I understand
correctly, today we don't use ROM data from EFI on either an initial
boot or a kexec.  After this patch, we could use EFI ROM data on the
initial boot, but not after a kexec.  So it's worse in the sense that
the kexec case doesn't match the initial boot, but at least it's not
something that used to work and is now broken.

> From ece31852159a6b2cf9a059031638354e9817a6a6 Mon Sep 17 00:00:00 2001
> From: Seth Forshee <seth.forshee@canonical.com>
> Date: Mon, 3 Dec 2012 13:55:50 -0600
> Subject: [PATCH] x86: Don't discard boot_params
>
> boot_params is now used at runtime on EFI systems to stash option ROMs
> that aren't available after exiting boot services, so it can no longer
> be marked __initdata.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  arch/x86/kernel/setup.c |    4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 468e98d..6e13035 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -143,11 +143,7 @@ int default_check_phys_apicid_present(int phys_apicid)
>  }
>  #endif
>
> -#ifndef CONFIG_DEBUG_BOOT_PARAMS
> -struct boot_params __initdata boot_params;
> -#else
>  struct boot_params boot_params;
> -#endif
>
>  /*
>   * Machine setup..
> --
> 1.7.9.5
>

Re: Use PCI ROMs from EFI boot services

[Matthew Garrett]

On Wed, Dec 05, 2012 at 01:09:25PM -0700, Bjorn Helgaas wrote:

> That's right; nobody stepped up to fix the section mismatch.  I'm
> happy to fold in your fix, especially if Matthew acks it.

Yes, sorry, I've been way behind on pretty much everything for the past 
few months. Please do add my Ack.

> David, Eric, what about the kexec question?  It looks to me like this
> wouldn't make things worse than they are today.  If I understand
> correctly, today we don't use ROM data from EFI on either an initial
> boot or a kexec.  After this patch, we could use EFI ROM data on the
> initial boot, but not after a kexec.  So it's worse in the sense that
> the kexec case doesn't match the initial boot, but at least it's not
> something that used to work and is now broken.

I think I'd agree here - it's not ideal, but it's no more broken than 
the current situation.

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

Re: Use PCI ROMs from EFI boot services

[David Woodhouse]

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

On Wed, 2012-12-05 at 13:09 -0700, Bjorn Helgaas wrote:
> 
> David, Eric, what about the kexec question?  It looks to me like this
> wouldn't make things worse than they are today.  If I understand
> correctly, today we don't use ROM data from EFI on either an initial
> boot or a kexec.  After this patch, we could use EFI ROM data on the
> initial boot, but not after a kexec.  So it's worse in the sense that
> the kexec case doesn't match the initial boot, but at least it's not
> something that used to work and is now broken.

Yeah, kexec under EFI doesn't work too well. I have a firmware running
in qemu locally which will let you call SetVirtualAddressMap more than
once, which is a step towards fixing it sanely. It got preempted, but
I'll take another look at it shortly.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org                              Intel Corporation




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

Re: Use PCI ROMs from EFI boot services

[Bjorn Helgaas]

On Wed, Dec 5, 2012 at 1:22 PM, Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org> wrote:
> On Wed, Dec 05, 2012 at 01:09:25PM -0700, Bjorn Helgaas wrote:
>
>> That's right; nobody stepped up to fix the section mismatch.  I'm
>> happy to fold in your fix, especially if Matthew acks it.
>
> Yes, sorry, I've been way behind on pretty much everything for the past
> few months. Please do add my Ack.
>
>> David, Eric, what about the kexec question?  It looks to me like this
>> wouldn't make things worse than they are today.  If I understand
>> correctly, today we don't use ROM data from EFI on either an initial
>> boot or a kexec.  After this patch, we could use EFI ROM data on the
>> initial boot, but not after a kexec.  So it's worse in the sense that
>> the kexec case doesn't match the initial boot, but at least it's not
>> something that used to work and is now broken.
>
> I think I'd agree here - it's not ideal, but it's no more broken than
> the current situation.

OK, I applied Seth's fix, added his Tested-by, and put this series in
my -next branch.  I plan to merge it during the v3.8 merge window next
week.

Thanks!

Bjorn

Re: Use PCI ROMs from EFI boot services

[Yinghai Lu]

On Mon, Dec 3, 2012 at 12:02 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> On Thu, Oct 25, 2012 at 11:35:57AM -0600, Bjorn Helgaas wrote:
>> On Thu, Aug 23, 2012 at 10:36 AM, Matthew Garrett <mjg@redhat.com> wrote:
>> > V3 just fixes all the casting issues and incorporates David's change in
>> > search ordering.
>>
>> I think there's still a section mismatch issue with these patches, so
>> I haven't merged them yet.
>>
>> I rebased my pci/mjg-pci-roms-from-efi branch to v3.7-rc2, and if we
>> get this issue fixed I'll put it in -next as v3.8 material.
>
> I still don't see this series in -next, so I take it the section
> mismatch was never fixed? How about the following?
>
>
>
> From ece31852159a6b2cf9a059031638354e9817a6a6 Mon Sep 17 00:00:00 2001
> From: Seth Forshee <seth.forshee@canonical.com>
> Date: Mon, 3 Dec 2012 13:55:50 -0600
> Subject: [PATCH] x86: Don't discard boot_params
>
> boot_params is now used at runtime on EFI systems to stash option ROMs
> that aren't available after exiting boot services, so it can no longer
> be marked __initdata.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  arch/x86/kernel/setup.c |    4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 468e98d..6e13035 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -143,11 +143,7 @@ int default_check_phys_apicid_present(int phys_apicid)
>  }
>  #endif
>
> -#ifndef CONFIG_DEBUG_BOOT_PARAMS
> -struct boot_params __initdata boot_params;
> -#else
>  struct boot_params boot_params;
> -#endif
>
>  /*
>   * Machine setup..

No, that is not a right fix

We should only cache pointer to setup_data.

at the same time we should export setup_data into /sys, so kexec could
append this pointer to command of
second kernel, just like kexec append acpi_rsdp.
That should address DavidW's concern.


Yinghai

Re: Use PCI ROMs from EFI boot services

[Matthew Garrett]

On Wed, Dec 05, 2012 at 04:15:56PM -0800, Yinghai Lu wrote:

> at the same time we should export setup_data into /sys, so kexec could
> append this pointer to command of
> second kernel, just like kexec append acpi_rsdp.
> That should address DavidW's concern.

Why should the kernel export data to userspace just so that that data 
can be passed back into the kernel?

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

Re: Use PCI ROMs from EFI boot services

[H. Peter Anvin]

On 12/05/2012 04:18 PM, Matthew Garrett wrote:
> On Wed, Dec 05, 2012 at 04:15:56PM -0800, Yinghai Lu wrote:
> 
>> at the same time we should export setup_data into /sys, so kexec could
>> append this pointer to command of
>> second kernel, just like kexec append acpi_rsdp.
>> That should address DavidW's concern.
> 
> Why should the kernel export data to userspace just so that that data 
> can be passed back into the kernel?
> 

Because it also needs to modify it.  Right now kexec userspace
synthesizes struct boot_params from scratch, and does so incorrectly to
boot.  I think we have setup_data exported via debugfs but IIRC we never
got a strong enough use case for sysfs.

	-hpa

Re: Use PCI ROMs from EFI boot services

[Yinghai Lu]

On Wed, Dec 5, 2012 at 4:18 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Wed, Dec 05, 2012 at 04:15:56PM -0800, Yinghai Lu wrote:
>
>> at the same time we should export setup_data into /sys, so kexec could
>> append this pointer to command of
>> second kernel, just like kexec append acpi_rsdp.
>> That should address DavidW's concern.
>
> Why should the kernel export data to userspace just so that that data
> can be passed back into the kernel?

then how the kexeced get those info?

first kernel already reserved those info as E820_RESERVED_KERN.
and those info are for bootloader's of first kernel.

Re: Use PCI ROMs from EFI boot services

[Eric W. Biederman]

Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org> writes:

> On Wed, Dec 05, 2012 at 04:15:56PM -0800, Yinghai Lu wrote:
>
>> at the same time we should export setup_data into /sys, so kexec could
>> append this pointer to command of
>> second kernel, just like kexec append acpi_rsdp.
>> That should address DavidW's concern.
>
> Why should the kernel export data to userspace just so that that data 
> can be passed back into the kernel?

Because it is useful for more than just kexec and because that is the
current architecture.

Eric

Re: Use PCI ROMs from EFI boot services

[Yinghai Lu]

On Wed, Dec 5, 2012 at 4:21 PM, H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:
> On 12/05/2012 04:18 PM, Matthew Garrett wrote:

> Because it also needs to modify it.  Right now kexec userspace
> synthesizes struct boot_params from scratch, and does so incorrectly to
> boot.  I think we have setup_data exported via debugfs but IIRC we never
> got a strong enough use case for sysfs.

maybe we could try  to export setup_data pointer only /sys, and that
could be safe enough.

Re: Use PCI ROMs from EFI boot services

[H. Peter Anvin]

On 12/05/2012 04:15 PM, Yinghai Lu wrote:
> 
> -#ifndef CONFIG_DEBUG_BOOT_PARAMS
> -struct boot_params __initdata boot_params;
> -#else
>  struct boot_params boot_params;
> -#endif

> 
> No, that is not a right fix
> 
> We should only cache pointer to setup_data.
> 
> at the same time we should export setup_data into /sys, so kexec could
> append this pointer to command of
> second kernel, just like kexec append acpi_rsdp.
> That should address DavidW's concern.
> 

I don't see why that isn't the right fix.  We copy the data into
boot_params early in the boot; that *is* the official copy as far as the
kernel is concerned.

So this patch very much seems like The Right Thing.

	-hpa

Re: Use PCI ROMs from EFI boot services

[H. Peter Anvin]

On 12/05/2012 04:30 PM, Yinghai Lu wrote:
> On Wed, Dec 5, 2012 at 4:21 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 12/05/2012 04:18 PM, Matthew Garrett wrote:
> 
>> Because it also needs to modify it.  Right now kexec userspace
>> synthesizes struct boot_params from scratch, and does so incorrectly to
>> boot.  I think we have setup_data exported via debugfs but IIRC we never
>> got a strong enough use case for sysfs.
> 
> maybe we could try  to export setup_data pointer only /sys, and that
> could be safe enough.
> 

I don't think there is anything security-sensitive about that
information, at least not to root.  I could be wrong, of course; I
wouldn't mind security people telling me about that.

	-hpa

Re: Use PCI ROMs from EFI boot services

[Yinghai Lu]

On Wed, Dec 5, 2012 at 4:36 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 12/05/2012 04:15 PM, Yinghai Lu wrote:
>>
>
> I don't see why that isn't the right fix.  We copy the data into
> boot_params early in the boot; that *is* the official copy as far as the
> kernel is concerned.
>
> So this patch very much seems like The Right Thing.

it moves boot_params from __initdata  to data.
and just for using pointer to setup_data.

should add setup_data pointer instead. so will not waste (4096 - 8) bytes.

Yinghai

Re: Use PCI ROMs from EFI boot services

[Yinghai Lu]

On Wed, Dec 5, 2012 at 4:51 PM, Yinghai Lu <yinghai-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Dec 5, 2012 at 4:36 PM, H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:
>> On 12/05/2012 04:15 PM, Yinghai Lu wrote:
>>>
>>
>> I don't see why that isn't the right fix.  We copy the data into
>> boot_params early in the boot; that *is* the official copy as far as the
>> kernel is concerned.
>>
>> So this patch very much seems like The Right Thing.
>
> it moves boot_params from __initdata  to data.

   should be from __initdata to bss

> and just for using pointer to setup_data.
>
> should add setup_data pointer instead. so will not waste (4096 - 8) bytes.
>
> Yinghai

Re: Use PCI ROMs from EFI boot services

[H. Peter Anvin]

On 12/05/2012 04:51 PM, Yinghai Lu wrote:
> 
> it moves boot_params from __initdata  to data.
> and just for using pointer to setup_data.
> 
> should add setup_data pointer instead. so will not waste (4096 - 8) bytes.
> 

That is not the only bit.  We already have covered how kexec could use
the whole structure, and really, how big a deal is a page of cache-cold
storage?

	-hpa

Re: Use PCI ROMs from EFI boot services

[Matthew Garrett]

"H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:

>I don't think there is anything security-sensitive about that
>information, at least not to root.  I could be wrong, of course; I
>wouldn't mind security people telling me about that.

I don't think there's anything at present, but we'll want to pass the hibernation encryption key from the bootloader to the kernel in the near future. setup_data seems like the easiest way to do that. 

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

Re: Use PCI ROMs from EFI boot services

[Matthew Garrett]

"H. Peter Anvin" <hpa@zytor.com> wrote:

>Because it also needs to modify it.  Right now kexec userspace
>synthesizes struct boot_params from scratch, and does so incorrectly to
>boot.  I think we have setup_data exported via debugfs but IIRC we
>never
>got a strong enough use case for sysfs.

Kexec userspace needs updating every time we add functionality to setup_data? That really doesn't sound ideal. If we want to be able to pass secrets between kernels then this needs to be done in kernel space, not userland. 

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Use PCI ROMs from EFI boot services

[H. Peter Anvin]

On 12/05/2012 04:57 PM, Matthew Garrett wrote:
> 
> 
> "H. Peter Anvin" <hpa@zytor.com> wrote:
> 
>> I don't think there is anything security-sensitive about that
>> information, at least not to root.  I could be wrong, of course; I
>> wouldn't mind security people telling me about that.
> 
> I don't think there's anything at present, but we'll want to pass the hibernation encryption key from the bootloader to the kernel in the near future. setup_data seems like the easiest way to do that. 
> 

And that presumably would be something that cannot be exposed to root?
If so we may want to use one of the bits in the setup_data type field as
a security flag, perhaps...

	-hpa

Re: Use PCI ROMs from EFI boot services

[Matthew Garrett]

"H. Peter Anvin" <hpa@zytor.com> wrote:

>And that presumably would be something that cannot be exposed to root?
>If so we may want to use one of the bits in the setup_data type field
>as
>a security flag, perhaps...

Yeah, it needs to be hidden from root - but ideally we'd be passing it to the second kernel if we kexec. Alternative would be for it to be capability bounded to a trusted signed kexec binary if we implement Vivek's IMA-based approach. 

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Use PCI ROMs from EFI boot services

[H. Peter Anvin]

On 12/05/2012 05:13 PM, Matthew Garrett wrote:
>
>
> "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:
>
>> And that presumably would be something that cannot be exposed to root?
>> If so we may want to use one of the bits in the setup_data type field
>> as
>> a security flag, perhaps...
>
> Yeah, it needs to be hidden from root - but ideally we'd be passing it to the second kernel if we kexec. Alternative would be for it to be capability bounded to a trusted signed kexec binary if we implement Vivek's IMA-based approach.
>

Either way a security flag in the type field makes sense.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Use PCI ROMs from EFI boot services

[Matthew Garrett]

On Wed, Dec 05, 2012 at 05:21:44PM -0800, H. Peter Anvin wrote:
> On 12/05/2012 05:13 PM, Matthew Garrett wrote:
> >Yeah, it needs to be hidden from root - but ideally we'd be passing it to the second kernel if we kexec. Alternative would be for it to be capability bounded to a trusted signed kexec binary if we implement Vivek's IMA-based approach.
> >
> 
> Either way a security flag in the type field makes sense.

I've no objection to that, although I'm not sure there's any real reason 
to expose an incomplete setup_data to userspace. Any scenario in which 
kexec can't read the full data is one where kexec won't be able to 
call sys_kexec() anyway.

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

Re: Use PCI ROMs from EFI boot services

[Bjorn Helgaas]

On Wed, Dec 5, 2012 at 5:52 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Dec 5, 2012 at 4:51 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Wed, Dec 5, 2012 at 4:36 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>> On 12/05/2012 04:15 PM, Yinghai Lu wrote:
>>>>
>>>
>>> I don't see why that isn't the right fix.  We copy the data into
>>> boot_params early in the boot; that *is* the official copy as far as the
>>> kernel is concerned.
>>>
>>> So this patch very much seems like The Right Thing.
>>
>> it moves boot_params from __initdata  to data.
>
>    should be from __initdata to bss
>
>> and just for using pointer to setup_data.
>>
>> should add setup_data pointer instead. so will not waste (4096 - 8) bytes.

I'm not following the whole discussion here, but my impression is that
what's in my -next branch is acceptable
(http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=commitdiff;h=328949ff10f2e3fcb11472571294beed39488342)

If not, please explain further and provide a patch to fix it.

Bjorn

Re: Use PCI ROMs from EFI boot services

[H. Peter Anvin]

On 12/06/2012 10:19 AM, Bjorn Helgaas wrote:
> On Wed, Dec 5, 2012 at 5:52 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Wed, Dec 5, 2012 at 4:51 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> On Wed, Dec 5, 2012 at 4:36 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>>> On 12/05/2012 04:15 PM, Yinghai Lu wrote:
>>>>>
>>>>
>>>> I don't see why that isn't the right fix.  We copy the data into
>>>> boot_params early in the boot; that *is* the official copy as far as the
>>>> kernel is concerned.
>>>>
>>>> So this patch very much seems like The Right Thing.
>>>
>>> it moves boot_params from __initdata  to data.
>>
>>     should be from __initdata to bss
>>
>>> and just for using pointer to setup_data.
>>>
>>> should add setup_data pointer instead. so will not waste (4096 - 8) bytes.
>
> I'm not following the whole discussion here, but my impression is that
> what's in my -next branch is acceptable
> (http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=commitdiff;h=328949ff10f2e3fcb11472571294beed39488342)
>
> If not, please explain further and provide a patch to fix it.
>

NAK on this bit:

+       if (boot_params.hdr.version < 0x0209)
+               return 0;

This field is kernel->bootloader documentation.  If a nonmaching value 
somehow leaks into the kernel, the kernel could either panic("Bootloader 
written by moron") or it should clear some fields, but littering the 
kernel with these kinds of tests is just plain braindead.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Use PCI ROMs from EFI boot services

[Matthew Garrett]

On Thu, Dec 06, 2012 at 10:26:01AM -0800, H. Peter Anvin wrote:

> NAK on this bit:
> 
> +       if (boot_params.hdr.version < 0x0209)
> +               return 0;
> 
> This field is kernel->bootloader documentation.  If a nonmaching
> value somehow leaks into the kernel, the kernel could either
> panic("Bootloader written by moron") or it should clear some fields,
> but littering the kernel with these kinds of tests is just plain
> braindead.

Dropping that should be fine. Bjorn, would you prefer a patch from me to 
do that?

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

Re: Use PCI ROMs from EFI boot services

[Yinghai Lu]

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

On Thu, Dec 6, 2012 at 10:54 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Thu, Dec 06, 2012 at 10:26:01AM -0800, H. Peter Anvin wrote:
>
>> NAK on this bit:
>>
>> +       if (boot_params.hdr.version < 0x0209)
>> +               return 0;
>>
>> This field is kernel->bootloader documentation.  If a nonmaching
>> value somehow leaks into the kernel, the kernel could either
>> panic("Bootloader written by moron") or it should clear some fields,
>> but littering the kernel with these kinds of tests is just plain
>> braindead.
>
> Dropping that should be fine. Bjorn, would you prefer a patch from me to
> do that?

the one : use pointer for setup_pci instead.

[-- Attachment #2: pa_data_setup_pci.patch --]
[-- Type: application/octet-stream, Size: 1784 bytes --]

Subject: [PATCH] x86, pci: move boot_params back to __initdata

Don't waste 4k for 8 bytes.

Also I don't see the reason why DTB could only have one entry
but SETUP_UP data have multiple.

diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
index dba7805..54358f8 100644
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -183,4 +183,6 @@ struct pci_setup_rom {
 	uint8_t romdata[0];
 };
 
+extern u64 pa_data_setup_pci;
+
 #endif /* _ASM_X86_PCI_H */
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 2bd40fd..8062f25 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -143,7 +143,11 @@ int default_check_phys_apicid_present(int phys_apicid)
 }
 #endif
 
+#ifndef CONFIG_DEBUG_BOOT_PARAMS
+struct boot_params __initdata boot_params;
+#else
 struct boot_params boot_params;
+#endif
 
 /*
  * Machine setup..
@@ -414,6 +418,8 @@ static void __init reserve_initrd(void)
 }
 #endif /* CONFIG_BLK_DEV_INITRD */
 
+u64 pa_data_setup_pci;
+
 static void __init parse_setup_data(void)
 {
 	struct setup_data *data;
@@ -442,6 +448,10 @@ static void __init parse_setup_data(void)
 		case SETUP_DTB:
 			add_dtb(pa_data);
 			break;
+		case SETUP_PCI:
+			if (!setup_data_pci)
+				pa_data_setup_pci = pa_data;
+			break;
 		default:
 			break;
 		}
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 47584f4..4e16651 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -613,12 +613,8 @@ int pcibios_add_device(struct pci_dev *dev)
 {
 	struct setup_data *data;
 	struct pci_setup_rom *rom;
-	u64 pa_data;
+	u64 pa_data = pa_data_setup_pci;
 
-	if (boot_params.hdr.version < 0x0209)
-		return 0;
-
-	pa_data = boot_params.hdr.setup_data;
 	while (pa_data) {
 		data = phys_to_virt(pa_data);
 

Re: Use PCI ROMs from EFI boot services

[Bjorn Helgaas]

On Thu, Dec 6, 2012 at 11:54 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Thu, Dec 06, 2012 at 10:26:01AM -0800, H. Peter Anvin wrote:
>
>> NAK on this bit:
>>
>> +       if (boot_params.hdr.version < 0x0209)
>> +               return 0;
>>
>> This field is kernel->bootloader documentation.  If a nonmaching
>> value somehow leaks into the kernel, the kernel could either
>> panic("Bootloader written by moron") or it should clear some fields,
>> but littering the kernel with these kinds of tests is just plain
>> braindead.
>
> Dropping that should be fine. Bjorn, would you prefer a patch from me to
> do that?

I dropped that check and re-pushed my -next branch.

Yinghai, I don't understand your pa_data_setup_pci.patch, but if
somebody else acks it and we can come up with an intelligible
changelog, I'll fold that in, too.