Re: Nested events in 64bit mini-OS

[Samuel Thibault <samuel.thibault@ens-lyon.org>]

Hello,

Xu Zhang, le Mon 05 Nov 2012 23:56:04 -0600, a écrit :
> I haven't seen any updates on this matter, so I try to come up with a fix.

Thanks!

> Generally speaking, I want to mimic 32-bit mini-OS behaviour,

Ok. Probably a good thing :)

> The "git diff" output is attached to this email. I only did some naive
> "tests" by running it inside gdb.

Do you mean that you actually ran a stack merging, even if only in gdb?
I would trust that well enough.

> I am wondering is there a test suite for mini-OS?

You can simply run it, giving it a blk dev (or also a fb dev), it will
run app_main(), which keeps reading stuff, and draws squares on the fb.

Ideally you should manage to trigger fixups, by stuffing a wait loop in
the critical section.

> A follow-up question is that given this fix, do you still need
> hypercall iret?

Actually it's worse than that, see below.

> +	movq %rbx,5*8(%rsp)
...
> +
> +	# check against re-entrance
> +	movq RIP(%rsp),%rbx
> +	cmpq $scrit,%rbx
> +	jb 10f
> +	cmpq $ecrit,%rbx
> +	jb  critical_region_fixup
> +
> +10:	movq RBX(%rsp),%rbx			# restore rbx

Do we really need to restore %rbx?  I don't think do_hypervisor_callback
uses it.

> +    .byte 0x78,0x78,0x78,0x78,0x78            # jmp    hypercall_page + (__HYPERVISOR_iret * 32)

Here we would also need a fixup table for the code at hypercall_page!
A nicer fix would be to inline the hypercall code here.  That said, I
have to say I don't know any detail about the NMI flag, I don't see it
defined in the Intel manual, and I don't see it being set in the Xen
hypervisor.  Unless somebody knows more about it, I would assume that it
currently never happens, and simply stuff a

ud2 /* TODO: fix re-entrance fixup for hypercall in NMI case (when does that happen actually?) */

before the jmp, to catch it if it ever does happen.

Also, it would be good to check against critical section size change, in
case somebody e.g. changes a value, or a macro like XEN_PUT_VCPU_INFO.
For instance, stuff right after the table:

	.if (ecrit-scrit) != (critical_fixup_table_end - critical_fixup_table)
	.error "The critical has changed, the fixup table needs updating"
	.endif

More generally, some cleanup could go along the patch, but I'd say
keep it as you have done, focused on only the fixup code, to have it
as such in the repository history, and then we could clean some things
afterwards.

Samuel