Re: [RFC V2] xen: interface: introduce pvclk interface

[Peng Fan <van.freenix@gmail.com>]

Hi Jan,

On Wed, Jan 20, 2016 at 07:16:36AM -0700, Jan Beulich wrote:
>>>> On 20.01.16 at 15:05, <van.freenix@gmail.com> wrote:
>> Hi Jan,
>> On Wed, Jan 20, 2016 at 05:01:40AM -0700, Jan Beulich wrote:
>>>>>> On 20.01.16 at 12:40, <van.freenix@gmail.com> wrote:
>>>> Hi Jan,
>>>> 
>>>> On Wed, Jan 20, 2016 at 03:14:20AM -0700, Jan Beulich wrote:
>>>>>>>> On 20.01.16 at 09:31, <van.freenix@gmail.com> wrote:
>>>>>> +/*
>>>>>> + * Backend response
>>>>>> + *
>>>>>> + * cmd: command for operation on clk, same with the cmd in request.
>>>>>> + * id: clk id, same with the id in request.
>>>>>> + * success: indicate failure or success for the cmd.
>>>>>> + * rate: clock rate. Used for get rate.
>>>>>> + *
>>>>>> + * cmd and id are filled by backend and passed to frontend to
>>>>>> + * let frontend check whether the response is for the current
>>>>>> + * request or not.
>>>>>> + */
>>>>>> +struct xen_clkif_response {
>>>>>> +	uint32_t cmd;
>>>>>> +	uint32_t id;
>>>>>> +	uint32_t success;
>>>>>> +	uint64_t rate;
>>>>>> +};
>>>>>
>>>>>This isn't 32-/64-bit clean. Question is whether echoing cmd is really
>>>>>needed.
>>>> 
>>>> As Juergen suggested, use a request id. I'll fix this in V3.
>>>> 32-/64-bit unclean, I can not get you here (:
>>>
>>>The layout of above structure may end up different for 32- and
>>>64-bit guests, depending on the alignment of uint64_t.
>> 
>> Thanks for teaching me. In V3, the layout will be changed to like this
>> struct xen_clkif_response {
>> 	uint32_t request_id;
>> 	int32_t status;
>> 	uint64_t rate;
>> };
>
>Okay. Just as a not - iirc other pv interfaces use 64-bit ID values,
>so not doing this here perhaps ought to be justified.

oh. I see uint64_t id in blkif.h :). Thanks.

>
>> And more macro definitions for the status entry:
>> #define XEN_CLK_OP_SUCCESS 0
>> #define XEN_CLK_ENABLE_FALIURE -1
>> #define XEN_CLK_DISABLE_FAILURE -2
>> #define XEN_CLK_PREPARE_FAILURE -3
>> #define XEN_CLK_UNPREPARE_FAILURE -4
>> #define XEN_CLK_SET_RATE_FAILURE -5
>> #define XEN_CLK_GET_RATE_FALIURE -6
>
>That's bogus - different kinds of errors would be meaningful,
>but an error per command is quite pointless imo. (Also please
>be aware of typos and parenthesization.)

Will fine tune this in V3.

>
>>>>>And what I'm missing throughout the file is some description of how
>>>>>clock events (interrupts?) are actually meant to make it into the
>>>>>guest.
>>>> 
>>>> I have a simple implementation v1 patch for linux, 
>>>> http://lists.xen.org/archives/html/xen-devel/2016-01/msg01860.html.
>>>> You can see how it works.
>>>
>>>No, sorry, that's not the point of the inquiry. It seems to me that
>>>there are more aspects to this interface, not directly related to
>>>the request/reply protocol written down here, which need to be
>>>spelled out event if they don't require any particular definitions
>>>or type declarations.
>> 
>> I try to follow you about clock events(interrupts?), not sure whether I 
>> understand correct or not.
>> clock in this file is the clk for a device. In linux side, it managed by clk 
>> subsystem, drivers/clk/xx.
>> This is not the clock events or clock source in drivers/clocksource/xx.
>> For the pvclk interface, there will be no interrupt for the guest.
>
>Then (also considering the set of commands you propose) what
>use is the clock to the guest? It can't get events from it, it can't
>read its current value, all it can is get/set its rate, enable/disable,
>and prepare/unprepare it. I may be lacking some ARM knowledge
>here, but all of this looks pretty odd to me.

I follow this https://events.linuxfoundation.org/sites/events/files/slides/talk_5.pdf to do platform device
passthrough on ARM platform. I met the same issue as note in the ppt, at page 24.

In my test case, the uart driver works well without xen. There is serveral function calls in the driver, such as
clk = clk_get("uart2_root"),rate = clk_get_rate(clk), use rate to set baudrate for uart.


There is a clk tree in kernel without XEN or dom0 kernel like the following (only an example):
osc -
    |-->pll1
    |-->pll2
         |-->pll2_div
	         |-->uart2_gate
		         |-->uart2_root  --> clk for uart2

uart2_root is directly handled by Dom0.If I assign uart2 to DomU, DomU does
not have the clk tree as above, so DomU can not handle directly handle uart2_root and the uart2 driver in
DomU will run into failure to intialize the uart2 hardware IP.

So I introudce pvclk. Pass the operation for uart2_root in DomU to Dom0 and Dom0 directly handle the clock management hardware IP.

Hope this is clear.

Thanks,
Peng.

>
>Jan
>