[patch 3/3] PS3: Add logical performance monitor driver support

[Geoff Levand]

On 01/05/2008 03:56 AM, Arnd Bergmann wrote:
> On Saturday 05 January 2008, Geoff Levand wrote:
>> From: Takashi Yamamoto <Takashi02_Yamamoto at hq.scei.sony.co.jp>
>> 
>> Add PS3 logical performance monitor (lpm) device driver.
>> 
>> The PS3's LV1 hypervisor provides a Logical Performance Monitor that
>> abstarcts the Cell processor's performance monitor features for use
>> by guest operating systems.
>> 
>> Signed-off-by: Takashi Yamamoto <Takashi02_Yamamoto at hq.scei.sony.co.jp>
>> Signed-off-by: Geoff Levand <geoffrey.levand at am.sony.com>
> 
> Wow, great stuff!
> 
> One bit of information I'm missing in the description is what the users
> of this code are. Your Kconfig text mentions that can be used for both
> perfmon and oprofile. Are these, or one of them, currently under
> work as well?

Yes, perfmon2 support is somewhat working, but still under development.
I added the perfmon2 patches into ps3-linux.git, so the easy way would
be to use those kernel sources.  This info written before I merged
perfmon2 into ps3-linux.git, but does give some info on how to use
it:

  http://www.kernel.org/pub/linux/kernel/people/geoff/cell/CELL-Linux-CL_20071220-ADDON/doc/ApplicationProgrammingEnvironment.html

I just started to work on the kernel oprofile support, but it is not
usable yet.  Any contributions would be welcome.  Currently the patch
is just something hacked up to use for testing:

  http://git.kernel.org/?p=linux/kernel/git/geoff/ps3-linux-patches.git;a=blob;f=ps3-wip/ps3-oprofile.patch;hb=HEAD

>> --- a/arch/powerpc/platforms/ps3/Kconfig
>> +++ b/arch/powerpc/platforms/ps3/Kconfig
>> @@ -138,4 +138,18 @@ config PS3_FLASH
>>  	  be disabled on the kernel command line using "ps3flash=off", to
>>  	  not allocate this fixed buffer.
>>  
>> +config PS3_LPM
>> +	tristate "PS3 Logical Performance Monitor support"
>> +	depends on PPC_PS3
>> +	default n
> 
> 'default n' is redundant.
> 
>> +	help
>> +	  Include support for the PS3 Logical Performance Monitor.
>> +
>> +	  This support is required to use the logical performance monitor
>> +	  of the PS3's LV1 hypervisor.
>> +
>> +	  If you intend to use the advanced performance monitoring and
>> +	  profiling support of the Cell processor with programs like
>> +	  oprofile and perfmon, then say Y or M, otherwise say N.
>> +
> 
> Should that be perfmon2 instead of perfmon?

Yes.

> I think once they high-level drivers are merged, it would make sense
> to autoselected by these, so the user doesn't have to read through
> the help text.

Yes, many things can be improved.

>> +/**
>> + * struct ps3_lpm_priv - private lpm device data.
>> + *
>> + * @mutex: Open/close mutex.
>> + * @rights: The lpm rigths granted by the system policy module.
>> + * @pu_id: The lv1 id of the BE prosessor for this lpm instance.
>> + * @lpm_id: The lv1 id of this lpm instance.
>> + * @outlet_id: The outlet created by lv1 for this lpm instance.
>> + * @constructed: A flag indicating the lpm driver has been opened -- can we just use (lpm_id == ???)
>> + * @tb_size: The lv1 trace buffer size
>> + * @tb_cache: Trace buffer cache
>> + * @tb_cache_internal: A flag indicating the trace buffer cache was allocated
>> + *                     by the driver.
>> + * @tb_cache: Trace buffer cache
>> + * @sizeof_traced_data: Traced data size
>> + * @sbd: the struct ps3_system_bus_device attached to this driver
>> + */
>> +
>> +struct ps3_lpm_priv {
>> +	struct mutex mutex;
>> +	u64 rights;
>> +	u64 pu_id;
>> +	u64 lpm_id;
>> +	u64 outlet_id;
>> +	int constructed;
>> +	u64 tb_size;
>> +	void *tb_cache;
>> +	u64 tb_cache_size;
>> +	int tb_cache_internal;
>> +	u64 sizeof_traced_data;
>> +	u64 sizeof_total_copied_data;
>> +	u64 shadow_pm_control;
>> +	u64 shadow_pm_start_stop;
>> +	u64 shadow_pm_interval;
>> +	u64 shadow_group_control;
>> +	u64 shadow_debug_bus_control;
>> +	struct ps3_system_bus_device *sbd;
>> +};
> 
> Some of the members in this struct are not present in the comment,
> which is somewhat confusing.

