lkml.org 
[lkml]   [2006]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Fastboot] [PATCH 01/03] kexec: Avoid overwriting the current pgd (V2, stubs)
Hi again Eric,

Thank you for your comments so far.

On 5/26/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
> "Magnus Damm" <magnus.damm@gmail.com> writes:
>
> > On 5/25/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
> >> Magnus Damm <magnus@valinux.co.jp> writes:
> >> > On Wed, 2006-05-24 at 20:41 -0600, Eric W. Biederman wrote:
> >> >> Magnus Damm <magnus@valinux.co.jp> writes:
> >>
> >> I believe I gave a complete explanation the first round.
> >>
> >> By having an extra extern variable you can export the offset of
> >> a variable on the control code page, or what you need to compute
> >> the offset.
> >
> > You explained some things last round, but there were still some
> > questions left open. The main question was regarding "additional
> > protection".
>
> Memory that the kernel never sets up for DMA ever.

I had hoped that I would get a more detailed answer. I asked you the
following follow-up questions during the first round:

---

I suppose you mean that control pages have additional protection that
struct kimage does _not_ have. Protection provided by
kimage_alloc_control_pages(), right?

I agree with you that this protection is good. But I do not see how
that applies to my patch, because the page_table_a[] pages pointed out
by struct kimage are read out by machine_kexec() and passed as
arguments to the assembly code. So the assembly code itself never
tries to access struct kimage. All data accessed by the assembly code
is allocated with kimage_alloc_control_pages(), isn't that good
enough? Or maybe I'm misunderstanding?

---

I'm trying to understand your argument, but it does not make sense to me.

If you are worried that struct kimage will get overwritten by
background DMA, then why the are you storing the pointer to the
control page there? That pointer is _very_ important...

And if your DMA issue it is that important, why do you not allocate
struct kimage from kimage_alloc_control_pages()?

> > I'd be happy to change to code into something that you would feel
> > comfortable with, I just like to understand why. Having a
> > per-architecture data area in struct kimage is by far the simplest way
> > IMO.
>
> But the problem is fundamentally hard. I do not want to encourage
> people to make changes without thinking through all of the consequences.

That makes sense.

> So far my impression is that an additional per-architecture data area
> is struct kimage encourages people to be sloppy, and it moves key structures
> farther from where they are used. I am coming to suspect it is as bad
> as ioctl.

I see it from the other side. By _not_ having per-architecture data
the current kexec code for x86_64 unnecessarily allocates 2 physically
contiguous pages (1-order allocation).

> I could probably be convinced with a good use of a per-architecture
> area and a well reasoned argument. But at the moment changing the
> infrastructure is unnecessary noise, so please drop that for now.

But then I would have to make the code overly complex and duplicate
code that overloads the control page with a structure - both for i386
and x86_64. By adding a per-architecture data structure the amount of
code duplication is reduced IMO.

> >> Part of the reason is you do more than one thing at a time, which makes
> >> review much more difficult than it needs to be.
> >
> > Yeah, I know. I'm sorry about that. I took some time to split the
> > patches in the most logical way I could think of. The reason behind
> > not separating the segment code and the page_table_a code was that
> > they both touched more or less the same lines.
>
> Dependent patches are not a problem.

Good. I'm sorry that the patches are unreadable, they tend to get that
when one is modifying major parts of a file. I'd recommend that
instead of reading the patch directly it may be better to apply it and
read the resulting code.

> > And by global structure you mean the dynamically allocated struct
> > kimage? If you find that abusive then I think that _not_ using an
> > already existing structure is abusive. =)
> >
> > I just want to keep things as simple as possible.
>
> Simplicity is good.
>
> Doing unnecessary things is confusing and the issue is not good,
> and at least until the Xen support is merged you were doing
> unnecessary things.

Yes and no. I agree that modifying kernel code to fit code outside of
the tree is a bad thing, but I think my modification alone has value
for the kernel.

I think we both can agree on that overwriting the current page tables
should be avoided regardless of Xen. It's just a matter of where we
store data and if we are using one or two page tables.

> Please just take it carefully. This is possibly the hardest
> to debug code path in the kernel and currently it works. I
> don't want to break that.

I don't want to break it either, so careful is good. I think kexec
works pretty well, but I would not say it's working correctly in all
cases.

One issue on i386 with the current page table overwriting code is that
it may break with different kernel/userspace splits. I think my
"page_table_a" approach should work for all cases.

Also, I cannot get one x86_64 box here to work with kexec. It does not
work with the vanilla kexec, and not with my page_table_a patch. The
troublesome machine has a cpu reported as "AMD Athlon(tm) 64 Processor
3200+". The same code does however run on hardware with dual "AMD
Opteron(tm) Processor 244".

I'd like to work on solving these issues too, but I'd like to sort out
merging my current patches before starting on something new.

Thanks,

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

\
 
 \ /
  Last update: 2006-05-26 04:27    [W:0.068 / U:0.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site