Re: [PATCH 5/5] x86/pv: Simplify emulation for the 64bit base MSRs

[Jan Beulich <jbeulich@xxxxxxxx>]

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.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
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