[L] Change in coreboot[main]: cpu/x86/smm: Add save state ops for different save states

[Nico Huber (Code Review)]

Attention is currently required from: Arthur Heymans, Jérémy Compostella.

Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/83064?usp=email )

Change subject: cpu/x86/smm: Add save state ops for different save states
......................................................................

Patch Set 4:

(7 comments)

Patchset:

PS4: 
Mostly duplication concerns...

PS4:
> Is it too late to bikeshed the new API? I feel silly because I […]

Looking ahead, it doesn't seem to matter if we'd change it now or later.

File src/cpu/amd/smm/amd64_save_state.c:

https://review.coreboot.org/c/coreboot/+/83064/comment/a2711e9a_d62dee09?usp... :
PS4, Line 10: 			- sizeof(amd64_smm_state_save_area_t));
Shouldn't this be covered by setting `.save_state_size` to the correct value?
Or can't we probe that before we are in SMM? But then, why do we have a callback
in the mp init to decide it?

Either way, it would seem better to make smm_get_save_state() return the correct
pointer right away.

https://review.coreboot.org/c/coreboot/+/83064/comment/11c0289e_56408e35?usp... :
PS4, Line 118: 	0x00030064,
Where to find these? Please mention in the commit message.

File src/cpu/intel/smm/em64t100_save_state.c:

https://review.coreboot.org/c/coreboot/+/83064/comment/241c9673_c541d4cc?usp... :
PS4, Line 38: static int em64t100_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 = em64t100_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 0;
            : 		}
            : 	}
            : 
            : 	return -1;
            : }
            : 
            : static int em64t100_get_reg(const enum cpu_reg reg, const int node, void *out,
            : 			    const uint8_t length)
            : {
            : 	return em64t100_get_set(GET, reg, node, out, length);
            : }
            : 
            : static int em64t100_set_reg(const enum cpu_reg reg, const int node, void *in,
            : 			    const uint8_t length)
            : {
            : 	return em64t100_get_set(SET, reg, node, in, length);
            : }
            : 
So these look like generic code. If we want to keep the API. The save_state_ops
cut could be made at get_reg_base() to avoid the duplication.

File src/cpu/intel/smm/em64t101_save_state.c:

https://review.coreboot.org/c/coreboot/+/83064/comment/66b8ad0c_7ab37b68?usp... :
PS4, Line 102: 	}
Could be made generic by taking `io_misc_info` & `rax` as parameters.

File src/cpu/x86/smm/legacy_save_state.c:

https://review.coreboot.org/c/coreboot/+/83064/comment/b09fcb31_a5155957?usp... :
PS4, Line 89: 	}
This looks like it would go wrong 1 out of 512 times on a dual core / socket system.
Is there no way to detect the AMPC or did just nobody look into it?