Re: [PATCH v2] HID: hyperv: streamline driver probe to avoid devres issues
On Tue, 12 Nov 2024, Vitaly Kuznetsov wrote: > >> Reviewed-by: Michael Kelley > > > > Tested V2 as well, please feel free to add, > > Tested-by: Saurabh Sengar > > Thank you! Applied to hid.git, thanks! -- Jiri Kosina SUSE Labs
[PATCH] x86/hyperv: Set X86_FEATURE_TSC_RELIABLE unconditionally
Enable X86_FEATURE_TSC_RELIABLE by default as X86_FEATURE_TSC_RELIABLE is
independent from invariant TSC and should have never been gated by the
HV_ACCESS_TSC_INVARIANT privilege.
To elaborate, the HV_ACCESS_TSC_INVARIANT privilege allows certain types of
guests to opt-in to invariant TSC by writing the
HV_X64_MSR_TSC_INVARIANT_CONTROL register. Not all guests will have this
privilege and the hypervisor will automatically opt-in certain types of
guests (e.g. EXO partitions) to invariant TSC, but this functionality is
unrelated to the TSC reliability.
Signed-off-by: Stanislav Kinsburskii
---
arch/x86/kernel/cpu/mshyperv.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index d18078834ded..14412afcc398 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -515,7 +515,7 @@ static void __init ms_hyperv_init_platform(void)
machine_ops.crash_shutdown = hv_machine_crash_shutdown;
#endif
#endif
- if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT) {
+ if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT)
/*
* Writing to synthetic MSR 0x4118 updates/changes the
* guest visible CPUIDs. Setting bit 0 of this MSR enables
@@ -526,8 +526,8 @@ static void __init ms_hyperv_init_platform(void)
* is called.
*/
wrmsrl(HV_X64_MSR_TSC_INVARIANT_CONTROL,
HV_EXPOSE_INVARIANT_TSC);
- setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
- }
+
+ setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
/*
* Generation 2 instances don't support reading the NMI status from
Re: [PATCH] TFrom: Stanislav Kinsburskii
Please disregard.
Stanislav
On Tue, Nov 12, 2024 at 06:16:26PM +, Stanislav Kinsburskii wrote:
> x86/hyperv: Set X86_FEATURE_TSC_RELIABLE unconditionally
>
> Enable X86_FEATURE_TSC_RELIABLE by default as X86_FEATURE_TSC_RELIABLE is
> independent from invariant TSC and should have never been gated by the
> HV_ACCESS_TSC_INVARIANT privilege.
>
> To elaborate, the HV_ACCESS_TSC_INVARIANT privilege allows certain types of
> guests to opt-in to invariant TSC by writing the
> HV_X64_MSR_TSC_INVARIANT_CONTROL register. Not all guests will have this
> privilege and the hypervisor will automatically opt-in certain types of
> guests (e.g. EXO partitions) to invariant TSC, but this functionality is
> unrelated to the TSC reliability.
>
> Signed-off-by: Stanislav Kinsburskii
> ---
> arch/x86/kernel/cpu/mshyperv.c |6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index d18078834ded..14412afcc398 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -515,7 +515,7 @@ static void __init ms_hyperv_init_platform(void)
> machine_ops.crash_shutdown = hv_machine_crash_shutdown;
> #endif
> #endif
> - if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT) {
> + if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT)
> /*
>* Writing to synthetic MSR 0x4118 updates/changes the
>* guest visible CPUIDs. Setting bit 0 of this MSR enables
> @@ -526,8 +526,8 @@ static void __init ms_hyperv_init_platform(void)
>* is called.
>*/
> wrmsrl(HV_X64_MSR_TSC_INVARIANT_CONTROL,
> HV_EXPOSE_INVARIANT_TSC);
> - setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
> - }
> +
> + setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
>
> /*
>* Generation 2 instances don't support reading the NMI status from
>
RE: [PATCH] x86/hyperv: Set X86_FEATURE_TSC_RELIABLE unconditionally
From: Stanislav Kinsburskii Sent: Tuesday,
November 12, 2024 10:18 AM
>
> Enable X86_FEATURE_TSC_RELIABLE by default as X86_FEATURE_TSC_RELIABLE is
> independent from invariant TSC and should have never been gated by the
> HV_ACCESS_TSC_INVARIANT privilege.
I think originally X86_FEATURE_TSC_RELIABLE was gated by the Hyper-V
TSC Invariant feature because otherwise VM live migration may cause
the TSC value reported by the RDTSC/RDTSCP instruction in the guest
to abruptly change frequency and value. In such cases, the TSC isn't
useable by the kernel or user space.
Enabling the Hyper-V TSC Invariant feature fixes that by using the
hardware scaling available in more recent processors to automatically
fixup the TSC value returned by RDTSC/RDTSCP in the guest.
Is there a practical problem that is fixed by always enabling
X86_FEATURE_TSC_RELIABLE?
Michael
>
> To elaborate, the HV_ACCESS_TSC_INVARIANT privilege allows certain types of
> guests to opt-in to invariant TSC by writing the
> HV_X64_MSR_TSC_INVARIANT_CONTROL register. Not all guests will have this
> privilege and the hypervisor will automatically opt-in certain types of
> guests (e.g. EXO partitions) to invariant TSC, but this functionality is
> unrelated to the TSC reliability.
>
> Signed-off-by: Stanislav Kinsburskii
> ---
> arch/x86/kernel/cpu/mshyperv.c |6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index d18078834ded..14412afcc398 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -515,7 +515,7 @@ static void __init ms_hyperv_init_platform(void)
> machine_ops.crash_shutdown = hv_machine_crash_shutdown;
> #endif
> #endif
> - if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT) {
> + if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT)
> /*
>* Writing to synthetic MSR 0x4118 updates/changes the
>* guest visible CPUIDs. Setting bit 0 of this MSR enables
> @@ -526,8 +526,8 @@ static void __init ms_hyperv_init_platform(void)
>* is called.
>*/
> wrmsrl(HV_X64_MSR_TSC_INVARIANT_CONTROL,
> HV_EXPOSE_INVARIANT_TSC);
> - setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
> - }
> +
> + setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
>
> /*
>* Generation 2 instances don't support reading the NMI status from
>
>
[PATCH] TFrom: Stanislav Kinsburskii
x86/hyperv: Set X86_FEATURE_TSC_RELIABLE unconditionally
Enable X86_FEATURE_TSC_RELIABLE by default as X86_FEATURE_TSC_RELIABLE is
independent from invariant TSC and should have never been gated by the
HV_ACCESS_TSC_INVARIANT privilege.
To elaborate, the HV_ACCESS_TSC_INVARIANT privilege allows certain types of
guests to opt-in to invariant TSC by writing the
HV_X64_MSR_TSC_INVARIANT_CONTROL register. Not all guests will have this
privilege and the hypervisor will automatically opt-in certain types of
guests (e.g. EXO partitions) to invariant TSC, but this functionality is
unrelated to the TSC reliability.
Signed-off-by: Stanislav Kinsburskii
---
arch/x86/kernel/cpu/mshyperv.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index d18078834ded..14412afcc398 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -515,7 +515,7 @@ static void __init ms_hyperv_init_platform(void)
machine_ops.crash_shutdown = hv_machine_crash_shutdown;
#endif
#endif
- if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT) {
+ if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT)
/*
* Writing to synthetic MSR 0x4118 updates/changes the
* guest visible CPUIDs. Setting bit 0 of this MSR enables
@@ -526,8 +526,8 @@ static void __init ms_hyperv_init_platform(void)
* is called.
*/
wrmsrl(HV_X64_MSR_TSC_INVARIANT_CONTROL,
HV_EXPOSE_INVARIANT_TSC);
- setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
- }
+
+ setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
/*
* Generation 2 instances don't support reading the NMI status from
[PATCH] hv/hv_kvp_daemon: Pass NIC name to hv_get_dns_info as well
The reference implementation of hv_get_dns_info which is in the tree uses /etc/resolv.conf to get DNS servers and this does not require to know which NIC is queried. Distro specific implementations, however, may want to provide per-NIC, fine grained information. E.g. NetworkManager keeps track of DNS servers per connection. Similar to hv_get_dhcp_info, pass NIC name as a parameter to hv_get_dns_info script. Signed-off-by: Vitaly Kuznetsov --- tools/hv/hv_kvp_daemon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c index ae57bf69ad4a..296a7a62c54d 100644 --- a/tools/hv/hv_kvp_daemon.c +++ b/tools/hv/hv_kvp_daemon.c @@ -725,7 +725,7 @@ static void kvp_get_ipconfig_info(char *if_name, * . */ - sprintf(cmd, KVP_SCRIPTS_PATH "%s", "hv_get_dns_info"); + sprintf(cmd, KVP_SCRIPTS_PATH "%s %s", "hv_get_dns_info", if_name); /* * Execute the command to gather DNS info. -- 2.47.0
Re: [PATCH v2] HID: hyperv: streamline driver probe to avoid devres issues
Saurabh Singh Sengar writes:
> On Mon, Nov 11, 2024 at 04:50:24PM +, Michael Kelley wrote:
>> From: Vitaly Kuznetsov Sent: Monday, November 11, 2024
>> 5:13 AM
>> >
>> > It was found that unloading 'hid_hyperv' module results in a devres
>> > complaint:
>> >
>> > ...
>> > hv_vmbus: unregistering driver hid_hyperv
>> > [ cut here ]
>> > WARNING: CPU: 2 PID: 3983 at drivers/base/devres.c:691
>> > devres_release_group+0x1f2/0x2c0
>> > ...
>> > Call Trace:
>> >
>> > ? devres_release_group+0x1f2/0x2c0
>> > ? __warn+0xd1/0x1c0
>> > ? devres_release_group+0x1f2/0x2c0
>> > ? report_bug+0x32a/0x3c0
>> > ? handle_bug+0x53/0xa0
>> > ? exc_invalid_op+0x18/0x50
>> > ? asm_exc_invalid_op+0x1a/0x20
>> > ? devres_release_group+0x1f2/0x2c0
>> > ? devres_release_group+0x90/0x2c0
>> > ? rcu_is_watching+0x15/0xb0
>> > ? __pfx_devres_release_group+0x10/0x10
>> > hid_device_remove+0xf5/0x220
>> > device_release_driver_internal+0x371/0x540
>> > ? klist_put+0xf3/0x170
>> > bus_remove_device+0x1f1/0x3f0
>> > device_del+0x33f/0x8c0
>> > ? __pfx_device_del+0x10/0x10
>> > ? cleanup_srcu_struct+0x337/0x500
>> > hid_destroy_device+0xc8/0x130
>> > mousevsc_remove+0xd2/0x1d0 [hid_hyperv]
>> > device_release_driver_internal+0x371/0x540
>> > driver_detach+0xc5/0x180
>> > bus_remove_driver+0x11e/0x2a0
>> > ? __mutex_unlock_slowpath+0x160/0x5e0
>> > vmbus_driver_unregister+0x62/0x2b0 [hv_vmbus]
>> > ...
>
> Do we want to mention the other error as well this patch is fixing:
The error itself is likely the same ('hid_dev->driver' override with
'mousevsc_hid_driver' stub which doesn't do anything) but the trace
stack is a bit different, yes.
>
> [ 68.237679] [ cut here ]
> [ 68.237681] WARNING: CPU: 23 PID: 579 at drivers/hid/hid-core.c:1262
> hid_open_report+0x2c0/0x350 [hid]
>
> [ 68.237741] RIP: 0010:hid_open_report+0x2c0/0x350 [hid]
> [ 68.237765] Call Trace:
> [ 68.237767]
> [ 68.237769] ? show_regs+0x6c/0x80
> [ 68.237774] ? __warn+0x8d/0x150
> [ 68.23] ? hid_open_report+0x2c0/0x350 [hid]
> [ 68.237784] ? report_bug+0x182/0x1b0
> [ 68.237788] ? handle_bug+0x6e/0xb0
> [ 68.237791] ? exc_invalid_op+0x18/0x80
> [ 68.237794] ? asm_exc_invalid_op+0x1b/0x20
> [ 68.237799] ? hid_open_report+0x2c0/0x350 [hid]
> [ 68.237805] mousevsc_probe+0x1d5/0x250 [hid_hyperv]
> [ 68.237808] vmbus_probe+0x3b/0xa0 [hv_vmbus]
> [ 68.237822] really_probe+0xee/0x3c0
> [ 68.237827] __driver_probe_device+0x8c/0x180
> [ 68.237830] driver_probe_device+0x24/0xd0
> [ 68.237832] __driver_attach_async_helper+0x6e/0x110
> [ 68.237835] async_run_entry_fn+0x30/0x130
> [ 68.237837] process_one_work+0x178/0x3d0
> [ 68.237839] worker_thread+0x2de/0x410
> [ 68.237841] ? __pfx_worker_thread+0x10/0x10
> [ 68.237843] kthread+0xe1/0x110
> [ 68.237847] ? __pfx_kthread+0x10/0x10
> [ 68.237849] ret_from_fork+0x44/0x70
> [ 68.237852] ? __pfx_kthread+0x10/0x10
> [ 68.237854] ret_from_fork_asm+0x1a/0x30
> [ 68.237858]
> [ 68.237859] ---[ end trace ]---
> [ 68.237861] hid-generic 0006:045E:0621.0002: parse failed
> [ 68.238276] hv_vmbus: probe failed for device
> 58f75a6d-d949-4320-99e1-a2a2576d581c (-19)
>
>
>> >
>> > And the issue seems to be that the corresponding devres group is not
>> > allocated. Normally, devres_open_group() is called from
>> > __hid_device_probe() but Hyper-V HID driver overrides 'hid_dev->driver'
>> > with 'mousevsc_hid_driver' stub and basically re-implements
>> > __hid_device_probe() by calling hid_parse() and hid_hw_start() but not
>> > devres_open_group(). hid_device_probe() does not call __hid_device_probe()
>> > for it. Later, when the driver is removed, hid_device_remove() calls
>> > devres_release_group() as it doesn't check whether hdev->driver was
>> > initially overridden or not.
>> >
>> > The issue seems to be related to the commit 62c68e7cee33 ("HID: ensure
>> > timely release of driver-allocated resources") but the commit itself seems
>> > to be correct.
>> >
>> > Fix the issue by dropping the 'hid_dev->driver' override and using
>> > hid_register_driver()/hid_unregister_driver() instead. Alternatively, it
>> > would have been possible to rely on the default handling but
>> > HID_CONNECT_DEFAULT implies HID_CONNECT_HIDRAW and it doesn't seem to work
>> > for mousevsc as-is.
>> >
>> > Fixes: 62c68e7cee33 ("HID: ensure timely release of driver-allocated
>> > resources")
>> > Suggested-by: Michael Kelley
>> > Signed-off-by: Vitaly Kuznetsov
>> > ---
>> > Changes since v1:
>> > - Keep custom HID driver for mousevsc but do it properly
>> > [Michael Kelley].
>> > - Add 'Fixes:' tag [Saurabh Singh Sengar].
>> > ---
>> > drivers/hid/hid-hyperv.c | 58
>> > 1 file changed, 41 insertions(+), 17 deletions(-)
>> >
>> > diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.
