[Cbe-oss-dev] [RFC/PATCH]: User-space access to Cell bookmark registers

[Arnd Bergmann]

On Wednesday 08 August 2007, Kevin Corry wrote:
> While working on the Perfmon2 code for Cell, I've gotten requests to enable 
> access to the Cell bookmark special-purpose-registers. These are not part of 
> the actual PMU, but writing to them can trigger actions in the PMU, such as 
> starting and/or stopping the hardware performance counters and generating 
> trace data. To provide user-space with access to these registers, I've 
> written a patch that adds the files /sys/devices/system/cpu/cpu*/bookmark. 
> The registers are write-only, so the sysfs files are also write-only.
> 
> A couple of questions: Is this the correct method (using sysfs) to provide 
> access to these registers (or other SPRs)?

Yes, that looks like the best approach to me.

> The patch adds the new code to the arch/powerpc/platforms/cell/pmu.c file.
> However, the registers aren't actually part of the PMU. 
> Is pmu.c appropriate for this code or should it go in some other file?

A new file specifically for this might be better, but I can see arguments
either way, I don't care much. Is there any possibility that this might
be useful for someone who is not using the pmu.c code?

> Add the files /sys/devices/system/cpu/cpu*/bookmark to provide user-space
> with a method for writing to the bookmark registers in the Cell processor.
> Writes to these registers can be used by the performance monitoring unit to
> generate trace data and to trigger the starting and stopping of the hardware
> performance counters.

The name 'bookmark' might be a little too generic, and not really obvious.
Maybe something like 'pmu-bookmark' would be more appropriate.

> +#define BOOKMARK_SPR_ADDR 1020

The definition should go into the include/asm-powerpc/reg.h file,
along with the other SPRN_* macros.

> +static ssize_t cbe_store_bookmark(struct sys_device *dev,
> +                                 const char *buf, size_t count)
> +{
> +       cpumask_t old_affinity = current->cpus_allowed;
> +       int rc, cpu = dev->id;
> +       u64 val;
> +
> +       rc = sscanf(buf, "%lu", &val);
> +       if (rc != 1)
> +               return -EINVAL;
> +
> +       if (set_cpus_allowed(current, cpumask_of_cpu(cpu)))
> +               return -EINVAL;
> +
> +       mtspr(BOOKMARK_SPR_ADDR, val);
> +
> +       set_cpus_allowed(current, old_affinity);
> +
> +       return count;
> +}

Looks good. I'd have used the smp_call_function_single() function
in order to go to the right CPU, but I don't see an important argument
to change it.

> +static int __init cbe_bookmark_init(void)
> +{
> +       return cpu_add_sysdev_attr(&attr_bookmark);
> +}
> +__initcall(cbe_bookmark_init);

This needs to check if it's running on a compatible machine, otherwise
you might give access to a register that should not be writable.
The check should probably be either (cur_cpu_spec->cpu_user_features &
PPC_FEATURE_CELL) if you want it to be available on the ps3 and celleb
platforms, or 'platform_is(cell)' if it's specific to only the bare
metal machines.

	Arnd <><