Re: [Xen-devel] [RFC V2] xen: interface: introduce pvclk interface

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.01.16 at 15:05, <van.freenix@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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.

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

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

Jan


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