Use PCI ROMs from EFI boot services

Use PCI ROMs from EFI boot services

[Matthew Garrett]

V3 just fixes all the casting issues and incorporates David's change in
search ordering.

[PATCH V3 1/4] EFI: Stash ROMs if they're not in the PCI BAR

[Matthew Garrett]

EFI provides support for providing PCI ROMs via means other than the ROM
BAR. This support vanishes after we've exited boot services, so add support
for stashing copies of the ROMs in setup_data if they're not otherwise
available.
---
 arch/x86/boot/compressed/eboot.c | 118 +++++++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/bootparam.h |   1 +
 arch/x86/include/asm/pci.h       |  12 ++++
 include/linux/efi.h              |  71 +++++++++++++++++++++++
 4 files changed, 202 insertions(+)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index b3e0227..8630578 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -8,6 +8,7 @@
  * ----------------------------------------------------------------------- */
 
 #include <linux/efi.h>
+#include <linux/pci.h>
 #include <asm/efi.h>
 #include <asm/setup.h>
 #include <asm/desc.h>
@@ -243,6 +244,121 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
 	*size = len;
 }
 
+static efi_status_t setup_efi_pci(struct boot_params *params)
+{
+	efi_pci_io_protocol *pci;
+	efi_status_t status;
+	void **pci_handle;
+	efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
+	unsigned long nr_pci, size = 0;
+	int i;
+	struct setup_data *data;
+
+	data = (struct setup_data *)params->hdr.setup_data;
+
+	while (data && data->next)
+		data = (struct setup_data *)data->next;
+
+	status = efi_call_phys5(sys_table->boottime->locate_handle,
+				EFI_LOCATE_BY_PROTOCOL, &pci_proto,
+				NULL, &size, pci_handle);
+
+	if (status == EFI_BUFFER_TOO_SMALL) {
+		status = efi_call_phys3(sys_table->boottime->allocate_pool,
+					EFI_LOADER_DATA, size, &pci_handle);
+
+		if (status != EFI_SUCCESS)
+			return status;
+
+		status = efi_call_phys5(sys_table->boottime->locate_handle,
+					EFI_LOCATE_BY_PROTOCOL, &pci_proto,
+					NULL, &size, pci_handle);
+	}
+
+	if (status != EFI_SUCCESS)
+		goto free_handle;
+
+	nr_pci = size / sizeof(void *);
+	for (i = 0; i < nr_pci; i++) {
+		void *h = pci_handle[i];
+		uint64_t attributes;
+		struct pci_setup_rom *rom;
+
+		status = efi_call_phys3(sys_table->boottime->handle_protocol,
+					h, &pci_proto, &pci);
+
+		if (status != EFI_SUCCESS)
+			continue;
+
+		if (!pci)
+			continue;
+
+		status = efi_call_phys4(pci->attributes, pci,
+					EfiPciIoAttributeOperationGet, 0,
+					&attributes);
+
+		if (status != EFI_SUCCESS)
+			continue;
+
+		if (!attributes & EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM)
+			continue;
+
+		if (!pci->romimage || !pci->romsize)
+			continue;
+
+		size = pci->romsize + sizeof(*rom);
+
+		status = efi_call_phys3(sys_table->boottime->allocate_pool,
+				EFI_LOADER_DATA, size, &rom);
+
+		if (status != EFI_SUCCESS)
+			continue;
+
+		rom->data.type = SETUP_PCI;
+		rom->data.len = size - sizeof(struct setup_data);
+		rom->data.next = 0;
+		rom->pcilen = pci->romsize;
+
+		status = efi_call_phys5(pci->pci.read, pci,
+					EfiPciIoWidthUint16, PCI_VENDOR_ID,
+					1, &(rom->vendor));
+
+		if (status != EFI_SUCCESS)
+			goto free_struct;
+
+		status = efi_call_phys5(pci->pci.read, pci,
+					EfiPciIoWidthUint16, PCI_DEVICE_ID,
+					1, &(rom->devid));
+
+		if (status != EFI_SUCCESS)
+			goto free_struct;
+
+		status = efi_call_phys5(pci->get_location, pci,
+					&(rom->segment), &(rom->bus),
+					&(rom->device), &(rom->function));
+
+		if (status != EFI_SUCCESS)
+			goto free_struct;
+
+		memcpy(rom->romdata, pci->romimage, pci->romsize);
+
+		if (data)
+			data->next = (uint64_t)rom;
+		else
+			params->hdr.setup_data = (uint64_t)rom;
+
+		data = (struct setup_data *)rom;
+
+		continue;
+	free_struct:
+		efi_call_phys1(sys_table->boottime->free_pool, rom);
+	}
+
+free_handle:
+	efi_call_phys1(sys_table->boottime->free_pool, pci_handle);
+	return status;
+}
+
 /*
  * See if we have Graphics Output Protocol
  */
@@ -1020,6 +1136,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
 
 	setup_graphics(boot_params);
 
+	setup_efi_pci(boot_params);
+
 	status = efi_call_phys3(sys_table->boottime->allocate_pool,
 				EFI_LOADER_DATA, sizeof(*gdt),
 				(void **)&gdt);
