linux-kernel - Re: [patch 06/10] cpu topology support for s390.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1205411307.26537.64.camel@localhost>
Date:	Thu, 13 Mar 2008 13:28:27 +0100
From:	Martin Schwidefsky <schwidefsky@...ibm.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
	heiko.carstens@...ibm.com
Subject: Re: [patch 06/10] cpu topology support for s390.

On Wed, 2008-03-12 at 16:11 -0700, Andrew Morton wrote:
> > +#include <asm/delay.h>
> > +#include <asm/s390_ext.h>
> > +
> > +#define CPU_BITS 64
> > +
> > +struct tl_cpu {
> > +	unsigned char reserved[6];
> > +	unsigned short origin;
> > +	unsigned long mask[CPU_BITS / BITS_PER_LONG];
> > +};
> 
> mask[] will be too small for CPU_BITS=65 ;)

We could add the +(BITS_PER_LONG - 1) logic but what for? The CPU_BITS
is defined right above and it will be increased in steps of 64.

> > ...
> >
> > +static union tl_entry *next_tle(union tl_entry *tle)
> > +{
> > +	if (tle->nl)
> > +		return (union tl_entry *)((struct tl_container *)tle + 1);
> > +	else
> > +		return (union tl_entry *)((struct tl_cpu *)tle + 1);
> > +}
> 
> omg.

The length of the current tle depends on the type, the next type is
located behind the current one. Expect for the typecasting and the union
trick this is what needs to be done.

> > +static void tl_to_cores(struct tl_info *info)
> > +{
> > +	union tl_entry *tle, *end;
> > +	struct core_info *core = &core_info;
> > +
> > +	mutex_lock(&smp_cpu_state_mutex);
> > +	clear_cores();
> > +	tle = (union tl_entry *)&info->tle;
> 
> and this cast was unneeded!
> 
> > +	end = (union tl_entry *)((unsigned long)info + info->length);
> 
> I'd suggest that you take a look at all the pointer arith games which are
> being played in this code and see if it can be done better with a more
> appropriate use of the C type system.  Before someone dies.

The only thing that I can see that we could do is to get rid of the
unions and do the pointer arithmetic with the tl_cpu / tl_container
structs by hand. The data stored by ptf is structured in a way that
makes it rather hard to write decent C code.

-- 
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.


--
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/