Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 18 Dec 2020 17:38:50 +0000
Message-ID: <87v9czqaj9.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Zenghui Yu <yuzenghui@huawei.com>
Cc:     <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <tglx@linutronix.de>,
        <kvm@vger.kernel.org>, <wanghaibin.wang@huawei.com>
Subject: Re: [PATCH] genirq/msi: Initialize msi_alloc_info to zero for msi_prepare API
In-Reply-To: <20201218060039.1770-1-yuzenghui@huawei.com>
References: <20201218060039.1770-1-yuzenghui@huawei.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

Hi Zenghui,

On Fri, 18 Dec 2020 06:00:39 +0000,
Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> Since commit 5fe71d271df8 ("irqchip/gic-v3-its: Tag ITS device as shared if
> allocating for a proxy device"), some of the devices are wrongly marked as
> "shared" by the ITS driver on systems equipped with the ITS(es). The
> problem is that the @info->flags may not be initialized anywhere and we end
> up looking at random bits on the stack. That's obviously not good.
> 
> The straightforward fix is to properly initialize msi_alloc_info inside the
> .prepare callback of affected MSI domains (its-pci-msi, its-platform-msi,
> etc). We can also perform the initialization in IRQ core layer for
> msi_domain_prepare_irqs() API and it looks much neater to me.
> 
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
> 
> This was noticed when I was playing with the assigned devices on arm64 and
> VFIO failed to enable MSI-X vectors for almost all VFs (CCed kvm list in
> case others will hit the same issue). It turned out that these VFs are
> marked as "shared" by mistake and have trouble with the following sequence:
> 
> 	pci_alloc_irq_vectors(pdev, 1, 1, flag);
> 	pci_free_irq_vectors(pdev);
> 	pci_alloc_irq_vectors(pdev, 1, 2, flag); --> we can only get
> 						     *one* vector
> 
> But besides VFIO, I guess there are already some devices get into trouble
> at probe time and can't work properly.
>
>  kernel/irq/msi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 2c0c4d6d0f83..dc0e2d7fbdfd 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -402,7 +402,7 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
>  	struct msi_domain_ops *ops = info->ops;
>  	struct irq_data *irq_data;
>  	struct msi_desc *desc;
> -	msi_alloc_info_t arg;
> +	msi_alloc_info_t arg = { };
>  	int i, ret, virq;
>  	bool can_reserve;

Thanks for having investigated this. I guess my only worry with this
is that msi_alloc_info_t is a pretty large structure on x86, and
zeroing it isn't totally free. But this definitely looks nicer than
some of the alternatives (.prepare isn't a good option, as we do rely
on the flag being set in __platform_msi_create_device_domain(), which
calls itself .prepare).

I'll queue it, and we can always revisit this later if Thomas (or
anyone else) has a better idea.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.