Re: [Xen-devel] [RFC/WIP] xen: clk: introudce pvclk for device passthrough

[Ian Campbell]

On Tue, 2016-01-19 at 10:43 +0800, Peng Fan wrote:
> Hello Ian,
> 
> On Mon, Jan 18, 2016 at 12:41:59PM +, Ian Campbell wrote:
> > On Mon, 2016-01-18 at 11:24 +, David Vrabel wrote:
> > > On 16/01/16 05:22, Peng Fan wrote:
> > > > This patch was just a initial patch, not sure whether this way
> > > > is ok from you side for handlding clk when doing platform device
> > > > passhthrough. Any comments are appreciated, and your comments may
> > > > give me a better direction.
> > > 
> > > There's no documentation on the interface, which makes it difficult
> > > to
> > > review.  At a first look it looks very specific to the particular
> > > Linux
> > > implementation of a clk subsystem.
> > > 
> > > > --- /dev/null
> > > > +++ b/include/xen/interface/io/clkif.h
> > > > @@ -0,0 +1,41 @@
> > > > +/*
> > > > + * The code contained herein is licensed under the GNU General
> > > > Public
> > > > + * License. You may obtain a copy of the GNU General Public
> > > > License
> > > > + * Version 2 or later at the following locations:
> > > > + *
> > > > + * http://www.opensource.org/licenses/gpl-license.html
> > > > + * http://www.gnu.org/copyleft/gpl.html
> > > > + */
> > > 
> > > ABIs should be under a more permissive license so they can be used by
> > > other (non-GPLv2) operating systems.
> > 
> > ... along the same lines proposals for new ABIs should be made in the
> > form
> > of patches to xen.git:xen/include/public/io/ before being submitted as
> > an
> > implementation for one particular os.
> 
> I had no idea about this before. Do you mean that before I implement
> pvclk for linux, I need to first post the clkif part to xen devel?
> 
> If it is, I'll split the interface part and send this part to
> xen-de...@lists.xenproject.org for review.

Yes, please.

xen.git contains the canonical definition of all Xen PV protocols, copies
are then taking into OSes for implementation purposes.

