[libvirt] [PATCH] Add new 'kvm' domain feature and ability to hide KVM signature

[Alex Williamson]

On Tue, 2014-08-19 at 12:40 -0400, Cole Robinson wrote:
> On 08/15/2014 12:32 PM, Alex Williamson wrote:
> > QEMU 2.1 added support for the kvm=off option to the -cpu command,
> > allowing the KVM hypervisor signature to be hidden from the guest.
> > This enables disabling of some paravirualization features in the
> > guest as well as allowing certain drivers which test for the
> > hypervisor to load.  Domain XML syntax is as follows:
> > 
> > <domain type='kvm>
> >   ...
> >   <features>
> >     ...
> >     <kvm>
> >       <hidden state='on'/>
> >     </kvm>
> >   </features>
> >   ...
> > 
> > Signed-off-by: Alex Williamson <alex.williamson at redhat.com>
> > ---
> > 
> > If it's not obvious, this patch is derived from copying and modifying
> > the similar hyperv feature support.  Hopefully I've found all the
> > right pieces.
> > 
> 
> Seems to cover all the bases. couple minor bits:
> 
> >  docs/formatdomain.html.in                          |   21 ++++
> >  docs/schemas/domaincommon.rng                      |   18 +++-
> >  src/conf/domain_conf.c                             |  100 ++++++++++++++++++++
> >  src/conf/domain_conf.h                             |    9 ++
> >  src/qemu/qemu_command.c                            |   22 ++++
> >  tests/qemuargv2xmltest.c                           |    2 
> >  .../qemuxml2argv-kvm-features-off.args             |    5 +
> >  .../qemuxml2argv-kvm-features-off.xml              |   27 +++++
> >  .../qemuxml2argv-kvm-features.args                 |    5 +
> >  .../qemuxml2argvdata/qemuxml2argv-kvm-features.xml |   27 +++++
> >  tests/qemuxml2argvtest.c                           |    3 +
> >  tests/qemuxml2xmltest.c                            |    3 +
> >  12 files changed, 240 insertions(+), 2 deletions(-)
> >  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-kvm-features-off.args
> >  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-kvm-features-off.xml
> >  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-kvm-features.args
> >  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-kvm-features.xml
> > 
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index bd99ae0..32cc381 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -1232,6 +1232,9 @@
> >        <vapic state='on'/>
> >        <spinlocks state='on' retries='4096'/>
> >      </hyperv>
> > +    <kvm>
> > +      <hidden state='on'/>
> > +    </kvm>
> >      <pvspinlock/>
> >  
> >    </features>
> > @@ -1310,7 +1313,23 @@
> >            can be explicitly disabled by using <code>state='off'</code>
> >            attribute.
> >        </dd>
> > -
> > +      <dt><code>kvm</code></dt>
> > +      <dd>Various features to change the behavior of the KVM hypervisor.
> > +      <table class="top_table">
> > +        <tr>
> > +          <th>Feature</th>
> > +          <th>Description</th>
> > +          <th>Value</th>
> > +          <th>Since</th>
> > +        </tr>
> > +        <tr>
> > +          <td>hidden</td>
> > +          <td>Hide the KVM hypervisor from standard MSR based discovery</td>
> > +          <td> on, off</td>
> > +          <td><span class="since">2.1.0 (QEMU only)</span></td>
> > +        </tr>
> > +      </table>
> > +      </dd>
> >      </dl>
> >  
> 
> I'd specify that the default value is 'off' if using KVM, 'on' otherwise, and
> can be explicitly disabled with 'on'.

At the beginning of this section we specify:

        All features are listed within the features element, omitting a
        togglable feature tag turns it off.

Our feature is whether to hide or not hide the KVM hypervisor signature.
Note that 'not hide' does not imply expose, IMO.  The default per the
section scope is 'off'.  Without KVM acceleration, there is no KVM
hypervisor signature to hide, so the value is meaningless and QEMU
accepts, but does not change behavior based on this.  With KVM, there is
of course a signature that can be hidden.

Do you think there's still something to change here?

> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 8a69976..77a84cc 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -6228,6 +6228,25 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver,
> >          }
> >      }
> >  
> > +    if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON) {
> > +        if (!have_cpu) {
> > +            virBufferAdd(&buf, default_model, -1);
> > +            have_cpu = true;
> > +        }
> > +
> > +        for (i = 0; i < VIR_DOMAIN_KVM_LAST; i++) {
> > +            switch ((virDomainKVM) i) {
> > +            case VIR_DOMAIN_KVM_HIDDEN:
> > +                if (def->kvm_features[i] == VIR_TRISTATE_SWITCH_ON)
> > +                    virBufferAsprintf(&buf, ",kvm=off");
> > +                break;
> > +
> 
> There's a 'make syntax-check' warning here with latest git:
> 
> src/qemu/qemu_command.c:6241:                    virBufferAsprintf(&buf,
> ",kvm=off");
> maint.mk: use virBufferAddLit, not virBufferAsprintf, with a string literal
> make: *** [sc_prohibit_virBufferAsprintf_with_string_literal] Error 1
> 
> The rest looks fine, I'll commit v2 if no one has objections to the XML or
> code (CCing Peter and Jan who did some of the hyperv work)

Ok, I'll make the switch.  Thanks for the review,

Alex