On Mon, Nov 23, 2020 at 10:10 PM ira.weiny@intel.com wrote:
From: Ira Weiny ira.weiny@intel.com
Currently struct irqentry_state_t only contains a single bool value which makes passing it by value is reasonable. However, future patches add information to this struct. This includes the PKRS thread state, included in this series, as well as information to store kmap reference tracking and PKS global state outside this series. In total, we anticipate 2 new 32 bit fields and an integer field to be added to the struct beyond the existing bool value.
Adding information to irqentry_state_t makes passing by value less efficient. Therefore, change the entry/exit calls to pass irq_state by reference in preparation for the changes which follow.
While at it, make the code easier to follow by changing all the usage sites to consistently use the variable name 'irq_state'.
After contemplating this for a bit, I think this isn't really the right approach. It *works*, but we've mostly just created a bit of an unfortunate situation. Our stack, on a (possibly nested) entry looks like:
previous frame (or empty if we came from usermode) --- SS RSP FLAGS CS RIP rest of pt_regs
C frame
irqentry_state_t (maybe -- the compiler is within its rights to play almost arbitrary games here)
more C stuff
So what we've accomplished is having two distinct arch register regions, one called pt_regs and the other stuck in irqentry_state_t. This is annoying because it means that, if we want to access this thing without passing a pointer around or access it at all from outer frames, we need to do something terrible with the unwinder, and we don't want to go there.
So I propose a somewhat different solution: lay out the stack like this.
SS RSP FLAGS CS RIP rest of pt_regs PKS ^^^^^^^^ extended_pt_regs points here
C frame more C stuff ...
IOW we have:
struct extended_pt_regs { bool rcu_whatever; other generic fields here; struct arch_extended_pt_regs arch_regs; struct pt_regs regs; };
and arch_extended_pt_regs has unsigned long pks;
and instead of passing a pointer to irqentry_state_t to the generic entry/exit code, we just pass a pt_regs pointer. And we have a little accessor like:
struct extended_pt_regs *extended_regs(struct pt_regs *) { return container_of(...); }
And we tell eBPF that extended_pt_regs is NOT ABI, and we will change it whenever we feel like just to keep you on your toes, thank you very much.
Does this seem reasonable? You get to drop patch 7 and instead modify the show_regs() stuff to just display one extra register.