From: Eric W. Biederman <hidden> Date: 2012-11-23 01:05:52
Daniel Kiper [off-list ref] writes:
On Tue, Nov 20, 2012 at 08:40:39AM -0800, ebiederm@xmission.com wrote:
quoted
Daniel Kiper [off-list ref] writes:
quoted
Some kexec/kdump implementations (e.g. Xen PVOPS) could not use default
functions or require some changes in behavior of kexec/kdump generic code.
To cope with that problem kexec_ops struct was introduced. It allows
a developer to replace all or some functions and control some
functionality of kexec/kdump generic code.
Default behavior of kexec/kdump generic code is not changed.
Ick.
quoted
v2 - suggestions/fixes:
- add comment for kexec_ops.crash_alloc_temp_store member
(suggested by Konrad Rzeszutek Wilk),
- simplify kexec_ops usage
(suggested by Konrad Rzeszutek Wilk).
Signed-off-by: Daniel Kiper <redacted>
---
include/linux/kexec.h | 26 ++++++++++
kernel/kexec.c | 131 +++++++++++++++++++++++++++++++++++++------------
2 files changed, 125 insertions(+), 32 deletions(-)
Why in the world would Xen not be able to access crash kernel memory?
As currently defined it is normal memory that the kernel chooses not to
use.
If relocate kernel can access that memory you definitely can access the
memory so the comment does not make any sense.
Crash kernel memory is reserved by Xen hypervisor and Xen hypervisor
only has access to it. dom0 does not have any mapping of this area.
However, relocate_kernel() has access to crash kernel memory
because it is executed by Xen hypervisor and whole machine
memory is identity mapped.
This is all weird. Doubly so since this code is multi-arch and you have
a set of requirements no other arch has had.
I recall that Xen uses kexec in a unique manner. What is the hypervisor
interface and how is it used?
Is this for when the hypervisor crashes and we want a crash dump of
that?
quoted
quoted
+ bool crash_alloc_temp_store;
+ struct page *(*kimage_alloc_pages)(gfp_t gfp_mask,
+ unsigned int order,
+ unsigned long limit);
+ void (*kimage_free_pages)(struct page *page);
+ unsigned long (*page_to_pfn)(struct page *page);
+ struct page *(*pfn_to_page)(unsigned long pfn);
+ unsigned long (*virt_to_phys)(volatile void *address);
+ void *(*phys_to_virt)(unsigned long address);
+ int (*machine_kexec_prepare)(struct kimage *image);
+ int (*machine_kexec_load)(struct kimage *image);
+ void (*machine_kexec_cleanup)(struct kimage *image);
+ void (*machine_kexec_unload)(struct kimage *image);
+ void (*machine_kexec_shutdown)(void);
+ void (*machine_kexec)(struct kimage *image);
+};
Ugh. This is a nasty abstraction.
You are mixing and matching a bunch of things together here.
If you need to override machine_kexec_xxx please do that on a per
architecture basis.
Yes, it is possible but I think that it is worth to do it at that
level because it could be useful for other archs too (e.g. Xen ARM port
is under development). Then we do not need to duplicate that functionality
in arch code. Additionally, Xen requires machine_kexec_load and
machine_kexec_unload hooks which are not available in current generic
kexec/kdump code.
Let me be clear. kexec_ops as you have implemented it is absolutely
unacceptable.
Your kexec_ops is not an abstraction but a hack that enshrines in stone
implementation details.
quoted
Special case overrides of page_to_pfn, pfn_to_page, virt_to_phys,
phys_to_virt, and friends seem completely inappropriate.
They are required in Xen PVOPS case. If we do not do that in that way
then we at least need to duplicate almost all generic kexec/kdump existing
code in arch depended files. I do not mention that we need to capture
relevant syscall and other things. I think that this is wrong way.
A different definition of phys_to_virt and page_to_pfn for one specific
function is total nonsense.
It may actually be better to have a completely different code path.
This looks more like code abuse than code reuse.
Successful code reuse depends upon not breaking the assumptions on which
the code relies, or modifying the code so that the new modified
assumptions are clear. In this case you might as well define up as down
for all of the sense kexec_ops makes.
quoted
There may be a point to all of these but you are mixing and matching
things badly.
Do you whish to split this kexec_ops struct to something which
works with addresses and something which is reponsible for
loading, unloading and executing kexec/kdump? I am able to change
that but I would like to know a bit about your vision first.
My vision is that we should have code that makes sense.
My suspicion is that what you want is a cousin of the existing kexec
system call. Perhaps what is needed is a flag to say use the firmware
kexec system call.
I absolutely do not understand what Xen is trying to do. kexec by
design should not require any firmware specific hooks. kexec at this
level should only need to care about the processor architeture. Clearly
what you are doing with Xen requires special hooks separate even from
the normal paravirt hooks. So I do not understand you are trying to do.
It needs to be clear from the code what is happening differently in the
Xen case. Otherwise the code is unmaintainable as no one will be able
to understand it.
Eric
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2012-11-22 17:47:46
The other thing that should be considered here is how utterly
preposterous the notion of doing in-guest crash dumping is in a system
that contains a hypervisor. The reason for kdump is that on bare metal
there are no other options, but in a hypervisor system the right thing
should be for the hypervisor to do the dump (possibly spawning a clean
I/O domain if the I/O domain is necessary to access the media.)
There is absolutely no reason to have a crashkernel sitting around in
each guest, consuming memory, and possibly get corrupt.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2012-11-22 18:58:17
On 11/22/2012 04:15 AM, Eric W. Biederman wrote:
Let me be clear. kexec_ops as you have implemented it is absolutely
unacceptable.
Your kexec_ops is not an abstraction but a hack that enshrines in stone
implementation details.
This is the kind of stuff that is absolutely endemic to the Xen
endeavour, and which is why Xen is such a disease. The design principle
seems to have been "hey, let's go and replace random Linux kernel
internals with our own stuff, and make them ABIs, so that they can never
change. Oh, and let's not bother documenting the constraints we're
imposing, that might make the code manageable."
I actually talked to Ian Jackson at LCE, and mentioned among other
things the bogosity of requiring a PUD page for three-level paging in
Linux -- a bogosity which has spread from Xen into native. It's a page
wasted for no good reason, since it only contains 32 bytes worth of
data, *inherently*. Furthermore, contrary to popular belief, it is
*not* pa page table per se.
Ian told me: "I didn't know we did that, and we shouldn't have to."
Here we have suffered this overhead for at least six years, because *XEN
FUCKED UP AND NOONE ELSE HAD ANY WAY OF KNOWING THAT*.
Now we know that it can "maybe"(!!!) be fixed, if we are willing to
spend time working on a dying platform, whereas we have already suffered
the damage during the height of its importance.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
From: Daniel Kiper <hidden> Date: 2012-11-23 09:47:38
On Thu, Nov 22, 2012 at 04:15:48AM -0800, ebiederm@xmission.com wrote:
Daniel Kiper [off-list ref] writes:
quoted
On Tue, Nov 20, 2012 at 08:40:39AM -0800, ebiederm@xmission.com wrote:
quoted
Daniel Kiper [off-list ref] writes:
quoted
Some kexec/kdump implementations (e.g. Xen PVOPS) could not use default
functions or require some changes in behavior of kexec/kdump generic code.
To cope with that problem kexec_ops struct was introduced. It allows
a developer to replace all or some functions and control some
functionality of kexec/kdump generic code.
Default behavior of kexec/kdump generic code is not changed.
Ick.
quoted
v2 - suggestions/fixes:
- add comment for kexec_ops.crash_alloc_temp_store member
(suggested by Konrad Rzeszutek Wilk),
- simplify kexec_ops usage
(suggested by Konrad Rzeszutek Wilk).
Signed-off-by: Daniel Kiper <redacted>
---
include/linux/kexec.h | 26 ++++++++++
kernel/kexec.c | 131 +++++++++++++++++++++++++++++++++++++------------
2 files changed, 125 insertions(+), 32 deletions(-)
Why in the world would Xen not be able to access crash kernel memory?
As currently defined it is normal memory that the kernel chooses not to
use.
If relocate kernel can access that memory you definitely can access the
memory so the comment does not make any sense.
Crash kernel memory is reserved by Xen hypervisor and Xen hypervisor
only has access to it. dom0 does not have any mapping of this area.
However, relocate_kernel() has access to crash kernel memory
because it is executed by Xen hypervisor and whole machine
memory is identity mapped.
This is all weird. Doubly so since this code is multi-arch and you have
a set of requirements no other arch has had.
I recall that Xen uses kexec in a unique manner. What is the hypervisor
interface and how is it used?
Is this for when the hypervisor crashes and we want a crash dump of
that?
dom0 at boot gets some info about kexec/kdump configuration from Xen hypervisor
(e.g. placement of crash kernel area). Later if you call kexec syscall most
things are done in the same way as on baremetal. However, after placing image
in memory, HYPERVISOR_kexec_op() hypercall must be called to inform hypervisor
that image is loaded (new hook machine_kexec_load is used for this;
machine_kexec_unload is used for unload). Then Xen establishes fixmap for pages
found in page_list[] and returns control to dom0. If dom0 crashes or "kexec execute"
is used by user then dom0 calls HYPERVISOR_kexec_op() to instruct hypervisor that
kexec/kdump image should be executed immediately. Xen calls relocate_kernel()
and all things runs as usual.
quoted
quoted
quoted
+ bool crash_alloc_temp_store;
+ struct page *(*kimage_alloc_pages)(gfp_t gfp_mask,
+ unsigned int order,
+ unsigned long limit);
+ void (*kimage_free_pages)(struct page *page);
+ unsigned long (*page_to_pfn)(struct page *page);
+ struct page *(*pfn_to_page)(unsigned long pfn);
+ unsigned long (*virt_to_phys)(volatile void *address);
+ void *(*phys_to_virt)(unsigned long address);
+ int (*machine_kexec_prepare)(struct kimage *image);
+ int (*machine_kexec_load)(struct kimage *image);
+ void (*machine_kexec_cleanup)(struct kimage *image);
+ void (*machine_kexec_unload)(struct kimage *image);
+ void (*machine_kexec_shutdown)(void);
+ void (*machine_kexec)(struct kimage *image);
+};
Ugh. This is a nasty abstraction.
You are mixing and matching a bunch of things together here.
If you need to override machine_kexec_xxx please do that on a per
architecture basis.
Yes, it is possible but I think that it is worth to do it at that
level because it could be useful for other archs too (e.g. Xen ARM port
is under development). Then we do not need to duplicate that functionality
in arch code. Additionally, Xen requires machine_kexec_load and
machine_kexec_unload hooks which are not available in current generic
kexec/kdump code.
Let me be clear. kexec_ops as you have implemented it is absolutely
unacceptable.
Your kexec_ops is not an abstraction but a hack that enshrines in stone
implementation details.
Roger.
quoted
quoted
Special case overrides of page_to_pfn, pfn_to_page, virt_to_phys,
phys_to_virt, and friends seem completely inappropriate.
They are required in Xen PVOPS case. If we do not do that in that way
then we at least need to duplicate almost all generic kexec/kdump existing
code in arch depended files. I do not mention that we need to capture
relevant syscall and other things. I think that this is wrong way.
A different definition of phys_to_virt and page_to_pfn for one specific
function is total nonsense.
It may actually be better to have a completely different code path.
This looks more like code abuse than code reuse.
Successful code reuse depends upon not breaking the assumptions on which
the code relies, or modifying the code so that the new modified
assumptions are clear. In this case you might as well define up as down
for all of the sense kexec_ops makes.
Hmmm... Well, problem with above mentioned functions is that they work
on physical addresses. In Xen PVOPS (currently dom0 is PVOPS) they
are useless in kexec/kdump case. It means that physical addresses
must be converted to/from machine addresses which has a real meaning
in Xen PVOPS case. That is why those funtions were introduced.
quoted
quoted
There may be a point to all of these but you are mixing and matching
things badly.
Do you whish to split this kexec_ops struct to something which
works with addresses and something which is reponsible for
loading, unloading and executing kexec/kdump? I am able to change
that but I would like to know a bit about your vision first.
My vision is that we should have code that makes sense.
My suspicion is that what you want is a cousin of the existing kexec
system call. Perhaps what is needed is a flag to say use the firmware
kexec system call.
I absolutely do not understand what Xen is trying to do. kexec by
design should not require any firmware specific hooks. kexec at this
level should only need to care about the processor architeture. Clearly
what you are doing with Xen requires special hooks separate even from
the normal paravirt hooks. So I do not understand you are trying to do.
It needs to be clear from the code what is happening differently in the
Xen case. Otherwise the code is unmaintainable as no one will be able
to understand it.
I agree. I could remove all machine_* hooks from kexec_ops and call Xen
specific functions from arch files. However, I need to add two new
machine calls, machine_kexec_load and machine_kexec_unload, in the same
manner as existing machine_* calls. In general they could be used to inform
firmware (in this case Xen) that kexec/kdump image is loaded.
kimage_alloc_pages, kimage_free_pages, page_to_pfn, pfn_to_page, virt_to_phys
and phys_to_virt are worse. If we could not find good solution how to replace
them then we end up with calling Xen specific version of kexec/kdump which
would contain nearly full copy of exisiting kexec/kdump code. Not good.
We could add some code to kernel/kexec.c which depends on CONFIG_XEN.
It could contain above mentioned functions which later will be called
by existing kexec code. This is not nice to be honest. However, I hope
that we could find better solution for that problem.
Daniel
From: Jan Beulich <hidden> Date: 2012-11-23 09:55:59
quoted
quoted
On 22.11.12 at 18:37, "H. Peter Anvin" [off-list ref] wrote:
I actually talked to Ian Jackson at LCE, and mentioned among other
things the bogosity of requiring a PUD page for three-level paging in
Linux -- a bogosity which has spread from Xen into native. It's a page
wasted for no good reason, since it only contains 32 bytes worth of
data, *inherently*. Furthermore, contrary to popular belief, it is
*not* pa page table per se.
Ian told me: "I didn't know we did that, and we shouldn't have to."
Here we have suffered this overhead for at least six years, ...
Even the Xen kernel only needs the full page when running on a
64-bit hypervisor (now that we don't have a 32-bit hypervisor
anymore, that of course basically means always). But yes, I too
never liked this enforced over-allocation for native kernels (and
was surprised that it was allowed in at all).
Jan
From: Ian Campbell <hidden> Date: 2012-11-23 10:53:18
On Fri, 2012-11-23 at 09:56 +0000, Jan Beulich wrote:
quoted
quoted
quoted
On 22.11.12 at 18:37, "H. Peter Anvin" [off-list ref] wrote:
I actually talked to Ian Jackson at LCE, and mentioned among other
That was me actually (this happens surprisingly often ;-)).
quoted
things the bogosity of requiring a PUD page for three-level paging in
Linux -- a bogosity which has spread from Xen into native. It's a page
wasted for no good reason, since it only contains 32 bytes worth of
data, *inherently*. Furthermore, contrary to popular belief, it is
*not* pa page table per se.
Ian told me: "I didn't know we did that, and we shouldn't have to."
Here we have suffered this overhead for at least six years, ...
Even the Xen kernel only needs the full page when running on a
64-bit hypervisor (now that we don't have a 32-bit hypervisor
anymore, that of course basically means always).
I took an, admittedly very brief, look at it on the plane on the way
home and it seems like the requirement for a complete page on the
pvops-xen side comes from the !SHARED_KERNEL_PMD stuff (so still a Xen
related thing). This requires a struct page for the list_head it
contains (see pgd_list_add et al) rather than because of the use of the
page as a pgd as such.
But yes, I too
never liked this enforced over-allocation for native kernels (and
was surprised that it was allowed in at all).
Completely agreed.
I did wonder if just doing something like:
- pgd = (pgd_t *)__get_free_page(PGALLOC_GFP);
+ if (SHARED_KERNEL_PMD)
+ pgd = some_appropriate_allocation_primitive(sizeof(*pgd));
+ else
+ pgd = (pgd_t *)__get_free_page(PGALLOC_GFP);
to pgd_alloc (+ the equivalent for the error path & free case, create
helper funcs as desired etc) would be sufficient to remove the over
allocation for the native case but haven't had time to properly
investigate.
Alternatively push the allocation down into paravirt_pgd_alloc to
taste :-/
Ian.