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?
> +/*
> + * 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.
> + 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'.
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.
> + return result;
> +}
Thanks,
tglx
