Re: [PATCH 2/2] apic, x86: Use BIOS settings for IBS and MCE threshold interrupt LVT offsets

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Wed, Oct 06, 2010 at 12:27:54PM +0200, Robert Richter wrote:
> We want the BIOS to setup the EILVT APIC registers. The offsets were
> hardcoded and BIOS settings were overwritten by the OS. Now, the
> subsystems for MCE threshold and IBS determine the LVT offset from the
> registers the BIOS has setup. If the BIOS setup is buggy on a family
> 10h system, a workaround enables IBS. If the OS determines an invalid
> register setup, a "[Firmware Bug]: " error message is reported.
> 
> We need this change also for upcomming cpu families.
> 
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---

Hi Robert, a few comments

...  
>  /*
>   * Program the next event, relative to now
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> index 5e97529..e13d4bd 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> @@ -131,7 +131,8 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
>  	u32 low = 0, high = 0, address = 0;
>  	unsigned int bank, block;
>  	struct thresh_restart tr;
> -	u8 lvt_off;
> +	int lvt_off = -1;
> +	u8 offset;
>  
>  	for (bank = 0; bank < NR_BANKS; ++bank) {
>  		for (block = 0; block < NR_BLOCKS; ++block) {
> @@ -165,8 +166,28 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
>  			if (shared_bank[bank] && c->cpu_core_id)
>  				break;
>  #endif
> -			lvt_off = setup_APIC_eilvt_mce(THRESHOLD_APIC_VECTOR,
> -						       APIC_EILVT_MSG_FIX, 0);
> +			offset = (high & MASK_LVTOFF_HI) >> 20;
> +			if (lvt_off < 0) {
> +				if (setup_APIC_eilvt(offset,
> +						     THRESHOLD_APIC_VECTOR,
> +						     APIC_EILVT_MSG_FIX, 0)) {
> +					pr_err(FW_BUG "cpu %d, failed to "
> +					       "setup threshold interrupt "
> +					       "for bank %d, block %d "
> +					       "(MSR%08X=0x%x%08x)",
> +					       smp_processor_id(), bank, block,
> +					       address, high, low);
> +					continue;
> +				}
> +				lvt_off = offset;
> +			} else if (lvt_off != offset) {

Could we put explicit type specificator here? For better readbility.
...

> +static int force_ibs_eilvt_setup(void)
> +{
> +	int i;
> +	int ret;
> +
> +	/* find the next free available EILVT entry */
> +	for (i = 1; i < 4; i++) {

APIC_EILVT_NR_MAX here, no?

> +		if (!eilvt_is_available(i))
> +			continue;
> +		ret = setup_ibs_ctl(i);
> +		if (ret)
> +			return ret;
> +		return 0;
>  	}
>  
 
  Cyrill