Re: [PATCH 3/6] mini-os/x86-64 entry: code refactoring; no functional changes

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

On 03/09/2013 03:03 PM, Samuel Thibault wrote:
>> +/* Macros */
>> +.macro zeroentry sym
>> +	movq (%rsp),%rcx
>> +	movq 8(%rsp),%r11
>> +	addq $0x10,%rsp /* skip rcx and r11 */
>> +	pushq $0	/* push error code/oldrax */
>> +	pushq %rax	/* push real oldrax to the rdi slot */
>> +	leaq  \sym(%rip),%rax
>> +	jmp error_entry
>> +.endm
>> +
>> +.macro errorentry sym
>> +	movq (%rsp),%rcx
>> +	movq 8(%rsp),%r11
>> +	addq $0x10,%rsp /* rsp points to the error code */
>> +	pushq %rax
>> +	leaq  \sym(%rip),%rax
>> +	jmp error_entry
>> +.endm
> I disagree on moving those, as they do have some context.

Well, if they have some context, they probably end off better before 
they first get called, say before hypervisor_callback. Note they are 
also used in exception handlers. Besides, it's kind of annoying to have 
these macros sit in between assembly routines. Probably it's just me. If 
you prefer leave them untouched, I am perfectly fine with that.

>
>>   retint_kernel:
>>   retint_restore_args:
> Please also drop these labels, they're not used.
>

I would argue it gonna be a different story if mini-os has an user 
space: "retint_kernel" returns to kernel, and presumably "retint_user" 
would do schedule, x87 lazy switch, handle softirqs and etc. But since 
it does not, I guess they can be dropped.