| Message ID | 20230227084547.404871-3-robert.hu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Linear Address Masking (LAM) KVM Enabling | expand |
On Mon, Feb 27, 2023 at 04:45:44PM +0800, Robert Hoo wrote: >kvm_read_cr4_bits() returns ulong, explicitly cast it bool when assign to a >bool variable. > >Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> >--- > arch/x86/kvm/x86.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >index 312aea1854ae..b9611690561d 100644 >--- a/arch/x86/kvm/x86.c >+++ b/arch/x86/kvm/x86.c >@@ -1236,7 +1236,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > bool skip_tlb_flush = false; > unsigned long pcid = 0; > #ifdef CONFIG_X86_64 >- bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); >+ bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > > if (pcid_enabled) { > skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH; pcid_enabled is used only once. You can drop it, i.e., if (kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE)) { >-- >2.31.1 >
On Thu, 2023-03-02 at 15:24 +0800, Chao Gao wrote: > > - bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > > + bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > > > > if (pcid_enabled) { > > skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH; > > pcid_enabled is used only once. You can drop it, i.e., > > if (kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE)) { > Emm, that's actually another point. Though I won't object so, wouldn't this be compiler optimized? And my point was: honor bool type, though in C implemention it's 0 and !0, it has its own type value: true, false. Implicit type casting always isn't good habit.
As Chao pointed out, this does not belong in the LAM series. And FWIW, I highly recommend NOT tagging things as Trivial. If you're wrong and the patch _isn't_ trivial, it only slows things down. And if you're right, then expediting the patch can't possibly be necessary. On Fri, Mar 03, 2023, Robert Hoo wrote: > On Thu, 2023-03-02 at 15:24 +0800, Chao Gao wrote: > > > - bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > > > + bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > > > > > > if (pcid_enabled) { > > > skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH; > > > > pcid_enabled is used only once. You can drop it, i.e., > > > > if (kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE)) { > > > Emm, that's actually another point. > Though I won't object so, wouldn't this be compiler optimized? > > And my point was: honor bool type, though in C implemention it's 0 and > !0, it has its own type value: true, false. > Implicit type casting always isn't good habit. I don't disagree, but I also don't particularly want to "fix" one case while ignoring the many others, e.g. kvm_handle_invpcid() has the exact same "buggy" pattern. I would be supportive of a patch that adds helpers and then converts all of the relevant CR0/CR4 checks though... diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h index 4c91f626c058..6e3cb958afdd 100644 --- a/arch/x86/kvm/kvm_cache_regs.h +++ b/arch/x86/kvm/kvm_cache_regs.h @@ -157,6 +157,14 @@ static inline ulong kvm_read_cr0_bits(struct kvm_vcpu *vcpu, ulong mask) return vcpu->arch.cr0 & mask; } +static __always_inline bool kvm_is_cr0_bit_set(struct kvm_vcpu *vcpu, + unsigned long cr0_bit) +{ + BUILD_BUG_ON(!is_power_of_2(cr0_bit)); + + return !!kvm_read_cr0_bits(vcpu, cr0_bit); +} + static inline ulong kvm_read_cr0(struct kvm_vcpu *vcpu) { return kvm_read_cr0_bits(vcpu, ~0UL); @@ -178,6 +186,14 @@ static inline ulong kvm_read_cr3(struct kvm_vcpu *vcpu) return vcpu->arch.cr3; } +static __always_inline bool kvm_is_cr4_bit_set(struct kvm_vcpu *vcpu, + unsigned long cr4_bit) +{ + BUILD_BUG_ON(!is_power_of_2(cr4_bit)); + + return !!kvm_read_cr4_bits(vcpu, cr4_bit); +} +
On 3/11/2023 4:22 AM, Sean Christopherson wrote: > As Chao pointed out, this does not belong in the LAM series. And FWIW, I highly > recommend NOT tagging things as Trivial. If you're wrong and the patch _isn't_ > trivial, it only slows things down. And if you're right, then expediting the > patch can't possibly be necessary. > > On Fri, Mar 03, 2023, Robert Hoo wrote: >> On Thu, 2023-03-02 at 15:24 +0800, Chao Gao wrote: >>>> - bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); >>>> + bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); >>>> >>>> if (pcid_enabled) { >>>> skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH; >>> pcid_enabled is used only once. You can drop it, i.e., >>> >>> if (kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE)) { >>> >> Emm, that's actually another point. >> Though I won't object so, wouldn't this be compiler optimized? >> >> And my point was: honor bool type, though in C implemention it's 0 and >> !0, it has its own type value: true, false. >> Implicit type casting always isn't good habit. > I don't disagree, but I also don't particularly want to "fix" one case while > ignoring the many others, e.g. kvm_handle_invpcid() has the exact same "buggy" > pattern. > > I would be supportive of a patch that adds helpers and then converts all of the > relevant CR0/CR4 checks though... Hi Sean, I can cook a patch by your suggesion and sent out the patch seperately. > > diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h > index 4c91f626c058..6e3cb958afdd 100644 > --- a/arch/x86/kvm/kvm_cache_regs.h > +++ b/arch/x86/kvm/kvm_cache_regs.h > @@ -157,6 +157,14 @@ static inline ulong kvm_read_cr0_bits(struct kvm_vcpu *vcpu, ulong mask) > return vcpu->arch.cr0 & mask; > } > > +static __always_inline bool kvm_is_cr0_bit_set(struct kvm_vcpu *vcpu, > + unsigned long cr0_bit) > +{ > + BUILD_BUG_ON(!is_power_of_2(cr0_bit)); > + > + return !!kvm_read_cr0_bits(vcpu, cr0_bit); > +} > + > static inline ulong kvm_read_cr0(struct kvm_vcpu *vcpu) > { > return kvm_read_cr0_bits(vcpu, ~0UL); > @@ -178,6 +186,14 @@ static inline ulong kvm_read_cr3(struct kvm_vcpu *vcpu) > return vcpu->arch.cr3; > } > > +static __always_inline bool kvm_is_cr4_bit_set(struct kvm_vcpu *vcpu, > + unsigned long cr4_bit) > +{ > + BUILD_BUG_ON(!is_power_of_2(cr4_bit)); > + > + return !!kvm_read_cr4_bits(vcpu, cr4_bit); > +} > +
On 3/20/2023 8:05 PM, Binbin Wu wrote: > > On 3/11/2023 4:22 AM, Sean Christopherson wrote: >> As Chao pointed out, this does not belong in the LAM series. And >> FWIW, I highly >> recommend NOT tagging things as Trivial. If you're wrong and the >> patch _isn't_ >> trivial, it only slows things down. And if you're right, then >> expediting the >> patch can't possibly be necessary. >> >> On Fri, Mar 03, 2023, Robert Hoo wrote: >>> On Thu, 2023-03-02 at 15:24 +0800, Chao Gao wrote: >>>>> - bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); >>>>> + bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); >>>>> >>>>> if (pcid_enabled) { >>>>> skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH; >>>> pcid_enabled is used only once. You can drop it, i.e., >>>> >>>> if (kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE)) { >>>> >>> Emm, that's actually another point. >>> Though I won't object so, wouldn't this be compiler optimized? >>> >>> And my point was: honor bool type, though in C implemention it's 0 and >>> !0, it has its own type value: true, false. >>> Implicit type casting always isn't good habit. >> I don't disagree, but I also don't particularly want to "fix" one >> case while >> ignoring the many others, e.g. kvm_handle_invpcid() has the exact >> same "buggy" >> pattern. >> >> I would be supportive of a patch that adds helpers and then converts >> all of the >> relevant CR0/CR4 checks though... > > Hi Sean, I can cook a patch by your suggesion and sent out the patch > seperately. Sean, besides the call of kvm_read_cr0_bits() and kvm_read_cr4_bits(), there are also a lot checks in if statement like if ( cr4 & X86_CR4_XXX ) or if ( cr0 & X86_CR0_XXX ) I suppose these usages are OK, right? > > >> >> diff --git a/arch/x86/kvm/kvm_cache_regs.h >> b/arch/x86/kvm/kvm_cache_regs.h >> index 4c91f626c058..6e3cb958afdd 100644 >> --- a/arch/x86/kvm/kvm_cache_regs.h >> +++ b/arch/x86/kvm/kvm_cache_regs.h >> @@ -157,6 +157,14 @@ static inline ulong kvm_read_cr0_bits(struct >> kvm_vcpu *vcpu, ulong mask) >> return vcpu->arch.cr0 & mask; >> } >> +static __always_inline bool kvm_is_cr0_bit_set(struct kvm_vcpu *vcpu, >> + unsigned long cr0_bit) >> +{ >> + BUILD_BUG_ON(!is_power_of_2(cr0_bit)); >> + >> + return !!kvm_read_cr0_bits(vcpu, cr0_bit); >> +} >> + >> static inline ulong kvm_read_cr0(struct kvm_vcpu *vcpu) >> { >> return kvm_read_cr0_bits(vcpu, ~0UL); >> @@ -178,6 +186,14 @@ static inline ulong kvm_read_cr3(struct kvm_vcpu >> *vcpu) >> return vcpu->arch.cr3; >> } >> +static __always_inline bool kvm_is_cr4_bit_set(struct kvm_vcpu *vcpu, >> + unsigned long cr4_bit) >> +{ >> + BUILD_BUG_ON(!is_power_of_2(cr4_bit)); >> + >> + return !!kvm_read_cr4_bits(vcpu, cr4_bit); >> +} >> +
On Mon, Mar 20, 2023, Binbin Wu wrote: > > On 3/20/2023 8:05 PM, Binbin Wu wrote: > > > > On 3/11/2023 4:22 AM, Sean Christopherson wrote: > > > As Chao pointed out, this does not belong in the LAM series.� And > > > FWIW, I highly > > > recommend NOT tagging things as Trivial.� If you're wrong and the > > > patch _isn't_ > > > trivial, it only slows things down.� And if you're right, then > > > expediting the > > > patch can't possibly be necessary. > > > > > > On Fri, Mar 03, 2023, Robert Hoo wrote: > > > > On Thu, 2023-03-02 at 15:24 +0800, Chao Gao wrote: > > > > > > -��� bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > > > > > > +��� bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > > > > > > > > > > > > ����if (pcid_enabled) { > > > > > > ������� skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH; > > > > > pcid_enabled is used only once. You can drop it, i.e., > > > > > > > > > > ����if (kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE)) { > > > > > > > > > Emm, that's actually another point. > > > > Though I won't object so, wouldn't this be compiler optimized? > > > > > > > > And my point was: honor bool type, though in C implemention it's 0 and > > > > !0, it has its own type value: true, false. > > > > Implicit type casting always isn't good habit. > > > I don't disagree, but I also don't particularly want to "fix" one > > > case while > > > ignoring the many others, e.g. kvm_handle_invpcid() has the exact > > > same "buggy" > > > pattern. > > > > > > I would be supportive of a patch that adds helpers and then converts > > > all of the > > > relevant CR0/CR4 checks though... > > > > Hi Sean, I can cook a patch by your suggesion and sent out the patch > > seperately. > > Sean, besides the call of kvm_read_cr0_bits() and kvm_read_cr4_bits(), there > are also a lot checks in if statement like > if ( cr4 & X86_CR4_XXX ) > or > if ( cr0 & X86_CR0_XXX ) > I suppose these usages are OK, right? Generally speaking, yes. Most flows of that nature use a local variable for very good reasons. The only one I would probably convert is this code in svm_can_emulate_instruction(). cr4 = kvm_read_cr4(vcpu); smep = cr4 & X86_CR4_SMEP; smap = cr4 & X86_CR4_SMAP;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 312aea1854ae..b9611690561d 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1236,7 +1236,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) bool skip_tlb_flush = false; unsigned long pcid = 0; #ifdef CONFIG_X86_64 - bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); + bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); if (pcid_enabled) { skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH;
kvm_read_cr4_bits() returns ulong, explicitly cast it bool when assign to a bool variable. Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> --- arch/x86/kvm/x86.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)