Messages in this thread | | | From | "Zhao, Yakui" <> | Subject | RE: [RFC PATCH 3/3] arch/x86/acrn: add hypercall for acrn_hypervisor | Date | Mon, 25 Mar 2019 06:12:25 +0000 |
| |
>-----Original Message----- >From: Thomas Gleixner [mailto:tglx@linutronix.de] >Sent: Saturday, March 23, 2019 12:02 AM >To: Zhao, Yakui <yakui.zhao@intel.com> >Cc: linux-kernel@vger.kernel.org; x86@kernel.org; Chen, Jason CJ ><jason.cj.chen@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
| |