diff --git a/arch/x86/include/asm/bootparam.h b/arch/x86/include/asm/bootparam.h
index 2ad874c..92862cd 100644
--- a/arch/x86/include/asm/bootparam.h
+++ b/arch/x86/include/asm/bootparam.h
@@ -13,6 +13,7 @@
 #define SETUP_NONE			0
 #define SETUP_E820_EXT			1
 #define SETUP_DTB			2
+#define SETUP_PCI			3
 
 /* extensible setup data list node */
 struct setup_data {
diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
index df75d07..11491d1 100644
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -171,4 +171,16 @@ cpumask_of_pcibus(const struct pci_bus *bus)
 }
 #endif
 
+struct pci_setup_rom {
+	struct setup_data data;
+	uint16_t vendor;
+	uint16_t devid;
+	uint64_t pcilen;
+	unsigned long segment;
+	unsigned long bus;
+	unsigned long device;
+	unsigned long function;
+	uint8_t romdata[0];
+};
+
 #endif /* _ASM_X86_PCI_H */
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ec45ccd..bf7e867 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -196,6 +196,77 @@ typedef struct {
 	void *create_event_ex;
 } efi_boot_services_t;
 
+typedef enum {
+	EfiPciIoWidthUint8,
+	EfiPciIoWidthUint16,
+	EfiPciIoWidthUint32,
+	EfiPciIoWidthUint64,
+	EfiPciIoWidthFifoUint8,
+	EfiPciIoWidthFifoUint16,
+	EfiPciIoWidthFifoUint32,
+	EfiPciIoWidthFifoUint64,
+	EfiPciIoWidthFillUint8,
+	EfiPciIoWidthFillUint16,
+	EfiPciIoWidthFillUint32,
+	EfiPciIoWidthFillUint64,
+	EfiPciIoWidthMaximum
+} EFI_PCI_IO_PROTOCOL_WIDTH;
+
+typedef enum {
+	EfiPciIoAttributeOperationGet,
+	EfiPciIoAttributeOperationSet,
+	EfiPciIoAttributeOperationEnable,
+	EfiPciIoAttributeOperationDisable,
+	EfiPciIoAttributeOperationSupported,
+    EfiPciIoAttributeOperationMaximum
+} EFI_PCI_IO_PROTOCOL_ATTRIBUTE_OPERATION;
+
+
+typedef struct {
+	void *read;
+	void *write;
+} efi_pci_io_protocol_access_t;
+
+typedef struct {
+	void *poll_mem;
+	void *poll_io;
+	efi_pci_io_protocol_access_t mem;
+	efi_pci_io_protocol_access_t io;
+	efi_pci_io_protocol_access_t pci;
+	void *copy_mem;
+	void *map;
+	void *unmap;
+	void *allocate_buffer;
+	void *free_buffer;
+	void *flush;
+	void *get_location;
+	void *attributes;
+	void *get_bar_attributes;
+	void *set_bar_attributes;
+	uint64_t romsize;
+	void *romimage;
+} efi_pci_io_protocol;
+
+#define EFI_PCI_IO_ATTRIBUTE_ISA_MOTHERBOARD_IO 0x0001
+#define EFI_PCI_IO_ATTRIBUTE_ISA_IO 0x0002
+#define EFI_PCI_IO_ATTRIBUTE_VGA_PALETTE_IO 0x0004
+#define EFI_PCI_IO_ATTRIBUTE_VGA_MEMORY 0x0008
+#define EFI_PCI_IO_ATTRIBUTE_VGA_IO 0x0010
+#define EFI_PCI_IO_ATTRIBUTE_IDE_PRIMARY_IO 0x0020
+#define EFI_PCI_IO_ATTRIBUTE_IDE_SECONDARY_IO 0x0040
+#define EFI_PCI_IO_ATTRIBUTE_MEMORY_WRITE_COMBINE 0x0080
+#define EFI_PCI_IO_ATTRIBUTE_IO 0x0100
+#define EFI_PCI_IO_ATTRIBUTE_MEMORY 0x0200
+#define EFI_PCI_IO_ATTRIBUTE_BUS_MASTER 0x0400
+#define EFI_PCI_IO_ATTRIBUTE_MEMORY_CACHED 0x0800
+#define EFI_PCI_IO_ATTRIBUTE_MEMORY_DISABLE 0x1000
+#define EFI_PCI_IO_ATTRIBUTE_EMBEDDED_DEVICE 0x2000
+#define EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM 0x4000
+#define EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE 0x8000
+#define EFI_PCI_IO_ATTRIBUTE_ISA_IO_16 0x10000
+#define EFI_PCI_IO_ATTRIBUTE_VGA_PALETTE_IO_16 0x20000
+#define EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 0x40000
+
 /*
  * Types and defines for EFI ResetSystem
  */
-- 
1.7.11.2

Re: [PATCH V3 1/4] EFI: Stash ROMs if they're not in the PCI BAR

[Bjorn Helgaas]

On Thu, Aug 23, 2012 at 10:36 AM, Matthew Garrett <mjg@redhat.com> wrote:
> EFI provides support for providing PCI ROMs via means other than the ROM
> BAR. This support vanishes after we've exited boot services, so add support
> for stashing copies of the ROMs in setup_data if they're not otherwise
> available.

