Message-ID: <5379EF0E.6090504@linux.vnet.ibm.com>
Date:	Mon, 19 May 2014 17:16:22 +0530
From:	Anshuman Khandual <khandual@...ux.vnet.ibm.com>
To:	Pedro Alves <palves@...hat.com>
CC:	mikey@...ling.org, avagin@...nvz.org, linux-kernel@...r.kernel.org,
	oleg@...hat.com, michael@...erman.id.au, linuxppc-dev@...abs.org
Subject: Re: [PATCH V2 2/3] powerpc, ptrace: Enable support for transactional
 memory register sets

On 05/15/2014 05:38 PM, Pedro Alves wrote:
> On 05/15/2014 09:25 AM, Anshuman Khandual wrote:
>> On 05/14/2014 04:45 PM, Pedro Alves wrote:
>>> On 05/14/14 06:46, Anshuman Khandual wrote:
>>>> On 05/13/2014 10:43 PM, Pedro Alves wrote:
>>>>> On 05/05/14 08:54, Anshuman Khandual wrote:
>>>>>> This patch enables get and set of transactional memory related register
>>>>>> sets through PTRACE_GETREGSET/PTRACE_SETREGSET interface by implementing
>>>>>> four new powerpc specific register sets i.e REGSET_TM_SPR, REGSET_TM_CGPR,
>>>>>> REGSET_TM_CFPR, REGSET_CVMX support corresponding to these following new
>>>>>> ELF core note types added previously in this regard.
>>>>>>
>>>>>> 	(1) NT_PPC_TM_SPR
>>>>>> 	(2) NT_PPC_TM_CGPR
>>>>>> 	(3) NT_PPC_TM_CFPR
>>>>>> 	(4) NT_PPC_TM_CVMX
>>>>>
>>>>> Sorry that I couldn't tell this from the code, but, what does the
>>>>> kernel return when the ptracer requests these registers and the
>>>>> program is not in a transaction?  Specifically I'm wondering whether
>>>>> this follows the same semantics as the s390 port.
>>>>>
>>>>
>>>> Right now, it still returns the saved state of the registers from thread
>>>> struct. I had assumed that the user must know the state of the transaction
>>>> before initiating the ptrace request. I guess its better to check for
>>>> the transaction status before processing the request. In case if TM is not
>>>> active on that thread, we should return -EINVAL.
>>>
>>> I think s390 returns ENODATA in that case.
>>>
>>>  https://sourceware.org/ml/gdb-patches/2013-06/msg00273.html
>>>
>>> We'll want some way to tell whether the system actually
>>> supports this.  That could be ENODATA vs something-else (EINVAL
>>> or perhaps better EIO for "request is invalid").
>>
>> As Mickey has pointed out, the transaction memory support in the system can be
>> checked from the HWCAP2 flags. So when the transaction is not active, we will
>> return ENODATA instead for TM related ptrace regset requests.
> 
> Returning ENODATA when the transaction is not active, like
> s390 is great.  Thank you.
> 
> But I think it's worth it to consider what should the kernel
> return when the machine doesn't have these registers at all.
> 
> Sure, for this case we happen to have the hwcap flag.  But in
> general, I don't know whether we will always have a hwcap bit
> for each register set that is added.  Maybe we will, so that
> the info ends up in core dumps.
> 
> Still, I think it's worth to consider this case in the
> general sense, irrespective of hwcap.
> 
> That is, what should PTRACE_GETREGSET/PTRACE_SETREGSET return
> when the machine doesn't have the registers at all.  We shouldn't
> need to consult something elsewhere (like hwcap) to determine
> what ENODATA means.  The kernel knows it right there.  I think
> s390 goofed here.
> 
> Taking a look at x86, for example, we see:
> 
> 	[REGSET_XSTATE] = {
> 		.core_note_type = NT_X86_XSTATE,
> 		.size = sizeof(u64), .align = sizeof(u64),
> 		.active = xstateregs_active, .get = xstateregs_get,
> 		.set = xstateregs_set
> 	},
> 
> Note that it installs the ".active" hook.
> 
>  24 /**
>  25  * user_regset_active_fn - type of @active function in &struct user_regset
>  26  * @target:     thread being examined
>  27  * @regset:     regset being examined
>  28  *
>  29  * Return -%ENODEV if not available on the hardware found.
>  30  * Return %0 if no interesting state in this thread.
>  31  * Return >%0 number of @size units of interesting state.
>  32  * Any get call fetching state beyond that number will
>  33  * see the default initialization state for this data,
>  34  * so a caller that knows what the default state is need
>  35  * not copy it all out.
>  36  * This call is optional; the pointer is %NULL if there
>  37  * is no inexpensive check to yield a value < @n.
>  38  */
>  39 typedef int user_regset_active_fn(struct task_struct *target,
>  40                                   const struct user_regset *regset);
>  41
> 
> Note the mention of ENODEV.
> 
> I couldn't actually find any arch that currently returns -ENODEV in
> the "active" hook.  I see that binfmt_elf.c doesn't handle
> regset->active() returning < 0.  Guess that may be why.  Looks like
> something that could be cleaned up, to me.
>

Also it does not consider the return value of regset->active(t->task, regset)
(whose objective is to figure out whether we need to request regset->n number
of elements or less than that) in the subsequent call to regset->get function.

> Anyway, notice x86's REGSET_XSTATE regset->get hook:
> 
> int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
> 		unsigned int pos, unsigned int count,
> 		void *kbuf, void __user *ubuf)
> {
> 	int ret;
> 
> 	if (!cpu_has_xsave)
> 		return -ENODEV;
>         ^^^^^^^^^^^^^^^^^^^^^^
> 
> And then we see that xfpregs_get has a similar ENODEV case.
> 
> So in sum, it very much looks like the intention is for
> PTRACE_GETREGSET/PTRACE_SETREGSET to return ENODEV in the
> case the regset doesn't exist on the running machine, and then
> it looks like at least x86 works that way.

Okay. Looks like for all the "get/set" hooks I have added for the brand new regsets,
we need to implement ENODATA error condition as that of s390 when TM is not active
on the thread in target and implement ENODEV error condition as that of x86 when
TM supports is not at all available on the system. So the code snippet which should
be added to all the new "get/set" functions will be something like this.

+       if (!cpu_has_feature(CPU_FTR_TM))
+               return -ENODEV;
+
+       if(!MSR_TM_ACTIVE(target->thread.regs->msr))
+               return -ENODATA;


Now coming to the installation of the .active hooks part for all the new regsets, it
should be pretty straight forward as well. Though its optional and used for elf_core_dump
purpose only, its worth adding them here. Example of an active function should be something
like this. The function is inexpensive as required.

+static int tm_spr_active(struct task_struct *target,
+                               const struct user_regset *regset)
+{
+       if (!cpu_has_feature(CPU_FTR_TM))
+               return -ENODEV;
+
+       if(!MSR_TM_ACTIVE(target->thread.regs->msr))
+               return 0;
+
+       return regset->n;
+}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists