| Message ID | 1456413312-24063-1-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25/02/2016 16:15, Lan Tianyu wrote: > x2apic feature is in the kvm_default_props and automatically added to all > CPU models when KVM is enabled. But userspace devices don't support x2apic > which can't be enabled without the in-kernel irqchip. It will trigger > warning of "host doesn't support requested feature: CPUID.01H:ECX.x2apic > [bit 21]" when kernel_irqchip is off. This patch is to fix it via removing > x2apic feature when kernel_irqchip is off. > > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > target-i386/cpu.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index c78f824..298fb62 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2125,6 +2125,10 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) > > /* Special cases not set in the X86CPUDefinition structs: */ > if (kvm_enabled()) { > + if (!kvm_irqchip_in_kernel()) { > + x86_cpu_change_kvm_default("x2apic", "off"); > + } > + > x86_cpu_apply_props(cpu, kvm_default_props); > } > > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
On Thu, Feb 25, 2016 at 11:15:12PM +0800, Lan Tianyu wrote: > x2apic feature is in the kvm_default_props and automatically added to all > CPU models when KVM is enabled. But userspace devices don't support x2apic > which can't be enabled without the in-kernel irqchip. It will trigger > warning of "host doesn't support requested feature: CPUID.01H:ECX.x2apic > [bit 21]" when kernel_irqchip is off. This patch is to fix it via removing > x2apic feature when kernel_irqchip is off. > > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > target-i386/cpu.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index c78f824..298fb62 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2125,6 +2125,10 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) > > /* Special cases not set in the X86CPUDefinition structs: */ > if (kvm_enabled()) { > + if (!kvm_irqchip_in_kernel()) { > + x86_cpu_change_kvm_default("x2apic", "off"); This should be NULL instead of "off". Otherwise, the warning will be disabled if using "-cpu ...,+x2apic". > + } > + This function runs multiple times (once for each VCPU being created). Should be harmless, though. Eventually we could move the whole kvm_default_props logic outside cpu.c, to a KVM-x86 accel class or to common PC initialization code. > x86_cpu_apply_props(cpu, kvm_default_props); > } > > -- > 1.9.3 >
On 2016?02?27? 03:54, Eduardo Habkost wrote: > On Thu, Feb 25, 2016 at 11:15:12PM +0800, Lan Tianyu wrote: >> x2apic feature is in the kvm_default_props and automatically added to all >> CPU models when KVM is enabled. But userspace devices don't support x2apic >> which can't be enabled without the in-kernel irqchip. It will trigger >> warning of "host doesn't support requested feature: CPUID.01H:ECX.x2apic >> [bit 21]" when kernel_irqchip is off. This patch is to fix it via removing >> x2apic feature when kernel_irqchip is off. >> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> --- >> target-i386/cpu.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> index c78f824..298fb62 100644 >> --- a/target-i386/cpu.c >> +++ b/target-i386/cpu.c >> @@ -2125,6 +2125,10 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) >> >> /* Special cases not set in the X86CPUDefinition structs: */ >> if (kvm_enabled()) { >> + if (!kvm_irqchip_in_kernel()) { >> + x86_cpu_change_kvm_default("x2apic", "off"); > > This should be NULL instead of "off". I tried "NULL" before. But some cpus modules(E,G SandyBridge, IvyBridge, haswell) already have x2apic feature in their default features of struct X86CPUDefinition, passing "NULL" is not to add x2apic feature to the cpu module and will not help to remove x2apic feature for these cpu modules. So I changed "NULL" to "off". > Otherwise, the warning will > be disabled if using "-cpu ...,+x2apic". > kvm_arch_get_supported_cpuid() always returns no x2apic support when kernel_irqchip is off and so it still triggers warning with "-cpu ...,+x2apic". #qemu-system-x86_64 -cpu qemu64,+x2apic -machine kernel-irqchip=off warning: TCG doesn't support requested feature: CPUID.01H:ECX.x2apic [bit 21] >> + } >> + > > This function runs multiple times (once for each VCPU being > created). Should be harmless, though. Eventually we could move > the whole kvm_default_props logic outside cpu.c, to a KVM-x86 > accel class or to common PC initialization code. Good suggestion and we can try that later. > > >> x86_cpu_apply_props(cpu, kvm_default_props); >> } >> >> -- >> 1.9.3 >> >
On Mon, Feb 29, 2016 at 10:56:04AM +0800, Lan Tianyu wrote: > On 2016?02?27? 03:54, Eduardo Habkost wrote: > > On Thu, Feb 25, 2016 at 11:15:12PM +0800, Lan Tianyu wrote: > >> x2apic feature is in the kvm_default_props and automatically added to all > >> CPU models when KVM is enabled. But userspace devices don't support x2apic > >> which can't be enabled without the in-kernel irqchip. It will trigger > >> warning of "host doesn't support requested feature: CPUID.01H:ECX.x2apic > >> [bit 21]" when kernel_irqchip is off. This patch is to fix it via removing > >> x2apic feature when kernel_irqchip is off. > >> > >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > >> --- > >> target-i386/cpu.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c > >> index c78f824..298fb62 100644 > >> --- a/target-i386/cpu.c > >> +++ b/target-i386/cpu.c > >> @@ -2125,6 +2125,10 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) > >> > >> /* Special cases not set in the X86CPUDefinition structs: */ > >> if (kvm_enabled()) { > >> + if (!kvm_irqchip_in_kernel()) { > >> + x86_cpu_change_kvm_default("x2apic", "off"); > > > > This should be NULL instead of "off". > > I tried "NULL" before. But some cpus modules(E,G SandyBridge, > IvyBridge, haswell) already have x2apic feature in their default > features of struct X86CPUDefinition, passing "NULL" is not to add x2apic > feature to the cpu module and will not help to remove x2apic feature for > these cpu modules. So I changed "NULL" to "off". In this case, I suggest we remove x2apic from these CPU models to avoid confusion, as the presence of the flag in the table makes no difference at all (this can be done in a separate patch). If we do that, NULL and "off" would have the same results, but NULL is clearer, IMO. NULL simply disables the KVM-specific hack (so we don't touch the property anymore), but "off" adds a new TCG-specific hack. I will submit that as a follow-up patch. Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > Otherwise, the warning will > > be disabled if using "-cpu ...,+x2apic". > > > > kvm_arch_get_supported_cpuid() always returns no x2apic support when > kernel_irqchip is off and so it still triggers warning with "-cpu > ...,+x2apic". > > #qemu-system-x86_64 -cpu qemu64,+x2apic -machine kernel-irqchip=off > warning: TCG doesn't support requested feature: CPUID.01H:ECX.x2apic > [bit 21] You are right. The +x2apic flag is applied after x86_cpu_load_def() runs. My mistake.
On 3/1/2016 10:00 PM, Eduardo Habkost wrote: > On Mon, Feb 29, 2016 at 10:56:04AM +0800, Lan Tianyu wrote: >> On 2016?02?27? 03:54, Eduardo Habkost wrote: >>> On Thu, Feb 25, 2016 at 11:15:12PM +0800, Lan Tianyu wrote: >>>> x2apic feature is in the kvm_default_props and automatically added to all >>>> CPU models when KVM is enabled. But userspace devices don't support x2apic >>>> which can't be enabled without the in-kernel irqchip. It will trigger >>>> warning of "host doesn't support requested feature: CPUID.01H:ECX.x2apic >>>> [bit 21]" when kernel_irqchip is off. This patch is to fix it via removing >>>> x2apic feature when kernel_irqchip is off. >>>> >>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >>>> --- >>>> target-i386/cpu.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >>>> index c78f824..298fb62 100644 >>>> --- a/target-i386/cpu.c >>>> +++ b/target-i386/cpu.c >>>> @@ -2125,6 +2125,10 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) >>>> >>>> /* Special cases not set in the X86CPUDefinition structs: */ >>>> if (kvm_enabled()) { >>>> + if (!kvm_irqchip_in_kernel()) { >>>> + x86_cpu_change_kvm_default("x2apic", "off"); >>> >>> This should be NULL instead of "off". >> >> I tried "NULL" before. But some cpus modules(E,G SandyBridge, >> IvyBridge, haswell) already have x2apic feature in their default >> features of struct X86CPUDefinition, passing "NULL" is not to add x2apic >> feature to the cpu module and will not help to remove x2apic feature for >> these cpu modules. So I changed "NULL" to "off". > > In this case, I suggest we remove x2apic from these CPU models to > avoid confusion, as the presence of the flag in the table makes > no difference at all (this can be done in a separate patch). > > If we do that, NULL and "off" would have the same results, but > NULL is clearer, IMO. NULL simply disables the KVM-specific hack > (so we don't touch the property anymore), but "off" adds a new > TCG-specific hack. I will submit that as a follow-up patch. Yes, that sounds reasonable. Thanks for your review. > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > >> >>> Otherwise, the warning will >>> be disabled if using "-cpu ...,+x2apic". >>> >> >> kvm_arch_get_supported_cpuid() always returns no x2apic support when >> kernel_irqchip is off and so it still triggers warning with "-cpu >> ...,+x2apic". >> >> #qemu-system-x86_64 -cpu qemu64,+x2apic -machine kernel-irqchip=off >> warning: TCG doesn't support requested feature: CPUID.01H:ECX.x2apic >> [bit 21] > > You are right. The +x2apic flag is applied after > x86_cpu_load_def() runs. My mistake. >
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index c78f824..298fb62 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2125,6 +2125,10 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) /* Special cases not set in the X86CPUDefinition structs: */ if (kvm_enabled()) { + if (!kvm_irqchip_in_kernel()) { + x86_cpu_change_kvm_default("x2apic", "off"); + } + x86_cpu_apply_props(cpu, kvm_default_props); }
x2apic feature is in the kvm_default_props and automatically added to all CPU models when KVM is enabled. But userspace devices don't support x2apic which can't be enabled without the in-kernel irqchip. It will trigger warning of "host doesn't support requested feature: CPUID.01H:ECX.x2apic [bit 21]" when kernel_irqchip is off. This patch is to fix it via removing x2apic feature when kernel_irqchip is off. Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- target-i386/cpu.c | 4 ++++ 1 file changed, 4 insertions(+)