> 
> > 
> > 
> > > > +   unsigned long rate;
> > > > +   char clk_name[32];
> > > 
> > > Where does the frontend get these names from?  31 character names
> > > seems
> > > rather limiting.
> > 
> > Indeed.
> > 
> > At a guess I would assume they come from the device-tree given to the
> > guest
> > and tie into the host device tree.
> 
> Yeah. the clk_name is got from DomU dts, and in Dom0 there is also a same
> name.
> 
> > 
> > I think a better model might be for each clk to have it's own
> > subdirectory
> > under the overall clock bus node, e.g. something like:
> > 
> > /local/domain/<...>/clk/0/nr-clks = 4
> > /local/domain/<...>/clk/0/clk-0/ ...
> > /local/domain/<...>/clk/0/clk-1/ ...
> > /lo
> > cal/domain/<...>/clk/0/clk-2/ ...
> > /local/domain/<...>/clk/0/clk-3/ ...
> > 
> > and for each subdirectory to contain the a node containing the
> > corresponding firmware table identifier (so path in the DT case), which
> > the toolstack knows, so it can setup the f/e and b/e to correspond as
> > necessary, and the f/e device needn't necessarily use the same name as
> > the backend/host).
> > 
> > The request would then include the index and not the name (and as David
> > observes the response only needs the id).
> 
> For now, I have not began the userspace libxl part for pvclk, I use the
> libxl pvusb code for test (:

Sure, but eventually this will need implementing in the toolstack and the
protocol design should be what is most suitable for the usecase, not what
happens to be most convenient for testing via some quick hack.

> The id acctually means what operation is needed, such as CLK_PREPARE,
> CLK_UNPREPARE, CLK_SET_RATE, CLK_GET_RATE. I'll add more text to document
> this in V2.

Ah, then for consistency with other PV protocols I would suggest renaming
your "id" as "cmd" and adding an "id" field which is simply echoed in the
response to allow the frontend to match responses to requests.

Note however that the important thing in my paragraphs above was the
decoupling of the naming from the f/e and b/e and avoiding the use of the
DT specific path in the ring requests.

The PV protocol should ideally be independent of DT (lets assume we will
want to use it for e.g. ACPI too), although there would probably in this
case need to be a binding from the DT world to the pvclk world to allow the
guest DT to remain consistent (i.e. so devices have something they can
point at which can be resolved into a pvclk).

> > 
> > I'd also like to see a description of the DT bindings, which I assume
> > must be needed such that the devices clocks property has something to
> > refer to. For example maybe it doesn't make sense for xenstore to
> > contain the path, but for the pvclk node in xenstore to contain the
> > index.
> 
> The DT bindings for xen pvclk, I use this:
> "
> clks: clks {
>   compatible = "xen,xen-clk";
>   #clock-cells = <1>;
>   clock-output-names = "uart2_root_clk";
>   };
> "
> the clock-output-names will be parsed and 

Re: [Xen-devel] [RFC/WIP] xen: clk: introudce pvclk for device passthrough

[George Dunlap]

On Sat, Jan 16, 2016 at 5:22 AM, Peng Fan  wrote:
> This patch was just a initial patch, not sure whether this way
> is ok from you side for handlding clk when doing platform device
> passhthrough. Any comments are appreciated, and your comments may
> give me a better direction.

Hey Peng,

Just speaking from the perspective of a Xen dev who's not an ARM dev:
a few more words on the relationship between pvclk and
device-passthrough would be helpful to set the context.  It sounds
like:

* On ARM, passing through a device requires a clocksource (at least
for many devices)

* dom0 has the hardware clocksource, but at the moment domUs don't
have a suitable clocksource

* This patch implements pvclk front/backend suitable for such devices

Is that right?  In which case something like the following would be helpful:

"This patch introduces pvclk, a paravirtualized clock source suitable
for devices to use when passing through to domUs on ARM systems."

(Obviously change it as necessary to make it accurate.)

Thanks,
 -George

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

Re: [Xen-devel] [RFC/WIP] xen: clk: introudce pvclk for device passthrough

[David Vrabel]

On 16/01/16 05:22, Peng Fan wrote:
> This patch was just a initial patch, not sure whether this way
> is ok from you side for handlding clk when doing platform device
> passhthrough. Any comments are appreciated, and your comments may
> give me a better direction.

There's no documentation on the interface, which makes it difficult to
review.  At a first look it looks very specific to the particular Linux
implementation of a clk subsystem.

> --- /dev/null
> +++ b/include/xen/interface/io/clkif.h
> @@ -0,0 +1,41 @@
> +/*
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */

ABIs should be under a more permissive license so they can be used by
other (non-GPLv2) operating systems.

> +
> +#ifndef __XEN_PUBLIC_IO_CLKIF_H__
> +#define __XEN_PUBLIC_IO_CLKIF_H__
> +
> +#include 
> +#include 
> +
> +/**/
> +enum {
> + XENCLK_PREPARE, /* clk_prepare_enable */
> + XENCLK_UNPREPARE,   /* clk_unprepare_disable */
> + XENCLK_GET_RATE,/* clk_get_rate */
> + XENCLK_SET_RATE,/* clk_set_rate */
> + XENCLK_END,
> +};
> +
> +struct xen_clkif_request {
> + int id;

You should use fixed width types so the ABI is the same on 32-bit and
64-bit guests.

> + unsigned long rate;
> + char clk_name[32];

Where does the frontend get these names from?  31 character names seems
rather limiting.

> +};
> +
> +struct xen_clkif_response {
> + int id;
> + int success;
> + unsigned long rate;
> + char clk_name[32];
> +};

I don't think you need to the name in the response.  The id will tie the
response to the request.

> +
> +DEFINE_RING_TYPES(xen_clkif, struct xen_clkif_request, struct 
> xen_clkif_response);
> +#define XEN_CLK_RING_SIZE __CONST_RING_SIZE(xen_clkif, PAGE_SIZE)
> +
> +#endif
> 


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

Re: [Xen-devel] [RFC/WIP] xen: clk: introudce pvclk for device passthrough

[Ian Campbell]

On Mon, 2016-01-18 at 11:24 +, David Vrabel wrote:
> On 16/01/16 05:22, Peng Fan wrote:
> > This patch was just a initial patch, not sure whether this way
> > is ok from you side for handlding clk when doing platform device
> > passhthrough. Any comments are appreciated, and your comments may
> > give me a better direction.
> 
> There's no documentation on the interface, which makes it difficult to
> review.  At a first look it looks very specific to the particular Linux
> implementation of a clk subsystem.
> 
> > --- /dev/null
> > +++ b/include/xen/interface/io/clkif.h
> > @@ -0,0 +1,41 @@
> > +/*
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> 
> ABIs should be under a more permissive license so they can be used by
> other (non-GPLv2) operating systems.

... along the same lines proposals for new ABIs should be made in the form
of patches to xen.git:xen/include/public/io/ before being submitted as an
implementation for one particular os.


> > +   unsigned long rate;
> > +   char clk_name[32];
> 
> Where does the frontend get these names from?  31 character names seems
> rather limiting.

Indeed.

At a guess I would assume they come from the device-tree given to the guest
and tie into the host device tree.

I think a better model might be for each clk to have it's own subdirectory
under the overall clock bus node, e.g. something like:

/local/domain/<...>/clk/0/nr-clks = 4
/local/domain/<...>/clk/0/clk-0/ ...
/local/domain/<...>/clk/0/clk-1/ ...
/lo
cal/domain/<...>/clk/0/clk-2/ ...
/local/domain/<...>/clk/0/clk-3/ ...

and for each subdirectory to contain the a node containing the corresponding 
firmware table identifier (so path in the DT case), which the toolstack knows, 
so it can setup the f/e and b/e to correspond as necessary, and the f/e device 
needn't necessarily use the same name as the backend/host).

The request would then include the index and not the name (and as David 
observes the response only needs the id).

As well as properly documenting the meaning of the operations 
the clkif.h should also define the xenstore nodes and contain the binary 
layouts of the req/rsp structs (see netif.h for examples of both, blkif.h also 
includes examples of the former).

I'd also like to see a description of the DT bindings, which I assume must be 
needed such that the devices clocks property has something to refer to. For 
example maybe it doesn't make sense for xenstore to contain the path, but for 
the pvclk node in xenstore to contain the index.

> > +
> > +DEFINE_RING_TYPES(xen_clkif, struct xen_clkif_request, struct
> > xen_clkif_response);
> > +#define XEN_CLK_RING_SIZE __CONST_RING_SIZE(xen_clkif, PAGE_SIZE)
> > +
> > +#endif
> > 
> 

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

Re: [Xen-devel] [RFC/WIP] xen: clk: introudce pvclk for device passthrough

[Peng Fan]

Hello David,

On Mon, Jan 18, 2016 at 11:24:08AM +, David Vrabel wrote:
>On 16/01/16 05:22, Peng Fan wrote:
>> This patch was just a initial patch, not sure whether this way
>> is ok from you side for handlding clk when doing platform device
>> passhthrough. Any comments are appreciated, and your comments may
>> give me a better direction.
>
>There's no documentation on the interface, which makes it difficult to
>review.  At a first look it looks very specific to the particular Linux
>implementation of a clk subsystem.

My bad. I'll add more text in commit log or cover letter in V2 to describe
the interface.

>
>> --- /dev/null
>> +++ b/include/xen/interface/io/clkif.h
>> @@ -0,0 +1,41 @@
>> +/*
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>
>ABIs should be under a more permissive license so they can be used by
>other (non-GPLv2) operating systems.

ok. I'll change this.

>
>> +
>> +#ifndef __XEN_PUBLIC_IO_CLKIF_H__
>> +#define __XEN_PUBLIC_IO_CLKIF_H__
>> +
>> +#include 
>> +#include 
>> +
>> +/**/
>> +enum {
>> +XENCLK_PREPARE, /* clk_prepare_enable */
>> +XENCLK_UNPREPARE,   /* clk_unprepare_disable */
>> +XENCLK_GET_RATE,/* clk_get_rate */
>> +XENCLK_SET_RATE,/* clk_set_rate */
>> +XENCLK_END,
>> +};
>> +
>> +struct xen_clkif_request {
>> +int id;
>
>You should use fixed width types so the ABI is the same on 32-bit and
>64-bit guests.

Will fix in V2.

>
>> +unsigned long rate;
>> +char clk_name[32];
>
>Where does the frontend get these names from?  31 character names seems
>rather limiting.

The clk_name is got from DomU device tree. In the commit log, I wrote this:
"
clks: clks {
compatible = "xen,xen-clk";
#clock-cells = <1>;
clock-output-names = "uart2_root_clk";
};
"
The clk_name is one of the entry from clock-output-names, clk_name will
be passed to Dom0, Dom0 will use the clk_name to find the clk structure using
__clk_lookup.


>
>> +};
>> +
>> +struct xen_clkif_response {
>> +int id;
>> +int success;
>> +unsigned long rate;
>> +char clk_name[32];
>> +};
>
>I don't think you need to the name in the response.  The id will tie the
>response to the request.

My original idea is to  use id and clk_name to check whether the response is 
for the requester.
You can see in the patch:
" if ((id == comp->id) && !strncmp(name, comp->clk_name, 
sizeof(comp->clk_name)))"

