Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node.

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2013-11-05 at 14:54 -0500, Jon Fraser wrote:
> On Tue, 2013-11-05 at 10:04 +0000, Ian Campbell wrote:
> > On Tue, 2013-11-05 at 09:56 +0000, Ian Campbell wrote:
> > > On Mon, 2013-11-04 at 13:35 -0500, Jon Fraser wrote:
> > > > On Mon, 2013-11-04 at 17:12 +0000, Ian Campbell wrote:
> > > > > On Fri, 2013-11-01 at 16:58 -0400, Jon Fraser wrote:
> > > > > > When creating the CPU DT node, copy the clock-frequency if present.
> > > > > > 
> > > > ...
> > > > > Julien's the expert but I think you need to use dt_property_read_u32
> > > > > here, to get the correct endianness conversion (as well as for pure
> > > > > forms sake of using the correct API for the job).
> > > > > 
> > > > I'll fix that.
> > > > 
> > > > > >              break;
> > > > > >          }
> > > > > >      }
> > > > > > @@ -457,6 +459,12 @@ static int make_cpus_node(const struct domain *d, void *fdt,
> > > > > >          if ( res )
> > > > > >              return res;
> > > > > >  
> > > > > > +        if (clock_frequency) {
> > > > > > +            res = fdt_property_cell(fdt, "clock-frequency", *(u32 *)clock_frequency);
> > > > > 
> > > > > I suppose there ought to be some API for this side of things too, but I
> > > > > can't see it right now...
> > > > > 
> > > > > Note that fdt_property_cell contains a cpu_to_fdt32 so it is converting
> > > > > while the read of the property not, so I think the code is broken as is?
> > > > 
> > > > Yesss, it is broken.  When I checked the property in /proc/device-tree,
> > > > I failed to realize it was endian swapped.
> > > 
> > > I guess nothing much critical is relying on this value. What is it
> > > supposed to be used for?
> > > linux/Documentation/devicetree/booting-without-of.txt seems to imply it
> > > is mostly optional for non-PPC.
> > 
> > BTW, I'm asking because I'm not sure if exposing the underlying clock
> > speed to the guest VCPU is the right thing to do. It can vary across
> > pCPUs (e.g. big.LITTLE) and with power control etc.
> > 
> > Ian.
> 
> As far as I can tell, it is intended to be used for load balancing
> for the case of differing cpu capabilities.   Looking down the road,
> if I was doing Power Management in Dom0, I would want to know
> the cpu topology and cpu speeds, but that info is probably available by
> other means.
> 
> I just wanted to squelch the annoying linux kernel error messages.
> 
> /cpus/cpu@0 missing clock-frequency property
> /cpus/cpu@1 missing clock-frequency property
> /cpus/cpu@2 missing clock-frequency property
> /cpus/cpu@3 missing clock-frequency property

Ah, I started noticing them too recently but didn't connect them with
this patch.

> Do you foresee, at this point, any problem with giving all the guest
> VCPUs the same clock frequency setting?

Do you mean same as in give all VCPUs a static hardcoded frequency or
what you did here passing through something real?

I think in general I would prefer to passthrough something real as you
have done. I'm always a little bit wary of accidentally creating an ABI
which we end up forced to support. Adding a hardcoded value could be
seen to do that, whereas passing through is going to naturally vary. One
would hope noone would ever rely on a hardcoded clock-frequency anyway.

I'm not sure what to do about domU, since I don't think the physical CPU
frequency is necessarily easily available to the toolstack. We have a
similar issue with the CPU compatiblity string. I guess that all needs
to be plumbed up. :-( (not your problem here though, so don't worry!)

Ian.