I'll add those in for the next post.

>> +
>> +/**
>> + * lpm_priv - Static instance of the lpm data.
>> + *
>> + * Since the exported routines don't support the notion of a device
>> + * instance we need to hold the instance in this static variable
>> + * and only allow at most one instance to be created.
>> + */
>> +
>> +static struct ps3_lpm_priv *lpm_priv;
> 
> Hmmm, my gut feeling about this is that it would be better to actually
> pass this around through the exported functions. 

Yes, I feel the same way.  In general, I think we need to make a
better platform abstraction of the processor's pm functions.  That
would include support for platform specific instance data, RTAS
for blade, lpm for ps3.

> That would also
> provide some arbitration between the possible users, e.g. oprofile
> would get -EBUSY when asking for the ps3_lpm while perfmon2 is
> already active.

That already happens in ps3_lpm_open(), using the lpm_priv.constructed
variable.

>> +inline u32 ps3_get_hw_thread_id(int cpu)
>> +{
>> +	return cpu;
>> +}
>> +EXPORT_SYMBOL_GPL(ps3_get_hw_thread_id);
> 
> The 'inline' is bogus, as that doesn't work across exported functions
> and you are not using the function in this file.
> 
> Should you perhaps return hard_smp_processor_id(cpu) instead of cpu
> here?

Yes, good point.

>> +/**
>> + * _ps3_copy_trace_buffer - Copy the trace buffer.
>> + */
>> +
>> +static u64 _ps3_copy_trace_buffer(u64 offset, u64 size, u64 *to, int to_user)
>> +{
>> +	int result;
>> +	u64 sizeof_copied_data;
>> +
>> +	if (offset >= lpm_priv->sizeof_traced_data)
>> +		return 0;
>> +
>> +	result = lv1_copy_lpm_trace_buffer(lpm_priv->lpm_id, offset, size,
>> +					&sizeof_copied_data);
>> +	if (result) {
>> +		dev_err(sbd_core(), "%s:%u: lv1_copy_lpm_trace_buffer failed: "
>> +			"offset 0x%lx, size 0x%lx: %s\n", __func__, __LINE__,
>> +			offset, size, ps3_result(result));
>> +		return 0;
>> +	}
>> +
>> +	if (to_user) {
>> +		result = copy_to_user((void __user *)to, lpm_priv->tb_cache,
>> +				      sizeof_copied_data);
>> +		if (result) {
>> +			dev_err(sbd_core(), "%s:%u: copy_to_user() error. "
>> +				"offset:0x%lx size:0x%lx dest:0x%p src:0x%p\n",
>> +				__func__, __LINE__, offset, sizeof_copied_data,
>> +				to, lpm_priv->tb_cache);
>> +			return 0;
>> +		}
>> +	} else
>> +		memcpy(to, lpm_priv->tb_cache, sizeof_copied_data);
>> +
>> +	return sizeof_copied_data;
>> +}
> 
> I don't like how this functions uses the same argument for kernel and user
> pointers, this will cause sparse warnings in the code using it. How about
> making this two separate functions?

OK, good point, I'll consider it.

>> +int ps3_lpm_open(void *tb_cache, u64 tb_cache_size, u64 tb_type)
>> +{
>> +	int result;
>> +	u64 cbe_node_id;
>> +	u64 tb_size;
>> +	u64 ctrl_opt;
>> +	u64 tb_cache_lpar_addr;
>> +	u64 lpm_id;
>> +	u64 outlet_id;
>> +	u64 used_tb_size;
>> +
>> +	if (!lpm_priv) {
>> +		BUG();
>> +		return -ENODEV;
>> +	}
>> +
>> +	mutex_lock(&lpm_priv->mutex);
> 
> This mutex is documented as the 'open/close' mutex and you use it
> to prevent concurrent execution of the open and close functions.
> However, it's unclear what data structures are actually protected
> by it.
> 
> I think this is the result of unusual lifetime rules for the lpm_priv
> object, which I already mentioned above.

Yes, we really just need to use the mutex to allow only a single instance,
and so do a mutex_trylock() in ps3_lpm_open() and return -EBUSY on fail,
and then a mutex_unlock() in ps3_lpm_close().

-Geoff