| Message ID | 20211009021236.4122790-28-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Halt-polling and x86 APICv overhaul | expand |
On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > Hoist the CPU => APIC ID conversion for the Posted Interrupt descriptor > out of the loop to write the descriptor, preemption is disabled so the > CPU won't change, and if the APIC ID changes KVM has bigger problems. > > No functional change intended. Is preemption always disabled in vmx_vcpu_pi_load? vmx_vcpu_pi_load is called from vmx_vcpu_load, which is called indirectly from vcpu_load which is called from many ioctls, which userspace does. In these places I don't think that preemption is disabled. Best regards, Maxim Levitsky > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/vmx/posted_intr.c | 25 +++++++++++-------------- > 1 file changed, 11 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c > index fea343dcc011..2b2206339174 100644 > --- a/arch/x86/kvm/vmx/posted_intr.c > +++ b/arch/x86/kvm/vmx/posted_intr.c > @@ -51,17 +51,15 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu) > goto after_clear_sn; > } > > - /* The full case. */ > + /* The full case. Set the new destination and clear SN. */ > + dest = cpu_physical_id(cpu); > + if (!x2apic_mode) > + dest = (dest << 8) & 0xFF00; > + > do { > old.control = new.control = READ_ONCE(pi_desc->control); > > - dest = cpu_physical_id(cpu); > - > - if (x2apic_mode) > - new.ndst = dest; > - else > - new.ndst = (dest << 8) & 0xFF00; > - > + new.ndst = dest; > new.sn = 0; > } while (cmpxchg64(&pi_desc->control, old.control, > new.control) != old.control); > @@ -103,15 +101,14 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) > WARN(pi_desc->nv != POSTED_INTR_WAKEUP_VECTOR, > "Wakeup handler not enabled while the vCPU was blocking"); > > + dest = cpu_physical_id(vcpu->cpu); > + if (!x2apic_mode) > + dest = (dest << 8) & 0xFF00; > + > do { > old.control = new.control = READ_ONCE(pi_desc->control); > > - dest = cpu_physical_id(vcpu->cpu); > - > - if (x2apic_mode) > - new.ndst = dest; > - else > - new.ndst = (dest << 8) & 0xFF00; > + new.ndst = dest; > > /* set 'NV' to 'notification vector' */ > new.nv = POSTED_INTR_VECTOR;
On Thu, 2021-10-28 at 14:28 +0300, Maxim Levitsky wrote: > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > Hoist the CPU => APIC ID conversion for the Posted Interrupt descriptor > > out of the loop to write the descriptor, preemption is disabled so the > > CPU won't change, and if the APIC ID changes KVM has bigger problems. > > > > No functional change intended. > > Is preemption always disabled in vmx_vcpu_pi_load? vmx_vcpu_pi_load is called from vmx_vcpu_load, > which is called indirectly from vcpu_load which is called from many ioctls, > which userspace does. In these places I don't think that preemption is disabled. You can disregard this, I missed the fact that we have 'int cpu = get_cpu();' which disables preemption in 'vcpu_load' Thus, Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky > > Best regards, > Maxim Levitsky > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/kvm/vmx/posted_intr.c | 25 +++++++++++-------------- > > 1 file changed, 11 insertions(+), 14 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c > > index fea343dcc011..2b2206339174 100644 > > --- a/arch/x86/kvm/vmx/posted_intr.c > > +++ b/arch/x86/kvm/vmx/posted_intr.c > > @@ -51,17 +51,15 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu) > > goto after_clear_sn; > > } > > > > - /* The full case. */ > > + /* The full case. Set the new destination and clear SN. */ > > + dest = cpu_physical_id(cpu); > > + if (!x2apic_mode) > > + dest = (dest << 8) & 0xFF00; > > + > > do { > > old.control = new.control = READ_ONCE(pi_desc->control); > > > > - dest = cpu_physical_id(cpu); > > - > > - if (x2apic_mode) > > - new.ndst = dest; > > - else > > - new.ndst = (dest << 8) & 0xFF00; > > - > > + new.ndst = dest; > > new.sn = 0; > > } while (cmpxchg64(&pi_desc->control, old.control, > > new.control) != old.control); > > @@ -103,15 +101,14 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) > > WARN(pi_desc->nv != POSTED_INTR_WAKEUP_VECTOR, > > "Wakeup handler not enabled while the vCPU was blocking"); > > > > + dest = cpu_physical_id(vcpu->cpu); > > + if (!x2apic_mode) > > + dest = (dest << 8) & 0xFF00; > > + > > do { > > old.control = new.control = READ_ONCE(pi_desc->control); > > > > - dest = cpu_physical_id(vcpu->cpu); > > - > > - if (x2apic_mode) > > - new.ndst = dest; > > - else > > - new.ndst = (dest << 8) & 0xFF00; > > + new.ndst = dest; > > > > /* set 'NV' to 'notification vector' */ > > new.nv = POSTED_INTR_VECTOR;
On Thu, Oct 28, 2021, Maxim Levitsky wrote: > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > Hoist the CPU => APIC ID conversion for the Posted Interrupt descriptor > > out of the loop to write the descriptor, preemption is disabled so the > > CPU won't change, and if the APIC ID changes KVM has bigger problems. > > > > No functional change intended. > > Is preemption always disabled in vmx_vcpu_pi_load? vmx_vcpu_pi_load is called > from vmx_vcpu_load, which is called indirectly from vcpu_load which is called > from many ioctls, which userspace does. In these places I don't think that > preemption is disabled. Preemption is disabled in vcpu_load() by the get_cpu(). The "cpu" param that's passed around the vcpu_load() stack is also why I think it's ok to _not_ assert that preemption is disabled in vmx_vcpu_pi_load(); if preemption is enabled, "cpu" is unstable and thus the entire "load" operation is busted. #define get_cpu() ({ preempt_disable(); __smp_processor_id(); }) #define put_cpu() preempt_enable() void vcpu_load(struct kvm_vcpu *vcpu) { int cpu = get_cpu(); __this_cpu_write(kvm_running_vcpu, vcpu); preempt_notifier_register(&vcpu->preempt_notifier); kvm_arch_vcpu_load(vcpu, cpu); put_cpu(); } EXPORT_SYMBOL_GPL(vcpu_load);
On Thu, 2021-10-28 at 16:12 +0000, Sean Christopherson wrote: > On Thu, Oct 28, 2021, Maxim Levitsky wrote: > > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > > Hoist the CPU => APIC ID conversion for the Posted Interrupt descriptor > > > out of the loop to write the descriptor, preemption is disabled so the > > > CPU won't change, and if the APIC ID changes KVM has bigger problems. > > > > > > No functional change intended. > > > > Is preemption always disabled in vmx_vcpu_pi_load? vmx_vcpu_pi_load is called > > from vmx_vcpu_load, which is called indirectly from vcpu_load which is called > > from many ioctls, which userspace does. In these places I don't think that > > preemption is disabled. > > Preemption is disabled in vcpu_load() by the get_cpu(). The "cpu" param that's > passed around the vcpu_load() stack is also why I think it's ok to _not_ assert > that preemption is disabled in vmx_vcpu_pi_load(); if preemption is enabled, > "cpu" is unstable and thus the entire "load" operation is busted. Yes, I even knew about the get_cpu() behavier which indeed has to disable preemption. But I didn't notice call to it, when I wrote this mail! Later I did notice it but it was too late. Sometimes sending all the review mails at once at the end does make sense after all, I guess. Best regards, Maxim Levitsky > > > #define get_cpu() ({ preempt_disable(); __smp_processor_id(); }) > #define put_cpu() preempt_enable() > > > void vcpu_load(struct kvm_vcpu *vcpu) > { > int cpu = get_cpu(); > > __this_cpu_write(kvm_running_vcpu, vcpu); > preempt_notifier_register(&vcpu->preempt_notifier); > kvm_arch_vcpu_load(vcpu, cpu); > put_cpu(); > } > EXPORT_SYMBOL_GPL(vcpu_load); >
diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c index fea343dcc011..2b2206339174 100644 --- a/arch/x86/kvm/vmx/posted_intr.c +++ b/arch/x86/kvm/vmx/posted_intr.c @@ -51,17 +51,15 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu) goto after_clear_sn; } - /* The full case. */ + /* The full case. Set the new destination and clear SN. */ + dest = cpu_physical_id(cpu); + if (!x2apic_mode) + dest = (dest << 8) & 0xFF00; + do { old.control = new.control = READ_ONCE(pi_desc->control); - dest = cpu_physical_id(cpu); - - if (x2apic_mode) - new.ndst = dest; - else - new.ndst = (dest << 8) & 0xFF00; - + new.ndst = dest; new.sn = 0; } while (cmpxchg64(&pi_desc->control, old.control, new.control) != old.control); @@ -103,15 +101,14 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) WARN(pi_desc->nv != POSTED_INTR_WAKEUP_VECTOR, "Wakeup handler not enabled while the vCPU was blocking"); + dest = cpu_physical_id(vcpu->cpu); + if (!x2apic_mode) + dest = (dest << 8) & 0xFF00; + do { old.control = new.control = READ_ONCE(pi_desc->control); - dest = cpu_physical_id(vcpu->cpu); - - if (x2apic_mode) - new.ndst = dest; - else - new.ndst = (dest << 8) & 0xFF00; + new.ndst = dest; /* set 'NV' to 'notification vector' */ new.nv = POSTED_INTR_VECTOR;
Hoist the CPU => APIC ID conversion for the Posted Interrupt descriptor out of the loop to write the descriptor, preemption is disabled so the CPU won't change, and if the APIC ID changes KVM has bigger problems. No functional change intended. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/vmx/posted_intr.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)