Re: [PATCH 09/21] amd64_edac: assign DRAM chip select base and mask in a family-specific way

Re: [PATCH 09/21] amd64_edac: assign DRAM chip select base and mask in a family-specific way

[Doug Thompson]

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

> From: Mauro Carvalho Chehab <mchehab@redhat.com>
> Subject: Re: [PATCH 09/21] amd64_edac: assign DRAM chip select base and mask in a family-specific way
> To: "Doug Thompson" <norsk5@yahoo.com>
> Cc: "Borislav Petkov" <borislav.petkov@amd.com>, 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: Tuesday, May 5, 2009, 3:39 PM
> Em Tue, 5 May 2009 12:28:01 -0700
> (PDT)
> Doug Thompson <norsk5@yahoo.com>
> escreveu:

> > BACKGROUND: The error checking of the pci config space
> read was added during the time when the kernel couldn't read
> the extended 4k config space via the AMD IOConfig port
> access function (12 bits of offset) not via MMCONFIG. 
> 
> As this is unlikely to happen, I would also replace the:
>     if (err != 0)
> 
> by:
>     if (unlikely(err))
> 
> in order to optimize the tests a little bit, avoiding the
> risk of cache flushes due to the branches.
> 
> Cheers,
> Mauro
> 

when the driver is configured without DEBUG config flag on, the statement becomes:

    if (exp)
         ;

which with a good optimizer the code disappears altogether in the production condition. Only with DEBUGGING on will these outputs be generated.

thanks for the extra review eyes

doug t

Re: [PATCH 09/21] amd64_edac: assign DRAM chip select base and mask in a family-specific way

[Doug Thompson]

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

