Hi Jean,
Thanks for the updated patches. One minor comment on this one.
On Mon, Dec 30, 2013 at 04:25:30PM +0000, Jean Pihet wrote:
> From: Jean Pihet <[email protected]>
>
> This patch implements the functions required for the perf registers API,
> allowing the perf tool to interface kernel register dumps with libunwind
> in order to provide userspace backtracing.
> Compat mode is also supported.
[...]
> +u64 perf_reg_value(struct pt_regs *regs, int idx)
> +{
> + if (WARN_ON_ONCE((u32)idx >= PERF_REG_ARM64_MAX))
> + return 0;
While this is probably fine, I'd feel more comfortable if you had separate
limit checks for native and compat...
> + /*
> + * Compat (i.e. 32 bit) mode:
> + * - PC has been set in the pt_regs struct in kernel_entry,
> + * - Handle SP and LR here.
> + */
> + if (compat_user_mode(regs)) {
i.e. have a WARN_ON_ONCE here for the compat structure size.
> + if ((u32)idx == PERF_REG_ARM64_SP)
> + return regs->compat_sp;
> + if ((u32)idx == PERF_REG_ARM64_LR)
> + return regs->compat_lr;
> + }
then stick an else here with the original check.
Make sense?
Will
Hi Will,
On 6 January 2014 19:30, Will Deacon <[email protected]> wrote:
> Hi Jean,
>
> Thanks for the updated patches. One minor comment on this one.
Thanks for reviewing!
> On Mon, Dec 30, 2013 at 04:25:30PM +0000, Jean Pihet wrote:
>> From: Jean Pihet <[email protected]>
>>
>> This patch implements the functions required for the perf registers API,
>> allowing the perf tool to interface kernel register dumps with libunwind
>> in order to provide userspace backtracing.
>> Compat mode is also supported.
>
> [...]
>
>> +u64 perf_reg_value(struct pt_regs *regs, int idx)
>> +{
>> + if (WARN_ON_ONCE((u32)idx >= PERF_REG_ARM64_MAX))
>> + return 0;
>
> While this is probably fine, I'd feel more comfortable if you had separate
> limit checks for native and compat...
>
>> + /*
>> + * Compat (i.e. 32 bit) mode:
>> + * - PC has been set in the pt_regs struct in kernel_entry,
>> + * - Handle SP and LR here.
>> + */
>> + if (compat_user_mode(regs)) {
>
> i.e. have a WARN_ON_ONCE here for the compat structure size.
>
>> + if ((u32)idx == PERF_REG_ARM64_SP)
>> + return regs->compat_sp;
>> + if ((u32)idx == PERF_REG_ARM64_LR)
>> + return regs->compat_lr;
>> + }
>
> then stick an else here with the original check.
>
> Make sense?
Yes
I will resend an updated version asap.
>
> Will
Jean
Hi Will,
On 6 January 2014 19:30, Will Deacon <[email protected]> wrote:
> Hi Jean,
>
> Thanks for the updated patches. One minor comment on this one.
>
> On Mon, Dec 30, 2013 at 04:25:30PM +0000, Jean Pihet wrote:
>> From: Jean Pihet <[email protected]>
>>
>> This patch implements the functions required for the perf registers API,
>> allowing the perf tool to interface kernel register dumps with libunwind
>> in order to provide userspace backtracing.
>> Compat mode is also supported.
>
> [...]
>
>> +u64 perf_reg_value(struct pt_regs *regs, int idx)
>> +{
>> + if (WARN_ON_ONCE((u32)idx >= PERF_REG_ARM64_MAX))
>> + return 0;
>
> While this is probably fine, I'd feel more comfortable if you had separate
> limit checks for native and compat...
In fact in the native and compat modes the same set of registers are
accessed, based on the native regs that are registered to the perf
event core, cf. the definition of PERF_REGS_MASK in
tools/perf/arch/arm64/include/perf_regs.h.
The regs set could be registered differently based on the binary to
trace, but unfortunately the perf core code does not allow that.
I would leave the code as is, what do you think?
Cheers,
Jean
>
>> + /*
>> + * Compat (i.e. 32 bit) mode:
>> + * - PC has been set in the pt_regs struct in kernel_entry,
>> + * - Handle SP and LR here.
>> + */
>> + if (compat_user_mode(regs)) {
>
> i.e. have a WARN_ON_ONCE here for the compat structure size.
>
>> + if ((u32)idx == PERF_REG_ARM64_SP)
>> + return regs->compat_sp;
>> + if ((u32)idx == PERF_REG_ARM64_LR)
>> + return regs->compat_lr;
>> + }
>
> then stick an else here with the original check.
>
> Make sense?
>
> Will
On Wed, Jan 15, 2014 at 10:30:48AM +0000, Jean Pihet wrote:
> Hi Will,
Hi Jean,
> On 6 January 2014 19:30, Will Deacon <[email protected]> wrote:
> > On Mon, Dec 30, 2013 at 04:25:30PM +0000, Jean Pihet wrote:
> >> From: Jean Pihet <[email protected]>
> >>
> >> This patch implements the functions required for the perf registers API,
> >> allowing the perf tool to interface kernel register dumps with libunwind
> >> in order to provide userspace backtracing.
> >> Compat mode is also supported.
> >
> > [...]
> >
> >> +u64 perf_reg_value(struct pt_regs *regs, int idx)
> >> +{
> >> + if (WARN_ON_ONCE((u32)idx >= PERF_REG_ARM64_MAX))
> >> + return 0;
> >
> > While this is probably fine, I'd feel more comfortable if you had separate
> > limit checks for native and compat...
> In fact in the native and compat modes the same set of registers are
> accessed, based on the native regs that are registered to the perf
> event core, cf. the definition of PERF_REGS_MASK in
> tools/perf/arch/arm64/include/perf_regs.h.
>
> The regs set could be registered differently based on the binary to
> trace, but unfortunately the perf core code does not allow that.
>
> I would leave the code as is, what do you think?
Well, what business would a compat task have accessing registers beyond the
compat subset? Since we don't expose the PC, we can simply lower the limit
as the compat registers form a prefix of the native registers, no?
Will
Will,
On 15 January 2014 12:07, Will Deacon <[email protected]> wrote:
> On Wed, Jan 15, 2014 at 10:30:48AM +0000, Jean Pihet wrote:
>> Hi Will,
>
> Hi Jean,
>
>> On 6 January 2014 19:30, Will Deacon <[email protected]> wrote:
>> > On Mon, Dec 30, 2013 at 04:25:30PM +0000, Jean Pihet wrote:
>> >> From: Jean Pihet <[email protected]>
>> >>
>> >> This patch implements the functions required for the perf registers API,
>> >> allowing the perf tool to interface kernel register dumps with libunwind
>> >> in order to provide userspace backtracing.
>> >> Compat mode is also supported.
>> >
>> > [...]
>> >
>> >> +u64 perf_reg_value(struct pt_regs *regs, int idx)
>> >> +{
>> >> + if (WARN_ON_ONCE((u32)idx >= PERF_REG_ARM64_MAX))
>> >> + return 0;
>> >
>> > While this is probably fine, I'd feel more comfortable if you had separate
>> > limit checks for native and compat...
>> In fact in the native and compat modes the same set of registers are
>> accessed, based on the native regs that are registered to the perf
>> event core, cf. the definition of PERF_REGS_MASK in
>> tools/perf/arch/arm64/include/perf_regs.h.
>>
>> The regs set could be registered differently based on the binary to
>> trace, but unfortunately the perf core code does not allow that.
>>
>> I would leave the code as is, what do you think?
>
> Well, what business would a compat task have accessing registers beyond the
> compat subset? Since we don't expose the PC, we can simply lower the limit
> as the compat registers form a prefix of the native registers, no?
Yes it is a good thing to clamp the range of registers, based on the
compat mode.
Unfortunately the perf core code does not handle the compat mode,
instead it always accesses all _native_ registers.
Here is the proposal (after our discussion on IRC):
- use the current patches on ARM64 so that we have a working solution
that one can test/stress/use on real workloads,
- re-factor the perf core code later to make able to handle multiple
ABIs and corresponding registers sets.
Are you ok with that?
Regards,
Jean
>
> Will