Add iSCSI iBFT support.
PATCH 1/1 Add iSCSI iBFT support.
This patch adds a /sysfs/firmware/ibft/table binary blob which exports the iSCSI Boot Firmware Table (iBFT) structure.
What is iSCSI Boot Firmware Table? It is a mechanism for the iSCSI tools to extract from the machine NICs the iSCSI connection information so that they can automagically mount the iSCSI share/target. Currently the iSCSI information is hard-coded in the initrd.
The full details of the structure are located at:
ftp://ftp.software.ibm.com/systems/support/system_x_pdf/ibm_iscsi_boot_firmware_table_v1.02.pdf
arch/i386/kernel/setup.c +13 -1
@@ -46,7 +46,7 @@ #include <linux/kexec.h>#include <linux/crash_dump.h> #include <linux/dmi.h> #include <linux/pfn.h> - +#include <linux/iscsi_ibft.h> #include <video/edid.h> #include <asm/apic.h>
@@ -150,6 +150,9 @@ static inline void copy_edd(void) }#endif +void *ibft_phys; +EXPORT_SYMBOL (ibft_phys); +int __initdata user_defined_memmap =0 ;/*
@@ -456,6 +459,15 @@ #ifdef CONFIG_KEXECreserve_bootmem (crashk_res.start, crashk_res.end - crashk_res.start +1 );#endif + +/* Scan for an iBFT (iSCSI Boot Firmware Table) */ + { +unsigned int ibft_len =find_ibft (); +if (ibft_len) +/* The specs says to scan for the table between 512k to 1MB. +We reserve it n case it is in the e820 RAM section. */ +reserve_bootmem (ibft_phys,PAGE_ALIGN (ibft_len)); + } }/*
arch/x86_64/kernel/setup.c +13 -0
@@ -44,6 +44,7 @@ #include <linux/cpufreq.h>#include <linux/dmi.h> #include <linux/dma-mapping.h> #include <linux/ctype.h> +#include <linux/iscsi_ibft.h> #include <asm/mtrr.h> #include <asm/uaccess.h>
@@ -196,6 +197,9 @@ static inline void copy_edd(void) }#endif +void *ibft_phys; +EXPORT_SYMBOL (ibft_phys); +#define EBDA_ADDR_POINTER 0x40Eunsigned __initdata ebda_addr;
@@ -365,6 +369,15 @@ #ifdef CONFIG_KEXEC crashk_res.end - crashk_res.start + 1); }#endif +/* Scan for an iBFT (iSCSI Boot Firmware Table) */ + { +unsigned int ibft_len =find_ibft (); +if (ibft_len) +/* The specs says to scan for the table between 512k to 1MB. +We reserve it in case it is in the e820 RAM section. */ +reserve_bootmem_generic ((unsigned long )ibft_phys, +PAGE_ALIGN (ibft_len)); + }paging_init ();
drivers/firmware/Kconfig +10 -0
@@ -93,4 +93,14 @@ config DMIID information from userspace through /sys/class/dmi/id/ or if you want DMI-based module auto-loading. +config ISCSI_IBFT + tristate "iSCSI Boot Firmware Table Attributes" + depends on X86 + default n + help + This option enables support for detection of an iSCSI + Boot Firmware Table (iBFT). If you wish to detect iSCSI boot + parameters dynamically during system boot, say Y. + Otherwise, say N. + endmenu
drivers/firmware/Makefile +1 -0
@@ -8,3 +8,4 @@ obj-$(CONFIG_EFI_PCDP) += pcdp.o obj-$(CONFIG_DELL_RBU) += dell_rbu.o obj-$(CONFIG_DCDBAS) += dcdbas.o obj-$(CONFIG_DMIID) += dmi-id.o +obj-$(CONFIG_ISCSI_IBFT) += iscsi_ibft.o
drivers/firmware/iscsi_ibft.c new file +201 -0
201 lines not on this page · raw message
include/linux/iscsi_ibft.h new file +58 -0
58 lines not on this page · raw message
Konrad Rzeszutek wrote:
[...]
drivers/firmware/iscsi_ibft.c line 64
Quoted 13 lines
drivers/firmware/iscsi_ibft.c new file +13
-0+static ssize_t +ibft_read_binary (struct kobject *kobj,struct bin_attribute *attr,char *buf, +loff_t off,size_t count) +{ + + struct ibft_device *ibft = container_of(kobj, struct ibft_device, kobj); +ssize_t len = ibft->hdr->length; + +if (off > len) +return 0 ; + +if (off + count > len) + count = len - off;
maybe you want to use:
count = min(count, len - off)
Quoted 5 lines
drivers/firmware/iscsi_ibft.c new file +5
-0+ +memcpy (buf, ibft->hdr + off, count); + +return count; +}
[...]
Quoted 21 lines
drivers/firmware/iscsi_ibft.c new file +21
-0+static struct ibft_device *ibft_idev; +/* +* ibft_init() - creates sysfs tree entry for ibft data +*/ +static int __init ibft_init (void ) +{ +int rc =0 ; + +printk (KERN_INFO "BIOS iBFT facility v%s %s\n" ,ISCSI_IBFT_VERSION , +ISCSI_IBFT_DATE ); + +if (!ibft_phys) +find_ibft (); + +/* What if the ibft_subsys is underneath another struct? */ + rc =firmware_register (&ibft_subsys); +if (rc) +return rc; + +if (ibft_phys) { + printk(KERN_INFO "iBFT detected at 0x%lx.\n",
drivers/firmware/iscsi_ibft.c line 184
Quoted 10 lines
drivers/firmware/iscsi_ibft.c new file +10
-0+ (unsigned long)ibft_phys); + ibft_idev =kzalloc (sizeof (*ibft_idev),GFP_KERNEL ); +if (!ibft_idev) +return -ENOMEM ; + + rc =ibft_device_register (ibft_idev); +if (rc) { +kfree (ibft_idev); +return rc; + }
you could do without this return statement (and the brackets) since rc is returned anyway...
drivers/firmware/iscsi_ibft.c line 187
Quoted 3 lines
drivers/firmware/iscsi_ibft.c new file +3
-0+ }else { +printk (KERN_INFO "No iBFT detected.\n" ); + }
these brackets are not required either
Review
Quoted 1 line
(quoted diff) +1
-0+ return rc;
... here
Quoted 1 line
+}
[...]
Roel
Quoted 2 lines · Konrad Rzeszutek
This patch adds a /sysfs/firmware/ibft/table binary blob which exports the iSCSI Boot Firmware Table (iBFT) structure.
Please don't do that. Binary files are for things that are "pass-through" only, not anything that the kernel knows the structure of, or cares about (like PCI config space, or firmware blobs for devices.)
Just export the individual fields of this table as individual files please.
thanks,
greg k-h
Quoted 11 lines · On Wednesday 26 September 2007 17:10:57 Greg KH wrote:
Quoted 2 lines · Konrad Rzeszutek
This patch adds a /sysfs/firmware/ibft/table binary blob which exports the iSCSI Boot Firmware Table (iBFT) structure.
Please don't do that. Binary files are for things that are "pass-through" only, not anything that the kernel knows the structure of, or cares about (like PCI config space, or firmware blobs for devices.)
Just export the individual fields of this table as individual files please.
My goal was to do that in the next version of this patch. My first step was to get the fundamental work reviewed (and hopefully accepted) and then build on top of that.
The exploiter of this binary file (/sys/firmware/ibft/table) is the iscsi-initiator-utils package and it has a library that parses the binary blob data. The thought was to get this first working (ie, iscsi-initiator-utils finds /sys/firmware/ibft/table, parses it and work) and then work to have the iscsi-initiator-support individual sysfs entries.
Or do you think I should skip the fundamental step and work on the next version of this patch that exports the data as individual data and post that one instead?
Quoted 4 lines
thanks,
greg k-h
Quoted 23 lines · Konrad Rzeszutek
Quoted 11 lines · On Wednesday 26 September 2007 17:10:57 Greg KH wrote:
Quoted 2 lines · Konrad Rzeszutek
This patch adds a /sysfs/firmware/ibft/table binary blob which exports the iSCSI Boot Firmware Table (iBFT) structure.
Please don't do that. Binary files are for things that are "pass-through" only, not anything that the kernel knows the structure of, or cares about (like PCI config space, or firmware blobs for devices.)
Just export the individual fields of this table as individual files please.
My goal was to do that in the next version of this patch. My first step was to get the fundamental work reviewed (and hopefully accepted) and then build on top of that.
The exploiter of this binary file (
/sys/firmware/ibft/table) is the iscsi-initiator-utils package and it has a library that parses the binary blob data. The thought was to get this first working (ie, iscsi-initiator-utils finds/sys/firmware/ibft/table, parses it and work) and then work to have the iscsi-initiator-support individual sysfs entries.
Quoted 3 lines
Or do you think I should skip the fundamental step and work on the next version of this patch that exports the data as individual data and post that one instead?
Just do the individual files, do not export binary structures through sysfs as that is not allowed.
The individual files should be much easier to export than the binary blog anyway :)
thanks,
greg k-h
Quoted 4 lines · On Wed, 26 Sep 2007 14:46:52 -0400 Konrad Rzeszutek wrote:
This patch adds a /sysfs/firmware/ibft/table binary blob which exports the iSCSI Boot Firmware Table (iBFT) structure.
What is iSCSI Boot Firmware Table?
i.e., what is this binary blob (?)
I don't see a binary blob in this patch (as stated in the first sentence). I'd say that this patch adds methods for exporting (or exposing) the ibft thru sysfs.
Is there some good reason that the iSCSI connection information shouldn't be exposed in real sysfs attribute files instead of just in a binary file?
Quoted 7 lines
It is a mechanism for the iSCSI
tools to extract from the machine NICs the iSCSI connection information so that they can automagically mount the iSCSI share/target. Currently the iSCSI information is hard-coded in the initrd.The full details of the structure are located at:
ftp://ftp.software.ibm.com/systems/support/system_x_pdf/ibm_iscsi_boot_firmware_table_v1.02.pdf
---
~Randy
Phaedrus says that Quality is about caring.
Quoted 6 lines
i.e., what is this binary blob (?)
I don't see a binary blob in this patch (as stated in the first sentence). I'd say that this patch adds methods for exporting (or exposing) the ibft thru sysfs.
I used the wrong choice of words. The correct one is, as you say, to add methods for exporting the iBFT through sysfs.
Quoted 4 lines
Is there some good reason that the iSCSI connection information shouldn't be exposed in real sysfs attribute files instead of just in a binary file?
My end goal is to export the iBFT data via individual sysfs attribute files. I was thinking to do that in the next version of this code and build on top of this patch.
This way the existing exploiter (iscsi-initiator-utils) can use the parsing code it already has to extract the data from the binary blob. Then in the next version of the iscsi-initiator-utils (and for the kernel) I can post a patch for supporting (and exporting in the kernel) individual sysfs attribute files.
Quoted 19 lines · On Wed, 26 Sep 2007 14:46:52 -0400 Konrad Rzeszutek wrote:
This patch adds a /sysfs/firmware/ibft/table binary blob which exports the iSCSI Boot Firmware Table (iBFT) structure.
What is iSCSI Boot Firmware Table? It is a mechanism for the iSCSI tools to extract from the machine NICs the iSCSI connection information so that they can automagically mount the iSCSI share/target. Currently the iSCSI information is hard-coded in the initrd.
The full details of the structure are located at:
ftp://ftp.software.ibm.com/systems/support/system_x_pdf/ibm_iscsi_boot_firmware_table_v1.02.pdfSigned-off-by Konrad Rzeszutek <[email protected]>Signed-off-by Peter Jones <[email protected]>drivers/firmware/Kconfig +0
-0
drivers/firmware/Kconfig config DMIID · line 98
Quoted 6 lines
drivers/firmware/Kconfig +3
-0@@ -93,4 +93,14 @@ config DMIID information from userspace through /sys/class/dmi/id/ or if you want DMI-based module auto-loading. +config ISCSI_IBFT + tristate "iSCSI Boot Firmware Table Attributes" + depends on X86
why only on X86?
Quoted 14 lines
drivers/firmware/Kconfig +7
-0+ default n + help + This option enables support for detection of an iSCSI + Boot Firmware Table (iBFT). If you wish to detect iSCSI boot + parameters dynamically during system boot, say Y. + Otherwise, say N. + endmenudrivers/firmware/iscsi_ibft.c new file +0
-0
drivers/firmware/iscsi_ibft.c line 2
Quoted 2 lines
drivers/firmware/iscsi_ibft.c new file +2
-0@@ -0,0 +1,201 @@ +/* +* drivers/firmware/iscsi_ibft.c
Don't repeat the file name.
drivers/firmware/iscsi_ibft.c line 8
Quoted 6 lines
drivers/firmware/iscsi_ibft.c new file +6
-0+* Copyright 2007 Red Hat, Inc. +* by Peter Jones <[email protected]> +* Copyright 2007 IBM +* by Konrad Rzeszutek <[email protected]> +* +* This code exposes the the iSCSI Boot Format Table to userland via sysfs.
~~~~~~~
yes.
Quoted 5 lines
drivers/firmware/iscsi_ibft.c new file +5
-0+*/ + +#include <linux/module.h> +#include <linux/string.h> +#include <linux/types.h>
drivers/firmware/iscsi_ibft.c line 32
Quoted 10 lines
drivers/firmware/iscsi_ibft.c new file +10
-0+#include <linux/init.h> +#include <linux/stat.h> +#include <linux/err.h> +#include <linux/ctype.h> +#include <linux/slab.h> +#include <linux/limits.h> +#include <linux/device.h> +#include <linux/pci.h> +#include <linux/blkdev.h> +
no blank line here.
Quoted 5 lines
drivers/firmware/iscsi_ibft.c new file +5
-0+#include <linux/iscsi_ibft.h> + +#define ISCSI_IBFT_VERSION "0.2" +#define ISCSI_IBFT_DATE "2007-Aug-29" +
drivers/firmware/iscsi_ibft.c line 54
Quoted 10 lines
drivers/firmware/iscsi_ibft.c new file +10
-0+MODULE_AUTHOR +("Peter Jones <[email protected]> and Konrad Rzeszutek <[email protected]>" ); +MODULE_DESCRIPTION ("sysfs interface to BIOS iBFT information" ); +MODULE_LICENSE ("GPL" ); +MODULE_VERSION (ISCSI_IBFT_VERSION ); + + +static ssize_t +ibft_read_binary (struct kobject *kobj,struct bin_attribute *attr,char *buf, +loff_t off,size_t count)
Put 'static ssize_t' on same line as function name, then put parameters on following lines as needed.
Quoted 8 lines
drivers/firmware/iscsi_ibft.c new file +8
-0+{ + + struct ibft_device *ibft = container_of(kobj, struct ibft_device, kobj); +ssize_t len = ibft->hdr->length; + +if (off > len) +return 0 ; +
drivers/firmware/iscsi_ibft.c line 72
Quoted 10 lines
drivers/firmware/iscsi_ibft.c new file +10
-0+if (off + count > len) + count = len - off; + +memcpy (buf, ibft->hdr + off, count); + +return count; +} +static int +ibft_mmap_binary (struct kobject *kobj,struct bin_attribute *attr, +struct vm_area_struct *vma)
ditto.
Quoted 1 line
drivers/firmware/iscsi_ibft.c new file +1
-0+{
...
Quoted 21 lines
drivers/firmware/iscsi_ibft.c new file +21
-0+} +static struct bin_attribute ibft_attribute_binary = { + .attr = { + .name ="binary" , + .mode =S_IRUSR , + .owner =THIS_MODULE , + }, + .read = ibft_read_binary, + .write =NULL , + .mmap = ibft_mmap_binary +}; +static struct kobj_type ktype_ibft = { + .release = ibft_release, +}; + +static decl_subsys(ibft, &ktype_ibft, NULL); + +static int ibft_device_register (struct ibft_device *idev) +{ +int error =0 ; +int len =0 ;
drivers/firmware/iscsi_ibft.c line 132
Quoted 10 lines
drivers/firmware/iscsi_ibft.c new file +10
-0+struct ibft_header *hdr; + +if (!idev) +return 1 ; + +/* Copy over the data */ + hdr = (struct ibft_header *)phys_to_virt ((unsigned long )ibft_phys); + len = hdr->length; + +/* Need PAGE_ALING for mmap functionality. */
PAGE_ALIGN
Quoted 22 lines
drivers/firmware/iscsi_ibft.c new file +22
-0+ idev->hdr =kzalloc (PAGE_ALIGN (len),GFP_KERNEL ); +if (!idev->hdr) +return -ENOMEM ; + +memcpy (idev->hdr, hdr, len); + +/* This is firmware/ibft */ +kobject_set_name (&idev->kobj,"table" ); +kobj_set_kset_s (idev, ibft_subsys); + error =kobject_register (&idev->kobj); + +if (!error) { + ibft_attribute_binary.size = idev->hdr->length; + error = +sysfs_create_bin_file (&idev->kobj, &ibft_attribute_binary); + } + +/* The de-allocation part is done by module_exit() */ +return error; +} + +static struct ibft_device *ibft_idev;
...
drivers/firmware/iscsi_ibft.c line 197
Quoted 7 lines
drivers/firmware/iscsi_ibft.c new file +7
-0+static void __exit ibft_exit(void) +{ +if (ibft_idev) +kobject_unregister (&ibft_idev->kobj); + +firmware_unregister (&ibft_subsys); +printk (KERN_INFO "BIOS iBFT unloaded.\n" );
Drop the unload message. ibft_init() is also quite noisy.
Quoted 28 lines
drivers/firmware/iscsi_ibft.c new file +4
-0+} + +module_init (ibft_init); +module_exit (ibft_exit);include/linux/iscsi_ibft.h new file +18
-0@@ -0,0 +1,58 @@ +#ifndef ISCSI_IBFT_H +#define ISCSI_IBFT_H + +extern void *ibft_phys; + +struct ibft_header { + char signature[4]; + u32 length; + u8 revision; + u8 checksum; + char oem_id[6]; + char oem_table_id[8]; + char reserved[24]; +}; + +struct ibft_device { + struct ibft_header *hdr; + struct kobject kobj;
include/linux/iscsi_ibft.h line 28
Quoted 10 lines
include/linux/iscsi_ibft.h new file +10
-0+}; + +#if defined(CONFIG_ISCSI_IBFT ) || defined(CONFIG_ISCSI_IBFT_MODULES ) + +#define IBFT_SIGN "iBFT" +#define IBFT_SIGN_LEN 4 +#define IBFT_START 0x80000/* 512kB */ +#define IBFT_END 0x100000/* 1MB */ +#define VGA_MEM 0xA0000/* VGA buffer */ +#define VGA_SIZE 0x20000/* 132kB */
Need blank line here... except why is this function in the header file? and why is it inline?
include/linux/iscsi_ibft.h line 31
Quoted 3 lines
include/linux/iscsi_ibft.h new file +3
-0+static inline ssize_t find_ibft (void ) +{ +unsigned long pos;
add blank line here between data / code.
Quoted 18 lines
include/linux/iscsi_ibft.h new file +18
-0+for (pos =IBFT_START ; pos <IBFT_END ; pos +=16 ) { +void *virt; +/* The table can't be inside the VGA BIOS reserved space, +* so skip that area */ +if (pos ==VGA_MEM -PAGE_SIZE ) + pos +=VGA_SIZE +PAGE_SIZE ; + virt =phys_to_virt (pos); +if (memcmp (virt,IBFT_SIGN ,IBFT_SIGN_LEN ) ==0 ) { +unsigned long *addr = + (unsigned long *)phys_to_virt (pos +4 ); +unsigned int len = *addr; +/* if the length of the table extends past 1M, +* the table cannot be valid. */ +if (pos + len <= (IBFT_END -1 )) { + ibft_phys = (void *)pos; +return len; + } + }
include/linux/iscsi_ibft.h
Quoted 10 lines
include/linux/iscsi_ibft.h new file +9
-1+ } +return 0 ; +} + +#else + +static inline ssize_t find_ibft (void ) {return 0 ; }; +#endif +#endif /* ISCSI_IBFT_H */ -
---
~Randy
Phaedrus says that Quality is about caring.
Quoted 5 lines
drivers/firmware/Kconfig config DMIID · line 98
Quoted 3 lines
drivers/firmware/Kconfig +3
-0+config ISCSI_IBFT + tristate "iSCSI Boot Firmware Table Attributes" + depends on X86why only on X86?
PowerPC exports this data via the OpenFirmware so it already shows in the /sysfs entries. I was thinking to combine those sysfs entries under this code, but that is something in the future.
In regards to all other platforms, I figured I would only make it supported under platforms that have been tested. Is there anything that stops this from working for example of IA64? Well no. The hardware that supports the iBFT is either in the BIOS or in NICs - so the SGI or HP boxes _should_ work, however I am not comfortable to make it supported unless I've tested it.
Quoted 3 lines
Quoted 1 line
+ *
drivers/firmware/iscsi_ibft.cDon't repeat the file name.
OK.
Quoted 5 lines
Quoted 2 lines
+ * This code exposes the the iSCSI Boot Format Table to userland via sysfs.
~~~~~~~yes.
Yup.
Quoted 3 lines
Quoted 1 line
+
no blank line here.
Fixed.
Quoted 6 lines
Quoted 3 lines
(quoted diff) +2
-0+static ssize_t +ibft_read_binary(struct kobject *kobj, struct bin_attribute *attr, char*buf, +
loff_toff,size_tcount)Put 'static
ssize_t' on same line as function name, then put parameters on following lines as needed.
Fixed.
Quoted 5 lines
drivers/firmware/iscsi_ibft.c line 72
Quoted 3 lines
drivers/firmware/iscsi_ibft.c new file +3
-0+static int +ibft_mmap_binary (struct kobject *kobj,struct bin_attribute *attr, +struct vm_area_struct *vma)ditto.
Fixed.
Quoted 3 lines
drivers/firmware/iscsi_ibft.c line 132
Quoted 1 line
drivers/firmware/iscsi_ibft.c new file +1
-0+/* Need PAGE_ALING for mmap functionality. */
PAGE_ALIGN
Fixed.
Quoted 3 lines
drivers/firmware/iscsi_ibft.c line 197
Quoted 1 line
drivers/firmware/iscsi_ibft.c new file +1
-0+printk (KERN_INFO "BIOS iBFT unloaded.\n" );Drop the unload message.
ibft_init()is also quite noisy.
Fixed.
Quoted 2 lines
Need blank line here... except why is this function in the header
Fixed blank line.
Quoted 1 line
file? and why is it inline?
Q: "Why is this function in the header file"
If the find_ibft() was to be implemented in firmware/iscsi_ibft.c code the firmware-driver couldn't be compiled as a module (or rather it could, but when the vmlinuz was to be linked it would complain about missing symbol - find_ibft). I was thinking to let the user give the choice whether they want to load this firmware driver or have it built-in the kernel.
Q:"Why is it inline"
Uhh. It does not need to. I will remove the 'inline' part.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +1
-0+ unsigned long pos;add blank line here between data / code.
Fixed.
Randy,
Thank you for taking time to do such thoroughly review.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 11 lines · Ram Dorai
Fixed.
> > +static int
> > +ibft_mmap_binary(struct kobject *kobj, structbin_attribute
*attr,
> > + structvm_area_struct*vma)
>Do we not put a space between binary and '('. Is that against the coding guidelines?
Right, we do not put a space there.
It's read_binary(ko, attr, vma);
so function names have no space following them, but if, for, switch, and while do have space following them.
Signature · 2 lines
~Randy Phaedrus says that Quality is about caring.
Quoted 10 lines · On Wed, 26 Sep 2007 20:52:43 -0400 Konrad Rzeszutek wrote:
Quoted 5 lines
drivers/firmware/Kconfig config DMIID · line 98
Quoted 3 lines
drivers/firmware/Kconfig +3
-0+config ISCSI_IBFT + tristate "iSCSI Boot Firmware Table Attributes" + depends on X86why only on X86?
PowerPC exports this data via the OpenFirmware so it already shows in the /sysfs entries. I was thinking to combine those sysfs entries under this code, but that is something in the future.
Quoted 5 lines
In regards to all other platforms, I figured I would only make it supported under platforms that have been tested. Is there anything that stops this from working for example of IA64? Well no. The hardware that supports the iBFT is either in the BIOS or in NICs - so the SGI or HP boxes _should_ work, however I am not comfortable to make it supported unless I've tested it.
That's not how Linux development works. You (we) have a huge test lab around the world. You practice "release early, release often" and get testing/feedback on it. Maybe even patches. :)
Quoted 12 lines
Quoted 1 line
Need blank line here... except why is this function in the header
Fixed blank line.
Quoted 1 line
file? and why is it inline?
Q: "Why is this function in the header file"
If thefind_ibft()was to be implemented infirmware/iscsi_ibft.ccode the firmware-driver couldn't be compiled as a module (or rather it could, but when the vmlinuz was to be linked it would complain about missing symbol -find_ibft). I was thinking to let the user give the choice whether they want to load this firmware driver or have it built-in the kernel.
Quoted 2 lines
Q:"Why is it inline"
Uhh. It does not need to. I will remove the 'inline' part.
But we strongly prefer not to have non-inline C code in header files, [and the function does not need to be inline]
so find_ibft() needs a home in some source file/code that won't be built as a loadable module, right? And preferably not duplicated (i386 & x86_64 versions; but we should see a merged x86/ arch soon, so it sounds). Would ia64 have its own version of find_ibft() or use this same code?
I think that for now you can put find_ibft() in both setup.c files and the merged x86/ arch tree can eliminate one of them.
On looking back at the patch, why aren't the ibft_phys and find_ibft() parts of both setup.c patches surrounded by #ifdef CONFIG_ISCSI_IBFT & #endif ?
---
~Randy
Phaedrus says that Quality is about caring.
Quoted 9 lines · Konrad Rzeszutek
Quoted 4 lines
drivers/firmware/Kconfig config DMIID · line 98
Quoted 3 lines
drivers/firmware/Kconfig +3
-0+config ISCSI_IBFT + tristate "iSCSI Boot Firmware Table Attributes" + depends on X86why only on X86?
PowerPC exports this data via the OpenFirmware so it already shows in the /sysfs entries. I was thinking to combine those sysfs entries under this code, but that is something in the future.
Quoted 6 lines
In regards to all other platforms, I figured I would only make it supported under platforms that have been tested. Is there anything that stops this from working for example of IA64? Well no. The hardware that supports the iBFT is either in the BIOS or in NICs - so the SGI or HP boxes _should_ work, however I am not comfortable to make it supported unless I've tested it.
It should, presumably, depend on ACPI, rather than on X86...?
-hpa
Peter Jones
maintainerQuoted 11 lines · H. Peter Anvin
Quoted 8 lines · Konrad Rzeszutek
Quoted 4 lines
drivers/firmware/Kconfig config DMIID · line 98
Quoted 3 lines
drivers/firmware/Kconfig +3
-0+config ISCSI_IBFT + tristate "iSCSI Boot Firmware Table Attributes" + depends on X86why only on X86?
PowerPC exports this data via the OpenFirmware so it already shows in the /sysfs entries. I was thinking to combine those sysfs entries under this code, but that is something in the future.
In regards to all other platforms, I figured I would only make it supported under platforms that have been tested. Is there anything that stops this from working for example of IA64? Well no. The hardware that supports the iBFT is either in the BIOS or in NICs - so the SGI or HP boxes _should_ work, however I am not comfortable to make it supported unless I've tested it.
It should, presumably, depend on ACPI, rather than on X86...?
Actually no. That /should/ be the correct answer, but none of the hardware vendors actually provide the table via ACPI yet. Also, if they did, the support for /sys/firmware/acpi/tables/* would be sufficient instead of having this code *at all*.
Signature · 1 line
Peter
Quoted 8 lines · Peter Jones
Quoted 2 lines
It should, presumably, depend on ACPI, rather than on X86...?
Actually no. That /should/ be the correct answer, but none of the hardware vendors actually provide the table via ACPI yet. Also, if they did, the support for
/sys/firmware/acpi/tables/*would be sufficient instead of having this code *at all*.
Is there anything other than the discovery which is braindead about iBFT? If so, can the tables code be taught to look for this additional table instead of having all its own mechanism?
(Sorry - I don't really know the ACPI code all that well.)
-hpa
Peter Jones
maintainerQuoted 11 lines · H. Peter Anvin
Quoted 6 lines · Peter Jones
Quoted 1 line
It should, presumably, depend on ACPI, rather than on X86...?
Actually no. That /should/ be the correct answer, but none of the hardware vendors actually provide the table via ACPI yet. Also, if they did, the support for
/sys/firmware/acpi/tables/*would be sufficient instead of having this code *at all*.Is there anything other than the discovery which is braindead about iBFT? If so, can the tables code be taught to look for this additional table instead of having all its own mechanism?
Well, the code for the the generic ACPI table sysfs functionality is expecting to find the tables indexed in the RSDT. This is essentially what the iBFT spec's authors seem to have planned, but it's simply never been implemented in the firmware.
AFAICS, it's technically feasible to remove the sysfs parts of this code entirely, make the probe code build a fake ACPI table header, and then add it explicitly if present at the end of acpi_system_sysfs_init() .
I don't know how the ACPI guys would feel about that. Len, thoughts?
Signature · 1 line
Peter
Quoted 18 lines · Peter Jones
Quoted 11 lines · H. Peter Anvin
Quoted 6 lines · Peter Jones
Quoted 1 line
It should, presumably, depend on ACPI, rather than on X86...?
Actually no. That /should/ be the correct answer, but none of the hardware vendors actually provide the table via ACPI yet. Also, if they did, the support for
/sys/firmware/acpi/tables/*would be sufficient instead of having this code *at all*.Is there anything other than the discovery which is braindead about iBFT? If so, can the tables code be taught to look for this additional table instead of having all its own mechanism?
Well, the code for the the generic ACPI table sysfs functionality is expecting to find the tables indexed in the RSDT. This is essentially what the iBFT spec's authors seem to have planned, but it's simply never been implemented in the firmware.
Quoted 5 lines
AFAICS, it's technically feasible to remove the sysfs parts of this code entirely, make the probe code build a fake ACPI table header, and then add it explicitly if present at the end of
acpi_system_sysfs_init().I don't know how the ACPI guys would feel about that. Len, thoughts?
Technically, ACPI knows only about devices that are soldered onto the motherboard when the BIOS is flashed.
So ACPI wouldn't be able to help you find this table when you put in an iSCSI card -- unless there were some arrangement between the motherboard BIOS and the add-in card BIOS extension, say, to have a dummy table that gets filled in or something.
Yes, OpenFirmware is extensible via add-in cards -- assuming they support OpenFirmware -- so in theory they could do this right even in the add-in case.
I wouldn't object to a platform hook to make the IBFT appear as just another ACPI table available in /sys/firmware/acpi. But that woudln't help a system that has IBFT but doesn't have ACPI, so it doesn't really solve the general problem.
For tables such as this,
ACPI is just the messenger anyway, we don't know anything about the content of the table, just about the envelope around it and how to find the envelope. In this case, we don't even know how to find the envelope, so ACPI doesn't add much value to the IBFT.
In summary, I think IBFT should depend on iSCSI alone, and not involve ACPI at all.
-Len