[PATCH] x86/hvm: Improve hvm_set_guest_pat() code generation

[PATCH] x86/hvm: Improve hvm_set_guest_pat() code generation

[Andrew Cooper]

This is a fastpath on virtual vmentry/exit, and forcing guest_pat to be
spilled to the stack is bad.  Performing the shift in a register is far more
efficient.

Drop the (IMO useless) log message.  MSR_PAT only gets altered on boot, and a
bad value will be entirely evident in the ensuing #GP backtrace.

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>
---
 xen/arch/x86/hvm/hvm.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d233550ae47b..e3c9b3794544 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -299,13 +299,13 @@ void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat)
         *guest_pat = v->arch.hvm.pat_cr;
 }
 
-int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
+int hvm_set_guest_pat(struct vcpu *v, uint64_t guest_pat)
 {
-    int i;
-    uint8_t *value = (uint8_t *)&guest_pat;
+    unsigned int i;
+    uint64_t tmp;
 
-    for ( i = 0; i < 8; i++ )
-        switch ( value[i] )
+    for ( i = 0, tmp = guest_pat; i < 8; i++, tmp >>= 8 )
+        switch ( tmp & 0xff )
         {
         case PAT_TYPE_UC_MINUS:
         case PAT_TYPE_UNCACHABLE:
@@ -313,10 +313,9 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
         case PAT_TYPE_WRCOMB:
         case PAT_TYPE_WRPROT:
         case PAT_TYPE_WRTHROUGH:
-            break;
+            continue;
+
         default:
-            HVM_DBG_LOG(DBG_LEVEL_MSR, "invalid guest PAT: %"PRIx64"\n",
-                        guest_pat); 
             return 0;
         }
 
-- 
2.11.0

Re: [PATCH] x86/hvm: Improve hvm_set_guest_pat() code generation

[Jan Beulich]

On 13.01.2022 14:50, Andrew Cooper wrote:
> This is a fastpath on virtual vmentry/exit, and forcing guest_pat to be
> spilled to the stack is bad.  Performing the shift in a register is far more
> efficient.
> 
> Drop the (IMO useless) log message.  MSR_PAT only gets altered on boot, and a
> bad value will be entirely evident in the ensuing #GP backtrace.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

I'm curious though why ...

> @@ -313,10 +313,9 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
>          case PAT_TYPE_WRCOMB:
>          case PAT_TYPE_WRPROT:
>          case PAT_TYPE_WRTHROUGH:
> -            break;
> +            continue;

... you're going from "break" to "continue" here.

Jan

Re: [PATCH] x86/hvm: Improve hvm_set_guest_pat() code generation

[Andrew Cooper]

On 13/01/2022 14:38, Jan Beulich wrote:
> On 13.01.2022 14:50, Andrew Cooper wrote:
>> This is a fastpath on virtual vmentry/exit, and forcing guest_pat to be
>> spilled to the stack is bad.  Performing the shift in a register is far more
>> efficient.
>>
>> Drop the (IMO useless) log message.  MSR_PAT only gets altered on boot, and a
>> bad value will be entirely evident in the ensuing #GP backtrace.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> I'm curious though why ...
>
>> @@ -313,10 +313,9 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
>>          case PAT_TYPE_WRCOMB:
>>          case PAT_TYPE_WRPROT:
>>          case PAT_TYPE_WRTHROUGH:
>> -            break;
>> +            continue;
> ... you're going from "break" to "continue" here.

I went through a couple of iterations, including one not having a switch
statement at all.

Personally, I think continue is clearer to follow in constructs such as
this, because it is clearly bound to the loop, while the break logic
only works due to the switch being the final (only) clause.

~Andrew

P.S. if you want to see a hilarious Clang (mis)feature, check out
https://godbolt.org/z/7z6PnKP31 - scroll to the bottom of the -O2 output.

Re: [PATCH] x86/hvm: Improve hvm_set_guest_pat() code generation

[Jan Beulich]

On 13.01.2022 15:45, Andrew Cooper wrote:
> On 13/01/2022 14:38, Jan Beulich wrote:
>> On 13.01.2022 14:50, Andrew Cooper wrote:
>>> This is a fastpath on virtual vmentry/exit, and forcing guest_pat to be
>>> spilled to the stack is bad.  Performing the shift in a register is far more
>>> efficient.
>>>
>>> Drop the (IMO useless) log message.  MSR_PAT only gets altered on boot, and a
>>> bad value will be entirely evident in the ensuing #GP backtrace.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> I'm curious though why ...
>>
>>> @@ -313,10 +313,9 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
>>>          case PAT_TYPE_WRCOMB:
>>>          case PAT_TYPE_WRPROT:
>>>          case PAT_TYPE_WRTHROUGH:
>>> -            break;
>>> +            continue;
>> ... you're going from "break" to "continue" here.
> 
> I went through a couple of iterations, including one not having a switch
> statement at all.
> 
> Personally, I think continue is clearer to follow in constructs such as
> this, because it is clearly bound to the loop, while the break logic
> only works due to the switch being the final (only) clause.

Perhaps I was wrong recalling you somewhat disliking such uses of
"continue" in the past.

> P.S. if you want to see a hilarious Clang (mis)feature, check out
> https://godbolt.org/z/7z6PnKP31 - scroll to the bottom of the -O2 output.

"Nice".

Jan

Re: [PATCH] x86/hvm: Improve hvm_set_guest_pat() code generation

[Andrew Cooper]

On 13/01/2022 14:59, Jan Beulich wrote:
> On 13.01.2022 15:45, Andrew Cooper wrote:
>> On 13/01/2022 14:38, Jan Beulich wrote:
>>> On 13.01.2022 14:50, Andrew Cooper wrote:
>>>> This is a fastpath on virtual vmentry/exit, and forcing guest_pat to be
>>>> spilled to the stack is bad.  Performing the shift in a register is far more
>>>> efficient.
>>>>
>>>> Drop the (IMO useless) log message.  MSR_PAT only gets altered on boot, and a
>>>> bad value will be entirely evident in the ensuing #GP backtrace.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> I'm curious though why ...
>>>
>>>> @@ -313,10 +313,9 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
>>>>          case PAT_TYPE_WRCOMB:
>>>>          case PAT_TYPE_WRPROT:
>>>>          case PAT_TYPE_WRTHROUGH:
>>>> -            break;
>>>> +            continue;
>>> ... you're going from "break" to "continue" here.
>> I went through a couple of iterations, including one not having a switch
>> statement at all.
>>
>> Personally, I think continue is clearer to follow in constructs such as
>> this, because it is clearly bound to the loop, while the break logic
>> only works due to the switch being the final (only) clause.
> Perhaps I was wrong recalling you somewhat disliking such uses of
> "continue" in the past.

Perhaps stockholm syndrome?  More likely, my judgement is subjective
based on what else is in the for loop.

I'll leave it as break to shrink the patch.

~Andrew