[PATCH] EDAC, ghes: Fix grain calculation

[PATCH] EDAC, ghes: Fix grain calculation

[James Morse]

ghes_edac_report_mem_error() attempts to calculate the 'grain' or
granule affected by the error from the firmware-provided 'physical address
mask'. This mask tells us which bits of the physical address are valid.

The current calculation:
| e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
will always cause the top bits to be set as they are cleared by &,
then set again by ~. For a hypervisor reporting its page-size as the
region affected by the error:
| {1}[Hardware Error]:   physical_address: 0x00000000deadbeef
| {1}[Hardware Error]:   physical_address_mask: 0xfffffffffffff000
| {1}[Hardware Error]:   error_type: 6, master abort
| EDAC MC0: 1 CE Master abort on unknown label ( page:0xdead offset:0xbeef
| grain:-61441 syndrome:0x0 - status(0x0000000000000001): reserved)

Here the grain has been miscalculated as the hypervisor reported a 4K
size granule, due to its page size, whereas the guest kernel uses 64K.
This gives us e->grain of 0xffffffffffff0fff

Fix this, calculating grain_bits directly from ~physical_address_mask,
and setting e->grain from that. In the same example we now get:
| EDAC MC0: 1 CE Master abort on unknown label ( page:0xdead offset:0xbeef
| grain:4096 syndrome:0x0 - status(0x0000000000000001): reserved)

Cc: Robert Richter <rrichter@marvell.com>
Cc: Toshi Kani <toshi.kani@hpe.com>
Signed-off-by: James Morse <james.morse@arm.com>
---
This has always been broken, so I suspect no-one cares about this, it was
added by:
Fixes: f04c62a7036a ("ghes_edac: add support for reporting errors via EDAC")

I've only tested this with firmware I've written.

 drivers/edac/ghes_edac.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 49396bf6ad88..fac96ff45b7e 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -202,8 +202,8 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
 	struct mem_ctl_info *mci;
 	struct ghes_edac_pvt *pvt = ghes_pvt;
 	unsigned long flags;
+	u8 grain_bits = 0;
 	char *p;
-	u8 grain_bits;
 
 	if (!pvt)
 		return;
@@ -318,8 +318,10 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
 	}
 
 	/* Error grain */
-	if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK)
-		e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
+	if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK) {
+		grain_bits = fls_long(~mem_err->physical_addr_mask);
+		e->grain = 1UL<<grain_bits;
+	}
 
 	/* Memory error location, mapped on e->location */
 	p = e->location;
@@ -436,7 +438,6 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
 		*(p - 1) = '\0';
 
 	/* Generate the trace event */
-	grain_bits = fls_long(e->grain);
 	snprintf(pvt->detail_location, sizeof(pvt->detail_location),
 		 "APEI location: %s %s", e->location, e->other_detail);
 	trace_mc_event(type, e->msg, e->label, e->error_count,
-- 
2.20.1

Re: [PATCH] EDAC, ghes: Fix grain calculation

[Kani, Toshi]

On Wed, 2019-05-29 at 16:22 +0100, James Morse wrote:
> ghes_edac_report_mem_error() attempts to calculate the 'grain' or
> granule affected by the error from the firmware-provided 'physical address
> mask'. This mask tells us which bits of the physical address are valid.
> 
> The current calculation:
> > e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
> 
> will always cause the top bits to be set as they are cleared by &,
> then set again by ~. For a hypervisor reporting its page-size as the
> region affected by the error:
> > {1}[Hardware Error]:   physical_address: 0x00000000deadbeef
> > {1}[Hardware Error]:   physical_address_mask: 0xfffffffffffff000
> > {1}[Hardware Error]:   error_type: 6, master abort
> > EDAC MC0: 1 CE Master abort on unknown label ( page:0xdead offset:0xbeef
> > grain:-61441 syndrome:0x0 - status(0x0000000000000001): reserved)
> 
> Here the grain has been miscalculated as the hypervisor reported a 4K
> size granule, due to its page size, whereas the guest kernel uses 64K.
> This gives us e->grain of 0xffffffffffff0fff
> 
> Fix this, calculating grain_bits directly from ~physical_address_mask,
> and setting e->grain from that. In the same example we now get:
> > EDAC MC0: 1 CE Master abort on unknown label ( page:0xdead offset:0xbeef
> > grain:4096 syndrome:0x0 - status(0x0000000000000001): reserved)
> 
> Cc: Robert Richter <rrichter@marvell.com>
> Cc: Toshi Kani <toshi.kani@hpe.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> This has always been broken, so I suspect no-one cares about this, it was
> added by:
> Fixes: f04c62a7036a ("ghes_edac: add support for reporting errors via EDAC")
> 
> I've only tested this with firmware I've written.

Thanks for the fix.  Tested on my x86 system.

Tested-by: Toshi Kani <toshi.kani@hpe.com>

Thanks,
-Toshi

Re: [PATCH] EDAC, ghes: Fix grain calculation

[Borislav Petkov]

On Wed, May 29, 2019 at 04:22:32PM +0100, James Morse wrote:
> ghes_edac_report_mem_error() attempts to calculate the 'grain' or
> granule affected by the error from the firmware-provided 'physical address
> mask'. This mask tells us which bits of the physical address are valid.
> 
> The current calculation:
> | e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
> will always cause the top bits to be set as they are cleared by &,
> then set again by ~. For a hypervisor reporting its page-size as the
> region affected by the error:
> | {1}[Hardware Error]:   physical_address: 0x00000000deadbeef
> | {1}[Hardware Error]:   physical_address_mask: 0xfffffffffffff000
> | {1}[Hardware Error]:   error_type: 6, master abort
> | EDAC MC0: 1 CE Master abort on unknown label ( page:0xdead offset:0xbeef
> | grain:-61441 syndrome:0x0 - status(0x0000000000000001): reserved)
> 
> Here the grain has been miscalculated as the hypervisor reported a 4K
> size granule, due to its page size, whereas the guest kernel uses 64K.
> This gives us e->grain of 0xffffffffffff0fff
> 
> Fix this, calculating grain_bits directly from ~physical_address_mask,
> and setting e->grain from that. In the same example we now get:
> | EDAC MC0: 1 CE Master abort on unknown label ( page:0xdead offset:0xbeef
> | grain:4096 syndrome:0x0 - status(0x0000000000000001): reserved)
> 
> Cc: Robert Richter <rrichter@marvell.com>
> Cc: Toshi Kani <toshi.kani@hpe.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> This has always been broken, so I suspect no-one cares about this, it was
> added by:
> Fixes: f04c62a7036a ("ghes_edac: add support for reporting errors via EDAC")
> 
> I've only tested this with firmware I've written.
> 
>  drivers/edac/ghes_edac.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index 49396bf6ad88..fac96ff45b7e 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -202,8 +202,8 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
>  	struct mem_ctl_info *mci;
>  	struct ghes_edac_pvt *pvt = ghes_pvt;
>  	unsigned long flags;
> +	u8 grain_bits = 0;
>  	char *p;
> -	u8 grain_bits;
>  
>  	if (!pvt)
>  		return;
> @@ -318,8 +318,10 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
>  	}
>  
>  	/* Error grain */
> -	if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK)
> -		e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
> +	if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK) {
> +		grain_bits = fls_long(~mem_err->physical_addr_mask);
> +		e->grain = 1UL<<grain_bits;

Do we need to set that e->grain at all?

I mean, we set it now so that grain_bits can be computed but since
you're doing that directly...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] EDAC, ghes: Fix grain calculation

[James Morse]

Hi Boris,

On 12/06/2019 05:34, Borislav Petkov wrote:
> On Wed, May 29, 2019 at 04:22:32PM +0100, James Morse wrote:
>> ghes_edac_report_mem_error() attempts to calculate the 'grain' or
>> granule affected by the error from the firmware-provided 'physical address
>> mask'. This mask tells us which bits of the physical address are valid.
>>
>> The current calculation:
>> | e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
>> will always cause the top bits to be set as they are cleared by &,
>> then set again by ~. For a hypervisor reporting its page-size as the
>> region affected by the error:
>> | {1}[Hardware Error]:   physical_address: 0x00000000deadbeef
>> | {1}[Hardware Error]:   physical_address_mask: 0xfffffffffffff000
>> | {1}[Hardware Error]:   error_type: 6, master abort
>> | EDAC MC0: 1 CE Master abort on unknown label ( page:0xdead offset:0xbeef
>> | grain:-61441 syndrome:0x0 - status(0x0000000000000001): reserved)
>>
>> Here the grain has been miscalculated as the hypervisor reported a 4K
>> size granule, due to its page size, whereas the guest kernel uses 64K.
>> This gives us e->grain of 0xffffffffffff0fff
>>
>> Fix this, calculating grain_bits directly from ~physical_address_mask,
>> and setting e->grain from that. In the same example we now get:
>> | EDAC MC0: 1 CE Master abort on unknown label ( page:0xdead offset:0xbeef
>> | grain:4096 syndrome:0x0 - status(0x0000000000000001): reserved)


>> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
>> index 49396bf6ad88..fac96ff45b7e 100644
>> --- a/drivers/edac/ghes_edac.c
>> +++ b/drivers/edac/ghes_edac.c
>> @@ -202,8 +202,8 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
>>  	struct mem_ctl_info *mci;
>>  	struct ghes_edac_pvt *pvt = ghes_pvt;
>>  	unsigned long flags;
>> +	u8 grain_bits = 0;
>>  	char *p;
>> -	u8 grain_bits;
>>  
>>  	if (!pvt)
>>  		return;
>> @@ -318,8 +318,10 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
>>  	}
>>  
>>  	/* Error grain */
>> -	if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK)
>> -		e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
>> +	if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK) {
>> +		grain_bits = fls_long(~mem_err->physical_addr_mask);
>> +		e->grain = 1UL<<grain_bits;
> 
> Do we need to set that e->grain at all?

edac_raw_mc_handle_error() prints it out,


> I mean, we set it now so that grain_bits can be computed but since
> you're doing that directly...

We could replace edac_raw_mc_handle_error()'s use of e->grain with e->grain_bits, which
would also make Robert's life easier when trying to merge those paths, but that's a more
invasive change.

I did did it like this to make it the minimum change if it was going as a fix. If we're
confident that no-one has-noticed/cared this can be papered over as part of Robert's
series, otherwise it will conflict.


Thanks,

James

Re: [PATCH] EDAC, ghes: Fix grain calculation

[Borislav Petkov]

On Thu, Jun 13, 2019 at 06:06:22PM +0100, James Morse wrote:
> edac_raw_mc_handle_error() prints it out,

Of course it does, sorry for not seeing it. ;-\

> We could replace edac_raw_mc_handle_error()'s use of e->grain with e->grain_bits, which
> would also make Robert's life easier when trying to merge those paths, but that's a more
> invasive change.
> 
> I did did it like this to make it the minimum change if it was going as a fix. If we're
> confident that no-one has-noticed/cared this can be papered over as part of Robert's
> series, otherwise it will conflict.

Yeah, lemme take your patch as is and we can still improve on that whole
flow once the dust of all that cleanup settles.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] EDAC, ghes: Fix grain calculation

[Robert Richter]

On 13.06.19 21:18:43, Borislav Petkov wrote:
> On Thu, Jun 13, 2019 at 06:06:22PM +0100, James Morse wrote:
> > edac_raw_mc_handle_error() prints it out,
> 
> Of course it does, sorry for not seeing it. ;-\
> 
> > We could replace edac_raw_mc_handle_error()'s use of e->grain with e->grain_bits, which
> > would also make Robert's life easier when trying to merge those paths, but that's a more
> > invasive change.
> > 
> > I did did it like this to make it the minimum change if it was going as a fix. If we're
> > confident that no-one has-noticed/cared this can be papered over as part of Robert's
> > series, otherwise it will conflict.
> 
> Yeah, lemme take your patch as is and we can still improve on that whole
> flow once the dust of all that cleanup settles.

I don't think that makes sense as the code should be unified with
edac_mc. It diverges more as edac_mc calculates the grain_bits
different:

 grain_bits = fls_long(e->grain) + 1;

In ras_event.h the grain then is calculated different to edac_mc
again:

 1 << __entry->grain_bits

So, this patch looks correct but is contrary to edac_mc. I would like
to fix both and unify it.

-Robert

Re: [PATCH] EDAC, ghes: Fix grain calculation

[Borislav Petkov]

On Thu, Jun 13, 2019 at 09:07:38PM +0000, Robert Richter wrote:
>  grain_bits = fls_long(e->grain) + 1;

For 4K grain this makes grain_bits == 12.

> In ras_event.h the grain then is calculated different to edac_mc
> again:
> 
>  1 << __entry->grain_bits

and this makes it 4096.

Looks ok to me.

But if in James' example, physical_address_mask is 0xfffffffffffff000,
doing

  grain_bits = fls_long(~mem_err->physical_addr_mask);

would make it 11 because ~mem_err->physical_addr_mask (negated) should
be 0xfff.

Is that what you mean?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] EDAC, ghes: Fix grain calculation

[Robert Richter]

On 13.06.19 23:07:31, Robert Richter wrote:
> On 13.06.19 21:18:43, Borislav Petkov wrote:
> > On Thu, Jun 13, 2019 at 06:06:22PM +0100, James Morse wrote:
> > > edac_raw_mc_handle_error() prints it out,
> > 
> > Of course it does, sorry for not seeing it. ;-\
> > 
> > > We could replace edac_raw_mc_handle_error()'s use of e->grain with e->grain_bits, which
> > > would also make Robert's life easier when trying to merge those paths, but that's a more
> > > invasive change.
> > > 
> > > I did did it like this to make it the minimum change if it was going as a fix. If we're
> > > confident that no-one has-noticed/cared this can be papered over as part of Robert's
> > > series, otherwise it will conflict.
> > 
> > Yeah, lemme take your patch as is and we can still improve on that whole
> > flow once the dust of all that cleanup settles.
> 
> I don't think that makes sense as the code should be unified with
> edac_mc. It diverges more as edac_mc calculates the grain_bits
> different:
> 
>  grain_bits = fls_long(e->grain) + 1;
> 
> In ras_event.h the grain then is calculated different to edac_mc
> again:
> 
>  1 << __entry->grain_bits
> 
> So, this patch looks correct but is contrary to edac_mc. I would like
> to fix both and unify it.

See here:

 https://lore.kernel.org/patchwork/patch/1080289/#1284056

-Robert

Re: [PATCH] EDAC, ghes: Fix grain calculation

[Robert Richter]

On 14.06.19 00:41:30, Borislav Petkov wrote:
> On Thu, Jun 13, 2019 at 09:07:38PM +0000, Robert Richter wrote:
> >  grain_bits = fls_long(e->grain) + 1;
> 
> For 4K grain this makes grain_bits == 12.

fls(0) == 0
fls(1) == 1
fls(2) == 2
fls(4) == 3
...
fls(0x1000) == 13

grain_bits = fls_long(0x1000) + 1 = 14

> 
> > In ras_event.h the grain then is calculated different to edac_mc
> > again:
> > 
> >  1 << __entry->grain_bits
> 
> and this makes it 4096.

It then calculates to 16k.

> 
> Looks ok to me.
> 
> But if in James' example, physical_address_mask is 0xfffffffffffff000,
> doing
> 
>   grain_bits = fls_long(~mem_err->physical_addr_mask);
> 
> would make it 11 because ~mem_err->physical_addr_mask (negated) should
> be 0xfff.

If it is not power of 2 (e.g. 0xfff), with

 fls_long(e->grain) + 1;

from edac_mc there is still an off by one (so James' calculation for
ghes is correct).

Correct for edac_mc is:

 grain_bits = fls_long(e->grain ? e->grain - 1 : 0);

This works for both, power of 2 and less:

 grain_bits(0x1000) == 12;
 grain_bits(0x0fff) == 12;
 grain_bits(0x1001) == 13;
 grain_bits(0) == 0
 grain_bits(1) == 0
 grain_bits(2) == 1

