subject:"\[tip\:irq\/urgent\] genirq\/msi\: Make sure PCI MSIs are activated early"

RE: [tip:irq/urgent] genirq/msi: Make sure PCI MSIs are activated early

2016-09-02 Thread Bharat Kumar Gogada

Thanks Marc and Thomas for addressing the issue.

> -Original Message-
> From: tip tree robot [mailto:tip...@zytor.com]
> Sent: Tuesday, August 09, 2016 12:59 PM
> To: linux-tip-comm...@vger.kernel.org
> Cc: Bharat Kumar Gogada <bhara...@xilinx.com>; bhelg...@google.com;
> h...@zytor.com; li...@matthiasprager.de; marc.zyng...@arm.com;
> jason.tay...@simplivity.com; linux-kernel@vger.kernel.org;
> fo...@forstwoof.ru; mi...@kernel.org; t...@linutronix.de
> Subject: [tip:irq/urgent] genirq/msi: Make sure PCI MSIs are activated early
> 
> Commit-ID:  f3b0946d629c8bfbd3e5f038e30cb9c711a35f10
> Gitweb:
> http://git.kernel.org/tip/f3b0946d629c8bfbd3e5f038e30cb9c711a35f10
> Author: Marc Zyngier <marc.zyng...@arm.com>
> AuthorDate: Wed, 13 Jul 2016 17:18:33 +0100
> Committer:  Thomas Gleixner <t...@linutronix.de>
> CommitDate: Tue, 9 Aug 2016 09:19:32 +0200
> 
> genirq/msi: Make sure PCI MSIs are activated early
> 
> Bharat Kumar Gogada reported issues with the generic MSI code, where the
> end-point ended up with garbage in its MSI configuration (both for the vector
> and the message).
> 
> It turns out that the two MSI paths in the kernel are doing slightly different
> things:
> 
> generic MSI: disable MSI -> allocate MSI -> enable MSI -> setup EP
> PCI MSI: disable MSI -> allocate MSI -> setup EP -> enable MSI
> 
> And it turns out that end-points are allowed to latch the content of the MSI
> configuration registers as soon as MSIs are enabled.  In Bharat's case, the
> end-point ends up using whatever was there already, which is not what you
> want.
> 
> In order to make things converge, we introduce a new MSI domain flag
> (MSI_FLAG_ACTIVATE_EARLY) that is unconditionally set for PCI/MSI. When set,
> this flag forces the programming of the end-point as soon as the MSIs are
> allocated.
> 
> A consequence of this is that we have an extra activate in irq_startup, but
> that should be without much consequence.
> 
> tglx:
> 
>  - Several people reported a VMWare regression with PCI/MSI-X passthrough. It
>turns out that the patch also cures that issue.
> 
>  - We need to have a look at the MSI disable interrupt path, where we write
>the msg to all zeros without disabling MSI in the PCI device. Is that
>correct?
> 
> Fixes: 52f518a3a7c2 "x86/MSI: Use hierarchical irqdomains to manage MSI
> interrupts"
> Reported-and-tested-by: Bharat Kumar Gogada
> <bharat.kumar.gog...@xilinx.com>
> Reported-and-tested-by: Foster Snowhill <fo...@forstwoof.ru>
> Reported-by: Matthias Prager <li...@matthiasprager.de>
> Reported-by: Jason Taylor <jason.tay...@simplivity.com>
> Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
> Acked-by: Bjorn Helgaas <bhelg...@google.com>
> Cc: linux-...@vger.kernel.org
> Cc: sta...@vger.kernel.org
> Link: http://lkml.kernel.org/r/1468426713-31431-1-git-send-email-
> marc.zyng...@arm.com
> Signed-off-by: Thomas Gleixner <t...@linutronix.de>
> 
> ---
>  drivers/pci/msi.c   |  2 ++
>  include/linux/msi.h |  2 ++
>  kernel/irq/msi.c| 11 +++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a02981e..eafa613 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1411,6 +1411,8 @@ struct irq_domain *pci_msi_create_irq_domain(struct
> fwnode_handle *fwnode,
>   if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
>   pci_msi_domain_update_chip_ops(info);
> 
> + info->flags |= MSI_FLAG_ACTIVATE_EARLY;
> +
>   domain = msi_create_irq_domain(fwnode, info, parent);
>   if (!domain)
>   return NULL;
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 4f0bfe5..e8c81fb 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -270,6 +270,8 @@ enum {
>   MSI_FLAG_MULTI_PCI_MSI  = (1 << 2),
>   /* Support PCI MSIX interrupts */
>   MSI_FLAG_PCI_MSIX   = (1 << 3),
> + /* Needs early activate, required for PCI */
> + MSI_FLAG_ACTIVATE_EARLY = (1 << 4),
>  };
> 
>  int msi_domain_set_affinity(struct irq_data *data, const struct cpumask 
> *mask,
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 5499935..19e9dfb 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -359,6 +359,17 @@ int msi_domain_alloc_irqs(struct irq_domain *domain,
> struct device *dev,
>   else
>   dev_dbg(dev, "irq [%d-%d] for MSI\n",
>   virq, virq + desc->nvec_used - 1);
> + /*
> +  * This f

RE: [tip:irq/urgent] genirq/msi: Make sure PCI MSIs are activated early

2016-09-02 Thread Bharat Kumar Gogada

Thanks Marc and Thomas for addressing the issue.

> -Original Message-
> From: tip tree robot [mailto:tip...@zytor.com]
> Sent: Tuesday, August 09, 2016 12:59 PM
> To: linux-tip-comm...@vger.kernel.org
> Cc: Bharat Kumar Gogada ; bhelg...@google.com;
> h...@zytor.com; li...@matthiasprager.de; marc.zyng...@arm.com;
> jason.tay...@simplivity.com; linux-kernel@vger.kernel.org;
> fo...@forstwoof.ru; mi...@kernel.org; t...@linutronix.de
> Subject: [tip:irq/urgent] genirq/msi: Make sure PCI MSIs are activated early
> 
> Commit-ID:  f3b0946d629c8bfbd3e5f038e30cb9c711a35f10
> Gitweb:
> http://git.kernel.org/tip/f3b0946d629c8bfbd3e5f038e30cb9c711a35f10
> Author: Marc Zyngier 
> AuthorDate: Wed, 13 Jul 2016 17:18:33 +0100
> Committer:  Thomas Gleixner 
> CommitDate: Tue, 9 Aug 2016 09:19:32 +0200
> 
> genirq/msi: Make sure PCI MSIs are activated early
> 
> Bharat Kumar Gogada reported issues with the generic MSI code, where the
> end-point ended up with garbage in its MSI configuration (both for the vector
> and the message).
> 
> It turns out that the two MSI paths in the kernel are doing slightly different
> things:
> 
> generic MSI: disable MSI -> allocate MSI -> enable MSI -> setup EP
> PCI MSI: disable MSI -> allocate MSI -> setup EP -> enable MSI
> 
> And it turns out that end-points are allowed to latch the content of the MSI
> configuration registers as soon as MSIs are enabled.  In Bharat's case, the
> end-point ends up using whatever was there already, which is not what you
> want.
> 
> In order to make things converge, we introduce a new MSI domain flag
> (MSI_FLAG_ACTIVATE_EARLY) that is unconditionally set for PCI/MSI. When set,
> this flag forces the programming of the end-point as soon as the MSIs are
> allocated.
> 
> A consequence of this is that we have an extra activate in irq_startup, but
> that should be without much consequence.
> 
> tglx:
> 
>  - Several people reported a VMWare regression with PCI/MSI-X passthrough. It
>turns out that the patch also cures that issue.
> 
>  - We need to have a look at the MSI disable interrupt path, where we write
>the msg to all zeros without disabling MSI in the PCI device. Is that
>correct?
> 
> Fixes: 52f518a3a7c2 "x86/MSI: Use hierarchical irqdomains to manage MSI
> interrupts"
> Reported-and-tested-by: Bharat Kumar Gogada
> 
> Reported-and-tested-by: Foster Snowhill 
> Reported-by: Matthias Prager 
> Reported-by: Jason Taylor 
> Signed-off-by: Marc Zyngier 
> Acked-by: Bjorn Helgaas 
> Cc: linux-...@vger.kernel.org
> Cc: sta...@vger.kernel.org
> Link: http://lkml.kernel.org/r/1468426713-31431-1-git-send-email-
> marc.zyng...@arm.com
> Signed-off-by: Thomas Gleixner 
> 
> ---
>  drivers/pci/msi.c   |  2 ++
>  include/linux/msi.h |  2 ++
>  kernel/irq/msi.c| 11 +++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a02981e..eafa613 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1411,6 +1411,8 @@ struct irq_domain *pci_msi_create_irq_domain(struct
> fwnode_handle *fwnode,
>   if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
>   pci_msi_domain_update_chip_ops(info);
> 
> + info->flags |= MSI_FLAG_ACTIVATE_EARLY;
> +
>   domain = msi_create_irq_domain(fwnode, info, parent);
>   if (!domain)
>   return NULL;
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 4f0bfe5..e8c81fb 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -270,6 +270,8 @@ enum {
>   MSI_FLAG_MULTI_PCI_MSI  = (1 << 2),
>   /* Support PCI MSIX interrupts */
>   MSI_FLAG_PCI_MSIX   = (1 << 3),
> + /* Needs early activate, required for PCI */
> + MSI_FLAG_ACTIVATE_EARLY = (1 << 4),
>  };
> 
>  int msi_domain_set_affinity(struct irq_data *data, const struct cpumask 
> *mask,
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 5499935..19e9dfb 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -359,6 +359,17 @@ int msi_domain_alloc_irqs(struct irq_domain *domain,
> struct device *dev,
>   else
>   dev_dbg(dev, "irq [%d-%d] for MSI\n",
>   virq, virq + desc->nvec_used - 1);
> + /*
> +  * This flag is set by the PCI layer as we need to activate
> +  * the MSI entries before the PCI layer enables MSI in the
> +  * card. Otherwise the card latches a random msi message.
> +  */
> + if (info->flags & MSI_FLAG_ACTIVATE_EARLY) {
> + struct irq_data *irq_data;
> +
> + irq_data = irq_domain_get_irq_data(domain, desc-
> >irq);
> + irq_domain_activate_irq(irq_data);
> + }
>   }
> 
>   return 0;

[tip:irq/urgent] genirq/msi: Make sure PCI MSIs are activated early

2016-08-09 Thread tip-bot for Marc Zyngier

Commit-ID:  f3b0946d629c8bfbd3e5f038e30cb9c711a35f10
Gitweb: http://git.kernel.org/tip/f3b0946d629c8bfbd3e5f038e30cb9c711a35f10
Author: Marc Zyngier 
AuthorDate: Wed, 13 Jul 2016 17:18:33 +0100
Committer:  Thomas Gleixner 
CommitDate: Tue, 9 Aug 2016 09:19:32 +0200

genirq/msi: Make sure PCI MSIs are activated early

Bharat Kumar Gogada reported issues with the generic MSI code, where the
end-point ended up with garbage in its MSI configuration (both for the vector
and the message).

It turns out that the two MSI paths in the kernel are doing slightly different
things:

generic MSI: disable MSI -> allocate MSI -> enable MSI -> setup EP
PCI MSI: disable MSI -> allocate MSI -> setup EP -> enable MSI

And it turns out that end-points are allowed to latch the content of the MSI
configuration registers as soon as MSIs are enabled.  In Bharat's case, the
end-point ends up using whatever was there already, which is not what you
want.

In order to make things converge, we introduce a new MSI domain flag
(MSI_FLAG_ACTIVATE_EARLY) that is unconditionally set for PCI/MSI. When set,
this flag forces the programming of the end-point as soon as the MSIs are
allocated.

A consequence of this is that we have an extra activate in irq_startup, but
that should be without much consequence.

tglx: 

 - Several people reported a VMWare regression with PCI/MSI-X passthrough. It
   turns out that the patch also cures that issue.

 - We need to have a look at the MSI disable interrupt path, where we write
   the msg to all zeros without disabling MSI in the PCI device. Is that
   correct?

Fixes: 52f518a3a7c2 "x86/MSI: Use hierarchical irqdomains to manage MSI 
interrupts"
Reported-and-tested-by: Bharat Kumar Gogada 
Reported-and-tested-by: Foster Snowhill 
Reported-by: Matthias Prager 
Reported-by: Jason Taylor 
Signed-off-by: Marc Zyngier 
Acked-by: Bjorn Helgaas 
Cc: linux-...@vger.kernel.org
Cc: sta...@vger.kernel.org
Link: 
http://lkml.kernel.org/r/1468426713-31431-1-git-send-email-marc.zyng...@arm.com
Signed-off-by: Thomas Gleixner 

---
 drivers/pci/msi.c   |  2 ++
 include/linux/msi.h |  2 ++
 kernel/irq/msi.c| 11 +++
 3 files changed, 15 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a02981e..eafa613 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1411,6 +1411,8 @@ struct irq_domain *pci_msi_create_irq_domain(struct 
fwnode_handle *fwnode,
if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
pci_msi_domain_update_chip_ops(info);
 
+   info->flags |= MSI_FLAG_ACTIVATE_EARLY;
+
domain = msi_create_irq_domain(fwnode, info, parent);
if (!domain)
return NULL;
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 4f0bfe5..e8c81fb 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -270,6 +270,8 @@ enum {
MSI_FLAG_MULTI_PCI_MSI  = (1 << 2),
/* Support PCI MSIX interrupts */
MSI_FLAG_PCI_MSIX   = (1 << 3),
+   /* Needs early activate, required for PCI */
+   MSI_FLAG_ACTIVATE_EARLY = (1 << 4),
 };
 
 int msi_domain_set_affinity(struct irq_data *data, const struct cpumask *mask,
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 5499935..19e9dfb 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -359,6 +359,17 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, 
struct device *dev,
else
dev_dbg(dev, "irq [%d-%d] for MSI\n",
virq, virq + desc->nvec_used - 1);
+   /*
+* This flag is set by the PCI layer as we need to activate
+* the MSI entries before the PCI layer enables MSI in the
+* card. Otherwise the card latches a random msi message.
+*/
+   if (info->flags & MSI_FLAG_ACTIVATE_EARLY) {
+   struct irq_data *irq_data;
+
+   irq_data = irq_domain_get_irq_data(domain, desc->irq);
+   irq_domain_activate_irq(irq_data);
+   }
}
 
return 0;

[tip:irq/urgent] genirq/msi: Make sure PCI MSIs are activated early

2016-08-09 Thread tip-bot for Marc Zyngier

Commit-ID:  f3b0946d629c8bfbd3e5f038e30cb9c711a35f10
Gitweb: http://git.kernel.org/tip/f3b0946d629c8bfbd3e5f038e30cb9c711a35f10
Author: Marc Zyngier 
AuthorDate: Wed, 13 Jul 2016 17:18:33 +0100
Committer:  Thomas Gleixner 
CommitDate: Tue, 9 Aug 2016 09:19:32 +0200

genirq/msi: Make sure PCI MSIs are activated early

Bharat Kumar Gogada reported issues with the generic MSI code, where the
end-point ended up with garbage in its MSI configuration (both for the vector
and the message).

It turns out that the two MSI paths in the kernel are doing slightly different
things:

generic MSI: disable MSI -> allocate MSI -> enable MSI -> setup EP
PCI MSI: disable MSI -> allocate MSI -> setup EP -> enable MSI

And it turns out that end-points are allowed to latch the content of the MSI
configuration registers as soon as MSIs are enabled.  In Bharat's case, the
end-point ends up using whatever was there already, which is not what you
want.

In order to make things converge, we introduce a new MSI domain flag
(MSI_FLAG_ACTIVATE_EARLY) that is unconditionally set for PCI/MSI. When set,
this flag forces the programming of the end-point as soon as the MSIs are
allocated.

A consequence of this is that we have an extra activate in irq_startup, but
that should be without much consequence.

tglx: 

 - Several people reported a VMWare regression with PCI/MSI-X passthrough. It
   turns out that the patch also cures that issue.

 - We need to have a look at the MSI disable interrupt path, where we write
   the msg to all zeros without disabling MSI in the PCI device. Is that
   correct?

Fixes: 52f518a3a7c2 "x86/MSI: Use hierarchical irqdomains to manage MSI 
interrupts"
Reported-and-tested-by: Bharat Kumar Gogada 
Reported-and-tested-by: Foster Snowhill 
Reported-by: Matthias Prager 
Reported-by: Jason Taylor 
Signed-off-by: Marc Zyngier 
Acked-by: Bjorn Helgaas 
Cc: linux-...@vger.kernel.org
Cc: sta...@vger.kernel.org
Link: 
http://lkml.kernel.org/r/1468426713-31431-1-git-send-email-marc.zyng...@arm.com
Signed-off-by: Thomas Gleixner 

---
 drivers/pci/msi.c   |  2 ++
 include/linux/msi.h |  2 ++
 kernel/irq/msi.c| 11 +++
 3 files changed, 15 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a02981e..eafa613 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1411,6 +1411,8 @@ struct irq_domain *pci_msi_create_irq_domain(struct 
fwnode_handle *fwnode,
if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
pci_msi_domain_update_chip_ops(info);
 
+   info->flags |= MSI_FLAG_ACTIVATE_EARLY;
+
domain = msi_create_irq_domain(fwnode, info, parent);
if (!domain)
return NULL;
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 4f0bfe5..e8c81fb 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -270,6 +270,8 @@ enum {
MSI_FLAG_MULTI_PCI_MSI  = (1 << 2),
/* Support PCI MSIX interrupts */
MSI_FLAG_PCI_MSIX   = (1 << 3),
+   /* Needs early activate, required for PCI */
+   MSI_FLAG_ACTIVATE_EARLY = (1 << 4),
 };
 
 int msi_domain_set_affinity(struct irq_data *data, const struct cpumask *mask,
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 5499935..19e9dfb 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -359,6 +359,17 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, 
struct device *dev,
else
dev_dbg(dev, "irq [%d-%d] for MSI\n",
virq, virq + desc->nvec_used - 1);
+   /*
+* This flag is set by the PCI layer as we need to activate
+* the MSI entries before the PCI layer enables MSI in the
+* card. Otherwise the card latches a random msi message.
+*/
+   if (info->flags & MSI_FLAG_ACTIVATE_EARLY) {
+   struct irq_data *irq_data;
+
+   irq_data = irq_domain_get_irq_data(domain, desc->irq);
+   irq_domain_activate_irq(irq_data);
+   }
}
 
return 0;

RE: [tip:irq/urgent] genirq/msi: Make sure PCI MSIs are activated early

RE: [tip:irq/urgent] genirq/msi: Make sure PCI MSIs are activated early

[tip:irq/urgent] genirq/msi: Make sure PCI MSIs are activated early

[tip:irq/urgent] genirq/msi: Make sure PCI MSIs are activated early

4 matches

Site Navigation

Mail list logo

Footer information