| Message ID | 4712D8F4B26E034E80552F30A67BE0B1A4E842@ORSMSX112.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 05, 2017 at 12:52:25AM +0000, Xu, Anthony wrote: > In KVM mode, enable kvmvapic only when host doesn't support > VAPIC capability. > > Save the time to set up kvmvapic in some hosts. > > > Signed-off -by: Anthony Xu <anthony.xu@intel.com> > s/Signed-off -by/Signed-off-by nit: I think we do prefer a single line summary for the commit message, a detailed description of the patch and only one blank line to separe the Signed-off-by tag [0]. > > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c > index c3829e3..d5c53af 100644 > --- a/hw/intc/apic_common.c > +++ b/hw/intc/apic_common.c > @@ -317,8 +317,11 @@ static void apic_common_realize(DeviceState *dev, Error **errp) > info = APIC_COMMON_GET_CLASS(s); > info->realize(dev, errp); > > - /* Note: We need at least 1M to map the VAPIC option ROM */ > + /* Note: We need at least 1M to map the VAPIC option ROM, > + if it is KVM, enable kvmvapic only when KVM doesn't have > + VAPIC capability */ > if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK && > + (!kvm_enabled() || (kvm_enabled() && !kvm_has_vapic())) && > !hax_enabled() && ram_size >= 1024 * 1024) { > vapic = sysbus_create_simple("kvmvapic", -1, NULL); > } > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index 24281fc..43e0e4c 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -215,6 +215,7 @@ extern KVMState *kvm_state; > > bool kvm_has_free_slot(MachineState *ms); > int kvm_has_sync_mmu(void); > +int kvm_has_vapic(void); > int kvm_has_vcpu_events(void); > int kvm_has_robust_singlestep(void); > int kvm_has_debugregs(void); > diff --git a/kvm-all.c b/kvm-all.c > index 90b8573..edcb6ea 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -2232,6 +2232,10 @@ int kvm_has_sync_mmu(void) > return kvm_check_extension(kvm_state, KVM_CAP_SYNC_MMU); > } > > +int kvm_has_vapic(void){ > + return !kvm_check_extension(kvm_state, KVM_CAP_VAPIC); > +} > + Is that function shouldn't return true if KVM is providing VAPIC capability? nit: I think you need to mark a return to the line before opening brace for a function [1]. [0] http://wiki.qemu-project.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message [1] http://git.qemu-project.org/?p=qemu.git;a=blob_plain;f=CODING_STYLE;hb=HEAD > int kvm_has_vcpu_events(void) > { > return kvm_state->vcpu_events;
> > --- a/kvm-all.c > > +++ b/kvm-all.c > > @@ -2232,6 +2232,10 @@ int kvm_has_sync_mmu(void) > > return kvm_check_extension(kvm_state, KVM_CAP_SYNC_MMU); > > } > > > > +int kvm_has_vapic(void){ > > + return !kvm_check_extension(kvm_state, KVM_CAP_VAPIC); > > +} > > + > > Is that function shouldn't return true if KVM is providing VAPIC > capability? > It should, but it doesn't. see below KVM kernel code segment, it returns false if KVM provides VAPIC. Thanks for your comments, Will resend the patch. Anthony arch/x86/kvm/x86.c case KVM_CAP_VAPIC: r = !kvm_x86_ops->cpu_has_accelerated_tpr(); break;
On Thu, Apr 06, 2017 at 06:18:33PM +0000, Xu, Anthony wrote: > > > --- a/kvm-all.c > > > +++ b/kvm-all.c > > > @@ -2232,6 +2232,10 @@ int kvm_has_sync_mmu(void) > > > return kvm_check_extension(kvm_state, KVM_CAP_SYNC_MMU); > > > } > > > > > > +int kvm_has_vapic(void){ > > > + return !kvm_check_extension(kvm_state, KVM_CAP_VAPIC); > > > +} > > > + > > > > Is that function shouldn't return true if KVM is providing VAPIC > > capability? > > > > It should, but it doesn't. see below KVM kernel code segment, > it returns false if KVM provides VAPIC. I think we shouldn't read it like that. It seems that KVM is always returning the VAPIC capability except when the CPU is providing a special acceleration [0]. I would say you can't really refer yourself at this bit to enable or not kvmapic in QEMU. Does that make sense? [0] https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?id=774ead3ad9bcbc05ef6aaebb9bdf8b4c3126923b > Thanks for your comments, > > Will resend the patch. > > > Anthony > > > > arch/x86/kvm/x86.c > > case KVM_CAP_VAPIC: > r = !kvm_x86_ops->cpu_has_accelerated_tpr(); > break; >
> I think we shouldn't read it like that. It seems that KVM is always > returning the VAPIC capability except when the CPU is providing a > special acceleration [0]. > > I would say you can't really refer yourself at this bit to enable or > not kvmapic in QEMU. > > Does that make sense? > > [0] > https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?id=774ead3ad9bc > bc05ef6aaebb9bdf8b4c3126923b From this patch and commit message, "Disable vapic support on Intel machines with FlexPriority" If the CPU has accelerated tpr, vapic should be disabled. The function pointer has the name cpu_has_accelerated_tpr. kvmvapic was created for cpu which doesn't have accelerated tpr, https://lists.nongnu.org/archive/html/qemu-devel/2012-02/msg00515.html from the commit message, I can infer VAPIC means kvmvapic. My understanding is they are referring to the same thing, though the return value of KVM_CAP_VAPIC is confusing. Didn't find explanation about the return value of KVM_CAP_VAPIC, We can interpret it as following from QEMU perspective, If KVM_CAP_VAPIC return 1, QEMU should enable VAPIC(kvmvapic). If KVM_CAP_VAPIC return 0, QEMU should disable VAPIC(kvmvapic). Anthony
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c index c3829e3..d5c53af 100644 --- a/hw/intc/apic_common.c +++ b/hw/intc/apic_common.c @@ -317,8 +317,11 @@ static void apic_common_realize(DeviceState *dev, Error **errp) info = APIC_COMMON_GET_CLASS(s); info->realize(dev, errp); - /* Note: We need at least 1M to map the VAPIC option ROM */ + /* Note: We need at least 1M to map the VAPIC option ROM, + if it is KVM, enable kvmvapic only when KVM doesn't have + VAPIC capability */ if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK && + (!kvm_enabled() || (kvm_enabled() && !kvm_has_vapic())) && !hax_enabled() && ram_size >= 1024 * 1024) { vapic = sysbus_create_simple("kvmvapic", -1, NULL); } diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index 24281fc..43e0e4c 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -215,6 +215,7 @@ extern KVMState *kvm_state; bool kvm_has_free_slot(MachineState *ms); int kvm_has_sync_mmu(void); +int kvm_has_vapic(void); int kvm_has_vcpu_events(void); int kvm_has_robust_singlestep(void); int kvm_has_debugregs(void); diff --git a/kvm-all.c b/kvm-all.c index 90b8573..edcb6ea 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -2232,6 +2232,10 @@ int kvm_has_sync_mmu(void) return kvm_check_extension(kvm_state, KVM_CAP_SYNC_MMU); } +int kvm_has_vapic(void){ + return !kvm_check_extension(kvm_state, KVM_CAP_VAPIC); +} + int kvm_has_vcpu_events(void) { return kvm_state->vcpu_events;