| Message ID | 1458281388-14452-13-git-send-email-Suravee.Suthikulpanit@amd.com |
|---|---|
| State | New |
| Headers | show |
Hi Radim, On 03/19/2016 04:44 AM, Radim Krčmář wrote: > 2016-03-18 01:09-0500, Suravee Suthikulpanit: >> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> >> + >> + WRITE_ONCE(*entry, new_entry); > > This will translate to two writes in 32 bit mode and we need to write > physical ID first to avoid spurious doorbells ... > is the order guaranteed? Hm.. not sure on that. >> + } else { >> + new_entry = READ_ONCE(*entry); >> + /** >> + * This handles the case when vcpu is scheduled out >> + * and has not yet not called blocking. We save the >> + * AVIC running flag so that we can restore later. >> + */ > > is_running must be disabled in between ...blocking and ...unblocking, > because we don't want to miss interrupts and block forever. > I somehow don't get it from the comment. :) Not sure if I understand your concern. However, the is_running bit setting/clearing should be handled in the avic_set_running below. This part only handles othe case when the is_running bit still set when calling vcpu_put (and later on loading some other vcpus). This way, when we are re-loading this vcpu, we can restore the is_running bit accordingly. Thanks, Suravee
Hi Radim, On 3/31/16 21:19, Radim Krčmář wrote: > 2016-03-31 15:52+0700, Suravee Suthikulpanit: >> On 03/19/2016 04:44 AM, Radim Krčmář wrote: >>> 2016-03-18 01:09-0500, Suravee Suthikulpanit: >>>> + } else { >>>> + new_entry = READ_ONCE(*entry); >>>> + /** >>>> + * This handles the case when vcpu is scheduled out >>>> + * and has not yet not called blocking. We save the >>>> + * AVIC running flag so that we can restore later. >>>> + */ >>> >>> is_running must be disabled in between ...blocking and ...unblocking, >>> because we don't want to miss interrupts and block forever. >>> I somehow don't get it from the comment. :) >> >> Not sure if I understand your concern. However, the is_running bit >> setting/clearing should be handled in the avic_set_running below. This part >> only handles othe case when the is_running bit still set when calling >> vcpu_put (and later on loading some other vcpus). This way, when we are >> re-loading this vcpu, we can restore the is_running bit accordingly. > > I think that the comment is misleading. The saved is_running flag only > matters after svm_vcpu_blocking, yet the comment says that it handles > the irrelevant case before. Actually, my understanding is if the svm_vcpu_blocking() is called, the is_running bit would have been cleared. At this point, if the vcpu is unloaded. We should not need to worry about it. Is that not the case here? > Another minor bug is that was_running isn't initialized to 1, so we need > to halt before is_running gets set. Just to make sure, you are referring to the point where the is_running is not set for first time the vcpu is loaded? If so, I agree. Thanks for the good catch. > It would be clearer to toggle a flag in svm_vcpu_(un)blocking and set > is_running = !is_blocking. Not sure what you meant here. We are already setting/unsetting the is_running bit when vcpu is blocking/unblocking. Are you suggesting just simply move the current avic_set_running() into the svm_vcpu_blocking and svm_vcpu_unblocking()? > Doing so will also be immeasurably faster, > because avic_vcpu_load is called far more than svm_vcpu_(un)blocking. Actually, this is not the same as handling normal vcpu blocking and unblocking, which we are already setting/un-setting the is_running bit in the avic_set_running(). The was_running should only be set to 1 if the vcpu is unloaded but has not yet calling halt. Am I missing your points somehow? Thanks, Suravee
Radim, On 04/05/2016 09:56 PM, Radim Krčmář wrote: > I meant to change the place where we remember that is_running must not > be true. Something like > > svm_vcpu_blocking(struct kvm_vcpu *vcpu): > vcpu->is_blocking = true; > avic_set_running(vcpu, false); > > avic_vcpu_load(struct kvm_vcpu *vcpu, bool is_load): > avic_set_running(vcpu, is_load && !vcpu->is_blocking) I assume that you also imply that we would also need: svm_vcpu_unblocking(struct kvm_vcpu *vcpu) { avic_set_running(vcpu, false); vcpu->is_blocking = false; } Thanks, Suravee
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index d5418c3..c5e8100 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -35,6 +35,7 @@ #include <linux/trace_events.h> #include <linux/slab.h> +#include <asm/apic.h> #include <asm/perf_event.h> #include <asm/tlbflush.h> #include <asm/desc.h> @@ -1334,6 +1335,110 @@ free_avic: return err; } +static int avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu, bool is_load) +{ + int h_phy_apic_id; + u64 *entry, new_entry; + struct vcpu_svm *svm = to_svm(vcpu); + int ret = 0; + + if (!svm_vcpu_avic_enabled(svm)) + return 0; + + if (!svm) + return -EINVAL; + + /* Note: APIC ID = 0xff is used for broadcast. + * APIC ID > 0xff is reserved. + */ + h_phy_apic_id = __default_cpu_present_to_apicid(cpu); + + if (h_phy_apic_id >= AVIC_PHY_APIC_ID_MAX) + return -EINVAL; + + entry = svm->avic_phy_apic_id_cache; + if (!entry) + return -EINVAL; + + if (is_load) { + new_entry = READ_ONCE(*entry); + + BUG_ON(new_entry & AVIC_PHY_APIC_ID__IS_RUN_MSK); + + new_entry &= ~AVIC_PHY_APIC_ID__HOST_PHY_APIC_ID_MSK; + new_entry |= (h_phy_apic_id & AVIC_PHY_APIC_ID__HOST_PHY_APIC_ID_MSK); + + /** + * Restore AVIC running flag if it was set during + * vcpu unload. + */ + if (svm->avic_was_running) + new_entry |= AVIC_PHY_APIC_ID__IS_RUN_MSK; + else + new_entry &= ~AVIC_PHY_APIC_ID__IS_RUN_MSK; + + WRITE_ONCE(*entry, new_entry); + + } else { + new_entry = READ_ONCE(*entry); + + /** + * This handles the case when vcpu is scheduled out + * and has not yet not called blocking. We save the + * AVIC running flag so that we can restore later. + */ + if (new_entry & AVIC_PHY_APIC_ID__IS_RUN_MSK) { + svm->avic_was_running = true; + new_entry &= ~AVIC_PHY_APIC_ID__IS_RUN_MSK; + WRITE_ONCE(*entry, new_entry); + } else { + svm->avic_was_running = false; + } + } + + return ret; +} + +/** + * This function is called during VCPU halt/unhalt. + */ +static int avic_set_running(struct kvm_vcpu *vcpu, bool is_run) +{ + int ret = 0; + int h_phy_apic_id; + u64 *entry, new_entry; + struct vcpu_svm *svm = to_svm(vcpu); + + if (!svm_vcpu_avic_enabled(svm)) + return 0; + + /* Note: APIC ID = 0xff is used for broadcast. + * APIC ID > 0xff is reserved. + */ + h_phy_apic_id = __default_cpu_present_to_apicid(vcpu->cpu); + + if (h_phy_apic_id >= AVIC_PHY_APIC_ID_MAX) + return -EINVAL; + + entry = svm->avic_phy_apic_id_cache; + if (!entry) + return -EINVAL; + + if (is_run) { + /* Handle vcpu unblocking after HLT */ + new_entry = READ_ONCE(*entry); + new_entry |= AVIC_PHY_APIC_ID__IS_RUN_MSK; + WRITE_ONCE(*entry, new_entry); + } else { + /* Handle vcpu blocking due to HLT */ + new_entry = READ_ONCE(*entry); + new_entry &= ~AVIC_PHY_APIC_ID__IS_RUN_MSK; + WRITE_ONCE(*entry, new_entry); + } + + return ret; +} + static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) { struct vcpu_svm *svm = to_svm(vcpu); @@ -1476,6 +1581,8 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu) /* This assumes that the kernel never uses MSR_TSC_AUX */ if (static_cpu_has(X86_FEATURE_RDTSCP)) wrmsrl(MSR_TSC_AUX, svm->tsc_aux); + + avic_vcpu_load(vcpu, cpu, true); } static void svm_vcpu_put(struct kvm_vcpu *vcpu) @@ -1483,6 +1590,8 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu) struct vcpu_svm *svm = to_svm(vcpu); int i; + avic_vcpu_load(vcpu, 0, false); + ++vcpu->stat.host_state_reload; kvm_load_ldt(svm->host.ldt); #ifdef CONFIG_X86_64 @@ -1498,6 +1607,16 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu) wrmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]); } +static void svm_vcpu_blocking(struct kvm_vcpu *vcpu) +{ + avic_set_running(vcpu, false); +} + +static void svm_vcpu_unblocking(struct kvm_vcpu *vcpu) +{ + avic_set_running(vcpu, true); +} + static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu) { return to_svm(vcpu)->vmcb->save.rflags; @@ -4876,6 +4995,8 @@ static struct kvm_x86_ops svm_x86_ops = { .prepare_guest_switch = svm_prepare_guest_switch, .vcpu_load = svm_vcpu_load, .vcpu_put = svm_vcpu_put, + .vcpu_blocking = svm_vcpu_blocking, + .vcpu_unblocking = svm_vcpu_unblocking, .update_bp_intercept = update_bp_intercept, .get_msr = svm_get_msr,