Maybe clk_name is redundant here. I will remove it in V2.

Thanks for your comments.

Peng.

>
>> +
>> +DEFINE_RING_TYPES(xen_clkif, struct xen_clkif_request, struct 
>> xen_clkif_response);
>> +#define XEN_CLK_RING_SIZE __CONST_RING_SIZE(xen_clkif, PAGE_SIZE)
>> +
>> +#endif
>> 
>

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

Re: [Xen-devel] [RFC/WIP] xen: clk: introudce pvclk for device passthrough

[Peng Fan]

Hello George,

On Mon, Jan 18, 2016 at 11:22:44AM +, George Dunlap wrote:
>On Sat, Jan 16, 2016 at 5:22 AM, Peng Fan  wrote:
>> This patch was just a initial patch, not sure whether this way
>> is ok from you side for handlding clk when doing platform device
>> passhthrough. Any comments are appreciated, and your comments may
>> give me a better direction.
>
>Hey Peng,
>
>Just speaking from the perspective of a Xen dev who's not an ARM dev:
>a few more words on the relationship between pvclk and
>device-passthrough would be helpful to set the context.  It sounds
>like:
>
>* On ARM, passing through a device requires a clocksource (at least
>for many devices)
>
>* dom0 has the hardware clocksource, but at the moment domUs don't
>have a suitable clocksource
>
>* This patch implements pvclk front/backend suitable for such devices
>
>Is that right?  In which case something like the following would be helpful:

