Messages in this thread | | | Date | Thu, 22 May 2014 10:38:17 +0530 | From | Anshuman Khandual <> | Subject | Re: [PATCH V2 2/3] powerpc, ptrace: Enable support for transactional memory register sets |
| |
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." >
:)
| |