Message ID | 20220113135035.23361-1-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/hvm: Improve hvm_set_guest_pat() code generation | expand |
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
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.
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
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
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; }
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(-)