Yeah. right. You have a better explaination than me :)

>
>"This patch introduces pvclk, a paravirtualized clock source suitable
>for devices to use when passing through to domUs on ARM systems."
>
>(Obviously change it as necessary to make it accurate.)

I'll add your words into the commit log.

Thanks,
Peng.

>
>Thanks,
> -George

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

Re: [Xen-devel] [RFC/WIP] xen: clk: introudce pvclk for device passthrough

[Peng Fan]

Hello Ian,

On Mon, Jan 18, 2016 at 12:41:59PM +, Ian Campbell wrote:
>On Mon, 2016-01-18 at 11:24 +, David Vrabel wrote:
>> On 16/01/16 05:22, Peng Fan wrote:
>> > This patch was just a initial patch, not sure whether this way
>> > is ok from you side for handlding clk when doing platform device
>> > passhthrough. Any comments are appreciated, and your comments may
>> > give me a better direction.
>> 
>> There's no documentation on the interface, which makes it difficult to
>> review.  At a first look it looks very specific to the particular Linux
>> implementation of a clk subsystem.
>> 
>> > --- /dev/null
>> > +++ b/include/xen/interface/io/clkif.h
>> > @@ -0,0 +1,41 @@
>> > +/*
>> > + * The code contained herein is licensed under the GNU General Public
>> > + * License. You may obtain a copy of the GNU General Public License
>> > + * Version 2 or later at the following locations:
>> > + *
>> > + * http://www.opensource.org/licenses/gpl-license.html
>> > + * http://www.gnu.org/copyleft/gpl.html
>> > + */
>> 
>> ABIs should be under a more permissive license so they can be used by
>> other (non-GPLv2) operating systems.
>
>... along the same lines proposals for new ABIs should be made in the form
>of patches to xen.git:xen/include/public/io/ before being submitted as an
>implementation for one particular os.

I had no idea about this before. Do you mean that before I implement
pvclk for linux, I need to first post the clkif part to xen devel?

If it is, I'll split the interface part and send this part to
xen-de...@lists.xenproject.org for review.

>
>
>> > +  unsigned long rate;
>> > +  char clk_name[32];
>> 
>> Where does the frontend get these names from?  31 character names seems
>> rather limiting.
>
>Indeed.
>
>At a guess I would assume they come from the device-tree given to the guest
>and tie into the host device tree.

Yeah. the clk_name is got from DomU dts, and in Dom0 there is also a same
name.

>
>I think a better model might be for each clk to have it's own subdirectory
>under the overall clock bus node, e.g. something like:
>
>/local/domain/<...>/clk/0/nr-clks = 4
>/local/domain/<...>/clk/0/clk-0/ ...
>/local/domain/<...>/clk/0/clk-1/ ...
>/lo
>cal/domain/<...>/clk/0/clk-2/ ...
>/local/domain/<...>/clk/0/clk-3/ ...
>
>and for each subdirectory to contain the a node containing the corresponding 
>firmware table identifier (so path in the DT case), which the toolstack knows, 
>so it can setup the f/e and b/e to correspond as necessary, and the f/e device 
>needn't necessarily use the same name as the backend/host).
>
>The request would then include the index and not the name (and as David 
>observes the response only needs the id).

For now, I have not began the userspace libxl part for pvclk, I use the libxl 
pvusb code for test (:
The id acctually means what operation is needed, such as CLK_PREPARE, 
CLK_UNPREPARE, CLK_SET_RATE, CLK_GET_RATE. I'll add more text to document this 
in V2.

>
>As well as properly documenting the meaning of the operations 
>the clkif.h should also define the xenstore nodes and contain the binary 
>layouts of the req/rsp structs (see netif.h for examples of both, blkif.h also 
>includes examples of the former).

ok. I'll take netif/blkif for reference.

>
>I'd also like to see a description of the DT bindings, which I assume must be 
>needed such that the devices clocks property has something to refer to. For 
>example maybe it doesn't make sense for xenstore to contain the path, but for 
>the pvclk node in xenstore to contain the index.

The DT bindings for xen pvclk, I use this:
"
clks: clks {
compatible = "xen,xen-clk";
#clock-cells = <1>;
clock-output-names = "uart2_root_clk";
};
"
the clock-output-names will be parsed and be registered as clk root. The device 
will use index to refer to the clk, as the following:
"
clocks = < 0>, < 0>;
clock-names = "ipg", "per";
"
0 means the first one in clock-output-names.

To the xenstore part, I am not sure whether need to expose the clock relate 
info to xenstore. I just want to store the clock names in xenstore to let
user know what clocks are now handled by DomU.

How about the following?

doamin1:
/local/domain/1/device/vclk/nr-clks
/local/domain/1/device/vclk/clk-0/name
/local/domain/1/device/vclk/clk-1/name

domain2:
/local/domain/2/device/vclk/nr-clks
/local/domain/2/device/vclk/clk-0/name
/local/domain/2/device/vclk/clk-1/name

domain0:
/local/domain/0/backend/vclk/1/0/nr-clks
/local/domain/0/backend/vclk/1/0/clk-0/name
/local/domain/0/backend/vclk/1/0/clk-1/name
/local/domain/0/backend/vclk/1/1/nr-clks
/local/domain/0/backend/vclk/1/1/clk-0/name
/local/domain/0/backend/vclk/1/1/clk-1/name

Or do not store the name and nr-clks in domain0?

Thanks,
Peng.

>
>> > +
>> > +DEFINE_RING_TYPES(xen_clkif, struct xen_clkif_request, struct
>> > xen_clkif_response);
>> > +#define