Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 25 Apr 2019 09:17:00 +0200
From:   Ingo Molnar <mingo@kernel.org>
To:     Zhao Yakui <yakui.zhao@intel.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org, tglx@linutronix.de,
        bp@alien8.de, Jason Chen CJ <jason.cj.chen@intel.com>
Subject: Re: [RFC PATCH v5 3/4] x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for
 ACRN guest upcall vector
Message-ID: <20190425071700.GB57256@gmail.com>
References: <1556067260-9128-1-git-send-email-yakui.zhao@intel.com>
 <1556067260-9128-4-git-send-email-yakui.zhao@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1556067260-9128-4-git-send-email-yakui.zhao@intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


* Zhao Yakui <yakui.zhao@intel.com> wrote:

> Linux kernel uses the HYPERVISOR_CALLBACK_VECTOR for hypervisor upcall
> vector. And it is already used for Xen and HyperV.

English sentences should not be started with 'and'.

> After ACRN hypervisor is detected, it will also use this defined vector
> to notify ACRN guest.

Missing 'the', twice.

> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_ACRN_H
> +#define _ASM_X86_ACRN_H
> +
> +void acrn_hv_callback_vector(void);

Please mark these with 'extern', as customary in x86 headers.

>  
> +#include <linux/interrupt.h>
> +#include <asm/acrn.h>
> +#include <asm/desc.h>
>  #include <asm/hypervisor.h>
> +#include <asm/irq_regs.h>
>  
>  static uint32_t __init acrn_detect(void)
>  {
> @@ -18,6 +22,8 @@ static uint32_t __init acrn_detect(void)
>  
>  static void __init acrn_init_platform(void)
>  {
> +	alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR,
> +			acrn_hv_callback_vector);

Why is this on two lines, not a single line?

> +static void (*acrn_intr_handler)(void);
> +
> +__visible void __irq_entry acrn_hv_vector_handler(struct pt_regs *regs)
> +{
> +	struct pt_regs *old_regs = set_irq_regs(regs);
> +
> +	entering_ack_irq();

Does the hypervisor model the APIC EOI command, i.e. does it require the 
APIC to be acked? I.e. would not acking the APIC create an IRQ storm?

> +	inc_irq_stat(irq_hv_callback_count);
> +
> +	if (acrn_intr_handler)
> +		acrn_intr_handler();

Nothing appears to be setting acrn_intr_handler, so this will never 
execute anything? Is more code relying on this?

Thanks,

	Ingo