Patchwork [3/7] cpu model bug fixes and definition corrections: Add kvm emulated x2apic flag to config defined cpu models

login
register
mail settings
Submitter Jan Kiszka
Date May 28, 2011, 8:39 a.m.
Message ID <4DE0B4B1.1000407@web.de>
Download mbox | patch
Permalink /patch/97774/
State New
Headers show

Comments

Jan Kiszka - May 28, 2011, 8:39 a.m.
On 2011-05-23 23:46, john cooper wrote:
> Add kvm emulated x2apic flag to config defined cpu models
> and general support for such hypervisor emulated flags.
> 
> In addition to checking user request flags against the host
> we also selectively check against kvm for emulated flags.

As you are already digging through this jungle, could you also check how
much of this diff from qemu-kvm is obsolete now?

     case 0x80000003:

If something is still missing in upstream, it should be ported - except
for kvm_nested which should be controllable via '-cpu ...,+/-svm'. TIA!

Jörg, how to deal with -enable-nesting in qemu-kvm to align behavior
with upstream?

Jan
Joerg Roedel - May 30, 2011, 8:18 a.m.
On Sat, May 28, 2011 at 04:39:13AM -0400, Jan Kiszka wrote:

> Jörg, how to deal with -enable-nesting in qemu-kvm to align behavior
> with upstream?

My personal preference is to just remove it. In upstream-qemu it is
enabled/disabled by +/-svm. -enable-nesting is just a historic thing
which can be wiped out.

	Joerg
Jan Kiszka - May 30, 2011, 2:04 p.m.
On 2011-05-30 10:18, Roedel, Joerg wrote:
> On Sat, May 28, 2011 at 04:39:13AM -0400, Jan Kiszka wrote:
> 
>> Jörg, how to deal with -enable-nesting in qemu-kvm to align behavior
>> with upstream?
> 
> My personal preference is to just remove it. In upstream-qemu it is
> enabled/disabled by +/-svm. -enable-nesting is just a historic thing
> which can be wiped out.

Does any user land depend on it? I guess we have to flag it deprecated
first, trigger some console output when it's specified, and then remove
it one or more releases later (the same applies to many other
qemu-kvm-specific switches).

Jan
Jan Kiszka - May 30, 2011, 3:15 p.m.
On 2011-05-30 17:10, Roedel, Joerg wrote:
> On Mon, May 30, 2011 at 11:04:02AM -0400, Jan Kiszka wrote:
>> On 2011-05-30 16:38, Nadav Har'El wrote:
>>> On Mon, May 30, 2011, Jan Kiszka wrote about "drop -enable-nesting (was: [PATCH 3/7] cpu model bug fixes and definition corrections...)":
>>>> On 2011-05-30 10:18, Roedel, Joerg wrote:
>>>>> On Sat, May 28, 2011 at 04:39:13AM -0400, Jan Kiszka wrote:
>>>>>
>>>>>> J�rg, how to deal with -enable-nesting in qemu-kvm to align behavior
>>>>>> with upstream?
>>>>>
>>>>> My personal preference is to just remove it. In upstream-qemu it is
>>>>> enabled/disabled by +/-svm. -enable-nesting is just a historic thing
>>>>> which can be wiped out.
>>>
>>> "-enable-nesting" could remain as a synonym for enabling either VMX or SVM
>>> in the guest, depending on what was available in the host (because KVM now
>>> supports both nested SVM and nested VMX, but not SVM-on-VMX or vice versa).
>>
>> Why? Once nesting is stable (I think SVM already is), there is no reason
>> for an explicit enable. And you can always mask it out via -cpu.
>>
>> BTW, what are the defaults for SVM right now in qemu-kvm and upstream?
>> Enable if the modeled CPU supports it?
> 
> qemu-kvm still needs -enable-nesting, otherwise it is disabled. Upstream
> qemu should enable it unconditionally (can be disabled with -cpu ,-svm).

Then let's start with aligning qemu-kvm defaults to upstream? I guess
that's what the diff I was citing yesterday is responsible for.

In the same run, -enable-nesting could dump a warning on the console
that this switch is obsolete and will be removed from future versions.

For VMX, I would suggest to keep it off by default until it matured,
asking the user to issue -cpu ...,+vmx.

Jan
Nadav Har'El - May 30, 2011, 3:16 p.m.
On Mon, May 30, 2011, Jan Kiszka wrote about "Re: drop -enable-nesting":
> > "-enable-nesting" could remain as a synonym for enabling either VMX or SVM
> > in the guest, depending on what was available in the host (because KVM now
> > supports both nested SVM and nested VMX, but not SVM-on-VMX or vice versa).
> 
> Why? Once nesting is stable (I think SVM already is), there is no reason
> for an explicit enable. And you can always mask it out via -cpu.

As far as I understand (and this was previously discussed on the QEMU mailing
list), the default emulated CPU does not include the "vmx" capability, and you
need to enable it with something like "-cpu qemu64,+vmx" (or "-cpu host").

I am not sure if it does enable the "svm" capability. If it does, it isn't
useful when KVM is enabled and the underlying host has VMX, not SVM.

Nadav.
Avi Kivity - May 30, 2011, 3:19 p.m.
On 05/30/2011 06:15 PM, Jan Kiszka wrote:
> On 2011-05-30 17:10, Roedel, Joerg wrote:
> >  On Mon, May 30, 2011 at 11:04:02AM -0400, Jan Kiszka wrote:
> >>  On 2011-05-30 16:38, Nadav Har'El wrote:
> >>>  On Mon, May 30, 2011, Jan Kiszka wrote about "drop -enable-nesting (was: [PATCH 3/7] cpu model bug fixes and definition corrections...)":
> >>>>  On 2011-05-30 10:18, Roedel, Joerg wrote:
> >>>>>  On Sat, May 28, 2011 at 04:39:13AM -0400, Jan Kiszka wrote:
> >>>>>
> >>>>>>  J�rg, how to deal with -enable-nesting in qemu-kvm to align behavior
> >>>>>>  with upstream?
> >>>>>
> >>>>>  My personal preference is to just remove it. In upstream-qemu it is
> >>>>>  enabled/disabled by +/-svm. -enable-nesting is just a historic thing
> >>>>>  which can be wiped out.
> >>>
> >>>  "-enable-nesting" could remain as a synonym for enabling either VMX or SVM
> >>>  in the guest, depending on what was available in the host (because KVM now
> >>>  supports both nested SVM and nested VMX, but not SVM-on-VMX or vice versa).
> >>
> >>  Why? Once nesting is stable (I think SVM already is), there is no reason
> >>  for an explicit enable. And you can always mask it out via -cpu.
> >>
> >>  BTW, what are the defaults for SVM right now in qemu-kvm and upstream?
> >>  Enable if the modeled CPU supports it?
> >
> >  qemu-kvm still needs -enable-nesting, otherwise it is disabled. Upstream
> >  qemu should enable it unconditionally (can be disabled with -cpu ,-svm).
>
> Then let's start with aligning qemu-kvm defaults to upstream? I guess
> that's what the diff I was citing yesterday is responsible for.
>
> In the same run, -enable-nesting could dump a warning on the console
> that this switch is obsolete and will be removed from future versions.

I think it's safe to drop -enable-nesting immediately.  Dan, does 
libvirt make use of it?

> For VMX, I would suggest to keep it off by default until it matured,
> asking the user to issue -cpu ...,+vmx.

We should do that for svm as well (except for -cpu host or -cpu 
something-with-svm).  vmx will be kept disabled by the module option, 
until it is deemed fit for general consumption.
Jan Kiszka - May 30, 2011, 3:27 p.m.
On 2011-05-30 17:19, Avi Kivity wrote:
> On 05/30/2011 06:15 PM, Jan Kiszka wrote:
>> On 2011-05-30 17:10, Roedel, Joerg wrote:
>>>  On Mon, May 30, 2011 at 11:04:02AM -0400, Jan Kiszka wrote:
>>>>  On 2011-05-30 16:38, Nadav Har'El wrote:
>>>>>  On Mon, May 30, 2011, Jan Kiszka wrote about "drop -enable-nesting (was: [PATCH 3/7] cpu model bug fixes and definition corrections...)":
>>>>>>  On 2011-05-30 10:18, Roedel, Joerg wrote:
>>>>>>>  On Sat, May 28, 2011 at 04:39:13AM -0400, Jan Kiszka wrote:
>>>>>>>
>>>>>>>>  J�rg, how to deal with -enable-nesting in qemu-kvm to align behavior
>>>>>>>>  with upstream?
>>>>>>>
>>>>>>>  My personal preference is to just remove it. In upstream-qemu it is
>>>>>>>  enabled/disabled by +/-svm. -enable-nesting is just a historic thing
>>>>>>>  which can be wiped out.
>>>>>
>>>>>  "-enable-nesting" could remain as a synonym for enabling either VMX or SVM
>>>>>  in the guest, depending on what was available in the host (because KVM now
>>>>>  supports both nested SVM and nested VMX, but not SVM-on-VMX or vice versa).
>>>>
>>>>  Why? Once nesting is stable (I think SVM already is), there is no reason
>>>>  for an explicit enable. And you can always mask it out via -cpu.
>>>>
>>>>  BTW, what are the defaults for SVM right now in qemu-kvm and upstream?
>>>>  Enable if the modeled CPU supports it?
>>>
>>>  qemu-kvm still needs -enable-nesting, otherwise it is disabled. Upstream
>>>  qemu should enable it unconditionally (can be disabled with -cpu ,-svm).
>>
>> Then let's start with aligning qemu-kvm defaults to upstream? I guess
>> that's what the diff I was citing yesterday is responsible for.
>>
>> In the same run, -enable-nesting could dump a warning on the console
>> that this switch is obsolete and will be removed from future versions.
> 
> I think it's safe to drop -enable-nesting immediately.  Dan, does 
> libvirt make use of it?

