Changes to existing kernel functions to support kload

[cattelan-thebarn.com]

Summary

This separates the changes to existing kernel code from the core of the kload functionality.

[emaste]

D3911 is the kload change itself

[kib]

sys/kern/kern_module.c

111

Why is this trivial wrapper needed ?

114

Stray blank line.

sys/x86/x86/intr_machdep.c

202

Stray printf.

209

Weird indent on this and previous line.

212

return (0);

sys/x86/x86/local_apic.c

611

Why is this commented out ?

Why the masking is needed at all, could the same effect achieved by disabling the local apic ?

Also note that we might be executing with x2APIC mode turned on, or with interrupt redirection active, or with DMAR active. All the listed hardware must be turned off gracefully, which I believe is not done at all.

1217

This chunk is unrelated and I doubt that it is useful. The names of the bits are equally cryptic for average user as the numeric value of the error register, but the code adds to the kernel text size.

sys/x86/x86/mp_x86.c

1026

Comments should be reformatted into the multi-line block before the call.

1065

This operation disables long mode. SDM requires that the processor operates in the compat mode (a speak for the 32bit code segment while in LM) and the instructions page must be identity-mapped.

I believe neither of the conditions are satisfied.

sys/x86/x86/nexus.c

681 ↗

(On Diff #9439)

Isn't this obsoleted by r270828 ?

[cattelan-thebarn.com]

sys/kern/kern_module.c

111

Originally so that module_shutdown didn't have to be changed
to a non static function.

Dropped the wrapper and just made module_shutdown non static.

114

removed

sys/x86/x86/intr_machdep.c

202

removed

209

hmm ya not sure how that happened.

fixed.

212

fixed.

sys/x86/x86/local_apic.c

611

I'm guessing you know quite a bit more about the lapic than I do.

The original problem that I was causing issues is that the lapic must
be masked and then disabled to clear any pending interrupts to that when
the new kernel renabled the lapic stray unhanded interrupts from previous
kernel would not show up.
Specifically the timer interrupt would show up before the kernel had installed
the timer interrupt handlers and the new kernel with panic in the unhandled interrupt
handler.

1217

I moved this to the optional apic debug patch.

It isn't needed for kload functionality but it does help quite
a bit when debugging kload apic issues.

sys/x86/x86/mp_x86.c

1026

fixed

1065

This is just setting the config registers to match the same state as what loader
normally hands the cpu off to the kernel.

At this point the processor should be ready to be halted and with a identity mapped page
table.

sys/x86/x86/nexus.c

681 ↗

(On Diff #9439)

Sorry to take a bit to follow up on this -- been working on getting the converted over to John's
smap sysctl.

It took a bit of mucking around since that sysctl is using bios_smap_xattr and the kernel
kenv is expecting bios_smap.

I think I have it working using the existing handler vs adding a new one.
So ya this will get dropped in the updated patch.

[kib]

sys/x86/x86/local_apic.c

611

Right, we must cope with pending interrupts on LAPIC enable during boot. In other words, lets move the handling to the boot time, to use the opportunity to fix booting on buggy BIOS implementations which could leave LAPICs in weird state after OS hand-off. I think it would become even more common with UEFI proliferation.

Other issues are still pending, e.g. we should turn off x2APIC mode at least. VT-d might need to be handled in the iommu driver.

sys/x86/x86/mp_x86.c

99

BTW, I think that having kload_pgtbl always exported is not that big cost.

1068

Ok, but what code ensures that we are executing on id-mapped page ?

[cattelan-thebarn.com]

sys/x86/x86/local_apic.c

611

Hmm didn't see this comment originally -- sorry about not following up.

The original process of developing kload was to get it working without changing
the kernel startup process. This was to prove that we would bring the state of the
system back to a known starting point -- that being the same state as what loader
does before it hands the system to the kernel.

But I do agree improving LAPIC startup code would be a good thing to do.
I did look at the way linux handles the LAPIC initialization and it is much more guarded about
the process since one of the uses of kexec on linux is to boot that kdump kernel. The kdump
kernel is coming from a panic so it has to assume none of the normal shutdown has happened.

I'll look into this and see what I can some up with.
I might be asking for some help / guidance. :-)

Good point about the x2APIC handling -- that might explain the issue I'm currently seeing when trying to get kload working on a ESXi based vm.

sys/x86/x86/mp_x86.c

99

kload_pgtbl defined in sys/kern/kern_kload.c which is not compiled when the KLOAD feature is not enabled. It was easier to just make is a local static when disabling the feature.

But now that I look at it there is a type inconsistency.
pt_entry_t *kload_pgtbl = NULL; /* used as a test */
vs
extern pt_entry_t kload_pgtbl;

Not fatal but still should be corrected.

1068

sys/amd64/amd64/kload.c: kload_build_page_table

Creates a page table that matches the page table installed by loader.
Since the kernel is placed at a fixed location in memory the mapping for
kernel instructions should always be at the same location / mapping (assuming
the page table is consistently mapping the physical pages consistently).

It is worth noting the code that does the kernel over-copy is copied out of the orignal kernel into
a page allocated in the first 1meg of memory so that it also is within the id-mapped pagetable.

Granted this is all a bit of hackery to make this all work :-)

[kib]

sys/x86/x86/mp_x86.c

1068

Hm, could you allocate an IPI vector specifically for kload suspend, and do the actions you need, in the specific handler, instead ?

[cattelan-thebarn.com]

Finally found some time to update this patch.

The kload cpu suspend IPI is not defined as a separate vector.

Also update the lapic shutdown code to handle newer versions.
(This was based on inspecting the linux kexec patches )

[kib]

So what do you want to do with this ? Is the patch needs to be committed to unblock kload commit ?

sys/amd64/amd64/apic_vector.S

309

Blank lines 206 and 308 are not needed.

sys/kern/kern_shutdown.c

325

I am not sure, is this a good idea to set RB_KLOAD unconditionally for the non-panic reboot ?

sys/kern/subr_smp.c

303

Blank like is needed before new function.

sys/x86/x86/local_apic.c

575

As I understand, XEN is not handled. Anyway, would it make sense to introduce a method in the apic_ops ?

581

This is explicitely non-stylish. The declaration block should be at the start of the function.

586

Line too long. maxlvt line may be too long as well.

611

So this is still done that way. At laest, add a comment, please, explaining that we do a cleanup which is usually done by hw reset or BIOS, but needed there since we jump into new kernel without clearing pending interrupts otherwise.

sys/x86/x86/mp_x86.c

975

This is IPI_KLOAD, I suppose.

[cattelan-thebarn.com]

More updates based on comments from kib

[cattelan-thebarn.com]

sys/x86/x86/local_apic.c

575

Still trying to understand / figure out if we need to do anything special for XEN.

I would think that a normal shutdown would have shutdown any active virtual instances.

Added the clear_lapic to the ops tables.

One question ... what about arches that don't set the ops?
It seems like the wrapper functions should check for a null function ptr before calling it?

[cattelan-thebarn.com]

no interest