Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor

[Dirk Behme <dirk.behme@xxxxxxxxxxxx>]

On 08.07.2016 04:50, Michael Turquette wrote:
> Quoting Dirk Behme (2016-07-07 00:32:34)
> > Hi Michael,
> >
> > On 06.07.2016 22:42, Michael Turquette wrote:
> > > Hi Julien,
> > >
> > > Quoting Julien Grall (2016-07-06 06:10:52)
> > > > On 06/07/16 02:34, Michael Turquette wrote:
> > > > > Hi!
> > > > Hello Michael,
> > > >
> > > > > Quoting Dirk Behme (2016-06-30 03:32:32)
> > > > > > Some clocks might be used by the Xen hypervisor and not by the Linux
> > > > > > kernel. If these are not registered by the Linux kernel, they might be
> > > > > > disabled by clk_disable_unused() as the kernel doesn't know that they
> > > > > > are used. The clock of the serial console handled by Xen is one
> > > > > > example for this. It might be disabled by clk_disable_unused() which
> > > > > > stops the whole serial output, even from Xen, then.
> > > > > This whole thread had me confused until I realized that it all boiled
> > > > > down to some nomenclature issues (for me).
> > > > >
> > > > > This code does not _register_ any clocks. It simply gets them and
> > > > > enables them, which is what every other clk consumer in the Linux kernel
> > > > > does. More details below.
> > > > >
> > > > > > Up to now, the workaround for this has been to use the Linux kernel
> > > > > > command line parameter 'clk_ignore_unused'. See Xen bug
> > > > > >
> > > > > > http://bugs.xenproject.org/xen/bug/45
> > > > > clk_ignore_unused is a band-aid, not a proper medical solution. Setting
> > > > > that flag will not turn clocks on for you, nor will it guarantee that
> > > > > those clocks are never turned off in the future. It looks like you
> > > > > figured this out correctly in the patch below but it is worth repeating.
> > > > >
> > > > > Also the new CLK_IS_CRITICAL flag might be of interest to you, but that
> > > > > flag only exists as a way to enable clocks that must be enabled for the
> > > > > system to function (hence, "critical") AND when those same clocks do not
> > > > > have an accompanying Linux driver to consume them and enable them.
> > > > I don't think we want the kernel to enable the clock for the hypervisor.
> > > > We want to tell the kernel "don't touch at all to this clock, it does
> > > > not belong to you".
> > > But the patch *does* touch the clock from the kernel. It enables the
> > > clock via a call clk_prepare_enable. I'm utterly confused.
> >
> > Maybe we need some advice here :)
> Sure!
>
> > I've used clk_prepare_enable() 'just' to get the enable count incremented
> clk_prepare_enabled will *enable* the clock signal if it currently
> disabled or gated. In other words, if the physical line is not toggling
> before the call, it will be after the call returns.
>
> > http://lxr.free-electrons.com/source/drivers/clk/clk.c#L751
> >
> > Because it's my understanding that enable_count is needed to prevent
> > clk_disable_unused() from disabling the clock.
> Having a positive enable_count will prevent the clock from being
> disabled by both clk_disable_unused AND from the Sneaky Sibling
> Attack(tm).
>
> The Sneaky Sibling Attack(tm) occurs when clock A and clock B are
> siblings and share the same parent, clock C. If clock A is enabled in
> hardware (by bootloader, firmware or hypervisor), but does NOT have a
> positive enable_count (in Linux), then it is possible that a driver
> might call clk_enable(clk_B) then clk_disable(clk_B), which will result
> in the disable action propagating up the parent chain and disabling
> clk_C, the shared parent. This will of course gate clk_A, which is
> clocked by clk_C, breaking things for you.
>
> So you need to be worried about more than just clk_disable_unused.
>
> The simple fact is is that if a piece of software knows that it needs
> for its clock to be enabled, it should actively enable it with
> clk_prepare_enable.
>
> Doing some weird stuff with CLK_IGNORE_UNUSED or anything else is just
> hoping that your clock will not be disabled, and that is the wrong
> strategy.
>
> > If there is an other / better / correct way to achieve that, please let
> > us know.
> Well, you should not "try to prevent a clock from being disabled", you
> should "enable the clock that you need to use".
>
> > I've had a look to use the CLK_IGNORE_UNUSED flag, too. But couldn't
> > find a function exported by the clock framework to set that flag (?)
> Right, the flags are immutable and must be set by the clock provider
> driver before registering the clock. Toggling flags at run-time is a
> misuse of the flags, and clock consumer drivers should never care about
> the flags. They are internal to the clock framework.
>
> In conclusion, I think that your patch does the right thing. The Xen
> node consumes the clocks that it needs to manage and it calls
> clk_prepare_enable on them.

Ok, thanks!


> The two issues to resolve are:
>
> 1) does consuming these hardware resources from the Linux kernel fit
> correctly with the Xen model?

I think so. Julien, what do you think?


> 2) the language of the binding description makes this way more confusing
> than it needs to be. Just claim the resources you need and enable them,
> which is an OS-agnostic action.

I'll post a v3 patch trying to update the description with your proposals.


Best regards

Dirk

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel