On 11/09/2020 11:01, Jan Beulich wrote:
> 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.
I guess the meaning got lost somewhere.
is_canonical_address() is currently not completely trivial, but also not
massively complicated either.
It will become much more complicated with LA57.
>
>> 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().
I'm not overly fussed, as this example is fairly trivial, but I was
attempting to go for something which ends up safe even in the case of a
bad edit to the outer switch statement.
I'd expect the compiler to be drop the both aspects you talk about.
~Andrew
