Re: [PATCH v10 5/5] x86/hyperv: Change interrupt vector for nested root partition

Borislav Petkov
2023-01-12T17:34:01Z

On Mon, Jan 02, 2023 at 07:12:55AM +0000, Jinank Jain wrote:
> Traditionally we have been using the HYPERVISOR_CALLBACK_VECTOR to relay

Who's "we"?

Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.

Also, pls read section "2) Describe your changes" in
Documentation/process/submitting-patches.rst for more details.

Also, see section "Changelog" in
Documentation/process/maintainer-tip.rst

Bottom line is: personal pronouns are ambiguous in text, especially with
so many parties/companies/etc developing the kernel so let's avoid them
please.

> the VMBus interrupt. But this does not work in case of nested
> hypervisor. Microsoft Hypervisor reserves 0x31 to 0x34 as the interrupt
> vector range for VMBus and thus we have to use one of the vectors from
> that range and setup the IDT accordingly.
>
> Signed-off-by: Jinank Jain <jinankjain@linux.microsoft.com>
> ---
> arch/x86/include/asm/idtentry.h | 2 ++
> arch/x86/include/asm/irq_vectors.h | 6 ++++++
> arch/x86/kernel/cpu/mshyperv.c | 15 +++++++++++++++
> arch/x86/kernel/idt.c | 10 ++++++++++
> drivers/hv/vmbus_drv.c | 3 ++-
> 5 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
> index 72184b0b2219..c0648e3e4d4a 100644
> --- a/arch/x86/include/asm/idtentry.h
> +++ b/arch/x86/include/asm/idtentry.h
> @@ -686,6 +686,8 @@ DECLARE_IDTENTRY_SYSVEC(POSTED_INTR_NESTED_VECTOR, sysvec_kvm_posted_intr_nested
> DECLARE_IDTENTRY_SYSVEC(HYPERVISOR_CALLBACK_VECTOR, sysvec_hyperv_callback);
> DECLARE_IDTENTRY_SYSVEC(HYPERV_REENLIGHTENMENT_VECTOR, sysvec_hyperv_reenlightenment);
> DECLARE_IDTENTRY_SYSVEC(HYPERV_STIMER0_VECTOR, sysvec_hyperv_stimer0);
> +DECLARE_IDTENTRY_SYSVEC(HYPERV_INTR_NESTED_VMBUS_VECTOR,
> + sysvec_hyperv_nested_vmbus_intr);
> #endif
>
> #if IS_ENABLED(CONFIG_ACRN_GUEST)
> diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h
> index 43dcb9284208..729d19eab7f5 100644
> --- a/arch/x86/include/asm/irq_vectors.h
> +++ b/arch/x86/include/asm/irq_vectors.h
> @@ -102,6 +102,12 @@
> #if IS_ENABLED(CONFIG_HYPERV)
> #define HYPERV_REENLIGHTENMENT_VECTOR 0xee
> #define HYPERV_STIMER0_VECTOR 0xed
> +/*
> + * FIXME: Change this, once Microsoft Hypervisor changes its assumption
^^^^^^

This patch looks like it is not ready to go anywhere yet...

> + * around VMBus interrupt vector allocation for nested root partition.

When is that going to happen? If at all...

> + * Or provides a better interface to detect this instead of hardcoding.
> + */
> +#define HYPERV_INTR_NESTED_VMBUS_VECTOR 0x31
> #endif
>
> #define LOCAL_TIMER_VECTOR 0xec
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 938fc82edf05..4dfe0f9d7be3 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -126,6 +126,21 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_callback)
> set_irq_regs(old_regs);
> }
>
> +DEFINE_IDTENTRY_SYSVEC(sysvec_hyperv_nested_vmbus_intr)
> +{
> + struct pt_regs *old_regs = set_irq_regs(regs);
> +
> + inc_irq_stat(irq_hv_callback_count);
> +
> + if (vmbus_handler)
> + vmbus_handler();
> +
> + if (ms_hyperv.hints & HV_DEPRECATING_AEOI_RECOMMENDED)
> + ack_APIC_irq();
> +
> + set_irq_regs(old_regs);
> +}
> +
> void hv_setup_vmbus_handler(void (*handler)(void))
> {
> vmbus_handler = handler;
> diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c
> index a58c6bc1cd68..3536935cea39 100644
> --- a/arch/x86/kernel/idt.c
> +++ b/arch/x86/kernel/idt.c
> @@ -160,6 +160,16 @@ static const __initconst struct idt_data apic_idts[] = {
> # endif
> INTG(SPURIOUS_APIC_VECTOR, asm_sysvec_spurious_apic_interrupt),
> INTG(ERROR_APIC_VECTOR, asm_sysvec_error_interrupt),
> +#ifdef CONFIG_HYPERV
> + /*
> + * This is a hack because we cannot install this interrupt handler
> + * via alloc_intr_gate as it does not allow interrupt vector less
> + * than FIRST_SYSTEM_VECTORS. And hyperv does not want anything other
> + * than 0x31-0x34 as the interrupt vector for vmbus interrupt in case

Well:

/*
* IDT vectors usable for external interrupt sources start at 0x20.
* (0x80 is the syscall vector, 0x30-0x3f are for ISA)
^^^^^^^^^^^^^^^^^^^^^^

*/
#define FIRST_EXTERNAL_VECTOR 0x20

I guess HyperV decided to reuse those...?

--
Regards/Gruss,
Boris.