[coreboot-gerrit] [M] Change in coreboot[master]: cpu/x86/smm: Drop fxsave/fxrstor logic

[Martin L Roth (Code Review)]

Martin L Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/75317?usp=email )

Change subject: cpu/x86/smm: Drop fxsave/fxrstor logic
......................................................................

cpu/x86/smm: Drop fxsave/fxrstor logic

Since we now explicitly compile both ramstage and smihandler code
without floating point operations and associated registers we don't need
to save/restore floating point registers.

Signed-off-by: Arthur Heymans 
Change-Id: I180b9781bf5849111501ae8e9806554a7851c0da
Reviewed-on: https://review.coreboot.org/c/coreboot/+/75317
Tested-by: build bot (Jenkins) 
Reviewed-by: Elyes Haouas 
---
M src/cpu/x86/smm/smm_module_loader.c
M src/cpu/x86/smm/smm_stub.S
M src/include/cpu/x86/smm.h
3 files changed, 15 insertions(+), 74 deletions(-)

Approvals:
  Elyes Haouas: Looks good to me, approved
  build bot (Jenkins): Verified

diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c
index 47a2831..f827e7b 100644
--- a/src/cpu/x86/smm/smm_module_loader.c
+++ b/src/cpu/x86/smm/smm_module_loader.c
@@ -13,13 +13,7 @@
 #include <string.h>
 #include <types.h>

-#define FXSAVE_SIZE 512
 #define SMM_CODE_SEGMENT_SIZE 0x10000
-/* FXSAVE area during relocation. While it may not be strictly needed the
-   SMM stub code relies on the FXSAVE area being non-zero to enable SSE
-   instructions within SMM mode. */
-static uint8_t fxsave_area_relocation[CONFIG_MAX_CPUS][FXSAVE_SIZE]
-__attribute__((aligned(16)));

/*
  * Components that make up the SMRAM:
@@ -56,7 +50,7 @@
  * developer's manuals (volume 3, chapter 34). SMRAM is divided up into the
  * following regions:
  * +-----------------+ Top of SMRAM
- * |                 | <- MSEG, FXSAVE
+ * |      MSEG       |
  * +-----------------+
  * |    common       |
  * |  smi handler    | 64K
@@ -240,8 +234,7 @@
  * by the permanent handler can be used during relocation.
  */
 static int smm_module_setup_stub(const uintptr_t smbase, const size_t smm_size,
-				 struct smm_loader_params *params,
-				 void *const fxsave_area)
+				 struct smm_loader_params *params)
 {
    struct rmodule smm_stub;
    if (rmodule_parse(&_binary_smmstub_start, &smm_stub)) {
@@ -266,8 +259,6 @@
    stub_params->stack_top = stack_top;
    stub_params->stack_size = g_stack_size;
    stub_params->c_handler = (uintptr_t)params->handler;
-	stub_params->fxsave_area = (uintptr_t)fxsave_area;
-	stub_params->fxsave_area_size = FXSAVE_SIZE;

/* This runs on the BSP. All the APs are its siblings */
    struct cpu_info *info = cpu_info();
@@ -322,8 +313,7 @@
    	params->num_cpus = CONFIG_MAX_CPUS;

printk(BIOS_SPEW, "%s: exit\n", __func__);
-	return smm_module_setup_stub(smram, SMM_DEFAULT_SIZE,
-				     params, fxsave_area_relocation);
+	return smm_module_setup_stub(smram, SMM_DEFAULT_SIZE, params);
 }

static void setup_smihandler_params(struct smm_runtime *mod_params,
@@ -363,8 +353,8 @@
           region_end(&region));
 }

-/* STM + FX_SAVE + Handler + (Stub + Save state) * CONFIG_MAX_CPUS + stacks */
-#define SMM_REGIONS_ARRAY_SIZE (1 + 1 + 1 + CONFIG_MAX_CPUS * 2 + 1)
+/* STM + Handler + (Stub + Save state) * CONFIG_MAX_CPUS + stacks */
+#define SMM_REGIONS_ARRAY_SIZE (1  + 1 + CONFIG_MAX_CPUS * 2 + 1)

static int append_and_check_region(const struct region smram,
    			   const struct region region,
@@ -406,8 +396,6 @@
  * | BIOS resource   |
  * | list (STM)      |
  * +-----------------+
- * |  fxsave area    |
- * +-----------------+
  * |  smi handler    |
  * |      ...        |
  * +-----------------+ <- cpu0
@@ -453,19 +441,10 @@
    	printk(BIOS_DEBUG, "BIOS res list 0x%x\n", CONFIG_BIOS_RESOURCE_LIST_SIZE);
    }

