Re: [PATCH V2 2/3] powerpc, ptrace: Enable support for transactional memory register sets

[Anshuman Khandual <khandual@linux.vnet.ibm.com>]

On 05/20/2014 04:03 PM, Pedro Alves wrote:
> On 05/20/2014 09:14 AM, Anshuman Khandual wrote:
>> On 05/19/2014 08:13 PM, Pedro Alves wrote:
>>> On 05/19/2014 12:46 PM, Anshuman Khandual wrote:
>>>
>>>>>> 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.
>>>
>>> Indeed.
>>>
>>> TBC, do you plan on fixing this?  Otherwise ...
>>
>> Sure, thinking something like this as mentioned below. But still not sure how to use
>> the return type of -ENODEV from the function regset->active(). Right now if any
>> regset does have the active hook and it returns anything but positive value, it will
>> be ignored and the control moves to the next regset in view. This prevents the thread
>> core note type being written to the core dump.
> 
> Looks to me that that's exactly what should happen for -ENODEV too.  The regset
> should be ignored.  If regset->active() returns -ENODEV, then the machine
> doesn't have the registers at all, so what makes sense to me is to not write the
> corresponding core note in the dump.  IOW, on such a machine, the kernel
> generates a core exactly like if the support for these registers that don't
> make sense for this machine wasn't compiled in at all.  And generates a core
> exactly like an older kernel that didn't know about that regset
> (which is fine for that same machine) yet.
> 

All of this happen right now even without specifically checking for the return type
of -ENODEV and just checking for a positive value. I guess thats the reason they had
omitted -ENODEV in the first place. 

 
>>
>> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
>> index aa3cb62..80672fb 100644
>> --- a/fs/binfmt_elf.c
>> +++ b/fs/binfmt_elf.c
>> @@ -1553,7 +1553,15 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
>>                 if (regset->core_note_type && regset->get &&
>>                     (!regset->active || regset->active(t->task, regset))) {
>>                         int ret;
> 
> So, here, this ?
> 
>                     (!regset->active || regset->active(t->task, regset) > 0)) {
> 
> 
>> -                       size_t size = regset->n * regset->size;
>> +                       size_t size;
>> +
>> +                       /* Request only the active elements in the regset */
>> +                       if (!regset->active)
>> +                               size = regset->n * regset->size;
>> +                       else
>> +                               size = regset->active(t->task, regset)
>> +                                                               * regset->size;
>> +
> 
> 
> I wonder if it wouldn't be cleaner to add a function like:
> 
> int
> regset_active (tast *task, regseg *regset)
> {
>    if (!regset->active)
>         return regset->n * regset->size;
>    else
>         return regset->active(task, regset);
> }
> 
> And then use it like
> 
>                if (regset->core_note_type && regset->get) {
>                    int size = regset_active (t->task, regset);
> 
>                    if (size > 0) {
>                           ...
>                    }
> 

Yeah this makes sense.

> Though at this point, we don't actually make use of
> the distinction between -ENODEV vs 0.  Guess that's what
> we should be thinking about.  Seems like there some details that
> need to be sorted out, and some verification that consumers aren't
> broken by outputting smaller notes -- e.g., ia64 makes me
> wonder that.

I agree.

> 
> Maybe we should leave this for another day, and have tm_spr_active
> return 0 instead of -ENODEV when the machine doesn't have the hardware,
> or not install that hook at all.  Seems like the effect will be the same,
> as the note isn't output if ->get fails.

Agree. Active hooks which return 0 in case of -ENODEV sounds good to me and shall
incorporate this in the next version.

> 
>>                         void *data = kmalloc(size, GFP_KERNEL);
>>                         if (unlikely(!data))
>>                                 return 0;
>>
>>>
>>>> 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;
>>>
>>> ... unfortunately this will do the wrong thing.
>>
>> I am not sure whether I understand this correctly. Are you saying that its wrong to return
>> -ENODEV in this case as above ?
> 
> No, sorry for not being clear.  The (...)'s were connected:
> 
>    "do you plan on fixing this?  Otherwise ... ... unfortunately
>     this will do the wrong thing."
> 

:)