Fix Xen PVH boot and init_physical_heap low-memory trimming bug#2137
Fix Xen PVH boot and init_physical_heap low-memory trimming bug#2137niklasfemerstrand wants to merge 1 commit intonanovms:masterfrom
Conversation
niklasfemerstrand
force-pushed
the
pvh_boot
branch
from
ead092e to
1b5faf7
Compare
francescolavra
left a comment
francescolavra
left a comment
There was a problem hiding this comment.
Thanks for your PR! A few comments inline
| init_acpi_tables(get_kernel_heaps()); | ||
| /* Read ACPI tables for MADT access (not available on PVH) */ | ||
| if (!pvh_boot) | ||
| init_acpi_tables(get_kernel_heaps()); |
There was a problem hiding this comment.
It should be possible to call both init_acpi_tables() and init_acpi() on a guest without ACPI, in fact Nanos runs just fine on ARM guests without ACPI. I also tested a modified kernel where the AcpiOsGetRootPointer() function in src/x86_64/acpi.c always returns 0 (to simulate an x86 guest without ACPI) and it boots just fine, both with and without KVM.
If the kernel crashes on a Xen PVH guest when trying to initialize ACPI, I would rather debug and fix that, and avoid the pvh_boot global variable.
| bits 32 | ||
| pvh_start32: | ||
| ; ESP is undefined at PVH entry, we need a temporary stack before calls | ||
| mov esp, 0xa000 |
There was a problem hiding this comment.
Good catch, but since there already is a stack pointer initialization a few instructions below this, I would remove that in favor of this one (and update/move the relevant comment).
| init_apic(kh); | ||
| /* APIC initialization, skip for PVH (Xen uses event channels) */ | ||
| if (!pvh_boot) | ||
| init_apic(kh); |
There was a problem hiding this comment.
I looked at the Xen source, and as far as I can tell, Xen PVH guests do have a local APIC (see https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/domain.c;h=9ba2774762ccfc2b79d6892c55eaa06e5e66ac29;hb=HEAD#l784, where the LAPIC emulation flag is marked as required for PVH domU). So if there are issues getting it to work, I prefer to sort them out instead of disabling the APIC code.
2 participants
#2136