-	const size_t fx_save_area_size = CONFIG(SSE) ? FXSAVE_SIZE * params->num_cpus : 0;
-	struct region fx_save = {};
-	if (CONFIG(SSE)) {
-		fx_save.offset = smram_top - stm_size - fx_save_area_size;
-		fx_save.size = fx_save_area_size;
-		if (append_and_check_region(smram, fx_save, region_list, "FX_SAVE"))
-			return -1;
-	}
-
    const size_t handler_size = rmodule_memory_size(&smi_handler);
    const size_t handler_alignment = rmodule_load_alignment(&smi_handler);
    const uintptr_t handler_base =
-		ALIGN_DOWN(smram_top - stm_size - fx_save_area_size - handler_size,
+		ALIGN_DOWN(smram_top - stm_size - handler_size,
    		   handler_alignment);
    struct region handler = {
    	.offset = handler_base,
@@ -506,6 +485,5 @@
    params->handler = rmodule_entry(&smi_handler);
    setup_smihandler_params(smihandler_params, smram_base, smram_size, params);

-	return smm_module_setup_stub(stub_segment_base, smram_size, params,
-				     (void *)region_offset(&fx_save));
+	return smm_module_setup_stub(stub_segment_base, smram_size, params);
 }
diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S
index e44e99d..673ab2c 100644
--- a/src/cpu/x86/smm/smm_stub.S
+++ b/src/cpu/x86/smm/smm_stub.S
@@ -22,10 +22,6 @@
 .long 0
 c_handler:
 .long 0
-fxsave_area:
-.long 0
-fxsave_area_size:
-.long 0
 /* apic_to_cpu_num is a table mapping the default APIC id to CPU num. If the
  * APIC id is found at the given index, the contiguous CPU number is index
  * into the table. */
@@ -171,9 +167,7 @@
     * not be assigned. Use the fallback stack and check this condition in
     * C handler. */
    movl	$(fallback_stack_top), %esp
-	/* Clear fxsave location as there will be no saving/restoring. */
-	xor	%edi, %edi
-	jmp	2f
+	jmp	align_stack
 1:
    movl	stack_size, %eax
    mul	%ecx /* %eax(stack_size) * %ecx(cpu) = %eax(offset) */
@@ -193,32 +187,10 @@
    pushl	$0x0
    mov	%esp, %ebp

-	/* Allocate locals (fxsave, efer_backup) */
-	subl	$0xc, %esp
+	/* Allocate locals (efer_backup) */
+	subl	$0x8, %esp

-	/* calculate fxsave location */
-	mov	fxsave_area, %edi
-	test	%edi, %edi
-	jz	2f
-	movl	fxsave_area_size, %eax
-	mul	%ecx
-	add	%eax, %edi
-
-2:
-	/* Save location of fxsave area. */
-	mov	%edi, -4(%ebp)
-	test	%edi, %edi
-	jz	1f
-
-	/* Enable sse instructions. */
-	mov	%cr4, %eax
-	orl	$(CR4_OSFXSR | CR4_OSXMMEXCPT), %eax
-	mov	%eax, %cr4
-
-	/* Save FP state. */
-	fxsave	(%edi)
-
-1:
+align_stack:
    /* Align stack to 16 bytes. Another 32 bytes are pushed below. */
    andl	$0xfffffff0, %esp

@@ -227,8 +199,8 @@
    /* Backup IA32_EFER. Preserves ebx. */
    movl	$(IA32_EFER), %ecx
    rdmsr
-	movl	%eax, -0x8(%ebp)
-	movl	%edx, -0xc(%ebp)
+	movl	%eax, -0x4(%ebp)
+	movl	%edx, -0x8(%ebp)

/* entry64.inc preserves ebx, esi, edi, ebp */
 #include <cpu/x86/64bit/entry64.inc>
@@ -262,8 +234,8 @@
    /* Restore IA32_EFER as RSM doesn't restore MSRs. */
    movl	$(IA32_EFER), %ecx
    rdmsr
-	movl	-0x8(%ebp), %eax
-	movl	-0xc(%ebp), %edx
+	movl	-0x4(%ebp), %eax
+	movl	-0x8(%ebp), %edx

wrmsr

@@ -275,13 +247,6 @@
    mov	c_handler, %eax
    call	*%eax
 #endif
-	/* Retrieve fxsave location. */
-	mov	-4(%ebp), %edi
-	test	%edi, %edi
-	jz	1f
-
-	/* Restore FP state. */
-	fxrstor	(%edi)

1:
    /* Exit from SM mode. */
diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h
index 4c97f21..68c7c3b 100644
--- a/src/include/cpu/x86/smm.h
+++ b/src/include/cpu/x86/smm.h
@@ -98,8 +98,6 @@
    u32 stack_size;
    u32 stack_top;
    u32 c_handler;
-	u32 fxsave_area;
-	u32 fxsave_area_size;
    /* The apic_id_to_cpu provides a mapping from APIC id to CPU number.
     * The CPU number is indicated by the index into the array by matching
     * the default APIC id and value at the index. The stub loader