[PATCH kvm-kmod] adjust timekeeping compatibility code

[PATCH kvm-kmod] adjust timekeeping compatibility code

[Paolo Bonzini]

kvm_get_xtime_nsec could overflow.  If we make kvm_get_boot_base_ns
compute the equivalent of 3.17's base_mono+offs_boot formula (instead of
just offs_boot), we can avoid that and drop kvm_get_xtime_nsec altogether.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 external-module-compat-comm.h |  3 +--
 external-module-compat.c      | 19 ++++---------------
 sync                          |  4 +---
 3 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/external-module-compat-comm.h b/external-module-compat-comm.h
index b6a2894..fdcd8e4 100644
--- a/external-module-compat-comm.h
+++ b/external-module-compat-comm.h
@@ -1412,9 +1412,8 @@ static inline void guest_exit(void)
  */
 #if LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0)
 extern u64 ktime_get_boot_ns(void);
-extern u64 kvm_get_boot_base_ns(void);
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(3,8,0)
 struct timekeeper;
-extern u64 kvm_get_xtime_nsec(struct timekeeper *tk);
+extern u64 kvm_get_boot_base_ns(struct timekeeper *tk);
 #endif
 #endif
diff --git a/external-module-compat.c b/external-module-compat.c
index eb7bc62..38717b6 100644
--- a/external-module-compat.c
+++ b/external-module-compat.c
@@ -350,26 +350,15 @@ u64 ktime_get_boot_ns(void)
 	return timespec_to_ns(&ts);
 }
 
-u64 kvm_get_boot_base_ns(void)
-{
-	struct timespec ts = { 0, 0 };
-
-	kvm_monotonic_to_bootbased(&ts);
-	return timespec_to_ns(&ts);
-}
-
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(3,8,0)
 #include <linux/timekeeper_internal.h>
 
-u64 kvm_get_xtime_nsec(struct timekeeper *tk)
+u64 kvm_get_boot_base_ns(struct timekeeper *tk)
 {
-	u64 monotonic_time_sec =
-		tk->xtime_sec + tk->wall_to_monotonic.tv_sec;
-	u64 monotonic_time_snsec =
-		tk->xtime_nsec + (tk->wall_to_monotonic.tv_nsec << tk->shift);
+	struct timespec ts = tk->wall_to_monotonic;
 
-	return ((monotonic_time_sec * (u64)NSEC_PER_SEC) << tk->shift) +
-		monotonic_time_snsec;
+	kvm_monotonic_to_bootbased(&ts);
+	return timespec_to_ns(&ts) + tk->xtime_sec * (u64)NSEC_PER_SEC;
 }
 #endif
 #endif
diff --git a/sync b/sync
index 0af0399..8b63ca7 100755
--- a/sync
+++ b/sync
@@ -306,9 +306,7 @@ def hack_content(fname, data):
             if match(r'tkr\.cycle_last') or match(r'tkr\.mask'):
                 w(sub(r'tkr\.', 'clock->', line))
             elif match(r'tkr\.base_mono'):
-                w('\tboot_ns = kvm_get_boot_base_ns();')
-            elif match(r'tkr\.xtime_nsec'):
-                w(sub(r'tk->tkr\.xtime_nsec', 'kvm_get_xtime_nsec(tk)', line))
+                w('\tboot_ns = kvm_get_boot_base_ns(tk);')
             else:
                 w(sub(r'tkr\.', '', line))
             line = '#endif'
-- 
1.8.3.1

Re: [PATCH kvm-kmod] adjust timekeeping compatibility code

[Jan Kiszka]

On 2014-09-23 10:22, Paolo Bonzini wrote:
> kvm_get_xtime_nsec could overflow.  If we make kvm_get_boot_base_ns
> compute the equivalent of 3.17's base_mono+offs_boot formula (instead of
> just offs_boot), we can avoid that and drop kvm_get_xtime_nsec altogether.

Applied, thanks.

Any suggestions how to deal with "kvm: Fix page ageing bugs"?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH kvm-kmod] adjust timekeeping compatibility code

[Paolo Bonzini]

Il 30/09/2014 09:54, Jan Kiszka ha scritto:
>> > kvm_get_xtime_nsec could overflow.  If we make kvm_get_boot_base_ns
>> > compute the equivalent of 3.17's base_mono+offs_boot formula (instead of
>> > just offs_boot), we can avoid that and drop kvm_get_xtime_nsec altogether.
> Applied, thanks.
> 
> Any suggestions how to deal with "kvm: Fix page ageing bugs"?

If you mean the new MMU notifier API, probably something like this:

diff --git a/sync b/sync
index 8b63ca7..fff85f3 100755
--- a/sync
+++ b/sync
@@ -310,6 +310,22 @@ def hack_content(fname, data):
             else:
                 w(sub(r'tkr\.', '', line))
             line = '#endif'
+        if match_block_end('^static int kvm_mmu_notifier_clear_flush_young', '^}'):
+            w(line)
+            w('#if LINUX_VERSION_CODE < KERNEL_VERSION(3,18,0)')
+            w('static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn,')
+            w('                                              struct mm_struct *mm,')
+            w('                                              unsigned long hva)')
+            w('{')
+            w('\treturn __kvm_mmu_notifier_clear_flush_young(mn, mm, hva, hva+1);')
+            w('}')
+            line = '#endif'
+        if match(r'^static int kvm_mmu_notifier_clear_flush_young'):
+            w('#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,18,0)')
+            w(line)
+            w('#else')
+            w(sub('kvm_', '__kvm_', line))
+            line = '#endif'
         if line == '#ifdef CONFIG_KEXEC':
             line = '#if defined(CONFIG_KEXEC) && LINUX_VERSION_CODE >= KERNEL_VERSION(3,8,0)'
         if line == '\tif (!cpu_has_vmx_apicv())':

(Not tested beyond compilation).

Paolo

Re: [PATCH kvm-kmod] adjust timekeeping compatibility code

[Jan Kiszka]

On 2014-09-30 10:43, Paolo Bonzini wrote:
> Il 30/09/2014 09:54, Jan Kiszka ha scritto:
>>>> kvm_get_xtime_nsec could overflow.  If we make kvm_get_boot_base_ns
>>>> compute the equivalent of 3.17's base_mono+offs_boot formula (instead of
>>>> just offs_boot), we can avoid that and drop kvm_get_xtime_nsec altogether.
>> Applied, thanks.
>>
>> Any suggestions how to deal with "kvm: Fix page ageing bugs"?
> 
> If you mean the new MMU notifier API, probably something like this:
> 
> diff --git a/sync b/sync
> index 8b63ca7..fff85f3 100755
> --- a/sync
> +++ b/sync
> @@ -310,6 +310,22 @@ def hack_content(fname, data):
>              else:
>                  w(sub(r'tkr\.', '', line))
>              line = '#endif'
> +        if match_block_end('^static int kvm_mmu_notifier_clear_flush_young', '^}'):
> +            w(line)
> +            w('#if LINUX_VERSION_CODE < KERNEL_VERSION(3,18,0)')
> +            w('static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn,')
> +            w('                                              struct mm_struct *mm,')
> +            w('                                              unsigned long hva)')
> +            w('{')
> +            w('\treturn __kvm_mmu_notifier_clear_flush_young(mn, mm, hva, hva+1);')

Ah, end=start+1, it's that easy!

> +            w('}')
> +            line = '#endif'
> +        if match(r'^static int kvm_mmu_notifier_clear_flush_young'):
> +            w('#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,18,0)')
> +            w(line)
> +            w('#else')
> +            w(sub('kvm_', '__kvm_', line))
> +            line = '#endif'
>          if line == '#ifdef CONFIG_KEXEC':
>              line = '#if defined(CONFIG_KEXEC) && LINUX_VERSION_CODE >= KERNEL_VERSION(3,8,0)'
>          if line == '\tif (!cpu_has_vmx_apicv())':
> 
> (Not tested beyond compilation).

Will check.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH kvm-kmod] adjust timekeeping compatibility code

[Paolo Bonzini]

Il 30/09/2014 10:48, Jan Kiszka ha scritto:
>> > +            w('{')
>> > +            w('\treturn __kvm_mmu_notifier_clear_flush_young(mn, mm, hva, hva+1);')
> Ah, end=start+1, it's that easy!
> 

Yes, that's how kvm_handle_hva is implemented on top of
kvm_handle_hva_range.  So, now that kvm_age_hva calls
kvm_handle_hva_range instead of kvm_handle_hva, you can apply the same
technique to the call of kvm_age_hva.

Of course the bug that Andres fixed will remain.

Paolo

Re: [PATCH kvm-kmod] adjust timekeeping compatibility code

[Jan Kiszka]

