On Mon, Aug 22, 2022, Michal Luczaj wrote:
> On 8/22/22 00:06, Michal Luczaj wrote:
> > Note that doing this the ASM_TRY() way would require extending
> > setup_idt() (to handle #DB) and introducing another ASM_TRY() variant
> > (one without the initial `movl $0, %%gs:4`).
>
> Replying to self as I was wrong regarding the need for another ASM_TRY() variant.
> Once setup_idt() is told to handle #DB,
Hmm, it might be a moot point for this patch (see below), but my vote is to have
setup_idt() wire up all known handlers to check_exception_table(). I don't see
any reason to skip some vectors. Code with __ASM_TRY() will explode no matter what,
so it's not like it risks suppressing completely unexpected faults.
for (i = 0; i < 32; i++) {
if (!idt_handlers[i])
continue;
set_idt_entry(i, idt_handlers[i], 0);
handle_exception(i, check_exception_table);
}
> test can be simplified to something like
>
> asm volatile("push %[ss]\n\t"
> __ASM_TRY(KVM_FEP "pop %%ss", "1f")
> "ex_blocked: mov $1, %[success]\n\t"
So I'm 99% certain this only passes because KVM doesn't emulate the `mov $1, %[success]`.
kvm_vcpu_check_code_breakpoint() honors EFLAGS.RF, but not MOV/POP_SS blocking.
I.e. I would expect this to fail if the `mov` were also tagged KVM_FEP.
Single-step and data #DBs appear to be broken as well, I don't see anything that
will suppress them for MOV/POP_SS. Of course, KVM doesn't emulate data #DBs at
all, so testing that case is probably not worthwhile at this time.
The reason I say the setup_idt() change is a moot point is because I think it
would be better to put this testcase into debug.c (where "this" testcase is really
going to be multiple testcases). With macro shenanigans (or code patching), it
should be fairly easy to run all testcases with both FEP=0 and FEP=1.
Then it's "just" a matter of adding a code #DB testcase. __run_single_step_db_test()
and run_ss_db_test() aren't fundamentally tied to single-step, i.e. can simply be
renamed. For simplicity, I'd say just skip the usermode test for code #DBs, IMO
it's not worth the extra coverage.
