RE: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver

[Vitaly Kuznetsov]

Michael Kelley <mikelley@xxxxxxxxxxxxx> writes:

> From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>  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