Date:   Wed, 13 Mar 2019 15:23:05 +0100
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Michael Kelley <mikelley@...rosoft.com>
Cc:     "catalin.marinas\@arm.com" <catalin.marinas@....com>,
        "mark.rutland\@arm.com" <mark.rutland@....com>,
        "will.deacon\@arm.com" <will.deacon@....com>,
        "marc.zyngier\@arm.com" <marc.zyngier@....com>,
        "linux-arm-kernel\@lists.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "gregkh\@linuxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv\@vger.kernel.org" <linux-hyperv@...r.kernel.org>,
        "olaf\@aepfle.de" <olaf@...fle.de>,
        "apw\@canonical.com" <apw@...onical.com>,
        "jasowang\@redhat.com" <jasowang@...hat.com>,
        "marcelo.cerri\@canonical.com" <marcelo.cerri@...onical.com>,
        Sunil Muthuswamy <sunilmut@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>
Subject: RE: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver

Michael Kelley <mikelley@...rosoft.com> writes:

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

Powered by blists - more mailing lists