RE: [RFC PATCH 3/3] arch/x86/acrn: add hypercall for acrn_hypervisor

[Zhao, Yakui]

>-----Original Message-----
>From: Thomas Gleixner [mailto:tglx@xxxxxxxxxxxxx]
>Sent: Saturday, March 23, 2019 12:02 AM
>To: Zhao, Yakui <yakui.zhao@xxxxxxxxx>
>Cc: linux-kernel@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; Chen, Jason CJ
><jason.cj.chen@xxxxxxxxx>
>Subject: Re: [RFC PATCH 3/3] arch/x86/acrn: add hypercall for acrn_hypervisor
>
>On Thu, 7 Mar 2019, Zhao Yakui wrote:
>
>> When acrn_hypervisor is detected, the other module may communicate
>> with
>
>Which other module?
>
>> the hypervisor to query/config some settings. For example: it can be
>> used to query the resources in hypervisor and manage the
>> CPU/memory/device/ interrupt for Guest system.
>
>> +++ b/arch/x86/include/asm/acrn_hypercall.h
>> @@ -0,0 +1,85 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause */
>
>So the hypercall header is GPL-2.0+ and the acrn.c file is GPL-2.0. Not that I
>personally care, but is this intended?

Sorry for my fault. It should be GPL-2.0

>
>> +/*
>> + * acrn_hypercall.h : acrn hypervisor call API  */
>> +
>> +#ifndef __ACRN_HYPERCALL_H__
>> +#define __ACRN_HYPERCALL_H__
>> +
>> +#include <linux/errno.h>
>> +
>> +#ifdef CONFIG_ACRN
>> +
>> +static inline long acrn_hypercall0(unsigned long hcall_id) {
>> +
>> +	/* x86-64 System V ABI register usage */
>
>Well, yes. But can you please provide a link to the hypercall specification in the
>changelog?
>
>Also instead of repeating the same comment over and over, explain the calling
>convention in a top level comment.

This is the low-level implementation of low-level hypercall. It is similar to 
Kvm_hypercall1/2/3 in arch/x86/include/asm/kvm_para.h.
Does it make sense that the below comments are added before acrn_hypercall0/1/2?
/*
 * Hypercalls for ACRN Guest
 * 
 * Hypercall number is passed in r8 register.
 * Return value will be placed in rax. 
 * Up to 2 arguments are passed in rdi, rsi.
 */   

Of course I will also remove the redundant comments.

>
>> +	register signed long    result asm("rax");
>> +	register unsigned long  r8 asm("r8")  = hcall_id;
>
>I appreciate the attempt of making this tabular, but it's unreadable and has
>random white space in it. Also please use 'long' and not 'signed long'.

Ok. The type will be refined.


>
>	register unsigned long	r8	asm("r8") = hcall_id;
>	register long		result	asm("rax");
>
>> +
>> +	/* Execute vmcall */
>> +	asm volatile(".byte 0x0F,0x01,0xC1\n"
>> +			: "=r"(result)
>> +			:  "r"(r8));
>> +
>> +	/* Return result to caller */
>
>Please do not comment the obvious. It's entirely clear that this returns 'result'
>to the caller. Comments are there to explain what's not obvious.

Thanks for pointing out the issue.
I will remove the redundant comment.

>
>> +	return result;
>> +}
>
>Thanks,
>
>	tglx