[coreboot-gerrit] Change in coreboot[master]: cpu/x86/smm: SMM module loader version 2

[Rocky Phagura (Code Review)]

Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )

Change subject: cpu/x86/smm: SMM module loader version 2
......................................................................

Patch Set 3:
> Patch Set 3: Code-Review+1
> > Patch Set 2:
> > > Patch Set 2:
> > > > Patch Set 2:
> > > > > Patch Set 2:
> > > > >
> > > > > I tested this on Lenovo X230 (CPU: Intel(R) Core(TM) i5-3320M CPU @ 2.60GHz) and Asrock B85M Pro4 (CPU: Intel(R) Core(TM) i7-4770S CPU @ 3.10GHz): both locked up inside `src/cpu/x86/smm/smi_trigger.c` function `apm_control()`.
> > > > >
> > > > > Log of the Asrock B85M Pro4: https://paste.flashrom.org/view.php?id=3349
> > > >
> > > > Angel, can you please set MAX_CPUS to a 8 or more in Kconfig?  In server mp_init code we error out if MAX_CPUS is less than what's found in the system, but that's not happening here and hence we are getting this error in the log:
> > > > smm_create_map: more CPUs than originally configured for
> > >
> > > Weird... I have MAX_CPUS=8 when building for the Asrock B85M Pro4 and the i7-4770S is a quad-core with HyperThreading, so it has 8 threads... Should be enough. I wonder where `cpus in one segment 30` comes from.
> >
> > Angel,
> > I think I found the issue. I've uploaded a new patch.  The changes are in smm_loaderv2.c only. It provides more debug information and solves the issue with error you were seeing related to:
> > smm_create_map: more CPUs than originally configured for. I tested this on my system by using various CPU threads and its working.  Please try with these changes. Thank you for your help!
>
> I tried again, I ended up with the same problem. I think I found out why, though.
>
> In `src/cpu/x86/mp_init.c`, function `smm_do_relocation`, there's a calculation that needs to match that of the SMM module loader. In my case, it didn't, and that's why I was getting hangs whenever SMM was called. Adding this offset worked for me on Haswell (but it's not a good idea to hardcode it):
>
> perm_smbase += (0x7faea000 - 0x7f800000);
>
> The first value is the location of the relocated SMBASE for CPU #0. The second value is smram_start.

Angel,
I completely understand the problem you are having. Yes, you are right.  In mp_init.c, there is some code that is needed.  Here is what I have in my mp_init.c, it was in my original patch but not in loader version 2, hence why are you seeing the problems. The code below needs to go in smm_do_relocation.

/*
     * The permanent handler runs with all cpus concurrently. Get the location
     * of the SMBASE for this CPU since the loader already created a map of all
     * CPU threads and where each entry point will reside in the SMRAM region
     */
    perm_smbase = smm_get_cpu_smbase(cpu);
    mp_state.perm_smbase = perm_smbase;
    if (perm_smbase <= 0) {
    	printk(BIOS_ERR, "%s : error, smbase 0x%x not found for this cpu 0x%x\n",
    	__func__, (int)perm_smbase, cpu);
    	return;
    }
    mp_state.ops.relocation_handler(cpu, curr_smbase, perm_smbase);

So I will add this snippet into mp_init.c and update this patch if you agree.