On 2014-09-30 12:39, Paolo Bonzini wrote:
> Il 30/09/2014 10:48, Jan Kiszka ha scritto:
>>>> +            w('{')
>>>> +            w('\treturn __kvm_mmu_notifier_clear_flush_young(mn, mm, hva, hva+1);')
>> Ah, end=start+1, it's that easy!
>>
> 
> Yes, that's how kvm_handle_hva is implemented on top of
> kvm_handle_hva_range.  So, now that kvm_age_hva calls
> kvm_handle_hva_range instead of kvm_handle_hva, you can apply the same
> technique to the call of kvm_age_hva.

[50757.107460] loaded kvm module (v3.17-rc1-168-gcec26bc)
[50764.182381] BUG: unable to handle kernel paging request at fffffffffffffffe
[50764.182402] IP: [<ffffffff81168449>] put_page+0x9/0x30
[50764.182414] PGD 1e15067 PUD 1e17067 PMD 0 
[50764.182419] Oops: 0000 [#1] PREEMPT SMP 
[50764.182424] Modules linked in: kvm_intel(O) kvm(O) cp210x mct_u232 pl2303 usbserial snd_usb_audio snd_usbmidi_lib snd_rawmidi xt_nat xt_pkttype xt_tcpudp xt_LOG xt_limit iptable_nat nf_nat_ipv4 nf_nat ctr ccm fuse bluetooth 6lowpan_iphc af_packet snd_pcm_oss snd_mixer_oss snd_seq snd_seq_device edd ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw xt_CT ipt_REJECT iptable_raw iptable_filter ip6table_mangle nf_conntrack_netbios_ns nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables cpufreq_conservative cpufreq_userspace cpufreq_powersave loop arc4 ath9k mac80211 ath9k_common ath9k_hw nvidia(PO) ath snd_hda_codec_realtek vhost_net macvtap macvlan snd_hda_codec_generic snd_hda_codec_hdmi cfg80211 vhost snd_
 hda_intel o2scr(O) tun snd_hda_controller e1000e snd_hda_codec pcmcia drm snd_hwdep snd_pcm irda sdhci_pci gpio_ich yenta_socket mei_me sdhci snd_timer pcmcia_rsrc tpm_infineon iTCO_wdt snd!
  mmc_core 
mei sr_mod ptp ppdev rfkill cdrom iTCO_vendor_support pps_core parport_pc tpm_tis pcmcia_core lpc_ich i2c_i801 tpm sg parport serio_raw intel_ips soundcore pcspkr video joydev mfd_core fujitsu_laptop acpi_cpufreq battery button ac crc_ccitt dm_crypt linear md_mod scsi_dh_alua scsi_dh_emc scsi_dh_hp_sw scsi_dh_rdac scsi_dh dm_snapshot dm_bufio dm_mod sha256_ssse3 sha256_generic cbc fan processor ata_generic thermal [last unloaded: kvm]
[50764.182544] CPU: 1 PID: 22091 Comm: qemu-system-x86 Tainted: P           O  3.15.5-1.g01d2774-desktop #2
[50764.182549] Hardware name: FUJITSU CELSIUS H700/FJNB211W, BIOS Version 1.20  01/24/2011
[50764.182554] task: ffff880045cac0d0 ti: ffff88022dd84000 task.ti: ffff88022dd84000
[50764.182558] RIP: 0010:[<ffffffff81168449>]  [<ffffffff81168449>] put_page+0x9/0x30
[50764.182563] RSP: 0018:ffff88022dd87d68  EFLAGS: 00010246
[50764.182567] RAX: 6dbd249249249000 RBX: ffff88016d040040 RCX: 0000000000000000
[50764.182570] RDX: 0000000000002014 RSI: 6dbd249249249000 RDI: fffffffffffffffe
[50764.182574] RBP: ffff88022dd87d68 R08: 0000000000000001 R09: 0000000000000000
[50764.182578] R10: dead000000100100 R11: 0000000000000000 R12: ffff88004760c000
[50764.182582] R13: 0000000000000000 R14: 0000000000000000 R15: 00000000ffffffea
[50764.182586] FS:  00007f9133bff700(0000) GS:ffff88023bc80000(0000) knlGS:0000000000000000
[50764.182590] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[50764.182594] CR2: fffffffffffffffe CR3: 00000000418d0000 CR4: 00000000000027e0
[50764.182598] Stack:
[50764.182600]  ffff88022dd87da8 ffffffffa12608bc 0000000000000000 0000000000000000
[50764.182605]  ffff88022dd87d01 00000000fee00900 ffff88016d040040 ffff88016d040040
[50764.182610]  ffff88022dd87dd8 ffffffffa12f503c ffff88004760c000 ffff880000000000
[50764.182615] Call Trace:
[50764.182633]  [<ffffffffa12608bc>] vmx_vcpu_reset+0x20c/0x3b0 [kvm_intel]
[50764.182697]  [<ffffffffa12f503c>] kvm_arch_vcpu_setup+0x2c/0x50 [kvm]
[50764.182734]  [<ffffffffa12e24d4>] kvm_vm_ioctl_create_vcpu+0x84/0x1e0 [kvm]
[50764.182753]  [<ffffffffa12e29ab>] kvm_vm_ioctl+0x37b/0x520 [kvm]
[50764.182770]  [<ffffffff811e1685>] do_vfs_ioctl+0x75/0x2c0
[50764.182777]  [<ffffffff811e1971>] SyS_ioctl+0xa1/0xb0
[50764.182785]  [<ffffffff8167ef29>] system_call_fastpath+0x16/0x1b
[50764.182797]  [<00007f917b868d87>] 0x7f917b868d86
[50764.182800] Code: 00 e8 9c fe ff ff 48 63 45 fc 65 48 01 04 25 70 fd 00 00 c9 c3 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 66 66 66 66 90 <48> f7 07 00 c0 00 00 75 14 f0 ff 4f 1c 74 08 c9 c3 66 0f 1f 44 
[50764.182828] RIP  [<ffffffff81168449>] put_page+0x9/0x30
[50764.182834]  RSP <ffff88022dd87d68>
[50764.182836] CR2: fffffffffffffffe

Your patch alone is not enough, I suppose. Let me look closer...

> 
> Of course the bug that Andres fixed will remain.

Yes, that's clear.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH kvm-kmod] adjust timekeeping compatibility code

[Paolo Bonzini]

Il 01/10/2014 09:40, Jan Kiszka ha scritto:
> [50764.182615] Call Trace:
> [50764.182633]  [<ffffffffa12608bc>] vmx_vcpu_reset+0x20c/0x3b0 [kvm_intel]
> [50764.182697]  [<ffffffffa12f503c>] kvm_arch_vcpu_setup+0x2c/0x50 [kvm]
> [50764.182734]  [<ffffffffa12e24d4>] kvm_vm_ioctl_create_vcpu+0x84/0x1e0 [kvm]
> [50764.182753]  [<ffffffffa12e29ab>] kvm_vm_ioctl+0x37b/0x520 [kvm]
> [50764.182770]  [<ffffffff811e1685>] do_vfs_ioctl+0x75/0x2c0
> [50764.182777]  [<ffffffff811e1971>] SyS_ioctl+0xa1/0xb0
> [50764.182785]  [<ffffffff8167ef29>] system_call_fastpath+0x16/0x1b
> [50764.182797]  [<00007f917b868d87>] 0x7f917b868d86
> [50764.182800] Code: 00 e8 9c fe ff ff 48 63 45 fc 65 48 01 04 25 70 fd 00 00 c9 c3 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 66 66 66 66 90 <48> f7 07 00 c0 00 00 75 14 f0 ff 4f 1c 74 08 c9 c3 66 0f 1f 44 
> [50764.182828] RIP  [<ffffffff81168449>] put_page+0x9/0x30
> [50764.182834]  RSP <ffff88022dd87d68>
> [50764.182836] CR2: fffffffffffffffe

The call stack must be vmx_vcpu_reset ->
kvm_vcpu_reload_apic_access_page -> put_page (tail call).

Is this with irqchip disabled?  If so, it's a KVM bug, it should be
fixed like

 void kvm_vcpu_reload_apic_access_page(struct kvm_vcpu *vcpu)
 {
 	struct page *page = NULL;

+	if (!irqchip_in_kernel(vcpu->kvm))
+		return;
+
 	if (!kvm_x86_ops->set_apic_access_page_addr)
 		return;

in x86.c, matching vm_need_virtualize_apic_accesses() in vmx.c.

Paolo

Re: [PATCH kvm-kmod] adjust timekeeping compatibility code

[Jan Kiszka]

On 2014-10-02 12:12, Paolo Bonzini wrote:
> Il 01/10/2014 09:40, Jan Kiszka ha scritto:
>> [50764.182615] Call Trace:
>> [50764.182633]  [<ffffffffa12608bc>] vmx_vcpu_reset+0x20c/0x3b0 [kvm_intel]
>> [50764.182697]  [<ffffffffa12f503c>] kvm_arch_vcpu_setup+0x2c/0x50 [kvm]
>> [50764.182734]  [<ffffffffa12e24d4>] kvm_vm_ioctl_create_vcpu+0x84/0x1e0 [kvm]
>> [50764.182753]  [<ffffffffa12e29ab>] kvm_vm_ioctl+0x37b/0x520 [kvm]
>> [50764.182770]  [<ffffffff811e1685>] do_vfs_ioctl+0x75/0x2c0
>> [50764.182777]  [<ffffffff811e1971>] SyS_ioctl+0xa1/0xb0
>> [50764.182785]  [<ffffffff8167ef29>] system_call_fastpath+0x16/0x1b
>> [50764.182797]  [<00007f917b868d87>] 0x7f917b868d86
>> [50764.182800] Code: 00 e8 9c fe ff ff 48 63 45 fc 65 48 01 04 25 70 fd 00 00 c9 c3 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 66 66 66 66 90 <48> f7 07 00 c0 00 00 75 14 f0 ff 4f 1c 74 08 c9 c3 66 0f 1f 44 
>> [50764.182828] RIP  [<ffffffff81168449>] put_page+0x9/0x30
>> [50764.182834]  RSP <ffff88022dd87d68>
>> [50764.182836] CR2: fffffffffffffffe
> 
> The call stack must be vmx_vcpu_reset ->
> kvm_vcpu_reload_apic_access_page -> put_page (tail call).
> 
> Is this with irqchip disabled?  If so, it's a KVM bug, it should be
> fixed like

Yes, that run was with irqchip off (due to IR emulation enabled). Will
give your fix a try once I can dare to crash my machine again. ;)

Thanks,
Jan

> 
>  void kvm_vcpu_reload_apic_access_page(struct kvm_vcpu *vcpu)
>  {
>  	struct page *page = NULL;
> 
> +	if (!irqchip_in_kernel(vcpu->kvm))
> +		return;
> +
>  	if (!kvm_x86_ops->set_apic_access_page_addr)
>  		return;
> 
> in x86.c, matching vm_need_virtualize_apic_accesses() in vmx.c.
> 
> Paolo
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH kvm-kmod] adjust timekeeping compatibility code

[Jan Kiszka]

On 2014-09-30 10:43, Paolo Bonzini wrote:
> Il 30/09/2014 09:54, Jan Kiszka ha scritto:
>>>> kvm_get_xtime_nsec could overflow.  If we make kvm_get_boot_base_ns
>>>> compute the equivalent of 3.17's base_mono+offs_boot formula (instead of
>>>> just offs_boot), we can avoid that and drop kvm_get_xtime_nsec altogether.
>> Applied, thanks.
>>
>> Any suggestions how to deal with "kvm: Fix page ageing bugs"?
> 
> If you mean the new MMU notifier API, probably something like this:
> 
> diff --git a/sync b/sync
> index 8b63ca7..fff85f3 100755
> --- a/sync
> +++ b/sync
> @@ -310,6 +310,22 @@ def hack_content(fname, data):
>              else:
>                  w(sub(r'tkr\.', '', line))
>              line = '#endif'
> +        if match_block_end('^static int kvm_mmu_notifier_clear_flush_young', '^}'):
> +            w(line)
> +            w('#if LINUX_VERSION_CODE < KERNEL_VERSION(3,18,0)')
> +            w('static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn,')
> +            w('                                              struct mm_struct *mm,')
> +            w('                                              unsigned long hva)')
> +            w('{')
> +            w('\treturn __kvm_mmu_notifier_clear_flush_young(mn, mm, hva, hva+1);')
> +            w('}')
> +            line = '#endif'
> +        if match(r'^static int kvm_mmu_notifier_clear_flush_young'):
> +            w('#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,18,0)')
> +            w(line)
> +            w('#else')
> +            w(sub('kvm_', '__kvm_', line))
> +            line = '#endif'
>          if line == '#ifdef CONFIG_KEXEC':
>              line = '#if defined(CONFIG_KEXEC) && LINUX_VERSION_CODE >= KERNEL_VERSION(3,8,0)'
>          if line == '\tif (!cpu_has_vmx_apicv())':
> 
> (Not tested beyond compilation).

Could you post as proper patch? It works fine here, and I'd like to move
kvm-kmod forward.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux