[PATCH] romsignature/checksum cleanup
On 01/07/2007 07:07 PM, Jeremy Fitzhardinge wrote:
Rene Herman wrote:
Doing the set_fs() and pagefault_{disable,enable} calls for every
single byte during the checksum seems rather silly.
Why?
Because it makes for dumb code. But oh well, given that it all compiles
to basically nothing I guess I'll stop objecting.
Andrew: The attached removes the assumption that if the first page of an
ISA ROM is mapped, it'll all be mapped. This'll also stop people reading
this code from wondering if they're looking at a bug.
This replaces "romsignature-checksum-cleanup.patch" in current -mm.
Signed-off-by: Rene Herman <[EMAIL PROTECTED]>
Not-strongly-objected-to-by: Jeremy Fitzhardinge <[EMAIL PROTECTED]>
Rene.
diff --git a/arch/i386/kernel/e820.c b/arch/i386/kernel/e820.c
index f391abc..8b8741f 100644
--- a/arch/i386/kernel/e820.c
+++ b/arch/i386/kernel/e820.c
@@ -156,29 +156,31 @@ static struct resource standard_io_resou
.flags = IORESOURCE_BUSY | IORESOURCE_IO
} };
-static int romsignature(const unsigned char *x)
+#define ROMSIGNATURE 0xaa55
+
+static int __init romsignature(const unsigned char *rom)
{
+ const unsigned short * const ptr = (const unsigned short *)rom;
unsigned short sig;
- int ret = 0;
- if (probe_kernel_address((const unsigned short *)x, sig) == 0)
- ret = (sig == 0xaa55);
- return ret;
+
+ return probe_kernel_address(ptr, sig) == 0 && sig == ROMSIGNATURE;
}
-static int __init romchecksum(unsigned char *rom, unsigned long length)
+static int __init romchecksum(const unsigned char *rom, unsigned long length)
{
- unsigned char *p, sum = 0;
+ unsigned char sum, c;
- for (p = rom; p < rom + length; p++)
- sum += *p;
- return sum == 0;
+ for (sum = 0; length && probe_kernel_address(rom++, c) == 0; length--)
+ sum += c;
+ return !length && !sum;
}
static void __init probe_roms(void)
{
+ const unsigned char *rom;
unsigned long start, length, upper;
- unsigned char *rom;
- int i;
+ unsigned char c;
+ int i;
/* video rom */
upper = adapter_rom_resources[0].start;
@@ -189,8 +191,11 @@ static void __init probe_roms(void)
video_rom_resource.start = start;
+ if (probe_kernel_address(rom + 2, c) != 0)
+ continue;
+
/* 0 < length <= 0x7f * 512, historically */
- length = rom[2] * 512;
+ length = c * 512;
/* if checksum okay, trust length byte */
if (length && romchecksum(rom, length))
@@ -224,8 +229,11 @@ static void __init probe_roms(void)
if (!romsignature(rom))
continue;
+ if (probe_kernel_address(rom + 2, c) != 0)
+ continue;
+
/* 0 < length <= 0x7f * 512, historically */
- length = rom[2] * 512;
+ length = c * 512;
/* but accept any length that fits if checksum okay */
if (!length || start + length > upper || !romchecksum(rom,
length))
Re: [PATCH] romsignature/checksum cleanup
Rene Herman wrote:
> Doing the set_fs() and pagefault_{disable,enable} calls for every
> single byte during the checksum seems rather silly.
Why? It's a bit of a performance hit, but that doesn't matter here.
probe_kernel_address() is semantically the right thing to be using;
open-coding its contents to avoid a few fairly cheap operations is a
backwards step.
> I disagree I'm afraid. Given what __get_user compiles to (nothing more
> than a .fixup entry, basically) they're largely "free" and it makes
> the code completely obvious: "If you're touching this, do so via
> __get_user and not directly" and frees it from any assumptions,
> however reasonable or unreasonable.
My point is that "__get_user" doesn't make much semantic sense here:
we're not talking about usermode pages. We used to use it quite often
for cases where an access may or may not fault, but now we spell that
"probe_kernel_address()".
> Would you _mind_ if I submit it? If not, if you could comment on
> whether or not these pagefault calls are still useful, that would be
> great.
I don't strongly object to using probe_kernel_address() for all ROM
memory accesses if it makes you feel happier, but I think putting an
open-coded implementation in here is definitely the wrong thing to do.
J
-
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] romsignature/checksum cleanup
On 01/07/2007 11:20 AM, Jeremy Fitzhardinge wrote:
Rene Herman wrote:
How is it for efficiency? I thought it was for correctness.
romsignature is using probe_kernel_adress() while all other accesses
to the ROMs there aren't.
If nothing else, anyone reading that code is likely to ask himself the
very same question -- why the one, and not the others.
Well, I was wondering about all the uses of __get_user; why not
probe_kernel_address() everywhere?
It's just a manual version of probe_kernel_adress():
#define probe_kernel_address(addr, retval) \
({ \
long ret; \
mm_segment_t old_fs = get_fs(); \
\
set_fs(KERNEL_DS); \
pagefault_disable();\
ret = __get_user(retval, [ ... ]); \
pagefault_enable(); \
set_fs(old_fs); \
ret;\
})
Doing the set_fs() and pagefault_{disable,enable} calls for every single
byte during the checksum seems rather silly. The patch as posted has the
set_fs() and pagefault_ calls only once in probe_roms() (as said when
posted, I'm not sure the pagefault calls are still useful now that it's
no longer a generic function/macro, but used directly at probe_roms() time).
I think its reasonable to assume that if the signature is mapped and
correct, then everything else is mapped. That's certainly the case
for Xen, which is why I added it. If you think this is unclear, then
I think a comment to explain this rather than code changes is the
appropriate fix.
I disagree I'm afraid. Given what __get_user compiles to (nothing more
than a .fixup entry, basically) they're largely "free" and it makes the
code completely obvious: "If you're touching this, do so via __get_user
and not directly" and frees it from any assumptions, however reasonable
or unreasonable.
Would you _mind_ if I submit it? If not, if you could comment on whether
or not these pagefault calls are still useful, that would be great. The
comment at probe_kernel_address() says:
* We ensure that the __get_user() is executed in atomic context so that
* do_page_fault() doesn't attempt to take mmap_sem. This makes
* probe_kernel_address() suitable for use within regions where the
* caller already holds mmap_sem, or other locks which nest inside
* mmap_sem
This sounds like it might be nonsensical at probe_roms() time, but I'm
not familiar with these virtualized environments -- I do not know which
assumptions break.
Patch attached again for reference...
Rene.
commit f153a588097c08cefdb799f22123192a9975d273
Author: Rene Herman <[EMAIL PROTECTED]>
Date: Sat Jan 6 04:09:32 2007 +0100
Use __get_user() for ISA ROM accesses.
In virtualized environments, the ISA ROMs may not be mapped so be careful
about touching them.
Signed-off-by: Rene Herman <[EMAIL PROTECTED]>
diff --git a/arch/i386/kernel/e820.c b/arch/i386/kernel/e820.c
index f391abc..8b54f65 100644
--- a/arch/i386/kernel/e820.c
+++ b/arch/i386/kernel/e820.c
@@ -156,29 +156,34 @@ static struct resource standard_io_resou
.flags = IORESOURCE_BUSY | IORESOURCE_IO
} };
-static int romsignature(const unsigned char *x)
+#define ROM_SIG 0xaa55
+
+static int __init romsignature(const unsigned char *rom)
{
unsigned short sig;
- int ret = 0;
- if (probe_kernel_address((const unsigned short *)x, sig) == 0)
- ret = (sig == 0xaa55);
- return ret;
+
+ return !__get_user(sig, (const unsigned short *)rom) && sig == ROM_SIG;
}
-static int __init romchecksum(unsigned char *rom, unsigned long length)
+static int __init romchecksum(const unsigned char *rom, unsigned long length)
{
- unsigned char *p, sum = 0;
+ unsigned char sum, c;
- for (p = rom; p < rom + length; p++)
- sum += *p;
- return sum == 0;
+ for (sum = 0; length && !__get_user(c, rom); rom++, length--)
+ sum += c;
+ return !length && !sum;
}
static void __init probe_roms(void)
{
+ const unsigned char *rom;
unsigned long start, length, upper;
- unsigned char *rom;
- int i;
+ unsigned char c;
+ int i;
+ mm_segment_t old_fs = get_fs();
+
+ set_fs(KERNEL_DS);
+ pagefault_disable();
/* video rom */
upper = adapter_rom_resources[0].start;
@@ -189,8 +194,11 @@ static void __init probe_roms(void)
video_rom_resource.start = start;
+ if (__get_user(c, rom + 2))
+ continue;
+
/* 0 < length <= 0x7f * 512, historically */
- length = rom[2] * 512;
+ length = c *
Re: [PATCH] romsignature/checksum cleanup
Rene Herman wrote: > How is it for efficiency? I thought it was for correctness. > romsignature is using probe_kernel_adress() while all other accesses > to the ROMs there aren't. > > If nothing else, anyone reading that code is likely to ask himself the > very same question -- why the one, and not the others. Well, I was wondering about all the uses of __get_user; why not probe_kernel_address() everywhere? I think its reasonable to assume that if the signature is mapped and correct, then everything else is mapped. That's certainly the case for Xen, which is why I added it. If you think this is unclear, then I think a comment to explain this rather than code changes is the appropriate fix. J - 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] romsignature/checksum cleanup
On 01/07/2007 09:59 AM, Jeremy Fitzhardinge wrote:
Rene Herman wrote:
In your opinion, is the attached (versus 2.6.20-rc3) better? This
uses probe_kernel_address() for all accesses. Or rather, an
expanded version thereof. The set_fs() and
pagefault_{disable,enable} calls are only done once in
probe_roms().
I don't think this is worthwhile. Its hardly a performance-critical
piece of code, and I think its better to use the straightforward
interface rather than complicating it for some nominal extra
efficiency.
How is it for efficiency? I thought it was for correctness. romsignature
is using probe_kernel_adress() while all other accesses to the ROMs
there aren't.
If nothing else, anyone reading that code is likely to ask himself the
very same question -- why the one, and not the others.
Rene.
-
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] romsignature/checksum cleanup
Rene Herman wrote:
> In your opinion, is the attached (versus 2.6.20-rc3) better? This uses
> probe_kernel_address() for all accesses. Or rather, an expanded
> version thereof. The set_fs() and pagefault_{disable,enable} calls are
> only done once in probe_roms().
>
> Accessing the length byte at rom[2] with __get_user() is overkill
> after just checking the signature at 0 and 1 but direcly accessing
> only that makes for inconsistent code IMO. It's only a .fixup entry...
>
> I can't say I'm all that sure that that pagefault_disable() call is
> still applicable now that it got expanded into the probe_roms() stage?
I don't think this is worthwhile. Its hardly a performance-critical
piece of code, and I think its better to use the straightforward
interface rather than complicating it for some nominal extra efficiency.
J
-
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] romsignature/checksum cleanup
Jeremy Fitzhardinge wrote:
Well, in the Xen case, where the pages are simply not mapped, then
the signature simply won't exist. In other cases, I guess its
possible the signature might exist but the rest of the ROM doesn't,
but that won't happen on normal hardware.
In your opinion, is the attached (versus 2.6.20-rc3) better? This uses
probe_kernel_address() for all accesses. Or rather, an expanded version
thereof. The set_fs() and pagefault_{disable,enable} calls are only done
once in probe_roms().
Accessing the length byte at rom[2] with __get_user() is overkill after
just checking the signature at 0 and 1 but direcly accessing only that
makes for inconsistent code IMO. It's only a .fixup entry...
I can't say I'm all that sure that that pagefault_disable() call is
still applicable now that it got expanded into the probe_roms() stage?
Rene.
commit f153a588097c08cefdb799f22123192a9975d273
Author: Rene Herman <[EMAIL PROTECTED]>
Date: Sat Jan 6 04:09:32 2007 +0100
Use __get_user() for ISA ROM accesses.
In virtualized environments, the ISA ROMs may not be mapped so be careful
about touching them.
Signed-off-by: Rene Herman <[EMAIL PROTECTED]>
diff --git a/arch/i386/kernel/e820.c b/arch/i386/kernel/e820.c
index f391abc..8b54f65 100644
--- a/arch/i386/kernel/e820.c
+++ b/arch/i386/kernel/e820.c
@@ -156,29 +156,34 @@ static struct resource standard_io_resou
.flags = IORESOURCE_BUSY | IORESOURCE_IO
} };
-static int romsignature(const unsigned char *x)
+#define ROM_SIG 0xaa55
+
+static int __init romsignature(const unsigned char *rom)
{
unsigned short sig;
- int ret = 0;
- if (probe_kernel_address((const unsigned short *)x, sig) == 0)
- ret = (sig == 0xaa55);
- return ret;
+
+ return !__get_user(sig, (const unsigned short *)rom) && sig == ROM_SIG;
}
-static int __init romchecksum(unsigned char *rom, unsigned long length)
+static int __init romchecksum(const unsigned char *rom, unsigned long length)
{
- unsigned char *p, sum = 0;
+ unsigned char sum, c;
- for (p = rom; p < rom + length; p++)
- sum += *p;
- return sum == 0;
+ for (sum = 0; length && !__get_user(c, rom); rom++, length--)
+ sum += c;
+ return !length && !sum;
}
static void __init probe_roms(void)
{
+ const unsigned char *rom;
unsigned long start, length, upper;
- unsigned char *rom;
- int i;
+ unsigned char c;
+ int i;
+ mm_segment_t old_fs = get_fs();
+
+ set_fs(KERNEL_DS);
+ pagefault_disable();
/* video rom */
upper = adapter_rom_resources[0].start;
@@ -189,8 +194,11 @@ static void __init probe_roms(void)
video_rom_resource.start = start;
+ if (__get_user(c, rom + 2))
+ continue;
+
/* 0 < length <= 0x7f * 512, historically */
- length = rom[2] * 512;
+ length = c * 512;
/* if checksum okay, trust length byte */
if (length && romchecksum(rom, length))
@@ -224,8 +232,11 @@ static void __init probe_roms(void)
if (!romsignature(rom))
continue;
+ if (__get_user(c, rom + 2))
+ continue;
+
/* 0 < length <= 0x7f * 512, historically */
- length = rom[2] * 512;
+ length = c * 512;
/* but accept any length that fits if checksum okay */
if (!length || start + length > upper || !romchecksum(rom,
length))
@@ -237,6 +248,9 @@ static void __init probe_roms(void)
start = adapter_rom_resources[i++].end & ~2047UL;
}
+
+ pagefault_enable();
+ set_fs(old_fs);
}
/*
Re: [PATCH] romsignature/checksum cleanup
Rene Herman wrote: > Jeremy? Is it okay to only check the signature word? The checksum will > generally be done over more than 1 (hw) page... That "presumably" > there seems a bit flaky? Well, in the Xen case, where the pages are simply not mapped, then the signature simply won't exist. In other cases, I guess its possible the signature might exist but the rest of the ROM doesn't, but that won't happen on normal hardware. J - 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] romsignature/checksum cleanup
Zachary Amsden wrote: Rusty Russell wrote: Rene Herman wrote: Hmm, by the way, if romsignature() needs this probe_kernel_address() thing, why doesn't romchecksum()? I assume it's all in the same page, but CC'ing Zach is easier than reading the code 8) Some hypervisors don't emulate the traditional physical layout of the first 1M of memory, so those pages might never get physical mappings established during the boot process, causing access to them to fault. Presumably, if the first page is there with a good signature, the entire ROM is mapped. I think Jeremy added this for Xen, and it's harmless on native hardware. Jeremy? Is it okay to only check the signature word? The checksum will generally be done over more than 1 (hw) page... That "presumably" there seems a bit flaky? Rene. - 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] romsignature/checksum cleanup
Rusty Russell wrote: On Mon, 2006-12-25 at 01:53 +0100, Rene Herman wrote: Rene Herman wrote: Use adding __init to romsignature() (it's only called from probe_roms() which is itself __init) as an excuse to submit a pedantic cleanup. Hmm, by the way, if romsignature() needs this probe_kernel_address() thing, why doesn't romchecksum()? I assume it's all in the same page, but CC'ing Zach is easier than reading the code 8) Some hypervisors don't emulate the traditional physical layout of the first 1M of memory, so those pages might never get physical mappings established during the boot process, causing access to them to fault. Presumably, if the first page is there with a good signature, the entire ROM is mapped. I think Jeremy added this for Xen, and it's harmless on native hardware. Zach - 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] romsignature/checksum cleanup
On Mon, 2006-12-25 at 01:53 +0100, Rene Herman wrote: > Rene Herman wrote: > > > Use adding __init to romsignature() (it's only called from probe_roms() > > which is itself __init) as an excuse to submit a pedantic cleanup. > > Hmm, by the way, if romsignature() needs this probe_kernel_address() > thing, why doesn't romchecksum()? I assume it's all in the same page, but CC'ing Zach is easier than reading the code 8) Rusty. - 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] romsignature/checksum cleanup
Rusty Russell wrote: On Mon, 2006-12-25 at 01:53 +0100, Rene Herman wrote: Hmm, by the way, if romsignature() needs this probe_kernel_address() thing, why doesn't romchecksum()? I assume it's all in the same page, but CC'ing Zach is easier than reading the code 8) If we're talking hardware pages here; the romchecksum() might be done over an area upto 0xff x 512 = 130560 bytes (there's also an acces to the length byte at rom[2] in probe_roms(). I assume that one's okay if romsignature() ensured that the first page is in). Rene. - 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] romsignature/checksum cleanup
Rene Herman wrote: Use adding __init to romsignature() (it's only called from probe_roms() which is itself __init) as an excuse to submit a pedantic cleanup. Hmm, by the way, if romsignature() needs this probe_kernel_address() thing, why doesn't romchecksum()? (Rusty in CC as author of bd472c794bbf6771c3fc1c58f188bc16c393d2fe) Rene. - 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] romsignature/checksum cleanup
Hi Andrew.
Use adding __init to romsignature() (it's only called from probe_roms()
which is itself __init) as an excuse to submit a pedantic cleanup.
Signed-off-by: Rene Herman <[EMAIL PROTECTED]>
diff --git a/arch/i386/kernel/e820.c b/arch/i386/kernel/e820.c
index f391abc..2565fac 100644
--- a/arch/i386/kernel/e820.c
+++ b/arch/i386/kernel/e820.c
@@ -156,21 +156,22 @@ static struct resource standard_io_resou
.flags = IORESOURCE_BUSY | IORESOURCE_IO
} };
-static int romsignature(const unsigned char *x)
+#define ROMSIGNATURE 0xaa55
+
+static int __init romsignature(const unsigned char *rom)
{
unsigned short sig;
- int ret = 0;
- if (probe_kernel_address((const unsigned short *)x, sig) == 0)
- ret = (sig == 0xaa55);
- return ret;
+
+ return probe_kernel_address((const unsigned short *)rom, sig) == 0 &&
+ sig == ROMSIGNATURE;
}
static int __init romchecksum(unsigned char *rom, unsigned long length)
{
- unsigned char *p, sum = 0;
+ unsigned char sum;
- for (p = rom; p < rom + length; p++)
- sum += *p;
+ for (sum = 0; length; length--)
+ sum += *rom++;
return sum == 0;
}
