Re: Nested events in 64bit mini-OS

[Xu Zhang <xzhang@cs.uic.edu>]

Samuel,

Thanks for the reply.

On 11/10/2012 07:52 AM, Samuel Thibault wrote:

> 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.
I'll try this in the following week. Thank you for pointing it out.

>> 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.
No I don't think so. I was just being pre-cautious. :-)

>> +    .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!
Nice catch!

> 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.
I don't know that either. I tried to rise guest domain an NMI with "xl 
trigger", but it seems that only works for HVM hosts.

>
> 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
Totally agree. And it somewhat saves the obligation of checking if the 
table size matches the one of critical section by looking at disassemble 
output. :-)

>
> 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
>
I really appreciate your feedback. Thanks again, Samuel!

Xu