>-----Original Message-----
>From: Thomas Gleixner [mailto:t...@linutronix.de]
>Sent: Saturday, March 23, 2019 12:02 AM
>To: Zhao, Yakui <yakui.z...@intel.com>
>Cc: linux-kernel@vger.kernel.org; x...@kernel.org; Chen, Jason CJ
><jason.cj.c...@intel.com>
>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
