Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
> That being said, I don't think there's any reason to expect the table to > show up on anything but i386 and x86_64, and maybe ia64. I've posted a new patch (http://lkml.org/lkml/2008/1/30/531) that includes that dependency in the Kconfig (i386, x86_64, ia64) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
That being said, I don't think there's any reason to expect the table to show up on anything but i386 and x86_64, and maybe ia64. I've posted a new patch (http://lkml.org/lkml/2008/1/30/531) that includes that dependency in the Kconfig (i386, x86_64, ia64) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI IBFT support (v0.4.5) - fixes to the header files.
On Tuesday 29 January 2008 14:15:15 Mike Christie wrote:
> Konrad Rzeszutek wrote:
> > +/*
> > + * Helper functions to parse data properly.
> > + */
> > +static ssize_t sprintf_ipaddr(char *buf, u8 *ip)
> > +{
> > + if (ip[0] == 0 && ip[1] == 0 && ip[2] == 0 && ip[3] == 0 &&
> > + ip[4] == 0 && ip[5] == 0 && ip[6] == 0 && ip[7] == 0 &&
> > + ip[8] == 0 && ip[9] == 0 && ip[10] == 0xff && ip[11] == 0xff) {
> > + /*
> > +* IPV4
> > +*/
> > + return sprintf(buf, "%d.%d.%d.%d\n", ip[12],
> > + ip[13], ip[14], ip[15]);
> > + } else
> > + return 0;
> > +}
>
> You probably just want to use the NIPQUAD_FMT and NIP6_FMT macros here.
Ah, I knew a macro like this _ought_ to be there somewhere. Thanks.
> Also why isn't ipv6 supported?
I didn't get to test the IPV6 yet so I didn't want to put in code that might
have worked or not :-(
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
On Tue, Jan 29, 2008 at 01:13:28PM -0600, Mike Christie wrote: > Konrad Rzeszutek wrote: >> On Sunday 27 January 2008 01:01:23 you wrote: On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek <[EMAIL PROTECTED]> wrote: Hey Andrew, Please add this patch along with Greg KH's kobject fixes. >>> erm, OK. But I don't think I'm the appropriate conduit for iscsi paches. >>> >>> By what path _does_ iscsi ode get into the tree, anyway? Mike is listed >>> as >>> maintainer... >> This is a bit tricky b/c this goes to the drivers/firmware and also >> depends on the kobject changes in Greg KH tree. But I should have included >> Mike on the CC which I keep on forgetting . > > It is probably better if it goes through Greg or Andrew. It will not > conflict with any iscsi patches. It looks like it is heavier on kobject and > sysfs and has some acpi digging magic, and almost no iscsi stuff in there. It does not need to go through me as all of the kobject changes it depended on are now in Linus's tree. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI IBFT support (v0.4.5) - fixes to the header files.
Konrad Rzeszutek wrote:
+/*
+ * Helper functions to parse data properly.
+ */
+static ssize_t sprintf_ipaddr(char *buf, u8 *ip)
+{
+ if (ip[0] == 0 && ip[1] == 0 && ip[2] == 0 && ip[3] == 0 &&
+ ip[4] == 0 && ip[5] == 0 && ip[6] == 0 && ip[7] == 0 &&
+ ip[8] == 0 && ip[9] == 0 && ip[10] == 0xff && ip[11] == 0xff) {
+ /*
+* IPV4
+*/
+ return sprintf(buf, "%d.%d.%d.%d\n", ip[12],
+ ip[13], ip[14], ip[15]);
+ } else
+ return 0;
+}
You probably just want to use the NIPQUAD_FMT and NIP6_FMT macros here.
Also why isn't ipv6 supported?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
Konrad Rzeszutek wrote: On Sunday 27 January 2008 01:01:23 you wrote: On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek <[EMAIL PROTECTED]> wrote: Hey Andrew, Please add this patch along with Greg KH's kobject fixes. erm, OK. But I don't think I'm the appropriate conduit for iscsi paches. By what path _does_ iscsi ode get into the tree, anyway? Mike is listed as maintainer... This is a bit tricky b/c this goes to the drivers/firmware and also depends on the kobject changes in Greg KH tree. But I should have included Mike on the CC which I keep on forgetting . It is probably better if it goes through Greg or Andrew. It will not conflict with any iscsi patches. It looks like it is heavier on kobject and sysfs and has some acpi digging magic, and almost no iscsi stuff in there. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
Konrad Rzeszutek wrote: On Sunday 27 January 2008 01:01:23 you wrote: On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek [EMAIL PROTECTED] wrote: Hey Andrew, Please add this patch along with Greg KH's kobject fixes. erm, OK. But I don't think I'm the appropriate conduit for iscsi paches. By what path _does_ iscsi ode get into the tree, anyway? Mike is listed as maintainer... This is a bit tricky b/c this goes to the drivers/firmware and also depends on the kobject changes in Greg KH tree. But I should have included Mike on the CC which I keep on forgetting sigh. It is probably better if it goes through Greg or Andrew. It will not conflict with any iscsi patches. It looks like it is heavier on kobject and sysfs and has some acpi digging magic, and almost no iscsi stuff in there. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
On Tue, Jan 29, 2008 at 01:13:28PM -0600, Mike Christie wrote: Konrad Rzeszutek wrote: On Sunday 27 January 2008 01:01:23 you wrote: On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek [EMAIL PROTECTED] wrote: Hey Andrew, Please add this patch along with Greg KH's kobject fixes. erm, OK. But I don't think I'm the appropriate conduit for iscsi paches. By what path _does_ iscsi ode get into the tree, anyway? Mike is listed as maintainer... This is a bit tricky b/c this goes to the drivers/firmware and also depends on the kobject changes in Greg KH tree. But I should have included Mike on the CC which I keep on forgetting sigh. It is probably better if it goes through Greg or Andrew. It will not conflict with any iscsi patches. It looks like it is heavier on kobject and sysfs and has some acpi digging magic, and almost no iscsi stuff in there. It does not need to go through me as all of the kobject changes it depended on are now in Linus's tree. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI IBFT support (v0.4.5) - fixes to the header files.
Konrad Rzeszutek wrote:
+/*
+ * Helper functions to parse data properly.
+ */
+static ssize_t sprintf_ipaddr(char *buf, u8 *ip)
+{
+ if (ip[0] == 0 ip[1] == 0 ip[2] == 0 ip[3] == 0
+ ip[4] == 0 ip[5] == 0 ip[6] == 0 ip[7] == 0
+ ip[8] == 0 ip[9] == 0 ip[10] == 0xff ip[11] == 0xff) {
+ /*
+* IPV4
+*/
+ return sprintf(buf, %d.%d.%d.%d\n, ip[12],
+ ip[13], ip[14], ip[15]);
+ } else
+ return 0;
+}
You probably just want to use the NIPQUAD_FMT and NIP6_FMT macros here.
Also why isn't ipv6 supported?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI IBFT support (v0.4.5) - fixes to the header files.
On Tuesday 29 January 2008 14:15:15 Mike Christie wrote:
Konrad Rzeszutek wrote:
+/*
+ * Helper functions to parse data properly.
+ */
+static ssize_t sprintf_ipaddr(char *buf, u8 *ip)
+{
+ if (ip[0] == 0 ip[1] == 0 ip[2] == 0 ip[3] == 0
+ ip[4] == 0 ip[5] == 0 ip[6] == 0 ip[7] == 0
+ ip[8] == 0 ip[9] == 0 ip[10] == 0xff ip[11] == 0xff) {
+ /*
+* IPV4
+*/
+ return sprintf(buf, %d.%d.%d.%d\n, ip[12],
+ ip[13], ip[14], ip[15]);
+ } else
+ return 0;
+}
You probably just want to use the NIPQUAD_FMT and NIP6_FMT macros here.
Ah, I knew a macro like this _ought_ to be there somewhere. Thanks.
Also why isn't ipv6 supported?
I didn't get to test the IPV6 yet so I didn't want to put in code that might
have worked or not :-(
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
Konrad Rzeszutek wrote: iBFT is not platform-independent; it only makes sense on platforms with ACPI (and even then, just barely; ACPI is a poor fit for it and it was probably "integrated" with ACPI for political reasons.) The spec just mentions that iBFT table has to be "compatible with an ACPI table format" and nothing else. Well, that's not quite accurate. It also mentions that OEM IDs for vendors come from the ACPI SIG, and they also reserved the "IBFT" signature with the ACPI SIG (it's in ACPI 3.0). It's also pretty clear that the "Locating the iBFT" section in the spec used to be a list with more than one entry and has been edited down to one. That being said, I don't think there's any reason to expect the table to show up on anything but i386 and x86_64, and maybe ia64. -- Peter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
> iBFT is not platform-independent; it only makes sense on platforms with > ACPI (and even then, just barely; ACPI is a poor fit for it and it was > probably "integrated" with ACPI for political reasons.) The spec just mentions that iBFT table has to be "compatible with an ACPI table format" and nothing else. In reality I've only tested this on x86_64 and i386. We can limit it to be X86 || IA64 but I wouldn't make it dependent on ACPI - as this data does not necessarily have to be exported via ACPI. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
Doug Maxey wrote: On Mon, 28 Jan 2008 14:04:51 EST, Konrad Rzeszutek wrote: +EXPORT_SYMBOL(find_ibft); Is this x86-specific? Are suitable Kconfig dependencies in place? Originally I had it to be x86-specific but was told that I should make it all platforms since the IBFT is platform independent. Somebody can very well insert a NIC with IBFT on a IA64 machine or PPC. I would beg to differ regarding the powerpc. On powerpc, the bios is invisible and ignored. We have our own "special" way, via the device-tree in procfs. iBFT is not platform-independent; it only makes sense on platforms with ACPI (and even then, just barely; ACPI is a poor fit for it and it was probably "integrated" with ACPI for political reasons.) -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
On Mon, 28 Jan 2008 14:04:51 EST, Konrad Rzeszutek wrote: > > > +EXPORT_SYMBOL(find_ibft); > > > > Is this x86-specific? Are suitable Kconfig dependencies in place? > > Originally I had it to be x86-specific but was told that I should make it all > platforms since the IBFT is platform independent. Somebody can very well > insert a NIC with IBFT on a IA64 machine or PPC. > I would beg to differ regarding the powerpc. On powerpc, the bios is invisible and ignored. We have our own "special" way, via the device-tree in procfs. ++doug -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
On Sunday 27 January 2008 01:01:23 you wrote:
> > On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek <[EMAIL PROTECTED]>
> > wrote: Hey Andrew,
> >
> > Please add this patch along with Greg KH's kobject fixes.
>
> erm, OK. But I don't think I'm the appropriate conduit for iscsi paches.
>
> By what path _does_ iscsi ode get into the tree, anyway? Mike is listed as
> maintainer...
This is a bit tricky b/c this goes to the drivers/firmware and also depends on
the kobject changes in Greg KH tree. But I should have included Mike on the
CC which I keep on forgetting .
>
> When adding new sysfs things (especially things as complex as this) please
> fully describe the user-visible interface in the changelog so that we can
> review your interface design.
Done. This repost:
http://lkml.org/lkml/2008/1/28/304
has the details.
>
> Does this code follow the one-value-per-sysfs-file convention?
Yes.
>
> > +#if defined(CONFIG_ISCSI_IBFT_FIND)
> > +static void __init reserve_ibft_region(void)
> > +{
> > + unsigned int ibft_len;
> > +
> > + ibft_len = find_ibft();
> > + if (ibft_len)
> > + reserve_bootmem((unsigned int)ibft_phys,
> > + PAGE_ALIGN(ibft_len));
> > +}
> > +#else
> > +static void __init reserve_ibft_region(void) { }
> > +#endif
>
> Usually we'd make the CONFIG_ISCSI_IBFT_FIND=n version of this function
> static inline in a header. As it stands this code will add a pointless
> empty function to the vmlinux image.
Fixed. The function now is in include/linux/iscsi_ibft.h
>
> > int __initdata user_defined_memmap = 0;
>
> checkpatch should have told you that this "= 0" shouldn't be there. But it
> doesn't.
Uhh, I didn't add that.
>
> > + struct kobject *kobj;
> > + int type; /* The enum of the type. This can be any value off:
> > + ibft_eth_properties_enum, ibft_tgt_properties_enum,
> > + or ibft_initiator_properties_enum. */
> > + struct list_head node;
> > +};
> > +
> > +static LIST_HEAD(ibft_attr_list);
> > +static LIST_HEAD(ibft_kobject_list);
>
> A brief search for the locking which protects these lists was unsuccessful.
> What's up?
The data structure does not change when the machine has booted up. The iBFT is
a read-only structure and there are no known mechanism to write to it via the
iBFT structure (or even BIOS up-calls). You have to use custom-vendor
applications to flash the BIOS with the new iBFT structure. Hence no need for
locking at this stage - when the specs becomes more advance I will add them
if they are required.
>
> > +/*
> > + * Helper functions to parse data properly.
> > + */
> > +static ssize_t sprintf_ipaddr(char *buf, u8 *ip)
> > +{
> > + if (ip[0] == 0 && ip[1] == 0 && ip[2] == 0 && ip[3] == 0 &&
> > + ip[4] == 0 && ip[5] == 0 && ip[6] == 0 && ip[7] == 0 &&
> > + ip[8] == 0 && ip[9] == 0 && ip[10] == 0xff && ip[11] == 0xff) {
> > + /*
> > +* IPV4
> > +*/
> > + return sprintf(buf, "%d.%d.%d.%d\n", ip[12],
> > + ip[13], ip[14], ip[15]);
> > + } else
> > + return 0;
> > +}
>
> I'm seeing an awful lot of sprintf()s in here which look like they should
> be snprintf(). By what means is this code bulletproof against overflows?
There is no reading of data from the user-land. All of it is just outputing to
the /sysfs entries from reading the data in the iBFT structure which the BIOS
created.
>
> > +static ssize_t sprintf_string(char *str, int len, char *buf)
> > +{
> > + return sprintf(str, "%.*s\n", len, buf);
> > +}
> > +
> > +/*
> > + * Helper function to verify the IBFT header.
> > + */
> > +static int ibft_verify_hdr(char *t, struct ibft_hdr *hdr, int id, int
> > length) +{
> > +#define IBFT_VERIFY_HDR_FIELD(val, name) \
> > + if (hdr->val != val) { \
> > + printk(KERN_ERR \
> > + "iBFT error: In structure %s field %s expected %d but" \
> > + " found %d!\n", \
> > + t, name, val, hdr->val); \
> > + return -ENODEV; \
> > + }
> > + IBFT_VERIFY_HDR_FIELD(id, "id");
> > + IBFT_VERIFY_HDR_FIELD(length, "length");
> > + return 0;
> > +}
>
> I'm not sure that two usages really justifies IBFT_VERIFY_HDR_FIELD's
> existence. If you're really attched to it then I'd suggest that it be
> undefed immediately after having been read, for readability reasons.
Done. Took out the #define.
>
> > +static void ibft_release(struct kobject *kobj)
> > +{
> > + struct ibft_kobject *ibft =
> > + container_of(kobj, struct ibft_kobject, kobj);
> > + kfree(ibft);
> > +}
> >
> > ...
> >
> > + for (pos = (u8 *)hdr; pos < (u8 *)hdr + len; pos ++)
>
> checkpatch should have caught the " ++" but didn't. I think it used to.
> It seems to be going backwards?
Fixed.
> > ...
> >
> > +
> > + /* kobject fief. We will let the reference counter do the job
> > +of deleting its name if we fail here. */
>
> what's a fief?
FIEF, or FEUD.
[PATCH] Add iSCSI IBFT support (v0.4.5) - fixes to the header files.
This patch (v0.4.5) adds /sysfs/firmware/ibft/[initiator|targetX|ethernetX] directories along with text properties which export the 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 /sysfs entries are read-only one-name-and-value fields. The usual set of data exposed is: # for a in `find /sys/firmware/ibft/ -type f -print`; do echo -n "$a: "; cat $a; done /sys/firmware/ibft/target0/target-name: iqn.2007.com.intel-sbx44:storage-10gb /sys/firmware/ibft/target0/nic-assoc: 0 /sys/firmware/ibft/target0/chap-type: 0 /sys/firmware/ibft/target0/lun: /sys/firmware/ibft/target0/port: 3260 /sys/firmware/ibft/target0/ip-addr: 192.168.79.116 /sys/firmware/ibft/target0/flags: 3 /sys/firmware/ibft/target0/index: 0 /sys/firmware/ibft/ethernet0/mac: 00:11:25:9d:8b:01 /sys/firmware/ibft/ethernet0/vlan: 0 /sys/firmware/ibft/ethernet0/gateway: 192.168.79.254 /sys/firmware/ibft/ethernet0/origin: 0 /sys/firmware/ibft/ethernet0/subnet-mask: 255.255.252.0 /sys/firmware/ibft/ethernet0/ip-addr: 192.168.77.41 /sys/firmware/ibft/ethernet0/flags: 7 /sys/firmware/ibft/ethernet0/index: 0 /sys/firmware/ibft/initiator/initiator-name: iqn.2007-07.com:konrad.initiator /sys/firmware/ibft/initiator/flags: 3 /sys/firmware/ibft/initiator/index: 0 For full details of the IBFT structure please take a look at: ftp://ftp.software.ibm.com/systems/support/system_x_pdf/ibm_iscsi_boot_firmware_table_v1.02.pdf Please note that this patch depends on the Greg KH patches tree kobject changes. Signed-off-by: Konrad Rzeszutek <[EMAIL PROTECTED]> Cc: Mike Christie <[EMAIL PROTECTED]> Cc: Peter Jones <[EMAIL PROTECTED]> Cc: Greg KH <[EMAIL PROTECTED]> Cc: James Bottomley <[EMAIL PROTECTED]> --- Documentation/ABI/testing/sysfs-ibft | 23 arch/x86/kernel/setup_32.c |3 arch/x86/kernel/setup_64.c |4 drivers/firmware/Kconfig | 19 drivers/firmware/Makefile|2 drivers/firmware/iscsi_ibft.c| 975 +++ drivers/firmware/iscsi_ibft_find.c | 77 ++ include/linux/iscsi_ibft.h | 52 + 8 files changed, 1155 insertions(+) diff --git a/Documentation/ABI/testing/sysfs-ibft b/Documentation/ABI/testing/sysfs-ibft new file mode 100644 index 000..062c009 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-ibft @@ -0,0 +1,23 @@ +What: /sys/firmware/ibft/initiator +Date: November 2007 +Contact: Konrad Rzeszutek <[EMAIL PROTECTED]> +Description: The /sys/firmware/ibft/initiator directory will contain + files that expose the iSCSI Boot Firmware Table initiator data. + Usually this contains the Initiator name. + +What: /sys/firmware/ibft/targetX +Date: November 2007 +Contact: Konrad Rzeszutek <[EMAIL PROTECTED]> +Description: The /sys/firmware/ibft/targetX directory will contain + files that expose the iSCSI Boot Firmware Table target data. + Usually this contains the target's IP address, boot LUN, + target name, and what NIC it is associated with. It can also + contain the CHAP name (and password), the reverse CHAP + name (and password) + +What: /sys/firmware/ibft/ethernetX +Date: November 2007 +Contact: Konrad Rzeszutek <[EMAIL PROTECTED]> +Description: The /sys/firmware/ibft/ethernetX directory will contain + files that expose the iSCSI Boot Firmware Table NIC data. + This can this can the IP address, MAC, and gateway of the NIC. diff --git a/arch/x86/kernel/setup_32.c b/arch/x86/kernel/setup_32.c index 9c24b45..5da7b8d 100644 --- a/arch/x86/kernel/setup_32.c +++ b/arch/x86/kernel/setup_32.c @@ -39,6 +39,7 @@ #include #include #include +#include #include #include #include @@ -496,6 +497,8 @@ void __init setup_bootmem_allocator(void) } #endif reserve_crashkernel(); + + reserve_ibft_region(); } /* diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c index 30d94d1..3393f2d 100644 --- a/arch/x86/kernel/setup_64.c +++ b/arch/x86/kernel/setup_64.c @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -403,6 +404,9 @@ void __init setup_arch(char **cmdline_p) } #endif reserve_crashkernel(); + + reserve_ibft_region(); + paging_init(); #ifdef CONFIG_PCI diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index 05f02a3..e2f7208 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -93,4 +93,23 @@ config DMIID information from userspace through /sys/class/dmi/id/ or if you want DMI-based module
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
On Sat, Jan 26, 2008 at 10:01:23PM -0800, Andrew Morton wrote: > > int __initdata user_defined_memmap = 0; > > checkpatch should have told you that this "= 0" shouldn't be there. But it > doesn't. Ok, this line would be correctly picked up if it was being added by this author, but this line is in the context only. We do not blame the current author for those. ERROR: do not initialise externals to 0 or NULL #1: FILE: Z57.c:1: +int __initdata user_defined_memmap = 0; > > + for (pos = (u8 *)hdr; pos < (u8 *)hdr + len; pos ++) > > checkpatch should have caught the " ++" but didn't. I think it used to. > It seems to be going backwards? Somehow this variant was not covered. Added to the tests and to the next version: ERROR: no space before that '++' (ctx:WxB) #3: FILE: Z57.c:3: + for (pos = (u8 *)hdr; pos < (u8 *)hdr + len; pos ++) -apw -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
On Mon, 28 Jan 2008 14:04:51 EST, Konrad Rzeszutek wrote: +EXPORT_SYMBOL(find_ibft); Is this x86-specific? Are suitable Kconfig dependencies in place? Originally I had it to be x86-specific but was told that I should make it all platforms since the IBFT is platform independent. Somebody can very well insert a NIC with IBFT on a IA64 machine or PPC. I would beg to differ regarding the powerpc. On powerpc, the bios is invisible and ignored. We have our own special way, via the device-tree in procfs. ++doug -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
On Sunday 27 January 2008 01:01:23 you wrote:
On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek [EMAIL PROTECTED]
wrote: Hey Andrew,
Please add this patch along with Greg KH's kobject fixes.
erm, OK. But I don't think I'm the appropriate conduit for iscsi paches.
By what path _does_ iscsi ode get into the tree, anyway? Mike is listed as
maintainer...
This is a bit tricky b/c this goes to the drivers/firmware and also depends on
the kobject changes in Greg KH tree. But I should have included Mike on the
CC which I keep on forgetting sigh.
When adding new sysfs things (especially things as complex as this) please
fully describe the user-visible interface in the changelog so that we can
review your interface design.
Done. This repost:
http://lkml.org/lkml/2008/1/28/304
has the details.
Does this code follow the one-value-per-sysfs-file convention?
Yes.
+#if defined(CONFIG_ISCSI_IBFT_FIND)
+static void __init reserve_ibft_region(void)
+{
+ unsigned int ibft_len;
+
+ ibft_len = find_ibft();
+ if (ibft_len)
+ reserve_bootmem((unsigned int)ibft_phys,
+ PAGE_ALIGN(ibft_len));
+}
+#else
+static void __init reserve_ibft_region(void) { }
+#endif
Usually we'd make the CONFIG_ISCSI_IBFT_FIND=n version of this function
static inline in a header. As it stands this code will add a pointless
empty function to the vmlinux image.
Fixed. The function now is in include/linux/iscsi_ibft.h
int __initdata user_defined_memmap = 0;
checkpatch should have told you that this = 0 shouldn't be there. But it
doesn't.
Uhh, I didn't add that.
+ struct kobject *kobj;
+ int type; /* The enum of the type. This can be any value off:
+ ibft_eth_properties_enum, ibft_tgt_properties_enum,
+ or ibft_initiator_properties_enum. */
+ struct list_head node;
+};
+
+static LIST_HEAD(ibft_attr_list);
+static LIST_HEAD(ibft_kobject_list);
A brief search for the locking which protects these lists was unsuccessful.
What's up?
The data structure does not change when the machine has booted up. The iBFT is
a read-only structure and there are no known mechanism to write to it via the
iBFT structure (or even BIOS up-calls). You have to use custom-vendor
applications to flash the BIOS with the new iBFT structure. Hence no need for
locking at this stage - when the specs becomes more advance I will add them
if they are required.
+/*
+ * Helper functions to parse data properly.
+ */
+static ssize_t sprintf_ipaddr(char *buf, u8 *ip)
+{
+ if (ip[0] == 0 ip[1] == 0 ip[2] == 0 ip[3] == 0
+ ip[4] == 0 ip[5] == 0 ip[6] == 0 ip[7] == 0
+ ip[8] == 0 ip[9] == 0 ip[10] == 0xff ip[11] == 0xff) {
+ /*
+* IPV4
+*/
+ return sprintf(buf, %d.%d.%d.%d\n, ip[12],
+ ip[13], ip[14], ip[15]);
+ } else
+ return 0;
+}
I'm seeing an awful lot of sprintf()s in here which look like they should
be snprintf(). By what means is this code bulletproof against overflows?
There is no reading of data from the user-land. All of it is just outputing to
the /sysfs entries from reading the data in the iBFT structure which the BIOS
created.
+static ssize_t sprintf_string(char *str, int len, char *buf)
+{
+ return sprintf(str, %.*s\n, len, buf);
+}
+
+/*
+ * Helper function to verify the IBFT header.
+ */
+static int ibft_verify_hdr(char *t, struct ibft_hdr *hdr, int id, int
length) +{
+#define IBFT_VERIFY_HDR_FIELD(val, name) \
+ if (hdr-val != val) { \
+ printk(KERN_ERR \
+ iBFT error: In structure %s field %s expected %d but \
+ found %d!\n, \
+ t, name, val, hdr-val); \
+ return -ENODEV; \
+ }
+ IBFT_VERIFY_HDR_FIELD(id, id);
+ IBFT_VERIFY_HDR_FIELD(length, length);
+ return 0;
+}
I'm not sure that two usages really justifies IBFT_VERIFY_HDR_FIELD's
existence. If you're really attched to it then I'd suggest that it be
undefed immediately after having been read, for readability reasons.
Done. Took out the #define.
+static void ibft_release(struct kobject *kobj)
+{
+ struct ibft_kobject *ibft =
+ container_of(kobj, struct ibft_kobject, kobj);
+ kfree(ibft);
+}
...
+ for (pos = (u8 *)hdr; pos (u8 *)hdr + len; pos ++)
checkpatch should have caught the ++ but didn't. I think it used to.
It seems to be going backwards?
Fixed.
...
+
+ /* kobject fief. We will let the reference counter do the job
+of deleting its name if we fail here. */
what's a fief?
FIEF, or FEUD. In its origin, a fief was a district of country allotted to
one of the chiefs who invaded the Roman empire, as a stipend or reward.
+/*
+ * Physical location of iSCSI Boot Format Table.
+ */
+void *ibft_phys;
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
iBFT is not platform-independent; it only makes sense on platforms with ACPI (and even then, just barely; ACPI is a poor fit for it and it was probably integrated with ACPI for political reasons.) The spec just mentions that iBFT table has to be compatible with an ACPI table format and nothing else. In reality I've only tested this on x86_64 and i386. We can limit it to be X86 || IA64 but I wouldn't make it dependent on ACPI - as this data does not necessarily have to be exported via ACPI. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
Konrad Rzeszutek wrote: iBFT is not platform-independent; it only makes sense on platforms with ACPI (and even then, just barely; ACPI is a poor fit for it and it was probably integrated with ACPI for political reasons.) The spec just mentions that iBFT table has to be compatible with an ACPI table format and nothing else. Well, that's not quite accurate. It also mentions that OEM IDs for vendors come from the ACPI SIG, and they also reserved the IBFT signature with the ACPI SIG (it's in ACPI 3.0). It's also pretty clear that the Locating the iBFT section in the spec used to be a list with more than one entry and has been edited down to one. That being said, I don't think there's any reason to expect the table to show up on anything but i386 and x86_64, and maybe ia64. -- Peter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Add iSCSI IBFT support (v0.4.5) - fixes to the header files.
This patch (v0.4.5) adds /sysfs/firmware/ibft/[initiator|targetX|ethernetX] directories along with text properties which export the 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 /sysfs entries are read-only one-name-and-value fields. The usual set of data exposed is: # for a in `find /sys/firmware/ibft/ -type f -print`; do echo -n $a: ; cat $a; done /sys/firmware/ibft/target0/target-name: iqn.2007.com.intel-sbx44:storage-10gb /sys/firmware/ibft/target0/nic-assoc: 0 /sys/firmware/ibft/target0/chap-type: 0 /sys/firmware/ibft/target0/lun: /sys/firmware/ibft/target0/port: 3260 /sys/firmware/ibft/target0/ip-addr: 192.168.79.116 /sys/firmware/ibft/target0/flags: 3 /sys/firmware/ibft/target0/index: 0 /sys/firmware/ibft/ethernet0/mac: 00:11:25:9d:8b:01 /sys/firmware/ibft/ethernet0/vlan: 0 /sys/firmware/ibft/ethernet0/gateway: 192.168.79.254 /sys/firmware/ibft/ethernet0/origin: 0 /sys/firmware/ibft/ethernet0/subnet-mask: 255.255.252.0 /sys/firmware/ibft/ethernet0/ip-addr: 192.168.77.41 /sys/firmware/ibft/ethernet0/flags: 7 /sys/firmware/ibft/ethernet0/index: 0 /sys/firmware/ibft/initiator/initiator-name: iqn.2007-07.com:konrad.initiator /sys/firmware/ibft/initiator/flags: 3 /sys/firmware/ibft/initiator/index: 0 For full details of the IBFT structure please take a look at: ftp://ftp.software.ibm.com/systems/support/system_x_pdf/ibm_iscsi_boot_firmware_table_v1.02.pdf Please note that this patch depends on the Greg KH patches tree kobject changes. Signed-off-by: Konrad Rzeszutek [EMAIL PROTECTED] Cc: Mike Christie [EMAIL PROTECTED] Cc: Peter Jones [EMAIL PROTECTED] Cc: Greg KH [EMAIL PROTECTED] Cc: James Bottomley [EMAIL PROTECTED] --- Documentation/ABI/testing/sysfs-ibft | 23 arch/x86/kernel/setup_32.c |3 arch/x86/kernel/setup_64.c |4 drivers/firmware/Kconfig | 19 drivers/firmware/Makefile|2 drivers/firmware/iscsi_ibft.c| 975 +++ drivers/firmware/iscsi_ibft_find.c | 77 ++ include/linux/iscsi_ibft.h | 52 + 8 files changed, 1155 insertions(+) diff --git a/Documentation/ABI/testing/sysfs-ibft b/Documentation/ABI/testing/sysfs-ibft new file mode 100644 index 000..062c009 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-ibft @@ -0,0 +1,23 @@ +What: /sys/firmware/ibft/initiator +Date: November 2007 +Contact: Konrad Rzeszutek [EMAIL PROTECTED] +Description: The /sys/firmware/ibft/initiator directory will contain + files that expose the iSCSI Boot Firmware Table initiator data. + Usually this contains the Initiator name. + +What: /sys/firmware/ibft/targetX +Date: November 2007 +Contact: Konrad Rzeszutek [EMAIL PROTECTED] +Description: The /sys/firmware/ibft/targetX directory will contain + files that expose the iSCSI Boot Firmware Table target data. + Usually this contains the target's IP address, boot LUN, + target name, and what NIC it is associated with. It can also + contain the CHAP name (and password), the reverse CHAP + name (and password) + +What: /sys/firmware/ibft/ethernetX +Date: November 2007 +Contact: Konrad Rzeszutek [EMAIL PROTECTED] +Description: The /sys/firmware/ibft/ethernetX directory will contain + files that expose the iSCSI Boot Firmware Table NIC data. + This can this can the IP address, MAC, and gateway of the NIC. diff --git a/arch/x86/kernel/setup_32.c b/arch/x86/kernel/setup_32.c index 9c24b45..5da7b8d 100644 --- a/arch/x86/kernel/setup_32.c +++ b/arch/x86/kernel/setup_32.c @@ -39,6 +39,7 @@ #include linux/efi.h #include linux/init.h #include linux/edd.h +#include linux/iscsi_ibft.h #include linux/nodemask.h #include linux/kexec.h #include linux/crash_dump.h @@ -496,6 +497,8 @@ void __init setup_bootmem_allocator(void) } #endif reserve_crashkernel(); + + reserve_ibft_region(); } /* diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c index 30d94d1..3393f2d 100644 --- a/arch/x86/kernel/setup_64.c +++ b/arch/x86/kernel/setup_64.c @@ -33,6 +33,7 @@ #include linux/acpi.h #include linux/kallsyms.h #include linux/edd.h +#include linux/iscsi_ibft.h #include linux/mmzone.h #include linux/kexec.h #include linux/cpufreq.h @@ -403,6 +404,9 @@ void __init setup_arch(char **cmdline_p) } #endif reserve_crashkernel(); + + reserve_ibft_region(); + paging_init(); #ifdef CONFIG_PCI diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index 05f02a3..e2f7208 100644 --- a/drivers/firmware/Kconfig
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
On Sat, Jan 26, 2008 at 10:01:23PM -0800, Andrew Morton wrote: int __initdata user_defined_memmap = 0; checkpatch should have told you that this = 0 shouldn't be there. But it doesn't. Ok, this line would be correctly picked up if it was being added by this author, but this line is in the context only. We do not blame the current author for those. ERROR: do not initialise externals to 0 or NULL #1: FILE: Z57.c:1: +int __initdata user_defined_memmap = 0; + for (pos = (u8 *)hdr; pos (u8 *)hdr + len; pos ++) checkpatch should have caught the ++ but didn't. I think it used to. It seems to be going backwards? Somehow this variant was not covered. Added to the tests and to the next version: ERROR: no space before that '++' (ctx:WxB) #3: FILE: Z57.c:3: + for (pos = (u8 *)hdr; pos (u8 *)hdr + len; pos ++) -apw -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
Doug Maxey wrote: On Mon, 28 Jan 2008 14:04:51 EST, Konrad Rzeszutek wrote: +EXPORT_SYMBOL(find_ibft); Is this x86-specific? Are suitable Kconfig dependencies in place? Originally I had it to be x86-specific but was told that I should make it all platforms since the IBFT is platform independent. Somebody can very well insert a NIC with IBFT on a IA64 machine or PPC. I would beg to differ regarding the powerpc. On powerpc, the bios is invisible and ignored. We have our own special way, via the device-tree in procfs. iBFT is not platform-independent; it only makes sense on platforms with ACPI (and even then, just barely; ACPI is a poor fit for it and it was probably integrated with ACPI for political reasons.) -hpa -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
Andrew Morton wrote: int __initdata user_defined_memmap = 0; checkpatch should have told you that this "= 0" shouldn't be there. But it doesn't. checkpatch checks for static initializers, not non-static ones. Should that be changed? + for (pos = (u8 *)hdr; pos < (u8 *)hdr + len; pos ++) checkpatch should have caught the " ++" but didn't. I think it used to. It seems to be going backwards? -- ~Randy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
Andrew Morton wrote: int __initdata user_defined_memmap = 0; checkpatch should have told you that this = 0 shouldn't be there. But it doesn't. checkpatch checks for static initializers, not non-static ones. Should that be changed? + for (pos = (u8 *)hdr; pos (u8 *)hdr + len; pos ++) checkpatch should have caught the ++ but didn't. I think it used to. It seems to be going backwards? -- ~Randy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
> On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek <[EMAIL PROTECTED]> wrote:
> Hey Andrew,
>
> Please add this patch along with Greg KH's kobject fixes.
erm, OK. But I don't think I'm the appropriate conduit for iscsi paches.
By what path _does_ iscsi ode get into the tree, anyway? Mike is listed as
maintainer...
Oh well, at least I get to read some code.
> This module
> is dependent on the fixes that Greg KH has in his patches git tree.
>
> This patch (v0.4.5) adds /sysfs/firmware/ibft/[initiator|targetX|ethernetX]
> directories along with text properties which export the 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.
>
> For full details of the IBFT structure please take a look at:
> ftp://ftp.software.ibm.com/systems/support/system_x_pdf/ibm_iscsi_boot_firmware_table_v1.02.pdf
When adding new sysfs things (especially things as complex as this) please
fully describe the user-visible interface in the changelog so that we can
review your interface design.
Does this code follow the one-value-per-sysfs-file convention?
> +#if defined(CONFIG_ISCSI_IBFT_FIND)
> +static void __init reserve_ibft_region(void)
> +{
> + unsigned int ibft_len;
> +
> + ibft_len = find_ibft();
> + if (ibft_len)
> + reserve_bootmem((unsigned int)ibft_phys,
> + PAGE_ALIGN(ibft_len));
> +}
> +#else
> +static void __init reserve_ibft_region(void) { }
> +#endif
Usually we'd make the CONFIG_ISCSI_IBFT_FIND=n version of this function
static inline in a header. As it stands this code will add a pointless
empty function to the vmlinux image.
> int __initdata user_defined_memmap = 0;
checkpatch should have told you that this "= 0" shouldn't be there. But it
doesn't.
> + struct kobject *kobj;
> + int type; /* The enum of the type. This can be any value off:
> + ibft_eth_properties_enum, ibft_tgt_properties_enum,
> + or ibft_initiator_properties_enum. */
> + struct list_head node;
> +};
> +
> +static LIST_HEAD(ibft_attr_list);
> +static LIST_HEAD(ibft_kobject_list);
A brief search for the locking which protects these lists was unsuccessful.
What's up?
> +/*
> + * Helper functions to parse data properly.
> + */
> +static ssize_t sprintf_ipaddr(char *buf, u8 *ip)
> +{
> + if (ip[0] == 0 && ip[1] == 0 && ip[2] == 0 && ip[3] == 0 &&
> + ip[4] == 0 && ip[5] == 0 && ip[6] == 0 && ip[7] == 0 &&
> + ip[8] == 0 && ip[9] == 0 && ip[10] == 0xff && ip[11] == 0xff) {
> + /*
> + * IPV4
> + */
> + return sprintf(buf, "%d.%d.%d.%d\n", ip[12],
> +ip[13], ip[14], ip[15]);
> + } else
> + return 0;
> +}
I'm seeing an awful lot of sprintf()s in here which look like they should
be snprintf(). By what means is this code bulletproof against overflows?
> +static ssize_t sprintf_string(char *str, int len, char *buf)
> +{
> + return sprintf(str, "%.*s\n", len, buf);
> +}
> +
> +/*
> + * Helper function to verify the IBFT header.
> + */
> +static int ibft_verify_hdr(char *t, struct ibft_hdr *hdr, int id, int length)
> +{
> +#define IBFT_VERIFY_HDR_FIELD(val, name) \
> + if (hdr->val != val) { \
> + printk(KERN_ERR \
> +"iBFT error: In structure %s field %s expected %d but" \
> +" found %d!\n", \
> +t, name, val, hdr->val); \
> + return -ENODEV; \
> + }
> + IBFT_VERIFY_HDR_FIELD(id, "id");
> + IBFT_VERIFY_HDR_FIELD(length, "length");
> + return 0;
> +}
I'm not sure that two usages really justifies IBFT_VERIFY_HDR_FIELD's
existence. If you're really attched to it then I'd suggest that it be
undefed immediately after having been read, for readability reasons.
> +static void ibft_release(struct kobject *kobj)
> +{
> + struct ibft_kobject *ibft =
> + container_of(kobj, struct ibft_kobject, kobj);
> + kfree(ibft);
> +}
>
> ...
>
> + for (pos = (u8 *)hdr; pos < (u8 *)hdr + len; pos ++)
>
checkpatch should have caught the " ++" but didn't. I think it used to.
It seems to be going backwards?
> + csum += *pos;
> +
> + if (csum) {
> + printk(KERN_ERR "iBFT has incorrect checksum (0x%x)!\n", csum);
> + return 1;
> + }
> +
> + ibft_device = kmalloc(len, GFP_KERNEL);
> + if (!ibft_device)
> + return -ENOMEM;
> +
> + memcpy(ibft_device, hdr, len);
> +
> + return 0;
> +}
> +
>
> ...
>
> +
> + /* kobject fief. We will let the reference counter do the job
> + of deleting its name if we fail here. */
what's a fief?
> +/*
> + * Physical location of iSCSI Boot Format Table.
> + */
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
Please always include a diffstat with non-trivial patches. > On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek <[EMAIL PROTECTED]> wrote: > --- a/arch/x86/kernel/setup_32.c > +++ b/arch/x86/kernel/setup_32.c lol. You touched x86 code. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
Please always include a diffstat with non-trivial patches. On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek [EMAIL PROTECTED] wrote: --- a/arch/x86/kernel/setup_32.c +++ b/arch/x86/kernel/setup_32.c lol. You touched x86 code. goes off to fix the inevitable rejects -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add iSCSI iBFT support (v0.4.5)
On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek [EMAIL PROTECTED] wrote:
Hey Andrew,
Please add this patch along with Greg KH's kobject fixes.
erm, OK. But I don't think I'm the appropriate conduit for iscsi paches.
By what path _does_ iscsi ode get into the tree, anyway? Mike is listed as
maintainer...
Oh well, at least I get to read some code.
This module
is dependent on the fixes that Greg KH has in his patches git tree.
This patch (v0.4.5) adds /sysfs/firmware/ibft/[initiator|targetX|ethernetX]
directories along with text properties which export the 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.
For full details of the IBFT structure please take a look at:
ftp://ftp.software.ibm.com/systems/support/system_x_pdf/ibm_iscsi_boot_firmware_table_v1.02.pdf
When adding new sysfs things (especially things as complex as this) please
fully describe the user-visible interface in the changelog so that we can
review your interface design.
Does this code follow the one-value-per-sysfs-file convention?
+#if defined(CONFIG_ISCSI_IBFT_FIND)
+static void __init reserve_ibft_region(void)
+{
+ unsigned int ibft_len;
+
+ ibft_len = find_ibft();
+ if (ibft_len)
+ reserve_bootmem((unsigned int)ibft_phys,
+ PAGE_ALIGN(ibft_len));
+}
+#else
+static void __init reserve_ibft_region(void) { }
+#endif
Usually we'd make the CONFIG_ISCSI_IBFT_FIND=n version of this function
static inline in a header. As it stands this code will add a pointless
empty function to the vmlinux image.
int __initdata user_defined_memmap = 0;
checkpatch should have told you that this = 0 shouldn't be there. But it
doesn't.
+ struct kobject *kobj;
+ int type; /* The enum of the type. This can be any value off:
+ ibft_eth_properties_enum, ibft_tgt_properties_enum,
+ or ibft_initiator_properties_enum. */
+ struct list_head node;
+};
+
+static LIST_HEAD(ibft_attr_list);
+static LIST_HEAD(ibft_kobject_list);
A brief search for the locking which protects these lists was unsuccessful.
What's up?
+/*
+ * Helper functions to parse data properly.
+ */
+static ssize_t sprintf_ipaddr(char *buf, u8 *ip)
+{
+ if (ip[0] == 0 ip[1] == 0 ip[2] == 0 ip[3] == 0
+ ip[4] == 0 ip[5] == 0 ip[6] == 0 ip[7] == 0
+ ip[8] == 0 ip[9] == 0 ip[10] == 0xff ip[11] == 0xff) {
+ /*
+ * IPV4
+ */
+ return sprintf(buf, %d.%d.%d.%d\n, ip[12],
+ip[13], ip[14], ip[15]);
+ } else
+ return 0;
+}
I'm seeing an awful lot of sprintf()s in here which look like they should
be snprintf(). By what means is this code bulletproof against overflows?
+static ssize_t sprintf_string(char *str, int len, char *buf)
+{
+ return sprintf(str, %.*s\n, len, buf);
+}
+
+/*
+ * Helper function to verify the IBFT header.
+ */
+static int ibft_verify_hdr(char *t, struct ibft_hdr *hdr, int id, int length)
+{
+#define IBFT_VERIFY_HDR_FIELD(val, name) \
+ if (hdr-val != val) { \
+ printk(KERN_ERR \
+iBFT error: In structure %s field %s expected %d but \
+ found %d!\n, \
+t, name, val, hdr-val); \
+ return -ENODEV; \
+ }
+ IBFT_VERIFY_HDR_FIELD(id, id);
+ IBFT_VERIFY_HDR_FIELD(length, length);
+ return 0;
+}
I'm not sure that two usages really justifies IBFT_VERIFY_HDR_FIELD's
existence. If you're really attched to it then I'd suggest that it be
undefed immediately after having been read, for readability reasons.
+static void ibft_release(struct kobject *kobj)
+{
+ struct ibft_kobject *ibft =
+ container_of(kobj, struct ibft_kobject, kobj);
+ kfree(ibft);
+}
...
+ for (pos = (u8 *)hdr; pos (u8 *)hdr + len; pos ++)
checkpatch should have caught the ++ but didn't. I think it used to.
It seems to be going backwards?
+ csum += *pos;
+
+ if (csum) {
+ printk(KERN_ERR iBFT has incorrect checksum (0x%x)!\n, csum);
+ return 1;
+ }
+
+ ibft_device = kmalloc(len, GFP_KERNEL);
+ if (!ibft_device)
+ return -ENOMEM;
+
+ memcpy(ibft_device, hdr, len);
+
+ return 0;
+}
+
...
+
+ /* kobject fief. We will let the reference counter do the job
+ of deleting its name if we fail here. */
what's a fief?
+/*
+ * Physical location of iSCSI Boot Format Table.
+ */
+void *ibft_phys;
+EXPORT_SYMBOL(ibft_phys);
+
+#define IBFT_SIGN iBFT
+#define IBFT_SIGN_LEN 4
+#define IBFT_START 0x8 /* 512kB */
+#define
[PATCH] Add iSCSI iBFT support (v0.4.5)
Hey Andrew,
Please add this patch along with Greg KH's kobject fixes. This module
is dependent on the fixes that Greg KH has in his patches git tree.
This patch (v0.4.5) adds /sysfs/firmware/ibft/[initiator|targetX|ethernetX]
directories along with text properties which export the 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.
For full details of the IBFT structure please take a look 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]>
diff --git a/Documentation/ABI/testing/sysfs-ibft
b/Documentation/ABI/testing/sysfs-ibft
new file mode 100644
index 000..062c009
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-ibft
@@ -0,0 +1,23 @@
+What: /sys/firmware/ibft/initiator
+Date: November 2007
+Contact: Konrad Rzeszutek <[EMAIL PROTECTED]>
+Description: The /sys/firmware/ibft/initiator directory will contain
+ files that expose the iSCSI Boot Firmware Table initiator data.
+ Usually this contains the Initiator name.
+
+What: /sys/firmware/ibft/targetX
+Date: November 2007
+Contact: Konrad Rzeszutek <[EMAIL PROTECTED]>
+Description: The /sys/firmware/ibft/targetX directory will contain
+ files that expose the iSCSI Boot Firmware Table target data.
+ Usually this contains the target's IP address, boot LUN,
+ target name, and what NIC it is associated with. It can also
+ contain the CHAP name (and password), the reverse CHAP
+ name (and password)
+
+What: /sys/firmware/ibft/ethernetX
+Date: November 2007
+Contact: Konrad Rzeszutek <[EMAIL PROTECTED]>
+Description: The /sys/firmware/ibft/ethernetX directory will contain
+ files that expose the iSCSI Boot Firmware Table NIC data.
+ This can this can the IP address, MAC, and gateway of the NIC.
diff --git a/arch/x86/kernel/setup_32.c b/arch/x86/kernel/setup_32.c
index 9c24b45..8127cad 100644
--- a/arch/x86/kernel/setup_32.c
+++ b/arch/x86/kernel/setup_32.c
@@ -39,6 +39,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -148,6 +149,20 @@ static inline void copy_edd(void)
}
#endif
+#if defined(CONFIG_ISCSI_IBFT_FIND)
+static void __init reserve_ibft_region(void)
+{
+ unsigned int ibft_len;
+
+ ibft_len = find_ibft();
+ if (ibft_len)
+ reserve_bootmem((unsigned int)ibft_phys,
+ PAGE_ALIGN(ibft_len));
+}
+#else
+static void __init reserve_ibft_region(void) { }
+#endif
+
int __initdata user_defined_memmap = 0;
/*
@@ -496,6 +511,8 @@ void __init setup_bootmem_allocator(void)
}
#endif
reserve_crashkernel();
+
+ reserve_ibft_region();
}
/*
diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c
index 30d94d1..307fb99 100644
--- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -198,6 +199,20 @@ static inline void copy_edd(void)
}
#endif
+#if defined(CONFIG_ISCSI_IBFT_FIND)
+static void __init reserve_ibft_region(void)
+{
+ unsigned int ibft_len;
+
+ ibft_len = find_ibft();
+ if (ibft_len)
+ reserve_bootmem_generic((unsigned long)ibft_phys,
+ PAGE_ALIGN(ibft_len));
+}
+#else
+static void __init reserve_ibft_region(void) { }
+#endif
+
#ifdef CONFIG_KEXEC
static void __init reserve_crashkernel(void)
{
@@ -403,6 +418,9 @@ void __init setup_arch(char **cmdline_p)
}
#endif
reserve_crashkernel();
+
+ reserve_ibft_region();
+
paging_init();
#ifdef CONFIG_PCI
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 05f02a3..e2f7208 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -93,4 +93,23 @@ config DMIID
information from userspace through /sys/class/dmi/id/ or if you want
DMI-based module auto-loading.
+config ISCSI_IBFT_FIND
+ bool "iSCSI Boot Firmware Table Attributes"
+ default n
+ help
+ This option enables the kernel to find the region of memory
+ in which the ISCSI Boot Firmware Table (iBFT) resides. This
+ is necessary for iSCSI Boot Firmware Table Attributes module to work
+ properly.
+
+config ISCSI_IBFT
+ tristate "iSCSI Boot Firmware Table Attributes module"
+ depends on ISCSI_IBFT_FIND
+ default n
+ help
+ This option enables support for detection and exposing of iSCSI
+ Boot Firmware Table
[PATCH] Add iSCSI iBFT support (v0.4.5)
Hey Andrew,
Please add this patch along with Greg KH's kobject fixes. This module
is dependent on the fixes that Greg KH has in his patches git tree.
This patch (v0.4.5) adds /sysfs/firmware/ibft/[initiator|targetX|ethernetX]
directories along with text properties which export the 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.
For full details of the IBFT structure please take a look 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]
diff --git a/Documentation/ABI/testing/sysfs-ibft
b/Documentation/ABI/testing/sysfs-ibft
new file mode 100644
index 000..062c009
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-ibft
@@ -0,0 +1,23 @@
+What: /sys/firmware/ibft/initiator
+Date: November 2007
+Contact: Konrad Rzeszutek [EMAIL PROTECTED]
+Description: The /sys/firmware/ibft/initiator directory will contain
+ files that expose the iSCSI Boot Firmware Table initiator data.
+ Usually this contains the Initiator name.
+
+What: /sys/firmware/ibft/targetX
+Date: November 2007
+Contact: Konrad Rzeszutek [EMAIL PROTECTED]
+Description: The /sys/firmware/ibft/targetX directory will contain
+ files that expose the iSCSI Boot Firmware Table target data.
+ Usually this contains the target's IP address, boot LUN,
+ target name, and what NIC it is associated with. It can also
+ contain the CHAP name (and password), the reverse CHAP
+ name (and password)
+
+What: /sys/firmware/ibft/ethernetX
+Date: November 2007
+Contact: Konrad Rzeszutek [EMAIL PROTECTED]
+Description: The /sys/firmware/ibft/ethernetX directory will contain
+ files that expose the iSCSI Boot Firmware Table NIC data.
+ This can this can the IP address, MAC, and gateway of the NIC.
diff --git a/arch/x86/kernel/setup_32.c b/arch/x86/kernel/setup_32.c
index 9c24b45..8127cad 100644
--- a/arch/x86/kernel/setup_32.c
+++ b/arch/x86/kernel/setup_32.c
@@ -39,6 +39,7 @@
#include linux/efi.h
#include linux/init.h
#include linux/edd.h
+#include linux/iscsi_ibft.h
#include linux/nodemask.h
#include linux/kexec.h
#include linux/crash_dump.h
@@ -148,6 +149,20 @@ static inline void copy_edd(void)
}
#endif
+#if defined(CONFIG_ISCSI_IBFT_FIND)
+static void __init reserve_ibft_region(void)
+{
+ unsigned int ibft_len;
+
+ ibft_len = find_ibft();
+ if (ibft_len)
+ reserve_bootmem((unsigned int)ibft_phys,
+ PAGE_ALIGN(ibft_len));
+}
+#else
+static void __init reserve_ibft_region(void) { }
+#endif
+
int __initdata user_defined_memmap = 0;
/*
@@ -496,6 +511,8 @@ void __init setup_bootmem_allocator(void)
}
#endif
reserve_crashkernel();
+
+ reserve_ibft_region();
}
/*
diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c
index 30d94d1..307fb99 100644
--- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -33,6 +33,7 @@
#include linux/acpi.h
#include linux/kallsyms.h
#include linux/edd.h
+#include linux/iscsi_ibft.h
#include linux/mmzone.h
#include linux/kexec.h
#include linux/cpufreq.h
@@ -198,6 +199,20 @@ static inline void copy_edd(void)
}
#endif
+#if defined(CONFIG_ISCSI_IBFT_FIND)
+static void __init reserve_ibft_region(void)
+{
+ unsigned int ibft_len;
+
+ ibft_len = find_ibft();
+ if (ibft_len)
+ reserve_bootmem_generic((unsigned long)ibft_phys,
+ PAGE_ALIGN(ibft_len));
+}
+#else
+static void __init reserve_ibft_region(void) { }
+#endif
+
#ifdef CONFIG_KEXEC
static void __init reserve_crashkernel(void)
{
@@ -403,6 +418,9 @@ void __init setup_arch(char **cmdline_p)
}
#endif
reserve_crashkernel();
+
+ reserve_ibft_region();
+
paging_init();
#ifdef CONFIG_PCI
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 05f02a3..e2f7208 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -93,4 +93,23 @@ config DMIID
information from userspace through /sys/class/dmi/id/ or if you want
DMI-based module auto-loading.
+config ISCSI_IBFT_FIND
+ bool iSCSI Boot Firmware Table Attributes
+ default n
+ help
+ This option enables the kernel to find the region of memory
+ in which the ISCSI Boot Firmware Table (iBFT) resides. This
+ is necessary for iSCSI Boot Firmware Table Attributes module to work
+ properly.
+
+config ISCSI_IBFT
+ tristate iSCSI Boot Firmware Table
