Re: [RFC PATCH v5 3/4] x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for ACRN guest upcall vector

[Zhao, Yakui]

On 2019å04æ25æ 15:17, Ingo Molnar wrote:
> * Zhao Yakui <yakui.zhao@xxxxxxxxx> 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'.

OK. I will remove it.

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

OK. It will be added.

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

OK. The "extern" will be added.

> >   
> > +#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?

At first it used the long function name for acrn_hv_callback_vector.
As it exceeds 80 columns, it is split into two lines.
I will check it and see whether it can be fit into one single line.
If it is ok, it will be in one 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?

The hypervisor requires that the APIC EOI should be acked. If the EOI 
APIC is not acked, the APIC ISR bit for the HYPERVISOR_CALLBACK_VECTOR 
will not be cleared and then it will block the interrupt whose vector is 
lower than HYPERVISOR_CALLBACK_VECTOR.

> > +	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?

Nothing will be done in this patch.
Later the driver part code will be added and it needs to add/remove the 
corresponding driver handler. It is similar to vmbus_handler in 
hyperv_vector_handler.

> Thanks,
>
> 	Ingo