On 09.09.2020 11:59, Andrew Cooper wrote:
> is_pv_32bit_domain() is an expensive predicate, but isn't used for speculative
> safety in this case. Swap to checking the Long Mode bit in the CPUID policy,
> which is the architecturally correct behaviour.
>
> is_canonical_address() isn't a trivial predicate, but it will become more
> complicated when 5-level support is added. Rearrange write_msr() to collapse
> the common checks.
Did you mean "is" instead of "isn't" or "and" instead of "but"? The
way it is it doesn't look very logical to me.
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
Reviewed-by: Jan Beulich <jbeul...@suse.com>
with one more remark:
> @@ -991,22 +993,22 @@ static int write_msr(unsigned int reg, uint64_t val,
> uint64_t temp;
>
> case MSR_FS_BASE:
> - if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) )
> - break;
> - write_fs_base(val);
> - return X86EMUL_OKAY;
> -
> case MSR_GS_BASE:
> - if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) )
> - break;
> - write_gs_base(val);
> - return X86EMUL_OKAY;
> -
> case MSR_SHADOW_GS_BASE:
> - if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) )
> + if ( !cp->extd.lm || !is_canonical_address(val) )
> break;
> - write_gs_shadow(val);
> - curr->arch.pv.gs_base_user = val;
> +
> + if ( reg == MSR_FS_BASE )
> + write_fs_base(val);
> + else if ( reg == MSR_GS_BASE )
> + write_gs_base(val);
> + else if ( reg == MSR_SHADOW_GS_BASE )
With the three case labels just above, I think this "else if" and ...
> + {
> + write_gs_shadow(val);
> + curr->arch.pv.gs_base_user = val;
> + }
> + else
> + ASSERT_UNREACHABLE();
... this assertion are at least close to being superfluous. Their
dropping would then also make me less inclined to ask for an
inner switch().
Jan
