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;
}
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.
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.
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.
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
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.
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
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);
}
/*
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
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.
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
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);
}
/*
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
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))