Re: [PATCH] Qemu/KVM: Remove x2apic feature from CPU model when kernel_irqchip is off

["Lan, Tianyu" <tianyu.lan@intel.com>]

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.
>