> From: Mauro Carvalho Chehab <mchehab@redhat.com>
> Subject: Re: [PATCH 09/21] amd64_edac: assign DRAM chip select base and mask in a family-specific way
> 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, 3:59 PM
> Borislav Petkov escreveu:
> > +    for (cs = 0; cs <
> CHIPSELECT_COUNT; cs++) {
> > +        reg = K8_DCSB0
> + (cs * 4);
> > +        err =
> pci_read_config_dword(pvt->dram_f2_ctl, reg,
> > +       
>            
>     &pvt->dcsb0[cs]);
> > +        if (err != 0)
> > +       
>     debugf0("%s() Reading K8_DCSB0[%d]
> failed\n",
> > +       
>         __func__, cs);
> > +
> > +        debugf0(" 
> DCSB0[%d]=0x%08x reg: F2x%x\n",
> > +       
>     cs, pvt->dcsb0[cs], reg);
> >   
> 
> Hmm... I suspect that there's a missing else before the
> debugf0(). If you got an error while reading it, you
> shouldn't be showing the results. 
> > +
> > +        /* If DCT are
> NOT ganged, then read in DCT1's base */
> > +        if
> (boot_cpu_data.x86 >= 0x10 &&
> !dct_ganging_enabled(pvt)) {
> > +       
>     reg = F10_DCSB1 + (cs * 4);
> > +       
>     err =
> pci_read_config_dword(pvt->dram_f2_ctl, reg,
> > +       
>            
>        
> &pvt->dcsb1[cs]);
> > +       
>     if (err != 0)
> > +       
>         debugf0("%s() Reading
> F10_DCSB1[%d] failed\n",
> > +       
>            
> __func__, cs);
> > +       
>     debugf0("  DCSB1[%d]=0x%08x reg:
> F2x%x\n",
> > +       
>         cs, pvt->dcsb1[cs],
> reg);
> >   
> The same issue here: if you got an error while reading it,
> you shouldn't be showing the results. 
> Cheers,
> Mauro.
> 

An 'else' could be inserted, but it is only under DEBUG state for development purposes.  

BACKGROUND: The error checking of the pci config space read was added during the time when the kernel couldn't read the extended 4k config space via the AMD IOConfig port access function (12 bits of offset) not via MMCONFIG. 

doug t

Re: [PATCH 09/21] amd64_edac: assign DRAM chip select base and mask in a family-specific way

[Mauro Carvalho Chehab]

Em Tue, 5 May 2009 12:28:01 -0700 (PDT)
Doug Thompson <norsk5@yahoo.com> escreveu:

> 
> --- On Mon, 5/4/09, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> 
> > From: Mauro Carvalho Chehab <mchehab@redhat.com>
> > Subject: Re: [PATCH 09/21] amd64_edac: assign DRAM chip select base and mask in a family-specific way
> > 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, 3:59 PM
> > Borislav Petkov escreveu:
> > > +    for (cs = 0; cs <
> > CHIPSELECT_COUNT; cs++) {
> > > +        reg = K8_DCSB0
> > + (cs * 4);
> > > +        err =
> > pci_read_config_dword(pvt->dram_f2_ctl, reg,
> > > +       
> >            
> >     &pvt->dcsb0[cs]);
> > > +        if (err != 0)
> > > +       
> >     debugf0("%s() Reading K8_DCSB0[%d]
> > failed\n",
> > > +       
> >         __func__, cs);
> > > +
> > > +        debugf0(" 
> > DCSB0[%d]=0x%08x reg: F2x%x\n",
> > > +       
> >     cs, pvt->dcsb0[cs], reg);
> > >   
> > 
> > Hmm... I suspect that there's a missing else before the
> > debugf0(). If you got an error while reading it, you
> > shouldn't be showing the results. 
> > > +
> > > +        /* If DCT are
> > NOT ganged, then read in DCT1's base */
> > > +        if
> > (boot_cpu_data.x86 >= 0x10 &&
> > !dct_ganging_enabled(pvt)) {
> > > +       
> >     reg = F10_DCSB1 + (cs * 4);
> > > +       
> >     err =
> > pci_read_config_dword(pvt->dram_f2_ctl, reg,
> > > +       
> >            
> >        
> > &pvt->dcsb1[cs]);
> > > +       
> >     if (err != 0)
> > > +       
> >         debugf0("%s() Reading
> > F10_DCSB1[%d] failed\n",
> > > +       
> >            
> > __func__, cs);
> > > +       
> >     debugf0("  DCSB1[%d]=0x%08x reg:
> > F2x%x\n",
> > > +       
> >         cs, pvt->dcsb1[cs],
> > reg);
> > >   
> > The same issue here: if you got an error while reading it,
> > you shouldn't be showing the results. 
> > Cheers,
> > Mauro.
> > 
> 
> An 'else' could be inserted, but it is only under DEBUG state for development purposes.  

Yet, it seems worthy to have it.
> 
> BACKGROUND: The error checking of the pci config space read was added during the time when the kernel couldn't read the extended 4k config space via the AMD IOConfig port access function (12 bits of offset) not via MMCONFIG. 

As this is unlikely to happen, I would also replace the:
	if (err != 0)

by:
	if (unlikely(err))

in order to optimize the tests a little bit, avoiding the risk of cache flushes due to the branches.

Cheers,
Mauro

[RFC PATCH 00/21 v2] amd64_edac: EDAC module for AMD64

[Borislav Petkov]

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(-)

[PATCH 09/21] amd64_edac: assign DRAM chip select base and mask in a family-specific way

[Borislav Petkov]

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 |  153 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 153 insertions(+), 0 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 4e84ccf..937e1f5 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1907,4 +1907,157 @@ static void amd64_read_dbam_reg(struct amd64_pvt *pvt)
 	}
 }
 
+/*
+ * amd64_set_dct_base_and_mask(pvt)
+ *
+ *	NOTE: CPU Revision Dependent code: Rev E and Rev F
+ *
+ *	Set the DCSB and DCSM mask values depending on the CPU revision value.
+ *	Also set the shift factor for the DCSB and DCSM values.
+ *
+ *	->dcs_mask_notused, REV E:
+ *
+ *	To find the max InputAddr for the csrow, start with the base
+ *	address and set all bits that are "don't care" bits in the test at
+ *	the start of section 3.5.4 (p. 84).
+ *
+ *	The "don't care" bits are all set bits in the mask and
+ *	all bits in the gaps between bit ranges [35-25] and [19-13].
+ *	The value REV_E_DCS_NOTUSED_BITS represents bits [24-20] and [12-0],
+ *	which are all bits in the above-mentioned gaps.
+ *
+ *	->dcs_mask_notused, REV F and later:
+ *
+ *	To find the max InputAddr for the csrow, start with the base
+ *	address and set all bits that are "don't care" bits in the test at
+ *	the start of NPT section 4.5.4 (p. 87).
+ *
+ *	The "don't care" bits are all set bits in the mask and
+ *	all bits in the gaps between bit ranges [36-27] and [21-13].
+ *
+ *	The value REV_F_F1Xh_DCS_NOTUSED_BITS represents bits [26-22] and
+ *	[12-0], which are all bits in the above-mentioned gaps.
+ */
+static void amd64_set_dct_base_and_mask(struct amd64_pvt *pvt)
+{
+	if (pvt->ext_model >= OPTERON_CPU_REV_F) {
+		pvt->dcsb_base        = REV_F_F1Xh_DCSB_BASE_BITS;
+		pvt->dcsm_mask        = REV_F_F1Xh_DCSM_MASK_BITS;
+		pvt->dcs_mask_notused    = REV_F_F1Xh_DCS_NOTUSED_BITS;
+		pvt->dcs_shift        = REV_F_F1Xh_DCS_SHIFT;
+
+		switch (boot_cpu_data.x86) {
+		case 0xf:
+			pvt->num_dcsm = REV_F_DCSM_COUNT;
+			break;
+
+		case 0x10:
+			pvt->num_dcsm = F10_DCSM_COUNT;
+			break;
+
+		case 0x11:
+			pvt->num_dcsm = F11_DCSM_COUNT;
+			break;
+
+		default:
+			amd64_printk(KERN_ERR, "Unsupported family!\n");
+			break;
+		}
+	} else {
+		pvt->dcsb_base        = REV_E_DCSB_BASE_BITS;
+		pvt->dcsm_mask        = REV_E_DCSM_MASK_BITS;
+		pvt->dcs_mask_notused    = REV_E_DCS_NOTUSED_BITS;
+		pvt->dcs_shift        = REV_E_DCS_SHIFT;
+		pvt->num_dcsm        = REV_E_DCSM_COUNT;
+	}
+}
+
+/*
+ * amd64_read_dct_base_mask
+ *
+ *	Function 2 Offset F10_DCSB0
+ *	Read in the DCS Base and DCS Mask hw registers
+ */
+static void amd64_read_dct_base_mask(struct amd64_pvt *pvt)
+{
+	int cs;
+	int err;
+	int reg;
+
+	debugf0("%s()\n", __func__);
+
+	amd64_set_dct_base_and_mask(pvt);
+
+	for (cs = 0; cs < CHIPSELECT_COUNT; cs++) {
+		reg = K8_DCSB0 + (cs * 4);
+		err = pci_read_config_dword(pvt->dram_f2_ctl, reg,
+						&pvt->dcsb0[cs]);
+		if (err != 0)
+			debugf0("%s() Reading K8_DCSB0[%d] failed\n",
+				__func__, cs);
+
+		debugf0("  DCSB0[%d]=0x%08x reg: F2x%x\n",
+			cs, pvt->dcsb0[cs], reg);
+
+		/* If DCT are NOT ganged, then read in DCT1's base */
+		if (boot_cpu_data.x86 >= 0x10 && !dct_ganging_enabled(pvt)) {
+			reg = F10_DCSB1 + (cs * 4);
+			err = pci_read_config_dword(pvt->dram_f2_ctl, reg,
+							&pvt->dcsb1[cs]);
+			if (err != 0)
+				debugf0("%s() Reading F10_DCSB1[%d] failed\n",
+					__func__, cs);
+			debugf0("  DCSB1[%d]=0x%08x reg: F2x%x\n",
+				cs, pvt->dcsb1[cs], reg);
+		} else {
+			pvt->dcsb1[cs] = 0;
+		}
+	}
+
+	for (cs = 0; cs < pvt->num_dcsm; cs++) {
+		reg = K8_DCSB0 + (cs * 4);
+		err = pci_read_config_dword(pvt->dram_f2_ctl, reg,
+					&pvt->dcsm0[cs]);
+		if (err != 0)
+			debugf0("%s() Reading K8_DCSM0 failed\n", __func__);
+		else
+			debugf0("    DCSM0[%d]=0x%08x reg: F2x%x\n",
+				cs, pvt->dcsm0[cs], reg);
+
+		/* If DCT are NOT ganged, then read in DCT1's mask */
+		if (boot_cpu_data.x86 >= 0x10 && !dct_ganging_enabled(pvt)) {
+			reg = F10_DCSM1 + (cs * 4);
+			err = pci_read_config_dword(pvt->dram_f2_ctl, reg,
+					&pvt->dcsm1[cs]);
+			if (err != 0)
+				debugf0("%s() Reading F10_DCSM1[%d] failed\n",
+					__func__, cs);
+			else
+				debugf0("    DCSM1[%d]=0x%08x reg: F2x%x\n",
+					cs, pvt->dcsm1[cs], reg);
+		} else
+			pvt->dcsm1[cs] = 0;
+	}
+}
+
+static enum mem_type amd64_determine_memory_type(struct amd64_pvt *pvt)
+{
+	enum mem_type type;
+
+	if (boot_cpu_data.x86 >= 0x10 || pvt->ext_model >= OPTERON_CPU_REV_F) {
+		/* Rev F and later */
+		type = (pvt->dclr0 & BIT(16)) ? MEM_DDR2 : MEM_RDDR2;
+	} else {
+		/* Rev E and earlier */
+		type = (pvt->dclr0 & BIT(18)) ? MEM_DDR : MEM_RDDR;
+	}
+
+	debugf1("  Memory type is: %s\n",
+		(type == MEM_DDR2) ? "MEM_DDR2" :
+		(type == MEM_RDDR2) ? "MEM_RDDR2" :
+		(type == MEM_DDR) ? "MEM_DDR" : "MEM_RDDR");
+
+	return type;
+}
+
 
-- 
1.6.2.4

Re: [PATCH 09/21] amd64_edac: assign DRAM chip select base and mask in a family-specific way

[Mauro Carvalho Chehab]

Borislav Petkov escreveu:
> +	for (cs = 0; cs < CHIPSELECT_COUNT; cs++) {
> +		reg = K8_DCSB0 + (cs * 4);
> +		err = pci_read_config_dword(pvt->dram_f2_ctl, reg,
> +						&pvt->dcsb0[cs]);
> +		if (err != 0)
> +			debugf0("%s() Reading K8_DCSB0[%d] failed\n",
> +				__func__, cs);
> +
> +		debugf0("  DCSB0[%d]=0x%08x reg: F2x%x\n",
> +			cs, pvt->dcsb0[cs], reg);
>   

Hmm... I suspect that there's a missing else before the debugf0(). If 
you got an error while reading it, you shouldn't be showing the results. 
> +
> +		/* If DCT are NOT ganged, then read in DCT1's base */
> +		if (boot_cpu_data.x86 >= 0x10 && !dct_ganging_enabled(pvt)) {
> +			reg = F10_DCSB1 + (cs * 4);
> +			err = pci_read_config_dword(pvt->dram_f2_ctl, reg,
> +							&pvt->dcsb1[cs]);
> +			if (err != 0)
> +				debugf0("%s() Reading F10_DCSB1[%d] failed\n",
> +					__func__, cs);
> +			debugf0("  DCSB1[%d]=0x%08x reg: F2x%x\n",
> +				cs, pvt->dcsb1[cs], reg);
>   
The same issue here: if you got an error while reading it, you shouldn't 
be showing the results. 

Cheers,
Mauro.

Re: [PATCH 09/21] amd64_edac: assign DRAM chip select base and mask in a family-specific way

[Borislav Petkov]

On Mon, May 04, 2009 at 06:59:14PM -0300, Mauro Carvalho Chehab wrote:
> Borislav Petkov escreveu:
>> +	for (cs = 0; cs < CHIPSELECT_COUNT; cs++) {
>> +		reg = K8_DCSB0 + (cs * 4);
>> +		err = pci_read_config_dword(pvt->dram_f2_ctl, reg,
>> +						&pvt->dcsb0[cs]);
>> +		if (err != 0)
>> +			debugf0("%s() Reading K8_DCSB0[%d] failed\n",
>> +				__func__, cs);
>> +
>> +		debugf0("  DCSB0[%d]=0x%08x reg: F2x%x\n",
>> +			cs, pvt->dcsb0[cs], reg);
>>   
>
> Hmm... I suspect that there's a missing else before the debugf0(). If  
> you got an error while reading it, you shouldn't be showing the results. 
>> +
>> +		/* If DCT are NOT ganged, then read in DCT1's base */
>> +		if (boot_cpu_data.x86 >= 0x10 && !dct_ganging_enabled(pvt)) {
>> +			reg = F10_DCSB1 + (cs * 4);
>> +			err = pci_read_config_dword(pvt->dram_f2_ctl, reg,
>> +							&pvt->dcsb1[cs]);
>> +			if (err != 0)
>> +				debugf0("%s() Reading F10_DCSB1[%d] failed\n",
>> +					__func__, cs);
>> +			debugf0("  DCSB1[%d]=0x%08x reg: F2x%x\n",
>> +				cs, pvt->dcsb1[cs], reg);
>>   
> The same issue here: if you got an error while reading it, you shouldn't  
> be showing the results. 

correct, thanks.

-- 
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632