-
Notifications
You must be signed in to change notification settings - Fork 20
drivers: hv: mshv_vtl: Implement restore partition time IOCTL #116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: product/hcl-main/6.12
Are you sure you want to change the base?
drivers: hv: mshv_vtl: Implement restore partition time IOCTL #116
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements support for the Hyper-V restore partition time hypercall, which allows Windows guests to restore from hibernate by resetting the TSC to provided values. The implementation adds a new IOCTL that stops all CPUs, makes the hypercall to update the TSC, and updates clock state to maintain forward progress.
Changes:
- Adds
MSHV_RESTORE_PARTITION_TIMEIOCTL and associated data structures in UAPI headers - Implements the IOCTL handler that uses
stop_machineto safely update partition time - Exports
hv_save_sched_clock_stateandhv_restore_sched_clock_statefunctions for clock state management
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| include/uapi/linux/mshv.h | Adds mshv_partition_time struct and MSHV_RESTORE_PARTITION_TIME IOCTL definition |
| include/uapi/hyperv/hvgdk_mini.h | Adds HVCALL_RESTORE_PARTITION_TIME hypercall code and x86-64 specific hv_input_restore_partition_time struct |
| include/asm-generic/hyperv-tlfs.h | Adds HVCALL_RESTORE_PARTITION_TIME hypercall code and hv_input_restore_partition_time struct |
| drivers/hv/mshv_vtl_main.c | Implements the main IOCTL handler and stop_machine callback for restoring partition time |
| arch/x86/kernel/cpu/mshyperv.c | Changes visibility of clock state save/restore functions from static to exported |
| arch/x86/include/asm/mshyperv.h | Adds function declarations for exported clock state functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct hv_input_restore_partition_time { | ||
| u64 partition_id; | ||
| u32 tsc_sequence; | ||
| u32 reserved; | ||
| u64 reference_time_in_100_ns; | ||
| u64 tsc; | ||
| } __packed; |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate struct definition: struct hv_input_restore_partition_time is defined in both include/uapi/hyperv/hvgdk_mini.h (lines 1316-1322 with __u64/__u32 types) and include/asm-generic/hyperv-tlfs.h (lines 1007-1013 with u64/u32 types). This duplication could lead to compilation issues or confusion. Since hvgdk_mini.h is a UAPI header and uses UAPI types (__u64, __u32), while hyperv-tlfs.h is a kernel header using kernel types (u64, u32), only one definition should exist. Consider keeping the definition in the appropriate header based on where it's primarily used.
| struct hv_input_restore_partition_time { | |
| u64 partition_id; | |
| u32 tsc_sequence; | |
| u32 reserved; | |
| u64 reference_time_in_100_ns; | |
| u64 tsc; | |
| } __packed; |
| local_irq_save(irq_flags); | ||
| ret = stop_machine(restore_partition_time_with_cpus_stopped, &partition_time, cpumask_of(smp_processor_id())); | ||
| local_irq_restore(irq_flags); |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary interrupt handling: The local_irq_save and local_irq_restore calls around stop_machine are redundant. The stop_machine function already handles interrupt disabling and restoration as part of its implementation. Calling local_irq_save before stop_machine is unnecessary and could potentially interfere with stop_machine's own interrupt handling. Consider removing these calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smp_processor_id() requires pre-emption to be disabled
|
|
||
| // Stop other CPUs, using the current one to restore partition time. | ||
| local_irq_save(irq_flags); | ||
| ret = stop_machine(restore_partition_time_with_cpus_stopped, &partition_time, cpumask_of(smp_processor_id())); |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear stop_machine usage: The code calls stop_machine(restore_partition_time_with_cpus_stopped, &partition_time, cpumask_of(smp_processor_id())), which stops all CPUs but only runs the callback on the current CPU. While this ensures no other CPUs are running during the TSC update (as indicated by the comment on line 878), it's worth verifying that making the HVCALL_RESTORE_PARTITION_TIME hypercall from only one CPU is sufficient to update the TSC for the entire system. If the TSC update is system-wide and a single hypercall is sufficient, the current implementation is correct. However, if each CPU needs to update its TSC state, the callback should run on all CPUs by passing NULL or cpu_online_mask instead.
| sched_clock_suspend(); | ||
| timekeeping_suspend(); | ||
| hv_save_sched_clock_state(); | ||
|
|
||
| // Interrupts are disabled, make the hypercall to update the TSC. | ||
| input = *this_cpu_ptr(hyperv_pcpu_input_arg); | ||
| input->partition_id = HV_PARTITION_ID_SELF; | ||
| input->tsc_sequence = partition_time->tsc_sequence; | ||
| input->reserved = 0; | ||
| input->reference_time_in_100_ns = partition_time->reference_time_in_100_ns; | ||
| input->tsc = partition_time->tsc; | ||
| status = hv_do_hypercall(HVCALL_RESTORE_PARTITION_TIME, input, NULL); | ||
| if (!hv_result_success(status)) { | ||
| pr_err("HVCALL_RESTORE_PARTITION_TIME failed ! [Err: %#llx\n]", status); | ||
| result = -EINVAL; | ||
| } | ||
|
|
||
| // Restore clock state using current TSC value. | ||
| hv_restore_sched_clock_state(); | ||
| timekeeping_resume(); |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect use of internal timekeeping functions: The functions timekeeping_suspend() and timekeeping_resume() are internal kernel functions registered as syscore_ops callbacks (see kernel/time/timekeeping.c:1940-1943). They are not exported symbols and are designed to be called only by the kernel's suspend/resume infrastructure. Calling them directly from driver code bypasses the proper system suspend/resume mechanisms and may cause system instability. These functions acquire locks and modify global state that should only be changed during proper system suspend/resume. Consider using a different approach that doesn't require direct manipulation of timekeeping state, or coordinate with the timekeeping subsystem maintainers for a proper solution.
| #include <asm/vmx.h> | ||
|
|
||
| #include "../../kernel/fpu/legacy.h" | ||
| #include "../../kernel/time/timekeeping.h" |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inclusion of internal kernel header: The file includes "../../kernel/time/timekeeping.h" using a relative path. This is an internal kernel header that is not part of the public kernel API. Driver code should not include internal headers using relative paths, as this breaks modularity and may cause build issues. The functions from this header (timekeeping_suspend, timekeeping_resume, sched_clock_suspend, sched_clock_resume) are also not exported symbols, which means they cannot be used from driver code that could potentially be built as a module.
| #include "../../kernel/time/timekeeping.h" | |
| #include <linux/timekeeping.h> |
Hyper-V supports a restore partition time hypercall that resets the TSC to provided values. This call is typically made by a Windows guest when restoring from hibernate. The paravisor needs to support this call in order to allow the guest to be able to successfully perform the operation. Since the reference time is being changed, clocks need to be updated to maintain consistent foward progress.