James' calculation for ghes is correct in this particular case
(assuming a bit mask with (power of 2 - 1)).

But my formula is more generic.

-Robert

Re: [PATCH] EDAC, ghes: Fix grain calculation

[Robert Richter]

On 14.06.19 09:21:39, Robert Richter wrote:
> Correct for edac_mc is:
> 
>  grain_bits = fls_long(e->grain ? e->grain - 1 : 0);

I have this fix in mind + patch 11/21 (unify trace_mc_event() code) on
top which merges ghes_edac and edac_mc:

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 64922c8fa7e3..0f8d983dd35f 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -1236,7 +1236,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 		*(p - 1) = '\0';
 
 	/* Report the error via the trace interface */
-	grain_bits = fls_long(e->grain) + 1;
+	grain_bits = fls_long(e->grain ? e->grain - 1 : 0);
 
 	if (IS_ENABLED(CONFIG_RAS))
 		trace_mc_event(type, e->msg, e->label, e->error_count,
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 7f19f1c672c3..ee65ff43ab6a 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -317,7 +317,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
 
 	/* Error grain */
 	if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK)
-		e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
+		e->grain = ~mem_err->physical_addr_mask + 1;
 
 	/* Memory error location, mapped on e->location */
 	p = e->location;
@@ -434,7 +434,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
 		*(p - 1) = '\0';
 
 	/* Generate the trace event */
-	grain_bits = fls_long(e->grain);
+	grain_bits = fls_long(e->grain ? e->grain - 1 : 0);
 	snprintf(pvt->detail_location, sizeof(pvt->detail_location),
 		 "APEI location: %s %s", e->location, e->other_detail);
 	trace_mc_event(type, e->msg, e->label, e->error_count,


-Robert

Re: [PATCH] EDAC, ghes: Fix grain calculation

[Borislav Petkov]

On Fri, Jun 14, 2019 at 07:21:47AM +0000, Robert Richter wrote:
> On 14.06.19 00:41:30, Borislav Petkov wrote:
> > On Thu, Jun 13, 2019 at 09:07:38PM +0000, Robert Richter wrote:
> > >  grain_bits = fls_long(e->grain) + 1;
> > 
> > For 4K grain this makes grain_bits == 12.
> 
> fls(0) == 0
> fls(1) == 1
> fls(2) == 2
> fls(4) == 3
> ...
> fls(0x1000) == 13

I believe the intent of the "+ 1" above is that e->grain was a mask of
the type 0xfff so you have to + 1 since we count from bit 0.

> Correct for edac_mc is:
> 
>  grain_bits = fls_long(e->grain ? e->grain - 1 : 0);

First of all, you don't wanna do fls(0).

Then, we don't want to use PAGE_MASK for grain computation because as
James showed, it causes problems.

> James' calculation for ghes is correct in this particular case
> (assuming a bit mask with (power of 2 - 1)).

And this is the question that needs to be clarified first: from what do
we compute the grain.

ghes_edac uses ~physical_addr_mask which can basically be:

	grain_bits = hweight_long(physical_addr_mask);

which, in the 0xfff case gives you the correct 12 bits. It is assuming
it is a contiguous mask though. It better be...

Now, if you look at how the grain gets read out in edac_mc_handle_error(), it
comes from dimm->grain. And that is defined as:

        u32 grain;              /* granularity of reported error in bytes */

in bytes!

Now, if you grep for "grain" in drivers/edac/ you see most (if not all)
initialized as bytes:

i82975x_edac.c:415:                     dimm->grain = 1 << 7;   /* 128Byte cache-line resolution */
pnd2_edac.c:1260:               dimm->grain = 32;
skx_common.c:313:       dimm->grain = 32;
highbank_mc_edac.c:223: dimm->grain = 8;
...

and so on.

So actually I think that

	grain_bits = fls_long(e->grain) + 1;

is wrong in that case. It should simply hand down e->grain as it is
already the number of bytes.

Which we should simply dump in the tracepoint too - the number of bytes
and not that silly shifting there:

	1 << __entry->grain_bits,

And once we've said that the grain is going to *everywhere* be the
granularity of the error in number of bytes, then we should stick to
that and fix all those drivers which don't do that.

Ok?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] EDAC, ghes: Fix grain calculation

[Robert Richter]

On 14.06.19 12:09:18, Borislav Petkov wrote:
> On Fri, Jun 14, 2019 at 07:21:47AM +0000, Robert Richter wrote:
> > On 14.06.19 00:41:30, Borislav Petkov wrote:
> > > On Thu, Jun 13, 2019 at 09:07:38PM +0000, Robert Richter wrote:
> > > >  grain_bits = fls_long(e->grain) + 1;
> > > 
> > > For 4K grain this makes grain_bits == 12.
> > 
> > fls(0) == 0
> > fls(1) == 1
> > fls(2) == 2
> > fls(4) == 3
> > ...
> > fls(0x1000) == 13
> 
> I believe the intent of the "+ 1" above is that e->grain was a mask of
> the type 0xfff so you have to + 1 since we count from bit 0.

No, as you grepped below, the granularity is the number of bytes, and
mostly power of 2. Also note that fls(0xfff) is 12 and fls(0x1000) is
13 ...

> 
> > Correct for edac_mc is:
> > 
> >  grain_bits = fls_long(e->grain ? e->grain - 1 : 0);

... thus the -1 here.

> 
> First of all, you don't wanna do fls(0).

You need to deal with fls(0) as you don't know what drivers fill in
for the grain. fls(0) calculates fine here to 1 grain bit which is a
one byte granularity.

> 
> Then, we don't want to use PAGE_MASK for grain computation because as
> James showed, it causes problems.
> 
> > James' calculation for ghes is correct in this particular case
> > (assuming a bit mask with (power of 2 - 1)).
> 
> And this is the question that needs to be clarified first: from what do
> we compute the grain.
> 
> ghes_edac uses ~physical_addr_mask which can basically be:
> 
> 	grain_bits = hweight_long(physical_addr_mask);
> 
> which, in the 0xfff case gives you the correct 12 bits. It is assuming
> it is a contiguous mask though. It better be...
> 
> Now, if you look at how the grain gets read out in edac_mc_handle_error(), it
> comes from dimm->grain. And that is defined as:
> 
>         u32 grain;              /* granularity of reported error in bytes */
> 
> in bytes!
> 
> Now, if you grep for "grain" in drivers/edac/ you see most (if not all)
> initialized as bytes:
> 
> i82975x_edac.c:415:                     dimm->grain = 1 << 7;   /* 128Byte cache-line resolution */
> pnd2_edac.c:1260:               dimm->grain = 32;
> skx_common.c:313:       dimm->grain = 32;
> highbank_mc_edac.c:223: dimm->grain = 8;
> ...
> 
> and so on.
> 
> So actually I think that
> 
> 	grain_bits = fls_long(e->grain) + 1;
> 
> is wrong in that case.

This calculates correctly:

	grain_bits = fls_long(e->grain ? e->grain - 1 : 0);

and also handles the case if the number of bytes is not power of 2.

> It should simply hand down e->grain as it is
> already the number of bytes.
> Which we should simply dump in the tracepoint too - the number of bytes
> and not that silly shifting there:
> 
> 	1 << __entry->grain_bits,

The tracepoint format for mc_event changes from byte to long for the
grain then. This is not backward compatible and breaks userland.

> 
> And once we've said that the grain is going to *everywhere* be the
> granularity of the error in number of bytes, then we should stick to
> that and fix all those drivers which don't do that.
> 
> Ok?

See my other mail in this thread with a diff. I would go with that
one.

-Robert

Re: [PATCH] EDAC, ghes: Fix grain calculation

[Borislav Petkov]

On Fri, Jun 14, 2019 at 02:40:01PM +0000, Robert Richter wrote:
> No, as you grepped below, the granularity is the number of bytes, and
> mostly power of 2. Also note that fls(0xfff) is 12 and fls(0x1000) is
> 13 ...

Yeah, so?!

You want it to be 12 to have a 4K grain.

So this takes too long over mail - find me on IRC and we can figure it
out.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.