linux-kernel - Re: [PATCH 0/4] amd64

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: <20090530081954.GA21954@liondog.tnic>
Date:	Sat, 30 May 2009 10:19:54 +0200
From:	Borislav Petkov <petkovbb@...glemail.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Borislav Petkov <borislav.petkov@....com>, greg@...ah.com,
	mingo@...e.hu, norsk5@...oo.com, tglx@...utronix.de, hpa@...or.com,
	mchehab@...hat.com, aris@...hat.com, edt@....ca,
	linux-kernel@...r.kernel.org, randy.dunlap@...cle.com
Subject: Re: [PATCH 0/4] amd64_edac: misc fixes

On Fri, May 29, 2009 at 01:01:15PM -0700, Andrew Morton wrote:
> gcc-4.0.2, gas-2.16.1

yep, just as I thought. binutils got the Fam10h support around July 2006
and yours are from June 2005.

> Here's your sent-off-list patch (please don't do that):

Ups, sorry. This wasn't meant to be a patch but only to show the
functional change. I'll integrate it as such in the patchset and rebase
the whole thing next week.

> diff -puN drivers/edac/amd64_edac.c~edac-build-fix drivers/edac/amd64_edac.c
> --- a/drivers/edac/amd64_edac.c~edac-build-fix
> +++ a/drivers/edac/amd64_edac.c
> @@ -1419,7 +1419,7 @@ static u32 f10_determine_channel(struct 
>  		if (dct_sel_interleave_addr(pvt) == 0)
>  			cs = sys_addr >> 6 & 1;
>  		else if ((dct_sel_interleave_addr(pvt) >> 1) & 1) {
> -			temp = popcnt((u32) ((sys_addr >> 16) & 0x1F)) % 2;
> +			temp = hweight_long((u32) ((sys_addr >> 16) & 0x1F)) % 2;
>  
>  			if (dct_sel_interleave_addr(pvt) & 1)
>  				cs = (sys_addr >> 9 & 1) ^ temp;
> diff -puN drivers/edac/amd64_edac.h~edac-build-fix drivers/edac/amd64_edac.h
> --- a/drivers/edac/amd64_edac.h~edac-build-fix
> +++ a/drivers/edac/amd64_edac.h
> @@ -443,19 +443,6 @@ enum {
>  #define K8_MSR_MC4STAT			0x0411
>  #define K8_MSR_MC4ADDR			0x0412
>  
> -/*
> - * popcnt - count the set bits in a bit vector.
> - * @vec - bit vector
> - *
> - * This instruction is supported only on F10h and later CPUs.
> - */
> -#define popcnt(x)						\
> -({								\
> -	typeof(x) __ret;					\
> -	__asm__("popcnt %1, %0" : "=r" (__ret) : "r" (x));	\
> -	__ret;							\
> -})
> -
>  /* AMD sets the first MC device at device ID 0x18. */
>  static inline int get_mc_node_id_from_pdev(struct pci_dev *pdev)
>  {
> _
> 
> That'll work.
> 
> A suitable way to solve all this would be to arrange for x86's hweight()
> to use the popcnt instruction, where that makes sense.

That's funny, actually we _have_ _been_ experimenting here with
replacing hweight with popcnt and there's only a minor speedup. We
originally thought it might be a good idea to do the Hamming weight
calculation in hardware, especially when this is used in hot paths like
the scheduler but the bitmask counting is not being done enough times
to actually notice any significant improvement. Besides, the hweight_*
thingies are quite fast the way they are.

But sure, I could dig out my patchset and rediff it for review - it is
pretty small, actually. Also, I've been thinking about how the old(er)
toolchain problem can be addressed and one fairly doable thing would be
if I'd query the gas version in the kernel Makefile and define popcnt
dependent on it and for older assemblers simply slap in the opcode and
fixate the operands in an inline assembly so that it works.

Possible issues with that would be if someone uses a different toolchain
but do we support that at all? I'm guessing distro-patched bintuils
won't be a problem since even if they introduced popcnt support earlier,
the opcode would still be correct.

Opinions? Comments?

-- 
Regards/Gruss,
    Boris.
--
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/