I'm currently checking with some customer who played with Proxmox and
nesting if that stack was aware of the switch or accepted it only via a
side channel.

> 
>> For VMX, I would suggest to keep it off by default until it matured,
>> asking the user to issue -cpu ...,+vmx.
> 
> We should do that for svm as well (except for -cpu host or -cpu 
> something-with-svm).

I assume that's what upstream is doing. Maybe it has it was part of the
artificial default qemu64 model which is AMD based.

>  vmx will be kept disabled by the module option, 
> until it is deemed fit for general consumption.
> 

Yes, even better - no need for duplicate controls.

Jan
Jan Kiszka - May 30, 2011, 3:42 p.m.
On 2011-05-30 17:27, Jan Kiszka wrote:
> On 2011-05-30 17:19, Avi Kivity wrote:
>> I think it's safe to drop -enable-nesting immediately.  Dan, does 
>> libvirt make use of it?
> 
> I'm currently checking with some customer who played with Proxmox and
> nesting if that stack was aware of the switch or accepted it only via a
> side channel.

It was a side channel for tweaking the command line options.

Jan
Jan Kiszka - May 30, 2011, 3:59 p.m.
On 2011-05-30 17:16, Nadav Har'El wrote:
> On Mon, May 30, 2011, Jan Kiszka wrote about "Re: drop -enable-nesting":
>>> "-enable-nesting" could remain as a synonym for enabling either VMX or SVM
>>> in the guest, depending on what was available in the host (because KVM now
>>> supports both nested SVM and nested VMX, but not SVM-on-VMX or vice versa).
>>
>> Why? Once nesting is stable (I think SVM already is), there is no reason
>> for an explicit enable. And you can always mask it out via -cpu.
> 
> As far as I understand (and this was previously discussed on the QEMU mailing
> list), the default emulated CPU does not include the "vmx" capability, and you
> need to enable it with something like "-cpu qemu64,+vmx" (or "-cpu host").

qemu64 is an artificial AMD model. Adding VMX to it may have interesting
effects on the guests. Better use host or a recent Intel model.

> 
> I am not sure if it does enable the "svm" capability. If it does, it isn't
> useful when KVM is enabled and the underlying host has VMX, not SVM.

That's what KVM is supposed to filter based on the host's capabilities.
I bet it does already.

Jan
Daniel P. Berrange - May 31, 2011, 8:44 a.m.
On Mon, May 30, 2011 at 06:19:14PM +0300, Avi Kivity wrote:
> On 05/30/2011 06:15 PM, Jan Kiszka wrote:
> >On 2011-05-30 17:10, Roedel, Joerg wrote:
> >>  On Mon, May 30, 2011 at 11:04:02AM -0400, Jan Kiszka wrote:
> >>>  On 2011-05-30 16:38, Nadav Har'El wrote:
> >>>>  On Mon, May 30, 2011, Jan Kiszka wrote about "drop -enable-nesting (was: [PATCH 3/7] cpu model bug fixes and definition corrections...)":
> >>>>>  On 2011-05-30 10:18, Roedel, Joerg wrote:
> >>>>>>  On Sat, May 28, 2011 at 04:39:13AM -0400, Jan Kiszka wrote:
> >>>>>>
> >>>>>>>  J�rg, how to deal with -enable-nesting in qemu-kvm to align behavior
> >>>>>>>  with upstream?
> >>>>>>
> >>>>>>  My personal preference is to just remove it. In upstream-qemu it is
> >>>>>>  enabled/disabled by +/-svm. -enable-nesting is just a historic thing
> >>>>>>  which can be wiped out.
> >>>>
> >>>>  "-enable-nesting" could remain as a synonym for enabling either VMX or SVM
> >>>>  in the guest, depending on what was available in the host (because KVM now
> >>>>  supports both nested SVM and nested VMX, but not SVM-on-VMX or vice versa).
> >>>
> >>>  Why? Once nesting is stable (I think SVM already is), there is no reason
> >>>  for an explicit enable. And you can always mask it out via -cpu.
> >>>
> >>>  BTW, what are the defaults for SVM right now in qemu-kvm and upstream?
> >>>  Enable if the modeled CPU supports it?
> >>
> >>  qemu-kvm still needs -enable-nesting, otherwise it is disabled. Upstream
> >>  qemu should enable it unconditionally (can be disabled with -cpu ,-svm).
> >
> >Then let's start with aligning qemu-kvm defaults to upstream? I guess
> >that's what the diff I was citing yesterday is responsible for.
> >
> >In the same run, -enable-nesting could dump a warning on the console
> >that this switch is obsolete and will be removed from future versions.
> 
> I think it's safe to drop -enable-nesting immediately.  Dan, does
> libvirt make use of it?

