From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1756960AbcFHN3v (ORCPT <rfc822;w@1wt.eu>);
	Wed, 8 Jun 2016 09:29:51 -0400
Received: from mail-wm0-f67.google.com ([74.125.82.67]:36161 "EHLO
	mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751194AbcFHN3u (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 8 Jun 2016 09:29:50 -0400
Date: Wed, 8 Jun 2016 15:29:44 +0200
From: Ingo Molnar <mingo@kernel.org>
To: Dave Hansen <dave@sr71.net>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, dave.hansen@linux.intel.com
Subject: Re: [PATCH 3/3] x86, mpx, selftests: add MPX self test
Message-ID: <20160608132944.GA18019@gmail.com>
References: <20160526183611.B2E4B414@viggo.jf.intel.com>
 <20160526183616.C73F080F@viggo.jf.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160526183616.C73F080F@viggo.jf.intel.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


These patches look good to me, but it would be nice to get rid of uglies like:

> +/*
> + * Written by Dave Hansen <dave.hansen@intel.com>
> + *
> + * run like this:
> + pid=31390; BDIR="$(cat /proc/$pid/smaps | grep -B1 2097152 | head -1 | awk -F- '{print $1}')"; ./mpx-dig $pid 0x$BDIR
> +
> +NOTE:
> + assumes that the only 2097152-kb VMA is the bounds dir
> + */

(weird vertical alignment)

> +long nr_incore(void *ptr, unsigned long size_bytes)
> +{
> +	int i;
> +	long ret = 0;
> +	long vec_len = size_bytes / PAGE_SIZE;
> +	unsigned char *vec = malloc(vec_len);
> +	if (!vec)

(missing newline)

> +		mpx_dig_abort();
> +
> +	int incore_ret = mincore(ptr, size_bytes, vec);

(weird non-kernel-style definition.)

> +char buf[100];
> +int open_proc(int pid, char *file)

(missing newline)

> +{
> +	int fd;
> +	sprintf(&buf[0], "/proc/%d/%s", pid, file);

(ditto, also unsafe looking sprintf())

> +void __dave_abort(int line)
> +{
> +	perror("abort");
> +	printf("abort @ %d\n", line);
> +	mpx_dig_abort();
> +}
> +#define dave_mpx_dig_abort() __dave_abort(__LINE__);

maybe s/dave// ? ;-)

> +//This mincore stuff works, but the bounds tables are not
> +//sparse enough to make it worthwhile

bad comment style.

> +	unsigned char incore_vec[MPX_BOUNDS_TABLE_SIZE_BYTES/ PAGE_SIZE];
> + 	int incore_ret = mincore(bt_buf, MPX_BOUNDS_TABLE_SIZE_BYTES, &incore_vec[0]);

stray whitespace.

> +		//if (!incore_vec[page_nr])
> +		//	continue;

ugly.

> +		if (this_bt_entry_for_vaddr > 0x00007fffffffffffUL) {
> +			this_bt_entry_for_vaddr |= 0xffff800000000000;
> +		}

unnecessary curly braces.

> +		// Each bounds directory entry controls 1MB of
> +		// virtual address space.  This variable is the
> +		// virtual address in the process of the
> +		// beginning of the area controlled by this
> +		// bounds_dir.

ugly.

> +		unsigned long bd_for_vaddr = bd_index * (1UL<<20);
> +		//printf("%s() at bd index: %lx for vaddr: %lx\n", __func__, bd_index, bd_for_vaddr);
> +		//printf("dir entry[%4ld @ %p]\n", bd_index, bounds_dir_global+i);

Ditto.

> +		int nr_entries = dump_table(bounds_dir_entry, bd_for_vaddr, bounds_dir_global+bd_offset_bytes+i);
> +		total_entries += nr_entries;
> +		continue;
> +		printf("dir entry[%4ld @ %p]: 0x%lx %6d entries total this buf: %7d bd_for_vaddrs: 0x%lx -> 0x%lx\n",
> +				bd_index, buf+i,
> +				bounds_dir_entry, nr_entries, total_entries, bd_for_vaddr, bd_for_vaddr + (1UL<<20));

hm?

> +	// there shouldn't practically be short reads of /proc/$pid/mem

> +
> + 	incore_ret = mincore(dig_bounds_dir_ptr, buffer_size_bytes, &vec[0]);

stray space.

> +		//if (this_entries)
> +		//	printf("entries this loop: %d total: %d (bufs skipped: %d)\n", this_entries, total_entries, bufs_skipped);

You know the drill!

> +#ifdef MPX_DIG_REMOTE
> +int main(int argc, char **argv)
> +{
> +	int err;
> +        char *c;

hm...

etc. - ad nauseum. Would be nice to tidy this all up a bit.

Thanks,

	Ingo