-ENO_SIGNED_OFF_BY

I'll add it if you confirm it.

> ---
>  arch/x86/boot/compressed/eboot.c | 118 +++++++++++++++++++++++++++++++++++++++
>  arch/x86/include/asm/bootparam.h |   1 +
>  arch/x86/include/asm/pci.h       |  12 ++++
>  include/linux/efi.h              |  71 +++++++++++++++++++++++
>  4 files changed, 202 insertions(+)
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index b3e0227..8630578 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -8,6 +8,7 @@
>   * ----------------------------------------------------------------------- */
>
>  #include <linux/efi.h>
> +#include <linux/pci.h>
>  #include <asm/efi.h>
>  #include <asm/setup.h>
>  #include <asm/desc.h>
> @@ -243,6 +244,121 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
>         *size = len;
>  }
>
> +static efi_status_t setup_efi_pci(struct boot_params *params)
> +{
> +       efi_pci_io_protocol *pci;
> +       efi_status_t status;
> +       void **pci_handle;
> +       efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
> +       unsigned long nr_pci, size = 0;
> +       int i;
> +       struct setup_data *data;
> +
> +       data = (struct setup_data *)params->hdr.setup_data;
> +
> +       while (data && data->next)
> +               data = (struct setup_data *)data->next;
> +
> +       status = efi_call_phys5(sys_table->boottime->locate_handle,
> +                               EFI_LOCATE_BY_PROTOCOL, &pci_proto,
> +                               NULL, &size, pci_handle);
> +
> +       if (status == EFI_BUFFER_TOO_SMALL) {
> +               status = efi_call_phys3(sys_table->boottime->allocate_pool,
> +                                       EFI_LOADER_DATA, size, &pci_handle);
> +
> +               if (status != EFI_SUCCESS)
> +                       return status;
> +
> +               status = efi_call_phys5(sys_table->boottime->locate_handle,
> +                                       EFI_LOCATE_BY_PROTOCOL, &pci_proto,
> +                                       NULL, &size, pci_handle);
> +       }
> +
> +       if (status != EFI_SUCCESS)
> +               goto free_handle;
> +
> +       nr_pci = size / sizeof(void *);
> +       for (i = 0; i < nr_pci; i++) {
> +               void *h = pci_handle[i];
> +               uint64_t attributes;
> +               struct pci_setup_rom *rom;
> +
> +               status = efi_call_phys3(sys_table->boottime->handle_protocol,
> +                                       h, &pci_proto, &pci);
> +
> +               if (status != EFI_SUCCESS)
> +                       continue;
> +
> +               if (!pci)
> +                       continue;
> +
> +               status = efi_call_phys4(pci->attributes, pci,
> +                                       EfiPciIoAttributeOperationGet, 0,
> +                                       &attributes);
> +
> +               if (status != EFI_SUCCESS)
> +                       continue;
> +
> +               if (!attributes & EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM)
> +                       continue;
> +
> +               if (!pci->romimage || !pci->romsize)
> +                       continue;
> +
> +               size = pci->romsize + sizeof(*rom);
> +
> +               status = efi_call_phys3(sys_table->boottime->allocate_pool,
> +                               EFI_LOADER_DATA, size, &rom);
> +
> +               if (status != EFI_SUCCESS)
> +                       continue;
> +
> +               rom->data.type = SETUP_PCI;
> +               rom->data.len = size - sizeof(struct setup_data);
> +               rom->data.next = 0;
> +               rom->pcilen = pci->romsize;
> +
> +               status = efi_call_phys5(pci->pci.read, pci,
> +                                       EfiPciIoWidthUint16, PCI_VENDOR_ID,
> +                                       1, &(rom->vendor));
> +
> +               if (status != EFI_SUCCESS)
> +                       goto free_struct;
> +
> +               status = efi_call_phys5(pci->pci.read, pci,
> +                                       EfiPciIoWidthUint16, PCI_DEVICE_ID,
> +                                       1, &(rom->devid));
> +
> +               if (status != EFI_SUCCESS)
> +                       goto free_struct;
> +
> +               status = efi_call_phys5(pci->get_location, pci,
> +                                       &(rom->segment), &(rom->bus),
> +                                       &(rom->device), &(rom->function));
> +
> +               if (status != EFI_SUCCESS)
> +                       goto free_struct;
> +
> +               memcpy(rom->romdata, pci->romimage, pci->romsize);
> +
> +               if (data)
> +                       data->next = (uint64_t)rom;
> +               else
> +                       params->hdr.setup_data = (uint64_t)rom;
> +
> +               data = (struct setup_data *)rom;
> +
> +               continue;
> +       free_struct:
> +               efi_call_phys1(sys_table->boottime->free_pool, rom);
> +       }
> +
> +free_handle:
> +       efi_call_phys1(sys_table->boottime->free_pool, pci_handle);
> +       return status;
> +}
> +
>  /*
>   * See if we have Graphics Output Protocol
>   */
> @@ -1020,6 +1136,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
>
>         setup_graphics(boot_params);
>
> +       setup_efi_pci(boot_params);
> +
>         status = efi_call_phys3(sys_table->boottime->allocate_pool,
>                                 EFI_LOADER_DATA, sizeof(*gdt),
>                                 (void **)&gdt);
> diff --git a/arch/x86/include/asm/bootparam.h b/arch/x86/include/asm/bootparam.h
> index 2ad874c..92862cd 100644
> --- a/arch/x86/include/asm/bootparam.h
> +++ b/arch/x86/include/asm/bootparam.h
> @@ -13,6 +13,7 @@
>  #define SETUP_NONE                     0
>  #define SETUP_E820_EXT                 1
>  #define SETUP_DTB                      2
> +#define SETUP_PCI                      3
>
>  /* extensible setup data list node */
>  struct setup_data {
> diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
> index df75d07..11491d1 100644
> --- a/arch/x86/include/asm/pci.h
> +++ b/arch/x86/include/asm/pci.h
> @@ -171,4 +171,16 @@ cpumask_of_pcibus(const struct pci_bus *bus)
>  }
>  #endif
>
> +struct pci_setup_rom {
> +       struct setup_data data;
> +       uint16_t vendor;
> +       uint16_t devid;
> +       uint64_t pcilen;
> +       unsigned long segment;
> +       unsigned long bus;
> +       unsigned long device;
> +       unsigned long function;
> +       uint8_t romdata[0];
> +};
> +
>  #endif /* _ASM_X86_PCI_H */
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index ec45ccd..bf7e867 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -196,6 +196,77 @@ typedef struct {
>         void *create_event_ex;
>  } efi_boot_services_t;
>
> +typedef enum {
> +       EfiPciIoWidthUint8,
> +       EfiPciIoWidthUint16,
> +       EfiPciIoWidthUint32,
> +       EfiPciIoWidthUint64,
> +       EfiPciIoWidthFifoUint8,
> +       EfiPciIoWidthFifoUint16,
> +       EfiPciIoWidthFifoUint32,
> +       EfiPciIoWidthFifoUint64,
> +       EfiPciIoWidthFillUint8,
> +       EfiPciIoWidthFillUint16,
> +       EfiPciIoWidthFillUint32,
> +       EfiPciIoWidthFillUint64,
> +       EfiPciIoWidthMaximum
> +} EFI_PCI_IO_PROTOCOL_WIDTH;
> +
> +typedef enum {
> +       EfiPciIoAttributeOperationGet,
> +       EfiPciIoAttributeOperationSet,
> +       EfiPciIoAttributeOperationEnable,
> +       EfiPciIoAttributeOperationDisable,
> +       EfiPciIoAttributeOperationSupported,
> +    EfiPciIoAttributeOperationMaximum
> +} EFI_PCI_IO_PROTOCOL_ATTRIBUTE_OPERATION;
> +
> +
> +typedef struct {
> +       void *read;
> +       void *write;
> +} efi_pci_io_protocol_access_t;
> +
> +typedef struct {
> +       void *poll_mem;
> +       void *poll_io;
> +       efi_pci_io_protocol_access_t mem;
> +       efi_pci_io_protocol_access_t io;
> +       efi_pci_io_protocol_access_t pci;
> +       void *copy_mem;
> +       void *map;
> +       void *unmap;
> +       void *allocate_buffer;
> +       void *free_buffer;
> +       void *flush;
> +       void *get_location;
> +       void *attributes;
> +       void *get_bar_attributes;
> +       void *set_bar_attributes;
> +       uint64_t romsize;
> +       void *romimage;
> +} efi_pci_io_protocol;
> +
> +#define EFI_PCI_IO_ATTRIBUTE_ISA_MOTHERBOARD_IO 0x0001
> +#define EFI_PCI_IO_ATTRIBUTE_ISA_IO 0x0002
> +#define EFI_PCI_IO_ATTRIBUTE_VGA_PALETTE_IO 0x0004
> +#define EFI_PCI_IO_ATTRIBUTE_VGA_MEMORY 0x0008
> +#define EFI_PCI_IO_ATTRIBUTE_VGA_IO 0x0010
> +#define EFI_PCI_IO_ATTRIBUTE_IDE_PRIMARY_IO 0x0020
> +#define EFI_PCI_IO_ATTRIBUTE_IDE_SECONDARY_IO 0x0040
> +#define EFI_PCI_IO_ATTRIBUTE_MEMORY_WRITE_COMBINE 0x0080
> +#define EFI_PCI_IO_ATTRIBUTE_IO 0x0100
> +#define EFI_PCI_IO_ATTRIBUTE_MEMORY 0x0200
> +#define EFI_PCI_IO_ATTRIBUTE_BUS_MASTER 0x0400
> +#define EFI_PCI_IO_ATTRIBUTE_MEMORY_CACHED 0x0800
> +#define EFI_PCI_IO_ATTRIBUTE_MEMORY_DISABLE 0x1000
> +#define EFI_PCI_IO_ATTRIBUTE_EMBEDDED_DEVICE 0x2000
> +#define EFI_PCI_IO_ATTRIBUTE_EMBEDDED_ROM 0x4000
> +#define EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE 0x8000
> +#define EFI_PCI_IO_ATTRIBUTE_ISA_IO_16 0x10000
> +#define EFI_PCI_IO_ATTRIBUTE_VGA_PALETTE_IO_16 0x20000
> +#define EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 0x40000
> +
>  /*
>   * Types and defines for EFI ResetSystem
>   */
> --
> 1.7.11.2
>