Yes, but it should be safe to drop it. Currently, if the user specifies
a CPU with the 'svm' flag present in libvirt guest XML, then we will
pass args '-cpu ....+svm -enable-nesting'. So if we drop --enable-nesting,
then libvirt will simply omit it and everything should still work because
we have still got +svm set.

Daniel
Avi Kivity - May 31, 2011, 8:58 a.m.
On 05/31/2011 11:44 AM, Daniel P. Berrange wrote:
> >  I think it's safe to drop -enable-nesting immediately.  Dan, does
> >  libvirt make use of it?
>
> Yes, but it should be safe to drop it. Currently, if the user specifies
> a CPU with the 'svm' flag present in libvirt guest XML, then we will
> pass args '-cpu ....+svm -enable-nesting'. So if we drop --enable-nesting,
> then libvirt will simply omit it and everything should still work because
> we have still got +svm set.

But qemu will complain about an option it can't parse.
Joerg Roedel - May 31, 2011, 9:06 a.m.
On Tue, May 31, 2011 at 04:58:16AM -0400, Avi Kivity wrote:
> On 05/31/2011 11:44 AM, Daniel P. Berrange wrote:
> > >  I think it's safe to drop -enable-nesting immediately.  Dan, does
> > >  libvirt make use of it?
> >
> > Yes, but it should be safe to drop it. Currently, if the user specifies
> > a CPU with the 'svm' flag present in libvirt guest XML, then we will
> > pass args '-cpu ....+svm -enable-nesting'. So if we drop --enable-nesting,
> > then libvirt will simply omit it and everything should still work because
> > we have still got +svm set.
> 
> But qemu will complain about an option it can't parse.

The best choice is probably to keep the option and make it a nop for the
lifetime of qemu-kvm. Optionally qemu-kvm can print a warning about the
deprecated option.

	Joerg
Paolo Bonzini - May 31, 2011, 9:15 a.m.
On 05/31/2011 10:58 AM, Avi Kivity wrote:
>
> But qemu will complain about an option it can't parse.

The presence of -enable-nesting is inferred from the help text.

Paolo
Avi Kivity - May 31, 2011, 9:16 a.m.
On 05/31/2011 12:15 PM, Paolo Bonzini wrote:
> On 05/31/2011 10:58 AM, Avi Kivity wrote:
>>
>> But qemu will complain about an option it can't parse.
>
> The presence of -enable-nesting is inferred from the help text.


Okay, so it can be safely dropped.

Patch

diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index e479a4d..091d812 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -1175,6 +1175,31 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t
index, uint32_t count,
                 *ecx |= 1 << 1;    /* CmpLegacy bit */
             }
         }
+        if (kvm_enabled()) {
+            uint32_t h_eax, h_edx;
+
+            host_cpuid(index, 0, &h_eax, NULL, NULL, &h_edx);
+
+            /* disable CPU features that the host does not support */
+
+            /* long mode */
+            if ((h_edx & 0x20000000) == 0 /* || !lm_capable_kernel */)
+                *edx &= ~0x20000000;
+            /* syscall */
+            if ((h_edx & 0x00000800) == 0)
+                *edx &= ~0x00000800;
+            /* nx */
+            if ((h_edx & 0x00100000) == 0)
+                *edx &= ~0x00100000;
+
+            /* disable CPU features that KVM cannot support */
+
+            /* svm */
+            if (!kvm_nested)
+                *ecx &= ~CPUID_EXT3_SVM;
+            /* 3dnow */
+            *edx &= ~0xc0000000;
+        }
         break;
     case 0x80000002: