| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 29 Apr 2008 15:07:04 +0200
From: Ingo Molnar <mingo@...e.hu>
To: yhlu.kernel@...il.com
Cc: Andrew Morton <akpm@...ux-foundation.org>,
"H. Peter Anvin" <hpa@...or.com>,
Thomas Gleixner <tglx@...utronix.de>,
Gabriel C <nix.or.die@...glemail.com>,
Mika Fischer <mika.fischer@...pnet.de>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] x86: mtrr cleanup for converting continuous to
discrete layout v8
a few minor cleanliness observations:
> +#ifdef CONFIG_MTRR_SANITIZER
> +
> +#ifdef CONFIG_MTRR_SANITIZER_ENABLE_DEFAULT
> +static int enable_mtrr_cleanup __initdata = 1;
> +#else
> +static int enable_mtrr_cleanup __initdata;
> +#endif
> +
> +#else
> +
> +static int enable_mtrr_cleanup __initdata = -1;
> +
> +#endif
this should be a single:
#ifdef CONFIG_MTRR_SANITIZER
static int mtrr_cleanup_enabled = CONFIG_MTRR_SANITIZER_DEFAULT;
#endif
block.
> +#define RANGE_NUM 256
small explaination (comment) about what the limit means.
> +static int __init add_range(struct res_range *range, int nr_range, unsigned long start,
> + unsigned long end, int merge)
looks cleaner this way:
static int __init
add_range(struct res_range *range, int nr_range, unsigned long start,
unsigned long end, int merge)
> +{
> + int i;
> +
> + if (!merge)
> + goto addit;
> +
> + /* try to merge it with old one */
> + for (i = 0; i < nr_range; i++) {
> + unsigned long final_start, final_end;
> + unsigned long common_start, common_end;
> +
> + if (!range[i].end)
> + continue;
> +
> + common_start = max(range[i].start, start);
> + common_end = min(range[i].end, end);
> + if (common_start > common_end + 1)
> + continue;
> +
> + final_start = min(range[i].start, start);
> + final_end = max(range[i].end, end);
> +
> + range[i].start = final_start;
> + range[i].end = final_end;
> + return nr_range;
> + }
> +
> +addit:
perhaps factor out the loop into a separate function and avoid the goto.
> +static void __init subtract_range(struct res_range *range, unsigned long start,
> + unsigned long end)
should be:
static void __init
subtract_range(struct res_range *range, unsigned long start,
unsigned long end)
> + int i;
> + int j;
can be:
int i, j;
> + }
> +
> +
stale newline.
> + if (start > range[j].start && end >= range[j].end && range[j].end > start - 1) {
should be some sort of more readable in_range() check?
> + range[j].end = start - 1;
> + continue;
> + }
> +
> + if (start > range[j].start && end < range[j].end) {
> + /* find the new spare */
> + for (i = 0; i < RANGE_NUM; i++) {
> + if (range[i].end == 0)
> + break;
> + }
> + if (i < RANGE_NUM) {
> + range[i].end = range[j].end;
> + range[i].start = end + 1;
> + } else {
> + printk(KERN_ERR "run of slot in ranges\n");
> + }
> + range[j].end = start - 1;
> + continue;
> + }
> + }
> +}
> +struct var_mtrr_state {
> + unsigned long range_startk, range_sizek;
> + unsigned long chunk_sizek;
> + unsigned long gran_sizek;
> + unsigned int reg;
> + unsigned address_bits;
> +};
s/unsigned address_bits/unsigned int address_bits/
also move range_sizek on a separate line.
plus we tend to align structures this way:
> +struct var_mtrr_state {
> + unsigned long range_startk;
> + unsigned long range_sizek;
> + unsigned long chunk_sizek;
> + unsigned long gran_sizek;
> + unsigned int reg;
> + unsigned int address_bits;
> +};
(to put the types and field names into a visually more consistent form)
> +static void __init set_var_mtrr(
> + unsigned int reg, unsigned long basek, unsigned long sizek,
> + unsigned char type, unsigned address_bits)
should be:
static void __init
set_var_mtrr(unsigned int reg, unsigned long basek, unsigned long sizek,
unsigned char type, unsigned address_bits)
> + u32 base_lo, base_hi, mask_lo, mask_hi;
> + unsigned address_mask_high;
s/unsigned/unsigned int
hm, will this work on 64-bit? Above-4G is controlled via separate
mechanisms though so i guess it does.
> + address_mask_high = ((1u << (address_bits - 32u)) - 1u);
use alignment macros instead.
> + unsigned long sizek;
> + /* Compute the maximum size I can make a range */
> + if (range_startk)
put extra newline between variable definition and code.
> + var_state.range_startk = 0;
> + var_state.range_sizek = 0;
> + var_state.reg = 0;
> + var_state.address_bits = address_bits;
> + var_state.chunk_sizek = mtrr_chunk_size >> 10;
> + var_state.gran_sizek = mtrr_gran_size >> 10;
initialization looks nicer with vertical alignment, i.e.:
> + var_state.range_startk = 0;
> + var_state.range_sizek = 0;
> + var_state.reg = 0;
> + var_state.address_bits = address_bits;
> + var_state.chunk_sizek = mtrr_chunk_size >> 10;
> + var_state.gran_sizek = mtrr_gran_size >> 10;
> + /* Clear out the extra MTRR's */
> + while (var_state.reg < num_var_ranges)
> + set_var_mtrr(var_state.reg++, 0, 0, 0, var_state.address_bits);
the ++ is a hard to notice side-effect of the loop. It's cleaner to
separate it out or to have a for() loop for it.
> +static int __init mtrr_cleanup(unsigned address_bits)
> +{
> + unsigned long i, base, size, def, dummy;
> + mtrr_type type;
> + struct res_range range[RANGE_NUM];
> + int nr_range;
> + unsigned long extra_remove_base, extra_remove_size;
try to use a 'christmas tree' ordering of variables, i.e.:
> + unsigned long extra_remove_base, extra_remove_size;
> + unsigned long i, base, size, def, dummy;
> + struct res_range range[RANGE_NUM];
> + mtrr_type type;
> + int nr_range;
> + return 1;
> +
> +}
superfluous newline.
all in one, this is a very useful and nice feature.
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists