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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Michael Kelley <mikelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, March 13, 2019 7:23 AM
>
>> >> > 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).
>> 
>
> That line just controls whether hyperv_syntimer.o is built.  It doesn't put
> it in the hv_vmbus module.  All of the clocksource .o files that are built go
> into the kernel, not in a module.  But thinking about it more, the above works
> correctly when CONFIG_HYPERV=y, but not when CONFIG_HYPERV=m.

Yes, that's what I meant.

> I'll have to introduce CONFIG_HYPERV_TIMER after all.  Will fix this in v2.  Thanks
> for the discussion!

Thanks!

-- 
Vitaly