linux-kernel - Re: [RFC PATCH 3/3] arch/x86/acrn: add hypercall for acrn

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.21.1903221644530.1729@nanos.tec.linutronix.de>
Date:   Fri, 22 Mar 2019 17:01:51 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Zhao Yakui <yakui.zhao@...el.com>
cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Jason Chen CJ <jason.cj.chen@...el.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?

> +/*
> + * 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