public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 00/21 v2] amd64_edac: EDAC module for AMD64
@ 2009-04-29 16:54 Borislav Petkov
  2009-04-29 16:54 ` [PATCH 01/21] x86: add methods for writing of an MSR on several CPUs Borislav Petkov
                   ` (21 more replies)
  0 siblings, 22 replies; 70+ messages in thread
From: Borislav Petkov @ 2009-04-29 16:54 UTC (permalink / raw)
  To: akpm, greg
  Cc: mingo, tglx, hpa, dougthompson, linux-kernel, Borislav Petkov,
	Doug Thompson

Hi,

thanks to all reviewers of the previous submission, here is the second
version of this series.

Highlights are the addition of two helpers to read/write MSRs on several
CPUs, denoted by a cpumask and using an array of MSR values per-CPU, as
Peter suggested. Since IMHO they look generic enough I've added them to
arch/x86/lib/msr-on-cpu.c (now renamed to msr.c).

Moreover, I've addressed all the issues raised from the previous series.
Please let me know should there be anything else remaining.

Thanks,
Boris.

 arch/x86/include/asm/msr.h |   11 +
 arch/x86/lib/Makefile      |    2 +-
 arch/x86/lib/msr-on-cpu.c  |   97 -
 arch/x86/lib/msr.c         |  151 ++
 drivers/edac/Kconfig       |   26 +
 drivers/edac/Makefile      |    1 +
 drivers/edac/amd64_edac.c  | 5385 ++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 5575 insertions(+), 98 deletions(-)


^ permalink raw reply	[flat|nested] 70+ messages in thread
* Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3
@ 2009-04-30  6:21 Doug Thompson
  0 siblings, 0 replies; 70+ messages in thread
From: Doug Thompson @ 2009-04-30  6:21 UTC (permalink / raw)
  To: Andrew Morton, Ingo Molnar
  Cc: torvalds, borislav.petkov, greg, tglx, hpa, dougthompson,
	linux-kernel


--- On Wed, 4/29/09, Ingo Molnar <mingo@elte.hu> wrote:

