diff mbox series

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

Message ID 20200909095920.25495-6-andrew.cooper3@citrix.com
State New
Headers show
Series x86/pv: Minor perf improvements in segment handling | expand

Commit Message

Andrew Cooper Sept. 9, 2020, 9:59 a.m. UTC
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.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

For reference, the diff is:

  add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-152 (-152)
  Function                                     old     new   delta
  read_msr                                    1075    1030     -45
  write_msr                                   1537    1430    -107

but this isn't the point of the change.
---
 xen/arch/x86/pv/emul-priv-op.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

Comments

Jan Beulich Sept. 11, 2020, 10:01 a.m. UTC | #1
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@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@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
Andrew Cooper Sept. 11, 2020, 4:10 p.m. UTC | #2
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.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@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
diff mbox series

Patch

diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 9dd1d59423..0fd95fe9fa 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -848,6 +848,7 @@  static int read_msr(unsigned int reg, uint64_t *val,
 {
     struct vcpu *curr = current;
     const struct domain *currd = curr->domain;
+    const struct cpuid_policy *cp = currd->arch.cpuid;
     bool vpmu_msr = false;
     int ret;
 
@@ -869,19 +870,19 @@  static int read_msr(unsigned int reg, uint64_t *val,
         return X86EMUL_OKAY;
 
     case MSR_FS_BASE:
-        if ( is_pv_32bit_domain(currd) )
+        if ( !cp->extd.lm )
             break;
         *val = read_fs_base();
         return X86EMUL_OKAY;
 
     case MSR_GS_BASE:
-        if ( is_pv_32bit_domain(currd) )
+        if ( !cp->extd.lm )
             break;
         *val = read_gs_base();
         return X86EMUL_OKAY;
 
     case MSR_SHADOW_GS_BASE:
-        if ( is_pv_32bit_domain(currd) )
+        if ( !cp->extd.lm )
             break;
         *val = curr->arch.pv.gs_base_user;
         return X86EMUL_OKAY;
@@ -975,6 +976,7 @@  static int write_msr(unsigned int reg, uint64_t val,
 {
     struct vcpu *curr = current;
     const struct domain *currd = curr->domain;
+    const struct cpuid_policy *cp = currd->arch.cpuid;
     bool vpmu_msr = false;
     int ret;
 
@@ -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 )
+        {
+            write_gs_shadow(val);
+            curr->arch.pv.gs_base_user = val;
+        }
+        else
+            ASSERT_UNREACHABLE();
         return X86EMUL_OKAY;
 
     case MSR_EFER: