On 15.08.2025 22:41, Andrew Cooper wrote:
> It turns out that using the higher level helpers adjacent like this leads to
> terrible code generation. Due to -fno-strict-alising, the store into state->
> invalidates the read_cr4() address calculation (which is really cpu_info->cr4
> under the hood), meaning that it can't be hoisted.
>
> As a result we get "locate the top of stack block, get cr4, and see if
> FSGSBASE is set" repeated 3 times, and an unreasoanble number of basic blocks.
>
> Hoist the calculation manually, which results in two basic blocks.
>
> Signed-off-by: Andrew Cooper <[email protected]>
Otoh the function here isn't really performance or size critical. I'm undecided
whether the undesirable open-coding or the bad code gen are the lesser evil.
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -118,9 +118,18 @@ static void read_registers(struct extra_state *state)
> state->cr3 = read_cr3();
> state->cr4 = read_cr4();
>
> - state->fsb = read_fs_base();
> - state->gsb = read_gs_base();
> - state->gss = read_gs_shadow();
> + if ( state->cr4 & X86_CR4_FSGSBASE )
> + {
> + state->fsb = __rdfsbase();
> + state->gsb = __rdgsbase();
> + state->gss = __rdgskern();
This, btw, supports my desire to not use "kern" but "shadow" in the new helper's
name.
Jan
