[coreboot-gerrit] [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:

(1 comment)

Patchset:

PS4: 
Is it too late to bikeshed the new API? I feel silly because I
missed the discussion and now realized that it's been merged
four years ago already.

I see one advantage in the pointer+length API over the intelblocks
one: One can set part of a register, which is common on x86
(and it allows it without noisy read-modify-write). Though, with
this API it's hard to see what is done, e.g.
```
        if (get_save_state_reg(RAX, apmc_node, &ax, sizeof(ax)))
```
and
```
        if (set_save_state_reg(RAX, apmc_node, &ax, sizeof(ax)))
```
pretty much looks the same to me. Changing the parameter order of
the getter to mimic an assignment could help here.

Another thought is the need to check for errors on every invocation.
I have some things on my mind, nothing that I prefer specifically.
I'll just leave a sketch here how I could imagine a call site. It
probably won't match everybody's taste, but feel free to ask me
for details if you're interested.
```
        if (init_save_state_api()) /* inits once, always checks apmc_node */
                return ouch;

ax = *save_state_ref16(RAX);
        ...
        *save_state_ref32(RAX) = ret;
```
This could involve die() on wrong API usage. But I guess things like
the register asked for should be hardcoded anyway. Hmmm, still thinking
init_save_state_api() could return the node (as an opaque handle), then
it would be impossible to forget to call it.