> From: Ingo Molnar <mingo@elte.hu>
> Subject: Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3
> To: "Andrew Morton" <akpm@linux-foundation.org>
> Cc: torvalds@linux-foundation.org, borislav.petkov@amd.com, greg@kroah.com, tglx@linutronix.de, hpa@zytor.com, dougthompson@xmission.com, linux-kernel@vger.kernel.org
> Date: Wednesday, April 29, 2009, 1:53 PM
> 
> * Andrew Morton <akpm@linux-foundation.org>
> wrote:
> 
> > On Wed, 29 Apr 2009 21:23:26 +0200
> > Ingo Molnar <mingo@elte.hu>
> wrote:
> > 
> > > > > > +   
>     if (CSFound >= 0) {
> > > > > > +   
>         *node_id = NodeID;
> > > > > > +   
>         *channel_select =
> ChannelSelect;
> > > > > > +   
>     }
> > > > > > +    }
> > > > > > +
> > > > > > +    return
> CSFound;
> > > > > > +}
> > > > > 
> > > > > this function is probably too large,
> and also it uses some weird 
> > > > > hungarian notation coding style. Please
> dont do that! It's 
> > > > > completely unacceptable.
> > > > 
> > > > These identifers (or at least,
> DctSelBaseOffsetLong, which is the 
> > > > only one I googled for) come straight out of
> the AMD "BIOS and 
> > > > Kernel Developer's Guide".
> > > > 
> > > > Sucky though they are, there's value in
> making the kernel code 
> > > > match up with the documentation.
> > > 
> > > I'm generally resisting patches that hungarinize
> arch/x86/ (and heck 
> > > there's been many attempts ...) but there's some
> conflicting advice 
> > > here. I've Cc:-ed Linus, maybe he has an opinion
> about this.
> > > 
> > > My gut reaction would be 'hell no'. There's
> other, structural 
> > > problems with this code too, and doing some saner
> naming would 
> > > mostly be a sed job and would take minimal amount
> of time. The 
> > > naming can still be intuitive. The symbols from
> the documentation 
> > > can perhaps be mentioned in a couple of comments
> to establish a 
> > > mapping.
> > 
> > I think I disagree.  For those identifiers which
> map 1:1 with the 
> > manufacturer's document, the ugliness involved in
> exactly copying 
> > the manufacturer's chosen identifiers is outweighed by
> the benefit 
> > of exactly copying the manufacturer's chosen
> identifiers.
> > 
> > Of course, we don't have to use StinkyIdentifiers
> anywhere else.  
> > And the nice thing about that is that when one reads
> the code and 
> > comes across a StinkyIdentifier, one immeditely knows
> that it's an 
> > AMD-provided thing rather than a Linux-provided
> thing.
> > 
> > Zillions of StinkyIdentifiers get merged via this
> logic.
> 
> Andrew, for heaven's sake, please review the patchset - as
> i did.
> 
> The thing is, up to 12/21, the patches look like normal
> Linux 
> patches. (there's problems with them too, but on a
> different level)
> 
> Then do the StinkyIdentifiers show up, in full force:
> 
> +static int f10_match_to_this_node(struct amd64_pvt *pvt,
> int DramRange,
> +               
>            
>    u64 SystemAddr,
> +               
>            
>    int *node_id,
> +               
>            
>    int *channel_select)
> +{
> +       int CSFound = -1;
> +       int NodeID;
> +       int HiRangeSelected;
> +       u32 IntlvEn, IntlvSel;
> +       u32 DramEn;
> +       u32 Ilog;
> +    u32 HoleOffset, HoleEn;
> +       u32 InputAddr, Temp;
> +       u32 DctSelBaseAddr,
> DctSelIntLvAddr;
> +       u32 DctSelHi;
> +       u32 ChannelSelect;
> +       u64 DramBaseLong,
> DramLimitLong;
> +    u64 DctSelBaseOffsetLong,
> ChannelAddrLong;
> 
> Tell me, how is 'SystemAddr' or 'Temp' or 'Ilog' an AMD
> document 
> thing?
> 
> I have a much simpler explanation really: someone got
> really bored 
> at converting some code written For Another OS, somewhere
> in the 
> middle - and started plopping Other OS Code into a Linux
> driver ...
> 
> I dont mind the occasional _constant_ that tells us a hw
> API detail 
> in whatever externally dictated style - but this thing
> stinks 
> HeadToToe ... ;-)
> 
>     Ingo
> 

I think I didn't reply to ALL on this, so sorry for a possible repost.

Right from the BKDG from the AMD website is the following reference code.

The format is lost in the mailing, but it IS the AMD reference code. This code also lacks the fixes for 2 bugs I found and which AMD emailed me and which is in the driver patches.  I did do some refactoring and tried to break the monolithic monster into a main function with supporting functions, I think this adds to the understanding somewhat. 

So, I am looking for what IS the policy for utilizing mfger's reference code in a linux driver?

Keeping the StinkyIdentifiers does help when referencing the the reference code. Maybe with Boris' help, we can get AMD to update their document with a better linux version of the reference code:

doug t


31116 Rev 3.06 - March 26, 2008 AMD Family 10h Processor BKDG
Page 67

(int,int,int,int) TranslateSysAddrToCS((uint64)SystemAddr){
int SwapDone, BadDramCs;
int CSFound, NodeID, CS, F1Offset, F2Offset, F2MaskOffset, Ilog, device;
int HiRangeSelected, DramRange;
uint32 IntlvEn, IntlvSel;
uint32 DramBaseLow, DramLimitLow, DramEn;
uint32 HoleOffset, HoleEn;
uint32 CSBase, CSLimit, CSMask, CSEn;
uint32 InputAddr, Temp;
uint32 OnlineSpareCTL;
uint32 DctSelBaseAddr, DctSelIntLvAddr, DctGangEn, DctSelIntLvEn;
uint32 DctSelHiRngEn,DctSelHi;
uint64 DramBaseLong, DramLimitLong;
uint64 DctSelBaseOffsetLong, ChannelOffsetLong,ChannelAddrLong;
// device is a user supplied value for the PCI device ID of the processor
// from which CSRs are initially read from (current processor is fastest).
// CH0SPARE_RANK and CH1SPARE_RANK are user supplied values, determined
// by BIOS during DIMM sizing.
CSFound = 0;
for(DramRange = 0; DramRange < 8; DramRange++)
{
F1Offset = 0x40 + (DramRange << 3);
DramBaseLow = Get_PCI(bus0, device, func1, F1Offset);
DramEn = DramBaseLow & 0x00000003;

IntlvEn = (DramBaseLow & 0x00000700) >> 8;
DramBaseLow = DramBaseLow & 0xFFFF0000;
DramBaseLong = ((Get_PCI(bus0, device, func1, F1Offset + 0x100))<<32 +
DramBaseLow)<<8;
DramLimitLow = Get_PCI(bus0, device, func1, F1Offset + 4);
NodeID = DramLimitLow & 0x00000007;
IntlvSel = (DramLimitLow & 0x00000700) >> 8;
DramLimitLow = DramLimitLow | 0x0000FFFF;
DramLimitLong = ((Get_PCI(bus0, device, func1, F1Offset + 0x104))<<32 +
DramLimitLow)<<8 | 0xFF;
HoleEn = Get_PCI(bus0, dev24 + NodeID, func1, 0xF0);
HoleOffset = (HoleEn & 0x0000FF80);
HoleEn = (HoleEn &0x00000003);
if(DramEn && DramBaseLong<=SystemAddr && SystemAddr <= DramLimitLong)
{
if(IntlvEn == 0 || IntlvSel == ((SystemAddr >> 12) & IntlvEn))
{
if(IntlvEn == 1) Ilog = 1;
else if(IntlvEn == 3) Ilog = 2;
else if(IntlvEn == 7) Ilog = 3;
else Ilog = 0;
Temp = Get_PCI(bus0, device, func2, 0x110);
DctSelHiRngEn = Temp & 1;
DctSelHi = Temp>>1 & 1;
DctSelIntLvEn = Temp & 4;
DctGangEn = Temp & 0x10;
DctSelIntLvAddr = (Temp>>6) & 3;
DctSelBaseAddr = Temp & 0xFFFFF800;
DctSelBaseOffsetLong = Get_PCI(bus0, device, func2, 0x114)<<16;
//Determine if High range is selected
if(DctSelHiRngEn && DctGangEn==0 && (SystemAddr>>27) >=
(DctSelBaseAddr>>11)) HiRangeSelected = 1;
else HiRangeSelected=0;
//Determine Channel
if(DctGangEn) ChannelSelect = 0;
else if (HiRangeSelected) ChannelSelect = DctSelHi;
else if (DctSelIntLvEn && DctSelIntLvAddr == 0)
ChannelSelect = SystemAddr>>6 & 1;
else if (DctSelIntLvEn && DctSelIntLvAddr>>1 & 1)
{
Temp = fUnaryXOR(SystemAddr>>16&0x1F); //function returns odd parity
//1= number of set bits in argument is odd.
//0= number of set bits in argument is even.
if(DctSelIntLvAddr & 1) ChannelSelect = (SystemAddr>>9 & 1)^Temp;
else ChannelSelect = (SystemAddr>>6 & 1)^Temp;
}
else if (DctSelIntLvEn && IntlvEn&4)ChannelSelect = SystemAddr>>15&1;
else if (DctSelIntLvEn && IntlvEn&2)ChannelSelect = SystemAddr>>14&1;
else if (DctSelIntLvEn && IntlvEn&1)ChannelSelect = SystemAddr>>13&1;
else if (DctSelIntLvEn) ChannelSelect = SystemAddr>>12&1;
else if (DctSelHiRngEn && DctGangEn==0) ChannelSelect = ~DctSelHi&1;
else ChannelSelect = 0;
//Determine Base address Offset to use
if(HiRangeSelected)
{
if(!(DctSelBaseAddr & 0xFFFF0000) && (HoleEn & 1) &&
(SystemAddr >= 0x1_00000000))
ChannelOffsetLong = HoleOffset<<16;
31116 Rev 3.06 - March 26, 2008 AMD Family 10h Processor BKDG
68
else
ChannelOffsetLong= DctSelBaseOffsetLong;
}
else
{
if((HoleEn & 1) && (SystemAddr >= 0x1_00000000))
ChannelOffsetLong = HoleOffset<<16;
else
ChannelOffsetLong = DramBaseLong & 0xFFFF_F8000000;
}
//Remove hoisting offset and normalize to DRAM bus addresses
ChannelAddrLong = SystemAddr & 0x0000FFFF_FFFFFFC0 -
ChannelOffsetLong & 0x0000FFFF_FF800000;
//Remove Node ID (in case of processor interleaving)
Temp = ChannelAddrLong & 0xFC0;
ChannelAddrLong = (ChannelAddrLong >>Ilog & 0xFFFF_FFFFF000)|Temp;
//Remove Channel interleave and hash
if(DctSelIntLvEn && DctSelHiRngEn==0 && DctGangEn==0)
{
if(DctSelIntLvAddr & 1 != 1)
ChannelAddrLong = (ChannelAddrLong>>1) & 0xFFFFFFFF_FFFFFFC0;
else if(DctSelIntLvAddr == 1)
{
Temp = ChannelAddrLong & 0xFC0;
ChannelAddrLong = ((ChannelAddrLong & 0xFFFFFFFF_FFFFE000) >> 1)| Temp;
}
else
{
Temp = ChannelAddrLong & 0x1C0;
ChannelAddrLong = ((ChannelAddrLong & 0xFFFFFFFF_FFFFFC00) >> 1)| Temp;
}
InputAddr = ChannelAddrLong>>8;
for(CS = 0; CS < 8; CS++)
{
F2Offset = 0x40 + (CS << 2);
if ((CS % 2) == 0)
F2MaskOffset = 0x60 + (CS << 1);
else
F2MaskOffset = 0x60 + ((CS-1) << 1);
if(ChannelSelect)
{
F2Offset+=0x100;
F2MaskOffset+=0x100;
}
CSBase = Get_PCI(bus0, dev24 + NodeID, func2, F2Offset);
CSEn = CSBase & 0x00000001;
CSBase = CSBase & 0x1FF83FE0;
CSMask = Get_PCI(bus0, dev24 + NodeID, func2, F2MaskOffset);
CSMask = (CSMask | 0x0007C01F) & 0x1FFFFFFF;
if(CSEn && ((InputAddr & ~CSMask) == (CSBase & ~CSMask)))
{
CSFound = 1;
OnlineSpareCTL = Get_PCI(bus0, dev24 + NodeID, func3, 0xB0);
if(ChannelSelect)
{
SwapDone = (OnlineSpareCTL >> 3) & 0x00000001;
BadDramCs = (OnlineSpareCTL >> 8) & 0x00000007;
if(SwapDone && CS == BadDramCs) CS=CH1SPARE_RANK;
}
else
31116 Rev 3.06 - March 26, 2008 AMD Family 10h Processor BKDG
69
{
SwapDone = (OnlineSpareCTL >> 1) & 0x00000001;
BadDramCs = (OnlineSpareCTL >> 4) & 0x00000007;
if(SwapDone && CS == BadDramCs) CS=CH0SPARE_RANK;
}
break;
}
}
}
}
if(CSFound) break;
} // for each DramRange
return(CSFound,NodeID,ChannelSelect,CS);


^ permalink raw reply	[flat|nested] 70+ messages in thread
* Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3
@ 2009-05-05 19:30 Doug Thompson
  0 siblings, 0 replies; 70+ messages in thread
From: Doug Thompson @ 2009-05-05 19:30 UTC (permalink / raw)
  To: Borislav Petkov, Mauro Carvalho Chehab
  Cc: akpm, greg, mingo, tglx, hpa, dougthompson, linux-kernel



--- On Mon, 5/4/09, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:

> From: Mauro Carvalho Chehab <mchehab@redhat.com>
> Subject: Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3
> To: "Borislav Petkov" <borislav.petkov@amd.com>
> Cc: akpm@linux-foundation.org, greg@kroah.com, mingo@elte.hu, tglx@linutronix.de, hpa@zytor.com, dougthompson@xmission.com, linux-kernel@vger.kernel.org
> Date: Monday, May 4, 2009, 5:36 PM
> Borislav Petkov escreveu:
> > From: Doug Thompson <dougthompson@xmission.com>
> > 
> > Signed-off-by: Doug Thompson <dougthompson@xmission.com>
> > Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> > ---
> >  drivers/edac/amd64_edac.c |  318
> +++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 318 insertions(+), 0
> deletions(-)
> > 
> > diff --git a/drivers/edac/amd64_edac.c
> b/drivers/edac/amd64_edac.c
> > index fe2342c..84075c0 100644
> > --- a/drivers/edac/amd64_edac.c
> > +++ b/drivers/edac/amd64_edac.c
> > @@ -2726,4 +2726,322 @@ static int
> f10_lookup_addr_in_dct(u32 InputAddr, u32 NodeID, u32
> ChannelSelect)
> >      return CSFound;
> >  }
> >  +/*
> > + * f10_match_to_this_node
> > + *
> > + * For a given 'DramRange' value, check if
> 'SystemAddr' fall within this value
> > + */
> > +static int f10_match_to_this_node(struct amd64_pvt
> *pvt, int DramRange,
> > +       
>         u64 SystemAddr,
> > +       
>         int *node_id,
> > +       
>         int *channel_select)
> > +{
> > +    int CSFound = -1;
> >   
> 
> As in the previous patch, please use a standard error code,
> instead of -1.
> 
> > +static int f10_translate_sysaddr_to_CS(struct
> amd64_pvt *pvt,
> > +       
>     u64 SysAddr,
> > +       
>     int *node,
> > +       
>     int *chanSel)
> > +{
> > +    int DramRange;
> > +    int CSFound = -1;
> >   
> Same here.
> 
> Cheers,
> Mauro
> 

Code was reference code from AMD. Probably safe to convert to use -EINVAL yes

doug t


^ permalink raw reply	[flat|nested] 70+ messages in thread

end of thread, other threads:[~2009-05-05 19:31 UTC | newest]

Thread overview: 70+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-04-29 16:54 [RFC PATCH 00/21 v2] amd64_edac: EDAC module for AMD64 Borislav Petkov
2009-04-29 16:54 ` [PATCH 01/21] x86: add methods for writing of an MSR on several CPUs Borislav Petkov
2009-04-29 17:39   ` H. Peter Anvin
2009-05-04 16:46     ` Borislav Petkov
2009-05-04 17:25       ` H. Peter Anvin
2009-05-04 17:53         ` Borislav Petkov
2009-05-04 20:51           ` Mauro Carvalho Chehab
2009-04-29 16:54 ` [PATCH 02/21] amd64_edac: add PCI config register defines Borislav Petkov
2009-05-04 20:54   ` Mauro Carvalho Chehab
2009-04-29 16:54 ` [PATCH 03/21] amd64_edac: add driver structs Borislav Petkov
2009-05-04 20:38   ` Mauro Carvalho Chehab
2009-04-29 16:54 ` [PATCH 04/21] amd64_edac: add memory scrubber interface Borislav Petkov
2009-05-04 21:02   ` Mauro Carvalho Chehab
2009-04-29 16:54 ` [PATCH 05/21] amd64_edac: add sys addr to memory controller mapping helpers Borislav Petkov
2009-05-04 21:08   ` Mauro Carvalho Chehab
2009-04-29 16:54 ` [PATCH 06/21] amd64_edac: add functionality to compute the DRAM hole Borislav Petkov
2009-05-04 21:22   ` Mauro Carvalho Chehab
2009-04-29 16:54 ` [PATCH 07/21] amd64_edac: add DRAM address type conversion facilities Borislav Petkov
2009-05-04 21:39   ` Mauro Carvalho Chehab
2009-04-29 16:54 ` [PATCH 08/21] amd64_edac: add helper to dump relevant registers Borislav Petkov
2009-05-04 21:43   ` Mauro Carvalho Chehab
2009-04-29 16:54 ` [PATCH 09/21] amd64_edac: assign DRAM chip select base and mask in a family-specific way Borislav Petkov
2009-05-04 21:59   ` Mauro Carvalho Chehab
2009-05-05 10:25     ` Borislav Petkov
2009-04-29 16:54 ` [PATCH 10/21] amd64_edac: add k8-specific methods Borislav Petkov
2009-05-04 22:06   ` Mauro Carvalho Chehab
2009-04-29 16:54 ` [PATCH 11/21] amd64_edac: add f10-and-later methods-p1 Borislav Petkov
2009-05-04 22:10   ` Mauro Carvalho Chehab
2009-04-29 16:54 ` [PATCH 12/21] amd64_edac: add f10-and-later methods-p2 Borislav Petkov
2009-05-04 23:25   ` Mauro Carvalho Chehab
2009-04-29 16:54 ` [PATCH 13/21] amd64_edac: add f10-and-later methods-p3 Borislav Petkov
2009-04-29 18:22   ` Ingo Molnar
2009-04-29 18:24     ` Ingo Molnar
2009-04-29 19:05     ` Andrew Morton
2009-04-29 19:23       ` Ingo Molnar
2009-04-29 19:42         ` Andrew Morton
2009-04-29 19:53           ` Ingo Molnar
2009-04-29 20:47             ` Ingo Molnar
2009-04-30 10:01               ` Borislav Petkov
2009-04-30 10:42                 ` Ingo Molnar
2009-05-04 23:36   ` Mauro Carvalho Chehab
2009-04-29 16:55 ` [PATCH 14/21] amd64_edac: add per-family descriptors Borislav Petkov
2009-05-04 23:39   ` Mauro Carvalho Chehab
2009-04-29 16:55 ` [PATCH 15/21] amd64_edac: add ECC chipkill syndrome mapping table Borislav Petkov
2009-05-04 23:42   ` Mauro Carvalho Chehab
2009-04-29 16:55 ` [PATCH 16/21] amd64_edac: add error decoding logic Borislav Petkov
2009-04-29 18:19   ` Ingo Molnar
2009-05-04 23:48   ` Mauro Carvalho Chehab
2009-04-29 16:55 ` [PATCH 17/21] amd64_edac: add EDAC core-related initializers Borislav Petkov
2009-05-04 23:53   ` Mauro Carvalho Chehab
2009-04-29 16:55 ` [PATCH 18/21] amd64_edac: add ECC reporting initializers Borislav Petkov
2009-05-04 23:59   ` Mauro Carvalho Chehab
2009-04-29 16:55 ` [PATCH 19/21] amd64_edac: add debugging/testing code Borislav Petkov
2009-04-29 18:18   ` Ingo Molnar
2009-04-29 16:55 ` [PATCH 20/21] amd64_edac: add DRAM error injection logic using sysfs Borislav Petkov
2009-04-29 18:17   ` Ingo Molnar
2009-05-05  0:06   ` Mauro Carvalho Chehab
2009-04-29 16:55 ` [PATCH 21/21] amd64_edac: add module registration routines Borislav Petkov
2009-05-05  0:10   ` Mauro Carvalho Chehab
2009-04-29 19:30 ` [RFC PATCH 00/21 v2] amd64_edac: EDAC module for AMD64 Andi Kleen
2009-04-30 11:57   ` Borislav Petkov
2009-04-30 12:21     ` Ingo Molnar
2009-04-30 12:47     ` Andi Kleen
2009-04-30 14:48       ` Aristeu Rozanski
2009-05-01  7:53         ` Borislav Petkov
2009-05-03  0:32           ` Aristeu Rozanski
2009-04-30 18:37       ` Mauro Carvalho Chehab
2009-05-01 12:39       ` Ingo Molnar
  -- strict thread matches above, loose matches on Subject: below --
2009-04-30  6:21 [PATCH 13/21] amd64_edac: add f10-and-later methods-p3 Doug Thompson
2009-05-05 19:30 Doug Thompson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox