[lkml]   [2014]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH 09/11] kexec: Provide a function to add a segment at fixed address
    On Mon, Mar 10, 2014 at 11:01:43AM +0100, Borislav Petkov wrote:

    > > I think address does not matter here. You can't add a segemnt after you
    > > have allocated a control page. So I am not sure how printing address will
    > > help.
    > Ok, so what's the urgency of that warning? The "can't add a segment"
    > thing sounds kinda final to me and that everything breaks if we do add a
    > segment after all, so maybe it should error out with -EINVAL and caller
    > should stop adding segments if we have allocated the control page..?
    > IOW, how is that error message supposed to help me when I see it as a
    > user?

    Actually this message is for kernel developer. It is an error for somebody
    who has written an kernel image loader.

    I think returning -EINVAL is also a good idea. Though it will travel all
    the way back to user and user again can't do anything about it.

    I think printing a warning backtrace is still a good idea so that
    we know which loader is doing wrong thing and issue can be reported
    on lkml.

    > > > That's the retval of validate_ram_range_callback, right? So
    > > >
    > > > if (!ret)
    > > >
    > > > And shouldn't the convention be the opposite? 0 on success, !0 on error?
    > >
    > > Ok, this one is little twisted.
    > >
    > > walk_system_ram_res() stops calling callback function if callback
    > > function returned non zero code.
    > >
    > > So in this case, once we have found the range to be valid, we don't want
    > > to continue to loop and look at any more ranges. So we return "1". If
    > > we return "0" for success, outer loop of walk_system_ram_res() will
    > > continue with next ranges.
    > Huh, I was only talking about flipping that logic, in walk_system_ram_res():
    > ret = (*func)(res.start, res.end, arg);
    > if (!res)
    You mean (!ret) ?

    > break;
    > This way you still can return negative values as errors.

    If I flip the logic in walk_system_ram_res() will outer loop not stop
    after one round.

    walk_system_ram_res() {
    while ((find_next_system_ram(&res, "System RAM") >= 0) {
    ret = (*func)(res.start, res.end, arg);
    if (!ret)

    Or are you suggesting that called function should return 0 to break out
    of loop otherwise return negative values to continue the loop? But that's
    counter intutive. If "func" returns error, then we should break out of
    while() loop and not call "func" again with any more qualifiying ranges.

    > > Given the fact that "0" is interpreted as success by walk_system_ram_res()
    > > and it continues with next set of ranges, I could not use 0 as final
    > > measure of success. Negative returns are errors. So I thought of using
    > And?
    > When the loop finishes, you will have the last negative error in ret...

    Yep, Any non-zero value (negative or positive) will be in "ret" when loop
    finishes and will be returned to caller. If final return value is negative
    caller knows an error occurred. If final return value is positive, caller
    knows that *callback* function succeeded.
    > Besides, in load_crashdump_segments() you have:
    > ret = walk_system_ram_res(KEXEC_BACKUP_SRC_START, KEXEC_BACKUP_SRC_END,
    > image, determine_backup_region);
    > /* Zero or postive return values are ok */
    > if (ret < 0)
    > return ret;
    > So 0 is ok, as you say.

    I kept 0 as ok because I am not enforcing that one found a memory range
    with-in 0-640K.

    So if return value is 1, we know that we found a RAM memory resource
    in the range of 0-640K. If return value is 0, we did not find a RAM
    memory resource in the range of 0-640K. I thought may be there are
    systems which don't have memory in that range and system can still
    boot. And that's why I said "0" is ok.

    I don't want to enforce that one *has* to have a memory range with-in
    first 640K otherwise kexec will fail. If there is none, that means there
    is no backup region.

    > Also:
    > /* Validate memory range */
    > ret = walk_system_ram_res(base, base + memsz - 1, &ksegment,
    > validate_ram_range_callback);
    > /* If a valid range is found, 1 is returned */
    > if (ret != 1)
    > return -EINVAL;
    > Now this looks a bit fragile - only 1 is ok? Normally we do it like this:

    Only "1" is ok here because I want to make sure that memory range I am
    looking for is present and valid. If not, we can't go ahead and it is
    an error. (This is different from backup region case).

    I can't use 0 as measure of success because walk_system_ram_res() can
    return 0 even called function did not succeed. For example, assume
    somebody calls walk_system_ram_res() with start=0, end=-1 so that one
    walks through all the memory ranges available in the system. Now callback
    function probably is loking for a certain range say val1-val2. If
    it finds that range in all the ranges, will be return 1, otherwise
    continue to return 0 till we have gone through all the ranges.

    So in above case, if 0 is returned, that does not mean callback
    function found what it was looking for.

    Now one can argue that why are you going through all the memory ranges.
    Just search for very narrow area to begin with. Say start=0xval1
    and end=0xval2 and now after first range callback function should
    be able to determine whether it found the value or not.

    But this now becomes dependent on what start and end values were passed
    to walk_system_ram_res().

    So I am not taking return "0" as measure of success of callback function.
    It could just mean that we went through all the overlapping ranges
    which matched search criterion and still callback function did not find
    what it was looking for.


     \ /
      Last update: 2014-03-10 18:21    [W:6.875 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site