Messages in this thread | | | From | Vitaly Kuznetsov <> | Subject | RE: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver | Date | Wed, 13 Mar 2019 15:23:05 +0100 |
| |
Michael Kelley <mikelley@microsoft.com> writes:
> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, March 13, 2019 1:28 AM >> >> > Clockevents code for Hyper-V synthetic timers is currently mixed >> > in with other Hyper-V code. Move the code to a Hyper-V specific >> > driver in the "clocksource" directory. Update the VMbus driver >> > to call initialization and cleanup routines since the Hyper-V >> > synthetic timers are not independently enumerated in ACPI. >> > >> >> I like the idea! Would it also make sense to consider moving Hyper-V >> clocksource from arch/x86/hyperv/hv_init.c? The thing is that we use it >> from arch-independent code (e.g. seee 'hyperv_cs' usage in >> drivers/hv/hv_util.c). > > That's what the second patch in the series does. :-) But let me > know if there's something you think I've missed. >
Oh, sorry, it's just me - your subject lines messed with my brain :-) I like your idea even more then!
>> >> Nitpicking: >> >> This has nothing to do with your patch but we have a mix of 'stimer', >> 'timer', 'syntimer' names when we refer to Hyper-V Synthetic timers, it >> may make sense to try to converge on something (stimer would probably be >> my personal preference but I don't really care as long as we use the >> same abbreviation). Examples: >> >> hv_init_timer_config(...) >> HV_MSR_SYNTIMER_AVAILABLE >> HYPERV_STIMER0_VECTOR >> hv_syntimer_init(...) >> ... > > Yes, you are right about the mish-mash of names. I also like converging > on "stimer". I'll certainly change things like hv_syntimer_init() to > hv_stimer_init() as part of the patch and avoid making the problem > worse. > > What about the name of the new .c and .h files? They include code > both the stimer-based clockevents, as well as the Hyper-V > reference time source based clocksource. Stay with "syntimer" or > shorten to "stimer", even though the code is slightly broader than > just the stimers? (which highlights the generic Linux naming issue that > "clocksource drivers" include code for both clockevents and > clocksources)
"hyperv_timers.*" maybe? (those who read TLFS may get confused because 'Synthetic timers' doesn't refer to clocksources - just to clockevents).
> >> >> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile >> > index be6e0fb..a887955 100644 >> > --- a/drivers/clocksource/Makefile >> > +++ b/drivers/clocksource/Makefile >> > @@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER) += timer-atcpit100.o >> > obj-$(CONFIG_RISCV_TIMER) += timer-riscv.o >> > obj-$(CONFIG_CSKY_MP_TIMER) += timer-mp-csky.o >> > obj-$(CONFIG_GX6605S_TIMER) += timer-gx6605s.o >> > +obj-$(CONFIG_HYPERV) += hyperv_syntimer.o >> >> (just a couple of spare thoughs) >> >> CONFIG_HYPERV can also be a module, are we OK with that? (we'll have to >> support module loading/unloading then and honestly I see no reason for >> that. I would prefer everything but VMBus devices to be in >> kernel.) If we don't want it to be a module we can create a hidden >> CONFIG_HYPERV_STIMER or something like that - just like we already do >> for CONFIG_HYPERV_TSCPAGE. >> >> There is, however, one additional dependency here: when running in >> non-direct mode, Hyper-V clockevent devices require functional Hyper-V >> messaging - which currently lives in VMBus code so that may explain why >> you may want to keep stimer code in the same entity. Or, alternatively, >> we can move Hyper-V messaging out of VMBus code (is it actually >> architecture-agnostic?) >> > > I thought about introducing CONFIG_HYPERV_STIMER, but in my > judgment it was just unnecessary complexity. The Hyper-V clocksource > driver can't exist independent of Hyper-V, and vice versa. When both the > clocksource and clockevents code is considered, the VMbus driver and > Hyper-V initialization code has to call directly into the driver since the > Hyper-V synthetic timers and reference time counter aren't independently > enumerated. Even if we could get the Hyper-V messaging out of VMbus > code, we would still need the clocksource initialization call directly from > hyperv_init(), which is not in a module (see the 2nd patch of the series). >
Right, so hv_init_clocksource() cannot live in hv_vmbus module and we need to somehow prevent hyperv_syntimer.o from going in there. And
+obj-$(CONFIG_HYPERV) += hyperv_syntimer.o
will do exactly the opposite - put it in hv_vmbus module. Or am I missing something? (I haven't tried to build your code yet, sorry).
> Michael
-- Vitaly
| |