Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44323 )
Change subject: [WIP] SMM save states ......................................................................
[WIP] SMM save states
Change-Id: I4a31d05c09065543424a9010ac434dde0dfb5836 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/smm/Makefile.inc A src/cpu/intel/smm/em64t101_save_state.c M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/save_state.c M src/cpu/x86/smm/smm.ld A src/include/cpu/x86/save_state.h M src/lib/program.ld M src/southbridge/intel/i82801ix/smihandler.c 8 files changed, 210 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/44323/1
diff --git a/src/cpu/intel/smm/Makefile.inc b/src/cpu/intel/smm/Makefile.inc index a49b796..df73ae7 100644 --- a/src/cpu/intel/smm/Makefile.inc +++ b/src/cpu/intel/smm/Makefile.inc @@ -1 +1,2 @@ ramstage-$(CONFIG_HAVE_SMI_HANDLER) += smm_reloc.c +smm-y += em64t101_save_state.c diff --git a/src/cpu/intel/smm/em64t101_save_state.c b/src/cpu/intel/smm/em64t101_save_state.c new file mode 100644 index 0000000..404906b --- /dev/null +++ b/src/cpu/intel/smm/em64t101_save_state.c @@ -0,0 +1,106 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <string.h> +#include <cpu/x86/save_state.h> +#include <cpu/intel/em64t101_save_state.h> + +static void *em64t101_get_reg_base(const enum cpu_reg reg, const int node) +{ + const em64t101_smm_state_save_area_t *save_state = (const em64t101_smm_state_save_area_t *)smm_get_save_state(node); + + switch (reg) { + case RAX: + return (void *)&save_state->rax; + case RBX: + return (void *)&save_state->rbx; + case RCX: + return (void *)&save_state->rcx; + case RDX: + return (void *)&save_state->rdx; + } + + return NULL; +} + +enum get_set { + GET, + SET +}; + +static int em64t101_get_set(const enum get_set op_type, const enum cpu_reg reg, + const int node, void *in_out, const uint8_t length) +{ + + void *reg_base = em64t101_get_reg_base(reg, node); + + if (!reg_base) + return -1; + + switch (length) { + case 1: + case 2: + case 4: + case 8: + switch (op_type) { + case GET: + memcpy(in_out, reg_base, length); + return 0; + case SET: + memcpy(reg_base, in_out, length); + } + } + + return -1; +} + +static int em64t101_get_reg(const enum cpu_reg reg, const int node, void *out, const uint8_t length) +{ + return em64t101_get_set(GET, reg, node, out, length); +} + +static int em64t101_set_reg(const enum cpu_reg reg, const int node, void *in, const uint8_t length) +{ + return em64t101_get_set(SET, reg, node, in, length); +} + +static int em64t101_apmc_node(u8 cmd) +{ + em64t101_smm_state_save_area_t *state; + int node; + + for (node = 0; node < CONFIG_MAX_CPUS; node++) { + state = smm_get_save_state(node); + + /* Check for Synchronous IO (bit0 == 1) */ + if (!(state->io_misc_info & (1 << 0))) + continue; + + /* Make sure it was a write (bit4 == 0) */ + if (state->io_misc_info & (1 << 4)) + continue; + + /* Check for APMC IO port */ + if (((state->io_misc_info >> 16) & 0xff) != APM_CNT) + continue; + + /* Check AX against the requested command */ + if ((state->rax & 0xff) != cmd) + continue; + + return node; + } + + return -1; +} + +static const uint32_t revisions[] = { + 0x00030101, + SMM_REV_INVALID, +}; + +static const struct smm_save_state_ops ops __smm_save_state_ops = { + .revision_table = revisions, + .get_reg = em64t101_get_reg, + .set_reg = em64t101_set_reg, + .apmc_node = em64t101_apmc_node, +}; diff --git a/src/cpu/x86/smm/Makefile.inc b/src/cpu/x86/smm/Makefile.inc index bbd96c6..2b3e073 100644 --- a/src/cpu/x86/smm/Makefile.inc +++ b/src/cpu/x86/smm/Makefile.inc @@ -28,6 +28,8 @@ ramstage-srcs += $(obj)/cpu/x86/smm/smm.manual endif
+smm-y += save_state.c + ifeq ($(CONFIG_SMM_TSEG),y)
ramstage-y += tseg_region.c diff --git a/src/cpu/x86/smm/save_state.c b/src/cpu/x86/smm/save_state.c new file mode 100644 index 0000000..7c757e3 --- /dev/null +++ b/src/cpu/x86/smm/save_state.c @@ -0,0 +1,45 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <cpu/x86/smm.h> +#include <cpu/x86/save_state.h> + +static struct smm_save_state_ops *save_state; + +/* Returns -1 on failure, 0 on success */ +static int init_save_state(void) +{ + const uint32_t revision = smm_revision(); + struct smm_save_state_ops *ops; + static bool initialized = false; + + if (initialized) + return 0; + + for (ops = _smm_save_state_ops; ops < _esmm_save_state_ops; ops++) { + const uint32_t *rev; + for (rev = ops->revision_table; *rev != SMM_REV_INVALID; rev++) + if (*rev == revision) { + save_state = ops; + initialized = true; + return 0; + } + } + + return -1; +} + +int get_apmc_node(u8 cmd) +{ + if (init_save_state()) + return -1; + + return save_state->apmc_node(cmd); +} + +int get_save_state_reg(const enum cpu_reg reg, const int node, void *out, const uint8_t length) +{ + if (init_save_state()) + return -1; + + return save_state->get_reg(reg, node, out, length); +} diff --git a/src/cpu/x86/smm/smm.ld b/src/cpu/x86/smm/smm.ld index 2f46975..39afee4 100644 --- a/src/cpu/x86/smm/smm.ld +++ b/src/cpu/x86/smm/smm.ld @@ -25,6 +25,11 @@
/* C read-only data of the SMM handler */ . = ALIGN(16); + + _smm_save_state_ops = .; + KEEP(*(.rodata.smm_ops)); + _esmm_save_state_ops = .; + *(.rodata) *(.rodata.*)
diff --git a/src/include/cpu/x86/save_state.h b/src/include/cpu/x86/save_state.h new file mode 100644 index 0000000..b97890b --- /dev/null +++ b/src/include/cpu/x86/save_state.h @@ -0,0 +1,41 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __CPU_X86_SAVE_STATE_H__ +#define __CPU_X86_SAVE_STATE_H__ + +#include <stdint.h> + +#if ENV_SMM +#define __smm_save_state_ops __attribute__((used, __section__(".rodata.smm_ops"))) +#else +#define __smm_save_state_ops __attribute__((unused)) +#endif + +enum cpu_reg { + RAX, + RBX, + RCX, + RDX +}; + +#define SMM_REV_INVALID 0xffffffff + +struct smm_save_state_ops { + const uint32_t *revision_table; + /* Accessors for CPU registers in the SMM save state + Returns -1 on failure, 0 on success */ + int (*get_reg)(const enum cpu_reg reg, const int node, void *out, const uint8_t length); + int(*set_reg)(const enum cpu_reg reg, const int node, void *in, const uint8_t length); + /* Returns -1 on failure, the node on which the 'cmd' was send on success */ + int(*apmc_node)(u8 cmd); +}; + +/** start of compile time generated pci driver array */ +extern struct smm_save_state_ops _smm_save_state_ops[]; +/** end of compile time generated pci driver array */ +extern struct smm_save_state_ops _esmm_save_state_ops[]; + +int get_apmc_node(u8 cmd); +int get_save_state_reg(const enum cpu_reg reg, const int node, void *out, const uint8_t length); + +#endif /* __CPU_X86_SAVE_STATE_H__ */ diff --git a/src/lib/program.ld b/src/lib/program.ld index 88a3126..1894ae0 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -44,6 +44,13 @@ _ecpu_drivers = .; #endif
+#if ENV_SMM || ENV_RMODULE + . = ALIGN(ARCH_POINTER_ALIGN_SIZE); + _smm_save_state_ops = .; + KEEP(*(.rodata.smm_ops)); + _esmm_save_state_ops = .; +#endif + . = ALIGN(ARCH_POINTER_ALIGN_SIZE); *(.rodata); *(.rodata.*); diff --git a/src/southbridge/intel/i82801ix/smihandler.c b/src/southbridge/intel/i82801ix/smihandler.c index 699f51f..9b37f05 100644 --- a/src/southbridge/intel/i82801ix/smihandler.c +++ b/src/southbridge/intel/i82801ix/smihandler.c @@ -3,6 +3,7 @@ #include <types.h> #include <console/console.h> #include <cpu/x86/smm.h> +#include <cpu/x86/save_state.h> #include <device/pci_def.h> #include <southbridge/intel/common/pmutil.h> #include "i82801ix.h" @@ -31,6 +32,8 @@ return 1; /* IO trap handled */ }
+ get_apmc_node(5); + /* Not handled */ return 0; }
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44323 )
Change subject: [WIP] SMM save states ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44323/1/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/44323/1/src/lib/program.ld@47 PS1, Line 47: ENV_SMM || ENV_RMODULE program.ld is only included with ENV_RMODULE=1. Any idea how to pass on -D__SMM__ too to avoid this?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44323 )
Change subject: [WIP] SMM save states ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/44323/1/src/cpu/intel/smm/em64t101_... File src/cpu/intel/smm/em64t101_save_state.c:
https://review.coreboot.org/c/coreboot/+/44323/1/src/cpu/intel/smm/em64t101_... PS1, Line 9: const em64t101_smm_state_save_area_t *save_state = (const em64t101_smm_state_save_area_t *)smm_get_save_state(node); line over 96 characters
https://review.coreboot.org/c/coreboot/+/44323/1/src/cpu/intel/smm/em64t101_... PS1, Line 56: static int em64t101_get_reg(const enum cpu_reg reg, const int node, void *out, const uint8_t length) line over 96 characters
https://review.coreboot.org/c/coreboot/+/44323/1/src/cpu/intel/smm/em64t101_... PS1, Line 61: static int em64t101_set_reg(const enum cpu_reg reg, const int node, void *in, const uint8_t length) line over 96 characters
https://review.coreboot.org/c/coreboot/+/44323/1/src/include/cpu/x86/save_st... File src/include/cpu/x86/save_state.h:
https://review.coreboot.org/c/coreboot/+/44323/1/src/include/cpu/x86/save_st... PS1, Line 28: int(*set_reg)(const enum cpu_reg reg, const int node, void *in, const uint8_t length); missing space after return type
https://review.coreboot.org/c/coreboot/+/44323/1/src/include/cpu/x86/save_st... PS1, Line 30: int(*apmc_node)(u8 cmd); missing space after return type
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44323
to look at the new patch set (#2).
Change subject: cpu/x86/smm: Add a common save state handling ......................................................................
cpu/x86/smm: Add a common save state handling
Currently coreboot has limited use for the SMM save state. Typically the only thing needed is to get or set a few registers and to know which CPU triggered the SMI (typically via an IO write). Abstracting away different SMM save states would allow to put some SMM functionality like the SMMSTORE entry in common places.
To save place platforms can select different SMM save sate ops that should be implemented. For instance AMD platforms don't need Intel SMM save state handling.
Some platforms can encounter CPUs with different save states, which the code then handles at runtime by comparing the SMM save state revision which is located at the same offset for all SMM save state types.
Change-Id: I4a31d05c09065543424a9010ac434dde0dfb5836 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/save_state.c A src/include/cpu/x86/save_state.h 3 files changed, 113 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/44323/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44323 )
Change subject: cpu/x86/smm: Add a common save state handling ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44323/2/src/include/cpu/x86/save_st... File src/include/cpu/x86/save_state.h:
https://review.coreboot.org/c/coreboot/+/44323/2/src/include/cpu/x86/save_st... PS2, Line 22: int(*set_reg)(const enum cpu_reg reg, const int node, void *in, const uint8_t length); missing space after return type
https://review.coreboot.org/c/coreboot/+/44323/2/src/include/cpu/x86/save_st... PS2, Line 24: int(*apmc_node)(u8 cmd); missing space after return type
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44323
to look at the new patch set (#3).
Change subject: cpu/x86/smm: Add a common save state handling ......................................................................
cpu/x86/smm: Add a common save state handling
Currently coreboot has limited use for the SMM save state. Typically the only thing needed is to get or set a few registers and to know which CPU triggered the SMI (typically via an IO write). Abstracting away different SMM save states would allow to put some SMM functionality like the SMMSTORE entry in common places.
To save place platforms can select different SMM save sate ops that should be implemented. For instance AMD platforms don't need Intel SMM save state handling.
Some platforms can encounter CPUs with different save states, which the code then handles at runtime by comparing the SMM save state revision which is located at the same offset for all SMM save state types.
Change-Id: I4a31d05c09065543424a9010ac434dde0dfb5836 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/save_state.c A src/include/cpu/x86/save_state.h 3 files changed, 113 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/44323/3
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44323
to look at the new patch set (#5).
Change subject: cpu/x86/smm: Add a common save state handling ......................................................................
cpu/x86/smm: Add a common save state handling
Currently coreboot has limited use for the SMM save state. Typically the only thing needed is to get or set a few registers and to know which CPU triggered the SMI (typically via an IO write). Abstracting away different SMM save states would allow to put some SMM functionality like the SMMSTORE entry in common places.
To save place platforms can select different SMM save sate ops that should be implemented. For instance AMD platforms don't need Intel SMM save state handling.
Some platforms can encounter CPUs with different save states, which the code then handles at runtime by comparing the SMM save state revision which is located at the same offset for all SMM save state types.
Change-Id: I4a31d05c09065543424a9010ac434dde0dfb5836 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/save_state.c A src/include/cpu/x86/save_state.h 3 files changed, 113 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/44323/5
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44323
to look at the new patch set (#6).
Change subject: cpu/x86/smm: Add a common save state handling ......................................................................
cpu/x86/smm: Add a common save state handling
Currently coreboot has limited use for the SMM save state. Typically the only thing needed is to get or set a few registers and to know which CPU triggered the SMI (typically via an IO write). Abstracting away different SMM save states would allow to put some SMM functionality like the SMMSTORE entry in common places.
To save place platforms can select different SMM save sate ops that should be implemented. For instance AMD platforms don't need Intel SMM save state handling.
Some platforms can encounter CPUs with different save states, which the code then handles at runtime by comparing the SMM save state revision which is located at the same offset for all SMM save state types.
Change-Id: I4a31d05c09065543424a9010ac434dde0dfb5836 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/save_state.c A src/include/cpu/x86/save_state.h 3 files changed, 113 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/44323/6
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44323 )
Change subject: cpu/x86/smm: Add a common save state handling ......................................................................
Patch Set 6: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44323 )
Change subject: cpu/x86/smm: Add a common save state handling ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44323 )
Change subject: cpu/x86/smm: Add a common save state handling ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44323/1/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/44323/1/src/lib/program.ld@47 PS1, Line 47: ENV_SMM || ENV_RMODULE
program.ld is only included with ENV_RMODULE=1. […]
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44323 )
Change subject: cpu/x86/smm: Add a common save state handling ......................................................................
cpu/x86/smm: Add a common save state handling
Currently coreboot has limited use for the SMM save state. Typically the only thing needed is to get or set a few registers and to know which CPU triggered the SMI (typically via an IO write). Abstracting away different SMM save states would allow to put some SMM functionality like the SMMSTORE entry in common places.
To save place platforms can select different SMM save sate ops that should be implemented. For instance AMD platforms don't need Intel SMM save state handling.
Some platforms can encounter CPUs with different save states, which the code then handles at runtime by comparing the SMM save state revision which is located at the same offset for all SMM save state types.
Change-Id: I4a31d05c09065543424a9010ac434dde0dfb5836 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/44323 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/save_state.c A src/include/cpu/x86/save_state.h 3 files changed, 113 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/cpu/x86/smm/Makefile.inc b/src/cpu/x86/smm/Makefile.inc index c2f49cf..eb386a6 100644 --- a/src/cpu/x86/smm/Makefile.inc +++ b/src/cpu/x86/smm/Makefile.inc @@ -32,6 +32,8 @@ ramstage-srcs += $(obj)/cpu/x86/smm/smm.manual endif
+smm-y += save_state.c + ifeq ($(CONFIG_SMM_TSEG),y)
ramstage-y += tseg_region.c diff --git a/src/cpu/x86/smm/save_state.c b/src/cpu/x86/smm/save_state.c new file mode 100644 index 0000000..bb08f86 --- /dev/null +++ b/src/cpu/x86/smm/save_state.c @@ -0,0 +1,77 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <cpu/x86/smm.h> +#include <cpu/x86/save_state.h> + +/* These are weakly linked such that platforms can link only the save state + ops they actually require. */ +const struct smm_save_state_ops *legacy_ops __weak = NULL; +const struct smm_save_state_ops *em64t100_ops __weak = NULL; +const struct smm_save_state_ops *em64t101_ops __weak = NULL; +const struct smm_save_state_ops *amd64_ops __weak = NULL; + +static const struct smm_save_state_ops *save_state; + +/* Returns -1 on failure, 0 on success */ +static int init_save_state(void) +{ + const uint32_t revision = smm_revision(); + int i; + static bool initialized = false; + const struct smm_save_state_ops *save_state_ops[] = { + legacy_ops, + em64t100_ops, + em64t101_ops, + amd64_ops, + }; + + if (initialized) + return 0; + + for (i = 0; i < ARRAY_SIZE(save_state_ops); i++) { + const struct smm_save_state_ops *ops = save_state_ops[i]; + const uint32_t *rev; + + if (ops == NULL) + continue; + + for (rev = ops->revision_table; *rev != SMM_REV_INVALID; rev++) + if (*rev == revision) { + save_state = ops; + initialized = true; + return 0; + } + } + + return -1; +} + +int get_apmc_node(u8 cmd) +{ + if (init_save_state()) + return -1; + + return save_state->apmc_node(cmd); +} + +int get_save_state_reg(const enum cpu_reg reg, const int node, void *out, const uint8_t length) +{ + if (init_save_state()) + return -1; + + if (node > CONFIG_MAX_CPUS) + return -1; + + return save_state->get_reg(reg, node, out, length); +} + +int set_save_state_reg(const enum cpu_reg reg, const int node, void *in, const uint8_t length) +{ + if (init_save_state()) + return -1; + + if (node > CONFIG_MAX_CPUS) + return -1; + + return save_state->set_reg(reg, node, in, length); +} diff --git a/src/include/cpu/x86/save_state.h b/src/include/cpu/x86/save_state.h new file mode 100644 index 0000000..d6fcf63 --- /dev/null +++ b/src/include/cpu/x86/save_state.h @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __CPU_X86_SAVE_STATE_H__ +#define __CPU_X86_SAVE_STATE_H__ + +#include <stdint.h> + +enum cpu_reg { + RAX, + RBX, + RCX, + RDX +}; + +#define SMM_REV_INVALID 0xffffffff + +struct smm_save_state_ops { + const uint32_t *revision_table; + /* Accessors for CPU registers in the SMM save state + Returns -1 on failure, 0 on success */ + int (*get_reg)(const enum cpu_reg reg, const int node, void *out, const uint8_t length); + int (*set_reg)(const enum cpu_reg reg, const int node, void *in, const uint8_t length); + /* Returns -1 on failure, the node on which the 'cmd' was send on success */ + int (*apmc_node)(u8 cmd); +}; + +/* Return -1 on failure, otherwise returns which CPU node issued an APMC IO write */ +int get_apmc_node(u8 cmd); +/* Return -1 on failure, 0 on succes. + Accessors for the SMM save state CPU registers RAX, RBX, RCX and RDX */ +int get_save_state_reg(const enum cpu_reg reg, const int node, void *out, const uint8_t length); +int set_save_state_reg(const enum cpu_reg reg, const int node, void *in, const uint8_t length); + +#endif /* __CPU_X86_SAVE_STATE_H__ */
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Patrick Georgi (Code Review)