Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Jinank Jain <jinankjain@linux.microsoft.com>
Cc:     kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org,
        decui@microsoft.com, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com,
        arnd@arndb.de, peterz@infradead.org, jpoimboe@kernel.org,
        jinankjain@linux.microsoft.com, seanjc@google.com,
        kirill.shutemov@linux.intel.com, ak@linux.intel.com,
        sathyanarayanan.kuppuswamy@linux.intel.com,
        linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arch@vger.kernel.org, anrayabh@linux.microsoft.com,
        mikelley@microsoft.com
Subject: Re: [PATCH v10 5/5] x86/hyperv: Change interrupt vector for nested
 root partition
In-Reply-To: <021f748f15870f3e41f417511aa88607627ec327.1672639707.git.jinankjain@linux.microsoft.com>
References: <cover.1672639707.git.jinankjain@linux.microsoft.com>
 <021f748f15870f3e41f417511aa88607627ec327.1672639707.git.jinankjain@linux.microsoft.com>
Date:   Thu, 12 Jan 2023 21:00:32 +0100
Message-ID: <87o7r3ed5r.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Jinank!

On Mon, Jan 02 2023 at 07:12, Jinank Jain wrote:
> --- a/arch/x86/include/asm/irq_vectors.h
> +++ b/arch/x86/include/asm/irq_vectors.h
> +/*
> + * FIXME: Change this, once Microsoft Hypervisor changes its assumption
> + * around VMBus interrupt vector allocation for nested root partition.
> + * Or provides a better interface to detect this instead of hardcoding.
> + */
> +#define HYPERV_INTR_NESTED_VMBUS_VECTOR	0x31

arch/x86/include/asm/irq_vectors.h line 47:

/*
 * Vectors 0x30-0x3f are used for ISA interrupts.
 *   round up to the next 16-vector boundary
 */
#define ISA_IRQ_VECTOR(irq)		(((FIRST_EXTERNAL_VECTOR + 16) & ~15) + irq)

So this overlaps with the legacy interrupt vector space.

> +#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
> +	 * of nested setup.
> +	 */
> +	INTG(HYPERV_INTR_NESTED_VMBUS_VECTOR, asm_sysvec_hyperv_nested_vmbus_intr),
> +#endif

I agree, that this is a hack, but that puts it mildly: It's a completely
broken hack.

> +DECLARE_IDTENTRY_SYSVEC(HYPERV_INTR_NESTED_VMBUS_VECTOR, sysvec_hyperv_nested_vmbus_intr);

This generates the low level entry stub for vector 0x31 at compile time,
which competes with the interrupt stub for external interrupts generated
by:

      SYM_CODE_START(irq_entries_start)


Now the above INTG() hard-codes the IDT entry for vector 0x31 into the
apic_idts table. That marks it as system vector which in turn prevents
idt_setup_apic_and_irq_gates() to install the IDT entry for the external
vector on _ALL_ systems unconditionally.

IOW, you broke world except for systems which do not use the legacy
interrupt space. Congrats!

That legacy space is hardcoded and that's clearly documented so.

0x31 becomes IRQ1 - usually the i8042 - which makes it pretty much
guaranteed that this collides and fails. The worst case consequence is a
fully uncontrolled interrupt storm which is not even detectable.

So this patch is /dev/null material and either the hypervisor side makes
it possible to use a different vector space or this needs some very
careful modifications to the legacy ISA vector assignment.

Thanks,

        tglx