| Message ID | 1648216709-44755-3-git-send-email-wanpengli@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: X86: Scaling Guest OS Critical Sections with boosting | expand |
On Fri, Mar 25, 2022, Wanpeng Li wrote: > From: Wanpeng Li <wanpengli@tencent.com> > > Let's get the information whether or not guests disable interruptions. This is missing critical information for _why_. It took me some staring to understand that this allows querying IRQs from a _different_ vCPU, which needs caching on VMX due to the need to do a VMREAD. > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > --- > arch/x86/include/asm/kvm_host.h | 1 + > arch/x86/kvm/x86.c | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 50f011a7445a..8e05cbfa9827 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -861,6 +861,7 @@ struct kvm_vcpu_arch { > bool preempt_count_enabled; > struct gfn_to_hva_cache preempt_count_cache; > } pv_pc; > + bool irq_disabled; This is going to at best be confusing, and at worst lead to bugs The flag is valid if and only if the vCPU is not loaded. I don't have a clever answer, but this needs to have some form of guard to (a) clarify when it's valid and (b) actively prevent misuse.
On Wed, 30 Mar 2022 at 08:04, Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Mar 25, 2022, Wanpeng Li wrote: > > From: Wanpeng Li <wanpengli@tencent.com> > > > > Let's get the information whether or not guests disable interruptions. > > This is missing critical information for _why_. It took me some staring to > understand that this allows querying IRQs from a _different_ vCPU, which needs > caching on VMX due to the need to do a VMREAD. Yes. > > > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > > --- > > arch/x86/include/asm/kvm_host.h | 1 + > > arch/x86/kvm/x86.c | 3 +++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index 50f011a7445a..8e05cbfa9827 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -861,6 +861,7 @@ struct kvm_vcpu_arch { > > bool preempt_count_enabled; > > struct gfn_to_hva_cache preempt_count_cache; > > } pv_pc; > > + bool irq_disabled; > > This is going to at best be confusing, and at worst lead to bugs The flag is > valid if and only if the vCPU is not loaded. I don't have a clever answer, but > this needs to have some form of guard to (a) clarify when it's valid and (b) actively > prevent misuse. How about renaming it to last_guest_irq_disabled and comments as /* Guest irq disabled state, valid iff the vCPU is not loaded */ Wanpeng
On Wed, Mar 30, 2022, Wanpeng Li wrote: > On Wed, 30 Mar 2022 at 08:04, Sean Christopherson <seanjc@google.com> wrote: > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > index 50f011a7445a..8e05cbfa9827 100644 > > > --- a/arch/x86/include/asm/kvm_host.h > > > +++ b/arch/x86/include/asm/kvm_host.h > > > @@ -861,6 +861,7 @@ struct kvm_vcpu_arch { > > > bool preempt_count_enabled; > > > struct gfn_to_hva_cache preempt_count_cache; > > > } pv_pc; > > > + bool irq_disabled; > > > > This is going to at best be confusing, and at worst lead to bugs The flag is > > valid if and only if the vCPU is not loaded. I don't have a clever answer, but > > this needs to have some form of guard to (a) clarify when it's valid and (b) actively > > prevent misuse. > > How about renaming it to last_guest_irq_disabled and comments as /* > Guest irq disabled state, valid iff the vCPU is not loaded */ What about usurping vcpu->run->if_flag? Userspace could manipulate the data, but that should be fine since the data is already guest-controlled.
On Fri, 1 Apr 2022 at 06:00, Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Mar 30, 2022, Wanpeng Li wrote: > > On Wed, 30 Mar 2022 at 08:04, Sean Christopherson <seanjc@google.com> wrote: > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > > index 50f011a7445a..8e05cbfa9827 100644 > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > @@ -861,6 +861,7 @@ struct kvm_vcpu_arch { > > > > bool preempt_count_enabled; > > > > struct gfn_to_hva_cache preempt_count_cache; > > > > } pv_pc; > > > > + bool irq_disabled; > > > > > > This is going to at best be confusing, and at worst lead to bugs The flag is > > > valid if and only if the vCPU is not loaded. I don't have a clever answer, but > > > this needs to have some form of guard to (a) clarify when it's valid and (b) actively > > > prevent misuse. > > > > How about renaming it to last_guest_irq_disabled and comments as /* > > Guest irq disabled state, valid iff the vCPU is not loaded */ > > What about usurping vcpu->run->if_flag? Userspace could manipulate the data, but > that should be fine since the data is already guest-controlled. We should at least update vcpu->run->if_flag during vcpu scheduled for the purpose of this patch, I think it looks strange for vcpu->run->if_flag. Wanpeng
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 50f011a7445a..8e05cbfa9827 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -861,6 +861,7 @@ struct kvm_vcpu_arch { bool preempt_count_enabled; struct gfn_to_hva_cache preempt_count_cache; } pv_pc; + bool irq_disabled; /* * Indicates the guest is trying to write a gfn that contains one or diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index af75e273cb32..425fd7f38fa9 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4576,6 +4576,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) static_call(kvm_x86_vcpu_load)(vcpu, cpu); + vcpu->arch.irq_disabled = false; /* Save host pkru register if supported */ vcpu->arch.host_pkru = read_pkru(); @@ -4668,6 +4669,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) static_call(kvm_x86_vcpu_put)(vcpu); vcpu->arch.last_host_tsc = rdtsc(); + vcpu->arch.irq_disabled = !static_call(kvm_x86_get_if_flag)(vcpu); } static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu, @@ -11225,6 +11227,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) vcpu->arch.pending_external_vector = -1; vcpu->arch.preempted_in_kernel = false; vcpu->arch.pv_pc.preempt_count_enabled = false; + vcpu->arch.irq_disabled = false; #if IS_ENABLED(CONFIG_HYPERV) vcpu->arch.hv_root_tdp = INVALID_PAGE;