Re: [PATCH V3 1/4] EFI: Stash ROMs if they're not in the PCI BAR

[Matthew Garrett]

On Thu, Aug 23, 2012 at 05:44:22PM -0600, Bjorn Helgaas wrote:
> On Thu, Aug 23, 2012 at 10:36 AM, Matthew Garrett <mjg@redhat.com> wrote:
> > EFI provides support for providing PCI ROMs via means other than the ROM
> > BAR. This support vanishes after we've exited boot services, so add support
> > for stashing copies of the ROMs in setup_data if they're not otherwise
> > available.
> 
> -ENO_SIGNED_OFF_BY
> 
> I'll add it if you confirm it.

Signed-off-by: Matthew Garrett <mjg@redhat.com>

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

Re: [PATCH V3 1/4] EFI: Stash ROMs if they're not in the PCI BAR

[Matthew Garrett]

On Thu, Aug 23, 2012 at 05:44:22PM -0600, Bjorn Helgaas wrote:
> On Thu, Aug 23, 2012 at 10:36 AM, Matthew Garrett <mjg@redhat.com> wrote:
> > EFI provides support for providing PCI ROMs via means other than the ROM
> > BAR. This support vanishes after we've exited boot services, so add support
> > for stashing copies of the ROMs in setup_data if they're not otherwise
> > available.
> 
> -ENO_SIGNED_OFF_BY
> 
> I'll add it if you confirm it.

