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

[Juergen Gross <jgross@suse.com>]

On 20/01/16 12:48, Peng Fan wrote:
> Hi Juergen,
> 
> On Wed, Jan 20, 2016 at 11:40:55AM +0100, Juergen Gross wrote:
>> On 20/01/16 10:25, Peng Fan wrote:
>>> Hi Juergen,
>>>
>>> On Wed, Jan 20, 2016 at 10:05:15AM +0100, Juergen Gross wrote:
>>>> On 20/01/16 09:31, Peng Fan wrote:
>>>>> Introduce pvclk interface which is useful when doing device passthrough
>>>>> on ARM platform.
>>>>
>>>> ...
>>>>
>>>>> +/*
>>>>> + * Frontend request
>>>>> + *
>>>>> + * cmd: command for operation on clk, should be XEN_CLK_[xx],
>>>>> + *	excluding XEN_CLK_END. id is the
>>>>> + * id: clk id
>>>>> + * rate: clock rate. Used for set rate.
>>>>
>>>> Which unit? Hz?
>>>
>>> Yeah. Hz. I'll add comments in V3.
>>>
>>>>
>>>>> + */
>>>>> +struct xen_clkif_request {
>>>>> +	uint32_t cmd;
>>>>> +	uint32_t id;
>>>>> +	uint64_t rate;
>>>>> +};
>>>>> +typedef struct xen_clkif_request xen_clkif_request_t;
>>>>> +
>>>>> +/*
>>>>> + * 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.
>>>>
>>>> Values?
>>>
>>> I'd like to let the frontend/backend driver to determine the value.
>>> In my implementation for linux, if the backend API supports return value,
>>> I'll fill the return value to success entry. If the backend API returns
>>> void, I'll just fill 0 to success entry.
>>
>> So please specify "0" to mean success and add some sensible error
>> values. There might be multiple frontend- and/or backend-variants which
>> all must agree using the same interface.
> 
> Will change this to `int status`, as also observed by Jan.
> I'll also define macros such as "#define XEN_CLK_ENABLE_OK 0", "#define XEN_CLK_ENABLE_FAILURE -1"
> 
>>
>>>>> + * 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.
>>>>
>>>> I'd rather let the frontend add a request Id to the request which
>>>> will be echoed here instead cmd and clock Id.
>>>
>>> If using request id, I think we can encode cmd and id into request id?
>>
>> This should just be an opaque value for the backend. The frontend is
>> free how to create this value, it should be unique for every outstanding
>> request of a domU, however. It could be built from cmd and id in case
>> there can't be multiple requests with the same cmd/id combination
>> active in a domU.
> 
> Thanks for teaching me on this. So the request id should be globally unique
> for backend. So "random value(generated in frontend) + frontend domid" is ok for this?

Being unique per frontend is enough, as each frontend will only ever see
responses for it's own requests. Make it as simple as possible. I guess
there will be a maximum of active requests possible, e.g. the number of
request slots in the request ring. You could use the index into the ring
as Id (pvSCSI frontend is doing it this way).

Juergen