linux-kernel - Re: [PATCH v2 1/2] mce: acpi/apei: Honour Firmware First for MCA banks listed in APEI HEST CMC

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]

Date:	Fri, 21 Jun 2013 01:44:00 +0530
From:	"Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
To:	Borislav Petkov <bp@...en8.de>
CC:	tony.luck@...el.com, ananth@...ibm.com, masbock@...ux.vnet.ibm.com,
	lcm@...ux.vnet.ibm.com, linux-kernel@...r.kernel.org,
	linux-acpi@...r.kernel.org, ying.huang@...el.com
Subject: Re: [PATCH v2 1/2] mce: acpi/apei: Honour Firmware First for MCA
 banks listed in APEI HEST CMC

On 06/21/2013 12:59 AM, Borislav Petkov wrote:
> On Fri, Jun 21, 2013 at 12:38:13AM +0530, Naveen N. Rao wrote:
>> We need this bitfield to prevent enabling CMCI in future
>> cmci_discover() invocations. See usage in cmci_discover() further
>> below.
>
> So?!
>
> 	/* Skip banks in firmware first mode */
> 	if (!test_bit(i, __get_cpu_var(mce_poll_banks))
> 		continue;

This won't work across cpu offline/online, right? We will end up _not_ 
enabling CMCI on certain banks where we should have.

>
> ...
>
>>> Yeah, let's call it ...disable_poll_bank because we're disabling polling
>>> for those banks. And yes, we poll for errors for which no MCE exception
>>> is generated and those happen to be corrected but still...
>>
>> We actually also disable CMCI here. So, in essence, we are disabling
>> these banks for all sorts of direct corrected error reporting. I
>> thought of naming this disable_ce_on_bank() or disable_ce_bank(),
>> but felt that the mce_ prefix would be good to have. If that isn't
>> necessary, I can rename this to disable_ce_on_bank() which sounds
>> more accurate to me. Is that ok?
>
> No, mce_disable_bank() removes the respective bank from the polling
> bitfield and cmci_disable_bank() actually disables CMCI which is
> Intel-only. So leave it at mce_disable_bank and that should be fine.

Ok. I will rename this to mce_disable_bank() from mce_disable_ce_bank().

Another thing: for hest_parse_cmc(), does the below look good?

         cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
         if (!cmc->enabled)
                 return 0;

#define ACPI_HEST_PARSING_DONE 1
         /*
          * We expect HEST to provide a list of MC banks that
          * report errors in firmware first mode.
          */
         if (!(cmc->flags & ACPI_HEST_FIRMWARE_FIRST) || 
!cmc->num_hardware_banks)
                 return ACPI_HEST_PARSING_DONE;

The return value doesn't really matter since we don't check it, but 
returning an error looked like the wrong thing to do as well.


Thanks,
Naveen

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