Signed-off-by: Matthew Garrett <mjg@redhat.com>

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

Re: [PATCH V3 1/4] EFI: Stash ROMs if they're not in the PCI BAR

[Bjorn Helgaas]

On Tue, Sep 4, 2012 at 6:45 AM, Matthew Garrett <mjg@redhat.com> wrote:
> On Thu, Aug 23, 2012 at 05:44:22PM -0600, Bjorn Helgaas wrote:
>> On Thu, Aug 23, 2012 at 10:36 AM, Matthew Garrett <mjg@redhat.com> wrote:
>> > EFI provides support for providing PCI ROMs via means other than the ROM
>> > BAR. This support vanishes after we've exited boot services, so add support
>> > for stashing copies of the ROMs in setup_data if they're not otherwise
>> > available.
>>
>> -ENO_SIGNED_OFF_BY
>>
>> I'll add it if you confirm it.
>
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

I put these on a branch
(http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=shortlog;h=refs/heads/pci/mjg-pci-roms-from-efi),
but Fengguang found a new warning on the branch, so I'm waiting for a
fix for that before merging it to -next.  I'll bounce you the info
again.

Bjorn

[PATCH] x86: Fix build warning on 32-bit

[Matthew Garrett]

Fix up the cast so it doesn't cause issues on 32-bit systems.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 arch/x86/pci/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 3d2752f..1e2deac 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -631,7 +631,7 @@ int pcibios_add_device(struct pci_dev *dev)
 			    (PCI_FUNC(dev->devfn) == rom->function) &&
 			    (dev->vendor == rom->vendor) &&
 			    (dev->device == rom->devid)) {
-				dev->rom = (void *)(pa_data +
+				dev->rom = (void *)(unsigned long)(pa_data +
 				      offsetof(struct pci_setup_rom, romdata));
 				dev->romlen = rom->pcilen;
 			}
-- 
1.7.11.4

Re: [PATCH] x86: Fix build warning on 32-bit

[Bjorn Helgaas]

On Thu, Sep 6, 2012 at 10:36 AM, Matthew Garrett <mjg@redhat.com> wrote:
> Fix up the cast so it doesn't cause issues on 32-bit systems.

Thanks!  I folded this into the original patch and pushed the updated
pci/mjg-pci-roms-from-efi branch.

> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  arch/x86/pci/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index 3d2752f..1e2deac 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -631,7 +631,7 @@ int pcibios_add_device(struct pci_dev *dev)
>                             (PCI_FUNC(dev->devfn) == rom->function) &&
>                             (dev->vendor == rom->vendor) &&
>                             (dev->device == rom->devid)) {
> -                               dev->rom = (void *)(pa_data +
> +                               dev->rom = (void *)(unsigned long)(pa_data +
>                                       offsetof(struct pci_setup_rom, romdata));
>                                 dev->romlen = rom->pcilen;
>                         }
> --
> 1.7.11.4
>

[PATCH V3 2/4] PCI: Add pcibios_add_device

[Matthew Garrett]

Platforms may want to provide architecture-specific functionality during
PCI enumeration. Add a pcibios_add_device() call that architectures can
override to do so.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/pci/bus.c   |  5 +++++
 drivers/pci/pci.c   | 13 +++++++++++++
 include/linux/pci.h |  1 +
 3 files changed, 19 insertions(+)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 4b0970b..e2f447e 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -166,6 +166,11 @@ int pci_bus_add_device(struct pci_dev *dev)
 	int retval;
 
 	pci_fixup_device(pci_fixup_final, dev);
+
+	retval = pcibios_add_device(dev);
+	if (retval)
+		return retval;
+
 	retval = device_add(&dev->dev);
 	if (retval)
 		return retval;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f3ea977..5a9a4d8 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1385,6 +1385,19 @@ void pcim_pin_device(struct pci_dev *pdev)
 		dr->pinned = 1;
 }
 
