linux-kernel - Re: [RFC PATCH v2 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 for Android: free password hash cracker in your pocket

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

Message-ID: <alpine.DEB.2.21.1904052110370.1802@nanos.tec.linutronix.de>
Date:   Fri, 5 Apr 2019 21:19:29 +0200 (CEST)
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 v2 3/3] arch/x86/acrn: add hypercall for acrn_guest

On Tue, 26 Mar 2019, Zhao Yakui wrote:

> When acrn_hypervisor is detected, the hypercall is needed so that the
> acrn guest can 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.
> 
> So the hypercall is added so that the kernel can communicate with the
> low-level acrn-hypervisor. On x86 it is implemented by using vmacll when

is implemented with the VMCALL instruction

> the acrn hypervisor is enabled.
> 
> +++ b/arch/x86/include/asm/acrn_hypercall.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * acrn_hypercall.h : acrn hypervisor call API

No file names in headers please. They are pointless and get out of sync
when files are renamed.

> + */
> +
> +#ifndef __ACRN_HYPERCALL_H__
> +#define __ACRN_HYPERCALL_H__

asm headers start with

    __ASM_X86_....

> +
> +#include <linux/errno.h>
> +
> +#ifdef CONFIG_ACRN_GUEST
> +
> +/*
> + * 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.
> + */
> +
> +static inline long acrn_hypercall0(unsigned long hcall_id)
> +{
> +

Remove the empty new line please.

> +	register long result asm("rax");
> +	register unsigned long r8 asm("r8") = hcall_id;

Please order them the other way around, like a reverse christmas tree:

	register unsigned long r8 asm("r8") = hcall_id;
	register long result asm("rax");

That's way simpler to read.

> +	asm volatile(".byte 0x0F,0x01,0xC1\n"
> +			: "=r"(result)
> +			:  "r"(r8));

Please mention in the changelog why this is implemented with bytes and not
with the proper mnemonic. I assume it depends on binutils, so please
document which version of binutils supports the mnemonic.

And in the first function, i.e. hypercall0, add a comment above the asm
volatile() to that effect as well. 

Thanks,

	tglx