Re: [PATCH] genirq/msi: Initialize msi_alloc_info to zero for msi_prepare API

[Marc Zyngier]

Hi Zenghui,

On Fri, 18 Dec 2020 06:00:39 +0000,
Zenghui Yu <yuzenghui@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 
> 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.