seL4
x86 FPU XSAVES Option Doesn't Exist
The x86 FPU XSave feature is defined here:
config_choice(
KernelXSave
KERNEL_XSAVE
"The XSAVE area supports multiple instructions to save
and restore to it. These instructions are dependent upon specific CPU support. See Chapter 13 of Volume \
1 of the Intel Architectures SOftware Developers Manual for discussion on the init and modified \
optimizations. \
XSAVE -> Original XSAVE instruction. This is the only XSAVE instruction that is guaranteed to exist if \
XSAVE is present \
XSAVEC -> Save state with compaction. This compaction has to do with minimizing the total size of \
XSAVE buffer, if using non contiguous features, XSAVEC will attempt to use the init optimization \
when saving \
XSAVEOPT -> Save state taking advantage of both the init optimization and modified optimization \
XSAVES -> Save state taking advantage of the modified optimization. This instruction is only \
available in OS code, and is the preferred save method if it exists."
"XSAVEOPT;KernelXSaveXSaveOpt;XSAVE_XSAVEOPT;KernelFPUXSave"
"XSAVE;KernelXSaveXSave;XSAVE_XSAVE;KernelFPUXSave"
"XSAVEC;KernelXSaveXSaveC;XSAVE_XSAVEC;KernelFPUXSave"
)
In the FPU source there are conditionals for the XSAVES option:
} else if (config_set(CONFIG_XSAVE_XSAVES)) {
if (!(xsave_instruction & BIT(3))) {
printf("XSAVES requested, but not supported\n");
return false;
}
/* AVX state from extended region should be in compacted format */
nullFpuState->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT;
/* initialize the XSS MSR */
x86_wrmsr(IA32_XSS_MSR, desired_features);
}
I set the XSAVE instruction in the app_settings like this:
set(KernelFPU XSAVE CACHE STRING "" FORCE)
set(KernelXSave XSAVES CACHE STRING "" FORCE)
But the generated CMake config defaults to the OPT instruction because XSAVES doesn't actually exist:
kernel/gen_config/kernel/gen_config.h
72:#define CONFIG_KERNEL_XSAVE XSAVEOPT
Would anyone have any issues if I just added the option, since support already exists in the source code?
In general, yes, no problem. Please have a look at #1325 which we are planning on finishing up soon and which may interact. I think it does have the same XSAVES conditionals, but @Indanz will be able to say more.
I'd like to figure out what went wrong here. It looks like this has worked before and then broke at some point when options were reshuffled. Maybe. We should also add at least a regression test to catch this in the future.
Yes, this issue is fixed by #1325, specifically by commit fd4e18efd9b04bcfea10d4933ddc4be20caa4ec4 (though I should have added it to the end instead of making it the default there, fixed by later). I also tried fixing the bugs mentioned in #179. Just adding the option won't give you XSAVES support, the current implementation is plain broken, the config mistake is the least of your problems.
I made a lot of changes for the FPU code, but haven't tested the changes to x86 virtualisation yet, any help you can provide with testing would be great.
I'd like to figure out what went wrong here. It looks like this has worked before and then broke at some point when options were reshuffled. Maybe. We should also add at least a regression test to catch this in the future.
What went wrong is that XSAVES was still in development and never actually properly tested. Perhaps they missed it not working because of the config bug, that's easy to miss when reviewing.
Looks like XSAVES was in the kernel prior to the switch to the CMake build system:
choice
prompt "XSAVE variant"
depends on XSAVE
default XSAVE_XSAVEOPT
help
The XSAVE area supports multiple instructions to save
and restore to it. These instructions are dependent upon
specific CPU support. See Chapter 13 of Volume 1 of the
Intel Architectures SOftware Developers Manual for
discussion on the init and modified optimizations
config XSAVE_XSAVE
bool "XSAVE"
help
Original XSAVE instruction. This is the only XSAVE
instruction that is guaranteed to exist if XSAVE
is present
config XSAVE_XSAVEC
bool "XSAVEC"
help
Save state with compaction. This compaction has
to do with minimizing the total size of XSAVE buffer,
if using non contiguous features, XSAVEC will attempt
to use the init optimization when saving
config XSAVE_XSAVEOPT
bool "XSAVEOPT"
help
Save state taking advantage of both the init
optimization and modified optimization
config XSAVE_XSAVES
bool "XSAVES"
help
Save state taking advantage of the modified
optimization. This instruction is only
available in OS code, and is the preferred
save method if it exists
And then the CMake build system was added here: https://github.com/seL4/seL4/commit/0b73072016e4898f6e08bd4a2c061c86bbaffc5f#diff-4a5070bb23239574585184693af3ffc14170cd738d0d453dbf61ea6d13402d34R130
And the XSAVES option was removed.
@lsf37 Quick question semi-related to this problem...
We have an x86 guest that needs to use the AVX-512 instructions. Setting the KernelXSaveFeatureSet to 0xE7 increases the KernelXSaveSize to 2688. That bumps the TCBBits from 12 to 13. Now in create_rootserver_objects I need to allocate the TCB Object before all of the other objects that are 1 page.
Unfortunately, on x86_64 this includes the VSpace object, so this block of code becomes rather convoluted.
#if (CONFIG_ROOT_CNODE_SIZE_BITS + seL4_SlotBits) > seL4_VSpaceBits
rootserver.cnode = alloc_rootserver_obj(cnode_size_bits, 1);
maybe_alloc_extra_bi(seL4_VSpaceBits, extra_bi_size_bits);
rootserver.vspace = alloc_rootserver_obj(seL4_VSpaceBits, 1);
#else
Is there are good way to deal with this? I have a solution that works, but since this is verified code it would be nice to get this officially supported.
I don't think the boot code is verified yet.
There is already:
/* TCBs on aarch32 can be larger than page tables in certain configs */
#if seL4_TCBBits >= seL4_PageTableBits
rootserver.tcb = alloc_rootserver_obj(seL4_TCBBits, 1);
#endif
Isn't it just a matter of moving that block to the beginning of the function? For non-x86 it doesn't matter in what order page sized objects are allocated I think, and it would fix it for x86_64.
What is your solution?
Is there are good way to deal with this? I have a solution that works, but since this is verified code it would be nice to get this officially supported.
Can you open a new issue or PR for that?
Closing this one as it should be fixed with PR #1325.