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
Signed-off-by: Konrad Rzeszutek <[email protected]>
Signed-off-by: Peter Jones <[email protected]>
diff --git a/arch/i386/kernel/setup.c b/arch/i386/kernel/setup.c
index d474cd6..11d700f 100644
--- a/arch/i386/kernel/setup.c
+++ b/arch/i386/kernel/setup.c
@@ -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_KEXEC
reserve_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));
+ }
}
/*
diff --git a/arch/x86_64/kernel/setup.c b/arch/x86_64/kernel/setup.c
index af838f6..0d12775 100644
--- a/arch/x86_64/kernel/setup.c
+++ b/arch/x86_64/kernel/setup.c
@@ -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 0x40E
unsigned __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();
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 05f02a3..2d9f01a 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -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
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 8d4ebc8..b6319f7 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -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
diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c
new file mode 100644
index 0000000..b3767fe
--- /dev/null
+++ b/drivers/firmware/iscsi_ibft.c
@@ -0,0 +1,201 @@
+/*
+ * drivers/firmware/iscsi_ibft.c
+ * 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.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License v2.0 as published by
+ * the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#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>
+
+#include <linux/iscsi_ibft.h>
+
+#define ISCSI_IBFT_VERSION "0.2"
+#define ISCSI_IBFT_DATE "2007-Aug-29"
+
+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 void ibft_release(struct kobject *kobj)
+{
+ struct ibft_device *ibft = container_of(kobj, struct ibft_device, kobj);
+ kfree(ibft->hdr);
+ kfree(ibft);
+}
+
+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;
+
+ 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)
+{
+ struct ibft_device *ibft = container_of(kobj, struct ibft_device, kobj);
+ ssize_t len = ibft->hdr->length;
+ unsigned long start = vma->vm_start;
+ unsigned long size = vma->vm_end - vma->vm_start;
+ unsigned long off = vma->vm_pgoff << PAGE_SHIFT;
+ unsigned long pos;
+ unsigned long pfn;
+ int i;
+
+ pos = (unsigned long)ibft->hdr;
+
+ if (off > len)
+ return -EINVAL;
+
+ if (vma->vm_flags & VM_WRITE)
+ return -EPERM;
+
+ for (i = 0; i < len; i += PAGE_SIZE) {
+ pfn = virt_to_phys((void *)(pos + off)) >> PAGE_SHIFT;
+ if (remap_pfn_range
+ (vma, start, pfn, PAGE_SIZE, vma->vm_page_prot))
+ return -EAGAIN;
+ start += PAGE_SIZE;
+ if (size <= PAGE_SIZE)
+ break;
+ size -= PAGE_SIZE;
+ }
+ return 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;
+ 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. */
+ 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;
+/*
+ * 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",
+ (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;
+ }
+ } else {
+ printk(KERN_INFO "No iBFT detected.\n");
+ }
+ return rc;
+}
+
+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");
+}
+
+module_init(ibft_init);
+module_exit(ibft_exit);
diff --git a/include/linux/iscsi_ibft.h b/include/linux/iscsi_ibft.h
new file mode 100644
index 0000000..5e7b267
--- /dev/null
+++ b/include/linux/iscsi_ibft.h
@@ -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;
+};
+
+#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 */
+static inline ssize_t find_ibft(void)
+{
+ unsigned long pos;
+ 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;
+ }
+ }
+ }
+ return 0;
+}
+
+#else
+
+static inline ssize_t find_ibft(void) { return 0; };
+#endif
+#endif /* ISCSI_IBFT_H */
Konrad Rzeszutek wrote:
[...]
> +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)
> +
> + memcpy(buf, ibft->hdr + off, count);
> +
> + return count;
> +}
[...]
> +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",
> + (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...
> + } else {
> + printk(KERN_INFO "No iBFT detected.\n");
> + }
these brackets are not required either
> + return rc;
... here
> +}
[...]
Roel
On Wed, Sep 26, 2007 at 02:46:52PM -0400, Konrad Rzeszutek wrote:
> 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
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?
> 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.
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.pdf
>
> Signed-off-by: Konrad Rzeszutek <[email protected]>
> Signed-off-by: Peter Jones <[email protected]>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 05f02a3..2d9f01a 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -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?
> + 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
> diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c
> new file mode 100644
> index 0000000..b3767fe
> --- /dev/null
> +++ b/drivers/firmware/iscsi_ibft.c
> @@ -0,0 +1,201 @@
> +/*
> + * drivers/firmware/iscsi_ibft.c
Don't repeat the file name.
> + * 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.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#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.
> +#include <linux/iscsi_ibft.h>
> +
> +#define ISCSI_IBFT_VERSION "0.2"
> +#define ISCSI_IBFT_DATE "2007-Aug-29"
> +
> +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.
> +{
> +
> + 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;
> +
> + 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.
> +{
...
> +}
> +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;
> + 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
> + 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;
...
> +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.
> +}
> +
> +module_init(ibft_init);
> +module_exit(ibft_exit);
> diff --git a/include/linux/iscsi_ibft.h b/include/linux/iscsi_ibft.h
> new file mode 100644
> index 0000000..5e7b267
> --- /dev/null
> +++ b/include/linux/iscsi_ibft.h
> @@ -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;
> +};
> +
> +#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?
> +static inline ssize_t find_ibft(void)
> +{
> + unsigned long pos;
add blank line here between data / code.
> + 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;
> + }
> + }
> + }
> + 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.
>
> 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.
>
> 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.
On Wednesday 26 September 2007 17:10:57 Greg KH wrote:
> On Wed, Sep 26, 2007 at 02:46:52PM -0400, Konrad Rzeszutek wrote:
> > 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?
>
> thanks,
>
> greg k-h
> > +config ISCSI_IBFT
> > + tristate "iSCSI Boot Firmware Table Attributes"
> > + depends on X86
>
> why 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.
> > + * drivers/firmware/iscsi_ibft.c
>
> Don't repeat the file name.
OK.
> > + * This code exposes the the iSCSI Boot Format Table to userland via
> > sysfs.
>
> ~~~~~~~
> yes.
Yup.
> > +
>
> no blank line here.
Fixed.
> > +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.
Fixed.
> > +static int
> > +ibft_mmap_binary(struct kobject *kobj, struct bin_attribute *attr,
> > + struct vm_area_struct *vma)
>
> ditto.
Fixed.
> > + /* Need PAGE_ALING for mmap functionality. */
>
> PAGE_ALIGN
Fixed.
> > + printk(KERN_INFO "BIOS iBFT unloaded.\n");
>
> Drop the unload message. ibft_init() is also quite noisy.
Fixed.
>
> Need blank line here... except why is this function in the header
Fixed blank 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.
>
> > + unsigned long pos;
>
> add blank line here between data / code.
Fixed.
Randy,
Thank you for taking time to do such thoroughly review.
On Wed, Sep 26, 2007 at 08:08:45PM -0400, Konrad Rzeszutek wrote:
> On Wednesday 26 September 2007 17:10:57 Greg KH wrote:
> > On Wed, Sep 26, 2007 at 02:46:52PM -0400, Konrad Rzeszutek wrote:
> > > 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?
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
Ram Dorai wrote:
>
> Fixed.
> > > +static int
> > > +ibft_mmap_binary(struct kobject *kobj, struct bin_attribute
> *attr,
> > > + struct vm_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.
--
~Randy
Phaedrus says that Quality is about caring.
On Wed, 26 Sep 2007 20:52:43 -0400 Konrad Rzeszutek wrote:
> > > +config ISCSI_IBFT
> > > + tristate "iSCSI Boot Firmware Table Attributes"
> > > + depends on X86
> >
> > why 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.
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. :)
> > Need blank line here... except why is this function in the header
>
> Fixed blank 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.
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.
Konrad Rzeszutek wrote:
>>> +config ISCSI_IBFT
>>> + tristate "iSCSI Boot Firmware Table Attributes"
>>> + depends on X86
>> why 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...?
-hpa
H. Peter Anvin wrote:
> Konrad Rzeszutek wrote:
>>>> +config ISCSI_IBFT
>>>> + tristate "iSCSI Boot Firmware Table Attributes"
>>>> + depends on X86
>>> why 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*.
--
Peter
Peter Jones wrote:
>>
>> 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
H. Peter Anvin wrote:
> Peter Jones wrote:
>>> 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?
--
Peter
On Thursday 27 September 2007 13:51, Peter Jones wrote:
> H. Peter Anvin wrote:
> > Peter Jones wrote:
> >>> 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?
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