[libvirt] [PATCH v3] qemu: don't use deprecated -no-kvm-pit-reinjection

[Ján Tomko]

On 11/04/2013 04:57 PM, Martin Kletzander wrote:
> On Thu, Sep 26, 2013 at 10:50:16AM +0200, Ján Tomko wrote:
>> Since qemu-kvm 1.1 [1] (since 1.3. in upstream QEMU [2])
>> '-no-kvm-pit-reinjection' has been deprecated.
>> Use -global kvm-pit.lost_tick_policy=discard instead.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=978719
>>
>> [1] http://git.kernel.org/cgit/virt/kvm/qemu-kvm.git/commit/?id=4e4fa39
>> [2] http://git.qemu.org/?p=qemu.git;a=commitdiff;h=c21fb4f
>> ---
>> v3: use -global instead of trying to add another -device
>>
>> re: https://www.redhat.com/archives/libvir-list/2013-July/msg00833.html
>> Unsetting the QEMU_CAPS_NO_KVM_PIT capability for QEMU >1.2 seems to work
>> okay with libvirtd upgrades.
>>
>> v2: https://www.redhat.com/archives/libvir-list/2013-July/msg00821.html
>> v1: https://www.redhat.com/archives/libvir-list/2013-July/msg00045.html
>>

>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index dc8f0be..06b71b5 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -242,6 +242,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>>                "usb-storage.removable",
>>                "virtio-mmio",
>>                "ich9-intel-hda",
>> +              "kvm-pit",
>>      );
>>  
>>  struct _virQEMUCaps {
>> @@ -1393,6 +1394,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
>>      { "usb-storage", QEMU_CAPS_DEVICE_USB_STORAGE },
>>      { "virtio-mmio", QEMU_CAPS_DEVICE_VIRTIO_MMIO },
>>      { "ich9-intel-hda", QEMU_CAPS_DEVICE_ICH9_INTEL_HDA },
>> +    { "kvm-pit", QEMU_CAPS_DEVICE_KVM_PIT },
>>  };
>>  
> 
> Cleaner way would be (IMHO):
> 
> static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsKvmPit[] = {
>     { "lost_tick_policy", QEMU_CAPS_KVM_PIT_TICK_POLICY },
> };
> 
> (or similar) and then adding into virQEMUCapsObjectProps[]:
> 
>     { "kvm-pit", virQEMUCapsObjectPropsKvmPit,
>     ARRAY_CARDINALITY(virQEMUCapsObjectPropsKvmPit) }
> 

Agreed.

>>  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = {
>> @@ -2477,13 +2479,12 @@ virQEMUCapsInitArchQMPBasic(virQEMUCapsPtr qemuCaps,
>>  
>>      /*
>>       * Currently only x86_64 and i686 support PCI-multibus,
>> -     * -no-acpi and -no-kvm-pit-reinjection.
>> +     * -no-acpi
>>       */
>>      if (qemuCaps->arch == VIR_ARCH_X86_64 ||
>>          qemuCaps->arch == VIR_ARCH_I686) {
>>          virQEMUCapsSet(qemuCaps, QEMU_CAPS_PCI_MULTIBUS);
>>          virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_ACPI);
>> -        virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_KVM_PIT);
>>      }
>>  
> 
> Sorry to dissapoint you in v3, but I must say, I still agree with
> Eric.  Since that flag is just 'deprecated', it doesn't mean that it
> is not recognized.  And thanks to the fact that you have a possibility
> to check whether the newer 'kvm-pit.lost_tick_policy' is supported,
> you can safely choose all three states even if there is a qemu with
> QMP on x86_64 without 'kvm-pit.lost_tick_policy' and it will still
> work correctly then.

I wanted to remove it to get rid of that special case because:
* in the cases of QEMU >=1.2 where this option is supported, .lost_tick_policy
should be supported as well
* in the case when it's not (non-KVM QEMU 1.2), .lost_tick_policy isn't supported

If QEMU decides to remove .lost_tick_policy, but keep -no-kvm-pit-reinjection,
it would be good to have NO_KVM_PIT capability hardcoded, other than that,
it's just one extra line in the code.

> 
> I'd keep the fallback until it is recognized, so it can be used even
> if there is no other (non-deprecated) option.

Okay, I'll drop the hunk dropping that capability.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131105/9b8d982d/attachment-0001.sig>