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

[Michael Kelley]

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.

> 
> 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)

> 
> > 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).

Michael