Change in coreboot[master]: soc/intel/xeon_sp: Allow ACPI SMI GNVS

[Marc Jones (Code Review)]

Marc Jones has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48492 )

Change subject: soc/intel/xeon_sp: Allow ACPI SMI GNVS
......................................................................

soc/intel/xeon_sp: Allow ACPI SMI GNVS

Now that SMM is enabled and GNVS is cleaned up, allow the GNVS SMI.

Change-Id: I5e0bd092ff3b76b63a4138eaf7ca7dfa57ad3e1e
Signed-off-by: Marc Jones 
---
M src/soc/intel/xeon_sp/Kconfig
1 file changed, 0 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/48492/1

diff --git a/src/soc/intel/xeon_sp/Kconfig b/src/soc/intel/xeon_sp/Kconfig
index 2028a5e..46e5e3d 100644
--- a/src/soc/intel/xeon_sp/Kconfig
+++ b/src/soc/intel/xeon_sp/Kconfig
@@ -36,7 +36,6 @@
    select POSTCAR_STAGE
    select IOAPIC
    select PARALLEL_MP
-	select ACPI_NO_SMI_GNVS
    select INTEL_DESCRIPTOR_MODE_CAPABLE
    select SOC_INTEL_COMMON_BLOCK
    select SOC_INTEL_COMMON_BLOCK_CPU

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48492 )

Change subject: soc/intel/xeon_sp: Allow ACPI SMI GNVS
......................................................................

Patch Set 2: Code-Review-1

This requires properly fetching SMM save states which is not implemented correctly with smmloader v2. Then again other things in that smihandler accessing the save state are also there and are also possibly broken...

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48492 )

Change subject: soc/intel/xeon_sp: Allow ACPI SMI GNVS
......................................................................

Patch Set 2: Code-Review-2
> Patch Set 2: Code-Review-1
>
> This requires properly fetching SMM save states which is not implemented correctly with smmloader v2. Then again other things in that smihandler accessing the save state are also there and are also possibly broken...

Thanks, I didn't realize that and I didn't have a test case yet. I'll abandon this.

[Marc Jones (Code Review)]

Marc Jones has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/48492 )

Change subject: soc/intel/xeon_sp: Allow ACPI SMI GNVS
......................................................................

Abandoned

[Rocky Phagura (Code Review)]

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

Change subject: soc/intel/xeon_sp: Allow ACPI SMI GNVS
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review-1
>
> This requires properly fetching SMM save states which is not implemented correctly with smmloader v2. Then again other things in that smihandler accessing the save state are also there and are also possibly broken...

smm loader v2 will work with state save.  The trick is to get the smbase using the MSR of the CPU executing the code (remember only 1 cpu thread is allowed to enter SMI handling code, while others wait).  Below is a snippet of code I'm using to modify memory in state save.

static uint64_t ras_get_smbase(void)
{
    msr_t msr = rdmsr(IA32_SMBASE);
    uint64_t smbase = 0;
    smbase = msr.hi;
    smbase = smbase << 32;
    smbase |= msr.lo;
    return smbase;
}
static void ras_post_cmci(void)
{
    em64t101_smm_state_save_area_t *save_state;
    uintptr_t base = ras_get_smbase();
    printk(BIOS_DEBUG, "%s, smbase = 0x%zx from MSR\n", __func__, base);
    save_state = (void *)(base + SMM_DEFAULT_SIZE - sizeof(*save_state));
    printk(BIOS_DEBUG, "%s, smbase & = %x\n", __func__, (uint32_t)&save_state->smbase);
    printk(BIOS_DEBUG, "%s, smbase = %x\n", __func__, save_state->smbase);
    printk(BIOS_DEBUG, "%s, smm_revision = %x\n", __func__, save_state->smm_revision);
    save_state->event_control = 0x1;
}

[Rocky Phagura (Code Review)]

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

Change subject: soc/intel/xeon_sp: Allow ACPI SMI GNVS
......................................................................

Patch Set 2:

Marc,
It's possible to use GNVS in SMI.  In fact, I'm working on some EINJ (error injection code) support that uses GNVS.

Secondly, I don't see any harm in turning on this feature as I am currently using it in my location development branch.  If you would like to wait, I can just include it as part of my EINJ patch.

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48492 )

Change subject: soc/intel/xeon_sp: Allow ACPI SMI GNVS
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2: Code-Review-1
> >
> > This requires properly fetching SMM save states which is not implemented correctly with smmloader v2. Then again other things in that smihandler accessing the save state are also there and are also possibly broken...
>
> smm loader v2 will work with state save.  The trick is to get the smbase using the MSR of the CPU executing the code (remember only 1 cpu thread is allowed to enter SMI handling code, while others wait).  Below is a snippet of code I'm using to modify memory in state save.
>
> static uint64_t ras_get_smbase(void)
> {
>    msr_t msr = rdmsr(IA32_SMBASE);
>    uint64_t smbase = 0;
>    smbase = msr.hi;
>    smbase = smbase << 32;
>    smbase |= msr.lo;
>    return smbase;
> }
> static void ras_post_cmci(void)
> {
>    em64t101_smm_state_save_area_t *save_state;
>    uintptr_t base = ras_get_smbase();
>    printk(BIOS_DEBUG, "%s, smbase = 0x%zx from MSR\n", __func__, base);
>    save_state = (void *)(base + SMM_DEFAULT_SIZE - sizeof(*save_state));
>    printk(BIOS_DEBUG, "%s, smbase & = %x\n", __func__, (uint32_t)&save_state->smbase);
>    printk(BIOS_DEBUG, "%s, smbase = %x\n", __func__, save_state->smbase);
>    printk(BIOS_DEBUG, "%s, smm_revision = %x\n", __func__, save_state->smm_revision);
>    save_state->event_control = 0x1;
> }

What our code currently does is to loop over all save states to find which core initiated the Synchronous IO SMI to account for cases where the core initiating the SMI != core in SMM (and where IA32_SMBASE is not supported). For this one needs to know the location of all save states which is a bit more complicated with smmloader v2. (https://review.coreboot.org/c/coreboot/+/47081/2 not working)

[Rocky Phagura (Code Review)]

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

Change subject: soc/intel/xeon_sp: Allow ACPI SMI GNVS
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2: Code-Review-1
> > >
> > > This requires properly fetching SMM save states which is not implemented correctly with smmloader v2. Then again other things in that smihandler accessing the save state are also there and are also possibly broken...
> >
> > smm loader v2 will work with state save.  The trick is to get the smbase using the MSR of the CPU executing the code (remember only 1 cpu thread is allowed to enter SMI handling code, while others wait).  Below is a snippet of code I'm using to modify memory in state save.
> >
> > static uint64_t ras_get_smbase(void)
> > {
> >    msr_t msr = rdmsr(IA32_SMBASE);
> >    uint64_t smbase = 0;
> >    smbase = msr.hi;
> >    smbase = smbase << 32;
> >    smbase |= msr.lo;
> >    return smbase;
> > }
> > static void ras_post_cmci(void)
> > {
> >    em64t101_smm_state_save_area_t *save_state;
> >    uintptr_t base = ras_get_smbase();
> >    printk(BIOS_DEBUG, "%s, smbase = 0x%zx from MSR\n", __func__, base);
> >    save_state = (void *)(base + SMM_DEFAULT_SIZE - sizeof(*save_state));
> >    printk(BIOS_DEBUG, "%s, smbase & = %x\n", __func__, (uint32_t)&save_state->smbase);
> >    printk(BIOS_DEBUG, "%s, smbase = %x\n", __func__, save_state->smbase);
> >    printk(BIOS_DEBUG, "%s, smm_revision = %x\n", __func__, save_state->smm_revision);
> >    save_state->event_control = 0x1;
> > }
>
> What our code currently does is to loop over all save states to find which core initiated the Synchronous IO SMI to account for cases where the core initiating the SMI != core in SMM (and where IA32_SMBASE is not supported). For this one needs to know the location of all save states which is a bit more complicated with smmloader v2. (https://review.coreboot.org/c/coreboot/+/47081/2 not working)

Agreed that would be a problem if you are trying to access other CPUs save states. So to help with us, I recommend you take a look at the the following:
struct cpu_smm_info {
    uint8_t active;
    uintptr_t smbase;
    uintptr_t entry;
    uintptr_t ss_start;
    uintptr_t code_start;
    uintptr_t code_end;
};

During SMM setup the version 2 loader code will create a map of all cpus and their save states.

smm_get_cpu_smbase - located in smm_module_loaderv2.c - if you pass in a CPU number (0 to MAX_CPUS) it will return the smbase. However, this is only used during ramstage for SMM initialization and not available in SMM.  Therefore you will need to copy it to SMM and use it as needed.