From: "Zhao, Yakui" <yakui.zhao@intel.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"x86@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
Date: Mon, 25 Mar 2019 06:12:25 +0000 [thread overview]
Message-ID: <01363028D91B0B448414F51B4258CAA574CD3DF5@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <alpine.DEB.2.21.1903221644530.1729@nanos.tec.linutronix.de>
>-----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
next prev parent reply other threads:[~2019-03-25 6:12 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-07 2:04 [RFC PATCH 0/3] arch/x86: Add the support of ACRN hypervisor under arch/x86 Zhao Yakui
2019-03-07 2:04 ` [RFC PATCH 1/3] arch/x86: add ACRN hypervisor guest Zhao Yakui
2019-03-22 15:20 ` Thomas Gleixner
2019-03-25 3:24 ` Zhao, Yakui
2019-03-23 15:17 ` Sean Christopherson
2019-03-07 2:04 ` [RFC PATCH 2/3] arch/x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for Acrn upcall vector Zhao Yakui
2019-03-22 15:44 ` Thomas Gleixner
2019-03-22 19:53 ` Borislav Petkov
2019-03-25 3:57 ` Zhao, Yakui
2019-03-25 8:27 ` Thomas Gleixner
2019-03-26 0:58 ` Zhao, Yakui
2019-03-07 2:04 ` [RFC PATCH 3/3] arch/x86/acrn: add hypercall for acrn_hypervisor Zhao Yakui
2019-03-22 16:01 ` Thomas Gleixner
2019-03-25 6:12 ` Zhao, Yakui [this message]
2019-03-25 8:30 ` Thomas Gleixner
2019-03-26 0:59 ` Zhao, Yakui
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=01363028D91B0B448414F51B4258CAA574CD3DF5@SHSMSX101.ccr.corp.intel.com \
--to=yakui.zhao@intel.com \
--cc=jason.cj.chen@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).