+/*
+ * pcibios_add_device - provide arch specific hooks when adding device dev
+ * @dev: the PCI device being added
+ *
+ * Permits the platform to provide architecture specific functionality when
+ * devices are added. This is the default implementation. Architecture
+ * implementations can override this.
+ */
+int __attribute__ ((weak)) pcibios_add_device (struct pci_dev *dev)
+{
+	return 0;
+}
+
 /**
  * pcibios_disable_device - disable arch specific PCI resources for device dev
  * @dev: the PCI device to disable
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5faa831..6a2625c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1582,6 +1582,7 @@ void pcibios_disable_device(struct pci_dev *dev);
 void pcibios_set_master(struct pci_dev *dev);
 int pcibios_set_pcie_reset_state(struct pci_dev *dev,
 				 enum pcie_reset_state state);
+int pcibios_add_device(struct pci_dev *dev);
 
 #ifdef CONFIG_PCI_MMCONFIG
 extern void __init pci_mmcfg_early_init(void);
-- 
1.7.11.2

[PATCH V3 3/4] PCI: Add support for non-BAR ROMs

[Matthew Garrett]

Platforms may provide their own mechanisms for obtaining ROMs. Add support
for using data provided by the platform in that case.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/pci/rom.c   | 11 +++++++++--
 include/linux/pci.h |  2 ++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index 48ebdb2..46026e4 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -118,11 +118,17 @@ void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
 	void __iomem *rom;
 
 	/*
+	 * Some devices may provide ROMs via a source other than the BAR
+	 */
+	if (pdev->rom && pdev->romlen) {
+		*size = pdev->romlen;
+		return phys_to_virt((phys_addr_t)pdev->rom);
+	/*
 	 * IORESOURCE_ROM_SHADOW set on x86, x86_64 and IA64 supports legacy
 	 * memory map if the VGA enable bit of the Bridge Control register is
 	 * set for embedded VGA.
 	 */
-	if (res->flags & IORESOURCE_ROM_SHADOW) {
+	} else if (res->flags & IORESOURCE_ROM_SHADOW) {
 		/* primary video rom always starts here */
 		start = (loff_t)0xC0000;
 		*size = 0x20000; /* cover C000:0 through E000:0 */
@@ -219,7 +225,8 @@ void pci_unmap_rom(struct pci_dev *pdev, void __iomem *rom)
 	if (res->flags & (IORESOURCE_ROM_COPY | IORESOURCE_ROM_BIOS_COPY))
 		return;
 
-	iounmap(rom);
+	if (!pdev->rom || !pdev->romlen)
+		iounmap(rom);
 
 	/* Disable again before continuing, leave enabled if pci=rom */
 	if (!(res->flags & (IORESOURCE_ROM_ENABLE | IORESOURCE_ROM_SHADOW)))
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6a2625c..2668bb9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -355,6 +355,8 @@ struct pci_dev {
 	};
 	struct pci_ats	*ats;	/* Address Translation Service */
 #endif
+	void *rom; /* Physical pointer to ROM if it's not from the BAR */
+	size_t romlen; /* Length of ROM if it's not from the BAR */
 };
 
 static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
-- 
1.7.11.2

Re: [PATCH V3 3/4] PCI: Add support for non-BAR ROMs

[Don Dutile]

On 08/23/2012 12:36 PM, Matthew Garrett wrote:
> Platforms may provide their own mechanisms for obtaining ROMs. Add support
> for using data provided by the platform in that case.
>
> Signed-off-by: Matthew Garrett<mjg@redhat.com>
> ---
>   drivers/pci/rom.c   | 11 +++++++++--
>   include/linux/pci.h |  2 ++
>   2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
> index 48ebdb2..46026e4 100644
> --- a/drivers/pci/rom.c
> +++ b/drivers/pci/rom.c
> @@ -118,11 +118,17 @@ void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
>   	void __iomem *rom;
>
>   	/*
> +	 * Some devices may provide ROMs via a source other than the BAR
> +	 */
> +	if (pdev->rom&&  pdev->romlen) {
> +		*size = pdev->romlen;
> +		return phys_to_virt((phys_addr_t)pdev->rom);
                        ^^^^^
                          ioremap_nocache() ? ... or is caching rom ok?

> +	/*
>   	 * IORESOURCE_ROM_SHADOW set on x86, x86_64 and IA64 supports legacy
>   	 * memory map if the VGA enable bit of the Bridge Control register is
>   	 * set for embedded VGA.
>   	 */
> -	if (res->flags&  IORESOURCE_ROM_SHADOW) {
> +	} else if (res->flags&  IORESOURCE_ROM_SHADOW) {
>   		/* primary video rom always starts here */
>   		start = (loff_t)0xC0000;
>   		*size = 0x20000; /* cover C000:0 through E000:0 */
> @@ -219,7 +225,8 @@ void pci_unmap_rom(struct pci_dev *pdev, void __iomem *rom)
>   	if (res->flags&  (IORESOURCE_ROM_COPY | IORESOURCE_ROM_BIOS_COPY))
>   		return;
>
> -	iounmap(rom);
> +	if (!pdev->rom || !pdev->romlen)
> +		iounmap(rom);
>
>   	/* Disable again before continuing, leave enabled if pci=rom */
>   	if (!(res->flags&  (IORESOURCE_ROM_ENABLE | IORESOURCE_ROM_SHADOW)))
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6a2625c..2668bb9 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -355,6 +355,8 @@ struct pci_dev {
>   	};
>   	struct pci_ats	*ats;	/* Address Translation Service */
>   #endif
> +	void *rom; /* Physical pointer to ROM if it's not from the BAR */
> +	size_t romlen; /* Length of ROM if it's not from the BAR */
>   };
>
>   static inline struct pci_dev *pci_physfn(struct pci_dev *dev)

Re: [PATCH V3 3/4] PCI: Add support for non-BAR ROMs

[Matthew Garrett]

On Tue, Sep 04, 2012 at 10:18:48PM -0400, Don Dutile wrote:
> >  	/*
> >+	 * Some devices may provide ROMs via a source other than the BAR
> >+	 */
> >+	if (pdev->rom&&  pdev->romlen) {
> >+		*size = pdev->romlen;
> >+		return phys_to_virt((phys_addr_t)pdev->rom);
>                        ^^^^^
>                          ioremap_nocache() ? ... or is caching rom ok?

If it's appearing through this pathway then it's not actually in ROM, 
the platform has pulled it out of somewhere else.

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

Re: [PATCH V3 3/4] PCI: Add support for non-BAR ROMs

[Alan Cox]

On Wed, 5 Sep 2012 03:29:32 +0100
Matthew Garrett <mjg@redhat.com> wrote:

> On Tue, Sep 04, 2012 at 10:18:48PM -0400, Don Dutile wrote:
> > >  	/*
> > >+	 * Some devices may provide ROMs via a source other than the BAR
> > >+	 */
> > >+	if (pdev->rom&&  pdev->romlen) {
> > >+		*size = pdev->romlen;
> > >+		return phys_to_virt((phys_addr_t)pdev->rom);
> >                        ^^^^^
> >                          ioremap_nocache() ? ... or is caching rom ok?
> 
> If it's appearing through this pathway then it's not actually in ROM, 
> the platform has pulled it out of somewhere else.

If that somewhere else is on the PCI bus then it should be a bus not a
virt translation surely ?

Re: [PATCH V3 3/4] PCI: Add support for non-BAR ROMs

[Matthew Garrett]

On Wed, Sep 05, 2012 at 01:46:21PM +0100, Alan Cox wrote:
> On Wed, 5 Sep 2012 03:29:32 +0100
> Matthew Garrett <mjg@redhat.com> wrote:
> 
> > On Tue, Sep 04, 2012 at 10:18:48PM -0400, Don Dutile wrote:
> > > >  	/*
> > > >+	 * Some devices may provide ROMs via a source other than the BAR
> > > >+	 */
> > > >+	if (pdev->rom&&  pdev->romlen) {
> > > >+		*size = pdev->romlen;
> > > >+		return phys_to_virt((phys_addr_t)pdev->rom);
> > >                        ^^^^^
> > >                          ioremap_nocache() ? ... or is caching rom ok?
> > 
> > If it's appearing through this pathway then it's not actually in ROM, 
> > the platform has pulled it out of somewhere else.
> 
> If that somewhere else is on the PCI bus then it should be a bus not a
> virt translation surely ?

We've no good way of knowing what the firmware's giving us - we copy it 
to RAM in the EFI init process, so by the time we're here it certainly 
shouldn't be on the PCI bus.

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

Re: [PATCH V3 3/4] PCI: Add support for non-BAR ROMs

[Alan Cox]

On Wed, 5 Sep 2012 14:20:07 +0100
Matthew Garrett <mjg@redhat.com> wrote:

> On Wed, Sep 05, 2012 at 01:46:21PM +0100, Alan Cox wrote:
> > On Wed, 5 Sep 2012 03:29:32 +0100
> > Matthew Garrett <mjg@redhat.com> wrote:
> > 
> > > On Tue, Sep 04, 2012 at 10:18:48PM -0400, Don Dutile wrote:
> > > > >  	/*
> > > > >+	 * Some devices may provide ROMs via a source other than the BAR
> > > > >+	 */
> > > > >+	if (pdev->rom&&  pdev->romlen) {
> > > > >+		*size = pdev->romlen;
> > > > >+		return phys_to_virt((phys_addr_t)pdev->rom);
> > > >                        ^^^^^
> > > >                          ioremap_nocache() ? ... or is caching rom ok?
> > > 
> > > If it's appearing through this pathway then it's not actually in ROM, 
> > > the platform has pulled it out of somewhere else.
> > 
> > If that somewhere else is on the PCI bus then it should be a bus not a
> > virt translation surely ?
> 
> We've no good way of knowing what the firmware's giving us - we copy it 
> to RAM in the EFI init process, so by the time we're here it certainly 
> shouldn't be on the PCI bus.

In which case how do you know that physical address you have been given
actually has a mapping in kernel virtual space. It seems like it ought to
be getting an ioremap in that case ?

phys_to_virt is not valid for arbitary addresses.

Alan

Re: [PATCH V3 3/4] PCI: Add support for non-BAR ROMs

[Matthew Garrett]

On Wed, Sep 05, 2012 at 02:30:10PM +0100, Alan Cox wrote:
> On Wed, 5 Sep 2012 14:20:07 +0100
> Matthew Garrett <mjg@redhat.com> wrote:
> > We've no good way of knowing what the firmware's giving us - we copy it 
> > to RAM in the EFI init process, so by the time we're here it certainly 
> > shouldn't be on the PCI bus.
> 
> In which case how do you know that physical address you have been given
> actually has a mapping in kernel virtual space. It seems like it ought to
> be getting an ioremap in that case ?
> 
> phys_to_virt is not valid for arbitary addresses.

The pages are allocated as EFI_LOADER_DATA, and that gets mapped as 
E820_RAM.

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

[PATCH V3 4/4] X86: Use PCI setup data

[Matthew Garrett]

EFI can provide PCI ROMs out of band via boot services, which may not be
available after boot. Add support for using the data handed off to us by
the boot stub or bootloader.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 arch/x86/pci/common.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 720e973f..3d2752f 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -17,6 +17,7 @@
 #include <asm/io.h>
 #include <asm/smp.h>
 #include <asm/pci_x86.h>
+#include <asm/setup.h>
 
 unsigned int pci_probe = PCI_PROBE_BIOS | PCI_PROBE_CONF1 | PCI_PROBE_CONF2 |
 				PCI_PROBE_MMCONF;
@@ -608,6 +609,38 @@ unsigned int pcibios_assign_all_busses(void)
 	return (pci_probe & PCI_ASSIGN_ALL_BUSSES) ? 1 : 0;
 }
 
+int pcibios_add_device(struct pci_dev *dev)
+{
+	struct setup_data *data;
+	struct pci_setup_rom *rom;
+	u64 pa_data;
+
+	if (boot_params.hdr.version < 0x0209)
+		return 0;
+
+	pa_data = boot_params.hdr.setup_data;
+	while (pa_data) {
+		data = phys_to_virt(pa_data);
+
+		if (data->type == SETUP_PCI) {
+			rom = (struct pci_setup_rom *)data;
+
+			if ((pci_domain_nr(dev->bus) == rom->segment) &&
+			    (dev->bus->number == rom->bus) &&
+			    (PCI_SLOT(dev->devfn) == rom->device) &&
+			    (PCI_FUNC(dev->devfn) == rom->function) &&
+			    (dev->vendor == rom->vendor) &&
+			    (dev->device == rom->devid)) {
+				dev->rom = (void *)(pa_data +
+				      offsetof(struct pci_setup_rom, romdata));
+				dev->romlen = rom->pcilen;
+			}
+		}
+		pa_data = data->next;
+	}
+	return 0;
+}
+
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
 	int err;
-- 
1.7.11.2

Re: Use PCI ROMs from EFI boot services

[David Woodhouse]

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

On Thu, 2012-08-23 at 12:36 -0400, Matthew Garrett wrote:
> V3 just fixes all the casting issues and incorporates David's change in
> search ordering.

I appreciate there are other issues with kexec under EFI, but let's not
make it worse. What is the plan for making this work *after* kexec? Do
these blobs get passed from the old kernel to the new? Does that work
already?

-- 
dwmw2

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

Re: Use PCI ROMs from EFI boot services

[Matthew Garrett]

On Fri, Aug 24, 2012 at 10:30:01AM +0100, David Woodhouse wrote:
> On Thu, 2012-08-23 at 12:36 -0400, Matthew Garrett wrote:
> > V3 just fixes all the casting issues and incorporates David's change in
> > search ordering.
> 
> I appreciate there are other issues with kexec under EFI, but let's not
> make it worse. What is the plan for making this work *after* kexec? Do
> these blobs get passed from the old kernel to the new? Does that work
> already?

kexec would presumably need to reconstruct the setup_data from the PCI 
objects.

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

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

[Bjorn Helgaas]

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.

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

[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@srcf.ucam.org

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

[Bjorn Helgaas]

On Wed, Dec 5, 2012 at 1:22 PM, Matthew Garrett <mjg59@srcf.ucam.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

[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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 958 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@intel.com                              Intel Corporation




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

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

[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@srcf.ucam.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: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

[Yinghai Lu]

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.

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

[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:

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

[H. Peter Anvin]

On 12/05/2012 05:13 PM, Matthew Garrett wrote:
>
>
> "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.
>

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

[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@srcf.ucam.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

[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

[Eric W. Biederman]

Matthew Garrett <mjg59@srcf.ucam.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

[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

[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

[Yinghai Lu]

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

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

[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@srcf.ucam.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.

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

[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