[PATCH 0/3] xen MSI code clean up

[PATCH 0/3] xen MSI code clean up

[Yijing Wang]

Yijing Wang (3):
  x86/xen: Introduce a global flag to fix the MSI mask bug
  x86/xen: Revert "PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()"
  s390/MSI: Use __msi_mask_irq() instead of default_msi_mask_irq()

 arch/s390/pci/pci.c             |    4 ++--
 arch/x86/include/asm/x86_init.h |    3 ---
 arch/x86/kernel/x86_init.c      |   10 ----------
 arch/x86/pci/xen.c              |   15 +++------------
 drivers/pci/msi.c               |   29 ++++++++++++-----------------
 include/linux/msi.h             |    5 +++--
 6 files changed, 20 insertions(+), 46 deletions(-)

[PATCH 1/3] x86/xen: Introduce a global flag to fix the MSI mask bug

[Yijing Wang]

Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
fixed MSI mask bug which may cause kernel crash. But the commit
made MSI code complex. Introduce a new global flag "pci_msi_ignore_mask"
to ignore MSI/MSI-X to fix this issue, it's a cleaner solution.
And the commit 0e4ccb1505a9 will be reverted in the later patch.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
CC: David Vrabel <david.vrabel@citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: xen-devel@lists.xenproject.org
---
 arch/x86/pci/xen.c  |    2 ++
 drivers/pci/msi.c   |    7 ++++++-
 include/linux/msi.h |    1 +
 3 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 093f5f4..5ef62ed 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -427,6 +427,7 @@ int __init pci_xen_init(void)
 	x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
 	x86_msi.msi_mask_irq = xen_nop_msi_mask_irq;
 	x86_msi.msix_mask_irq = xen_nop_msix_mask_irq;
+	pci_msi_ignore_mask = 1;
 #endif
 	return 0;
 }
@@ -508,6 +509,7 @@ int __init pci_xen_initial_domain(void)
 	x86_msi.restore_msi_irqs = xen_initdom_restore_msi_irqs;
 	x86_msi.msi_mask_irq = xen_nop_msi_mask_irq;
 	x86_msi.msix_mask_irq = xen_nop_msix_mask_irq;
+	pci_msi_ignore_mask = 1;
 #endif
 	xen_setup_acpi_sci();
 	__acpi_register_gsi = acpi_register_gsi_xen;
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 38511d9..ecb5f54 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -23,6 +23,7 @@
 #include "pci.h"
 
 static int pci_msi_enable = 1;
+int pci_msi_ignore_mask;
 
 #define msix_table_size(flags)	((flags & PCI_MSIX_FLAGS_QSIZE) + 1)
 
@@ -166,7 +167,7 @@ u32 default_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
 {
 	u32 mask_bits = desc->masked;
 
-	if (!desc->msi_attrib.maskbit)
+	if (pci_msi_ignore_mask || !desc->msi_attrib.maskbit)
 		return 0;
 
 	mask_bits &= ~mask;
@@ -198,6 +199,10 @@ u32 default_msix_mask_irq(struct msi_desc *desc, u32 flag)
 	u32 mask_bits = desc->masked;
 	unsigned offset = desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
 						PCI_MSIX_ENTRY_VECTOR_CTRL;
+
+	if (pci_msi_ignore_mask)
+		return 0;
+
 	mask_bits &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
 	if (flag)
 		mask_bits |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 44f4746..86dc501 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -10,6 +10,7 @@ struct msi_msg {
 	u32	data;		/* 16 bits of msi message data */
 };
 
+extern int pci_msi_ignore_mask;
 /* Helper functions */
 struct irq_data;
 struct msi_desc;
-- 
1.7.1

[PATCH 2/3] x86/xen: Revert "PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()"

[Yijing Wang]

Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
introduced two __weak arch functions arch_msix_mask_irq() and
arch_msi_mask_irq() to work around a bug when running xen in x86.
These two functions made msi code more complex. We have introduced
a global flag to ignore MSI mask to fix the issue in previous patch,
so we can revert the commit safely. This patch is also preparation
to use MSI chip framework instead of weak arch MSI functions in all
platforms. Because s390 MSI also use default_msi_mask_irq() functions
introduced by commit 0e4ccb1505a9. We keep default_msi_mask_irq()
and default_msix_mask_irq() temporarily. They will be removed in later
patch.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
CC: David Vrabel <david.vrabel@citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: xen-devel@lists.xenproject.org
---
 arch/x86/include/asm/x86_init.h |    3 ---
 arch/x86/kernel/x86_init.c      |   10 ----------
 arch/x86/pci/xen.c              |   13 +------------
 drivers/pci/msi.c               |   22 ++++++----------------
 include/linux/msi.h             |    6 ++++--
 5 files changed, 11 insertions(+), 43 deletions(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index e45e4da..f58a9c7 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -172,7 +172,6 @@ struct x86_platform_ops {
 
 struct pci_dev;
 struct msi_msg;
-struct msi_desc;
 
 struct x86_msi_ops {
 	int (*setup_msi_irqs)(struct pci_dev *dev, int nvec, int type);
@@ -183,8 +182,6 @@ struct x86_msi_ops {
 	void (*teardown_msi_irqs)(struct pci_dev *dev);
 	void (*restore_msi_irqs)(struct pci_dev *dev);
 	int  (*setup_hpet_msi)(unsigned int irq, unsigned int id);
-	u32 (*msi_mask_irq)(struct msi_desc *desc, u32 mask, u32 flag);
-	u32 (*msix_mask_irq)(struct msi_desc *desc, u32 flag);
 };
 
 struct IO_APIC_route_entry;
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index e48b674..234b072 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -116,8 +116,6 @@ struct x86_msi_ops x86_msi = {
 	.teardown_msi_irqs	= default_teardown_msi_irqs,
 	.restore_msi_irqs	= default_restore_msi_irqs,
 	.setup_hpet_msi		= default_setup_hpet_msi,
-	.msi_mask_irq		= default_msi_mask_irq,
-	.msix_mask_irq		= default_msix_mask_irq,
 };
 
 /* MSI arch specific hooks */
@@ -140,14 +138,6 @@ void arch_restore_msi_irqs(struct pci_dev *dev)
 {
 	x86_msi.restore_msi_irqs(dev);
 }
-u32 arch_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
-{
-	return x86_msi.msi_mask_irq(desc, mask, flag);
-}
-u32 arch_msix_mask_irq(struct msi_desc *desc, u32 flag)
-{
-	return x86_msi.msix_mask_irq(desc, flag);
-}
 #endif
 
 struct x86_io_apic_ops x86_io_apic_ops = {
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 5ef62ed..466b978 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -394,14 +394,7 @@ static void xen_teardown_msi_irq(unsigned int irq)
 {
 	xen_destroy_irq(irq);
 }
-static u32 xen_nop_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
-{
-	return 0;
-}
-static u32 xen_nop_msix_mask_irq(struct msi_desc *desc, u32 flag)
-{
-	return 0;
-}
+
 #endif
 
 int __init pci_xen_init(void)
@@ -425,8 +418,6 @@ int __init pci_xen_init(void)
 	x86_msi.setup_msi_irqs = xen_setup_msi_irqs;
 	x86_msi.teardown_msi_irq = xen_teardown_msi_irq;
 	x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
-	x86_msi.msi_mask_irq = xen_nop_msi_mask_irq;
-	x86_msi.msix_mask_irq = xen_nop_msix_mask_irq;
 	pci_msi_ignore_mask = 1;
 #endif
 	return 0;
@@ -507,8 +498,6 @@ int __init pci_xen_initial_domain(void)
 	x86_msi.setup_msi_irqs = xen_initdom_setup_msi_irqs;
 	x86_msi.teardown_msi_irq = xen_teardown_msi_irq;
 	x86_msi.restore_msi_irqs = xen_initdom_restore_msi_irqs;
-	x86_msi.msi_mask_irq = xen_nop_msi_mask_irq;
-	x86_msi.msix_mask_irq = xen_nop_msix_mask_irq;
 	pci_msi_ignore_mask = 1;
 #endif
 	xen_setup_acpi_sci();
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index ecb5f54..805c7d8 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -163,7 +163,7 @@ static inline __attribute_const__ u32 msi_mask(unsigned x)
  * reliably as devices without an INTx disable bit will then generate a
  * level IRQ which will never be cleared.
  */
-u32 default_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
+u32 __msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
 {
 	u32 mask_bits = desc->masked;
 
@@ -177,14 +177,9 @@ u32 default_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
 	return mask_bits;
 }
 
-__weak u32 arch_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
-{
-	return default_msi_mask_irq(desc, mask, flag);
-}
-
 static void msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
 {
-	desc->masked = arch_msi_mask_irq(desc, mask, flag);
+	desc->masked = __msi_mask_irq(desc, mask, flag);
 }
 
 /*
@@ -194,7 +189,7 @@ static void msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
  * file.  This saves a few milliseconds when initialising devices with lots
  * of MSI-X interrupts.
  */
-u32 default_msix_mask_irq(struct msi_desc *desc, u32 flag)
+u32 __msix_mask_irq(struct msi_desc *desc, u32 flag)
 {
 	u32 mask_bits = desc->masked;
 	unsigned offset = desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
@@ -211,14 +206,9 @@ u32 default_msix_mask_irq(struct msi_desc *desc, u32 flag)
 	return mask_bits;
 }
 
-__weak u32 arch_msix_mask_irq(struct msi_desc *desc, u32 flag)
-{
-	return default_msix_mask_irq(desc, flag);
-}
-
 static void msix_mask_irq(struct msi_desc *desc, u32 flag)
 {
-	desc->masked = arch_msix_mask_irq(desc, flag);
+	desc->masked = __msix_mask_irq(desc, flag);
 }
 
 static void msi_set_mask_bit(struct irq_data *data, u32 flag)
@@ -873,7 +863,7 @@ void pci_msi_shutdown(struct pci_dev *dev)
 	/* Return the device with MSI unmasked as initial states */
 	mask = msi_mask(desc->msi_attrib.multi_cap);
 	/* Keep cached state to be restored */
-	arch_msi_mask_irq(desc, mask, ~mask);
+	__msi_mask_irq(desc, mask, ~mask);
 
 	/* Restore dev->irq to its default pin-assertion irq */
 	dev->irq = desc->msi_attrib.default_irq;
@@ -971,7 +961,7 @@ void pci_msix_shutdown(struct pci_dev *dev)
 	/* Return the device with MSI-X masked as initial states */
 	list_for_each_entry(entry, &dev->msi_list, list) {
 		/* Keep cached states to be restored */
-		arch_msix_mask_irq(entry, 1);
+		__msix_mask_irq(entry, 1);
 	}
 
 	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 86dc501..f6630a5 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -22,6 +22,8 @@ void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
 void read_msi_msg(unsigned int irq, struct msi_msg *msg);
 void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg);
 void write_msi_msg(unsigned int irq, struct msi_msg *msg);
+u32 __msix_mask_irq(struct msi_desc *desc, u32 flag);
+u32 __msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag);
 
 struct msi_desc {
 	struct {
@@ -62,8 +64,8 @@ void arch_restore_msi_irqs(struct pci_dev *dev);
 
 void default_teardown_msi_irqs(struct pci_dev *dev);
 void default_restore_msi_irqs(struct pci_dev *dev);
-u32 default_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag);
-u32 default_msix_mask_irq(struct msi_desc *desc, u32 flag);
+#define default_msi_mask_irq  __msi_mask_irq
+#define default_msix_mask_irq  __msix_mask_irq
 
 struct msi_chip {
 	struct module *owner;
-- 
1.7.1

[PATCH 3/3] s390/MSI: Use __msi_mask_irq() instead of default_msi_mask_irq()

[Yijing Wang]

Now only s390/MSI use default_msi_mask_irq() and
default_msix_mask_irq(), replace them with the common
msi mask irq functions __msi_mask_irq() and __msix_mask_irq().
Remove default_msi_mask_irq() and default_msix_mask_irq().

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
CC: Sebastian Ott <sebott@linux.vnet.ibm.com>
CC: linux-s390@vger.kernel.org
---
 arch/s390/pci/pci.c |    4 ++--
 include/linux/msi.h |    2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 2fa7b14..552b990 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -448,9 +448,9 @@ void arch_teardown_msi_irqs(struct pci_dev *pdev)
 	/* Release MSI interrupts */
 	list_for_each_entry(msi, &pdev->msi_list, list) {
 		if (msi->msi_attrib.is_msix)
-			default_msix_mask_irq(msi, 1);
+			__msix_mask_irq(msi, 1);
 		else
-			default_msi_mask_irq(msi, 1, 1);
+			__msi_mask_irq(msi, 1, 1);
 		irq_set_msi_desc(msi->irq, NULL);
 		irq_free_desc(msi->irq);
 		msi->msg.address_lo = 0;
diff --git a/include/linux/msi.h b/include/linux/msi.h
index f6630a5..efad127 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -64,8 +64,6 @@ void arch_restore_msi_irqs(struct pci_dev *dev);
 
 void default_teardown_msi_irqs(struct pci_dev *dev);
 void default_restore_msi_irqs(struct pci_dev *dev);
-#define default_msi_mask_irq  __msi_mask_irq
-#define default_msix_mask_irq  __msix_mask_irq
 
 struct msi_chip {
 	struct module *owner;
-- 
1.7.1

Re: [Xen-devel] [PATCH 1/3] x86/xen: Introduce a global flag to fix the MSI mask bug

[David Vrabel]

On 27/10/14 02:44, Yijing Wang wrote:
> Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
> fixed MSI mask bug which may cause kernel crash. But the commit
> made MSI code complex. Introduce a new global flag "pci_msi_ignore_mask"
> to ignore MSI/MSI-X to fix this issue, it's a cleaner solution.
> And the commit 0e4ccb1505a9 will be reverted in the later patch.

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

In the sense that it keeps the odd Xen behaviour.  But...

Konrad, why was this fixed like this in the first place?  IMO, it would
have been better to get Xen to trap-and-emulate accesses to the relevant
MSI/MSI-X registers.  The mask/unmask on setup/teardown isn't
performance critical.

David

Re: [PATCH 2/3] x86/xen: Revert "PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()"

[David Vrabel]

On 27/10/14 02:44, Yijing Wang wrote:
> Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
> introduced two __weak arch functions arch_msix_mask_irq() and
> arch_msi_mask_irq() to work around a bug when running xen in x86.
> These two functions made msi code more complex. We have introduced
> a global flag to ignore MSI mask to fix the issue in previous patch,
> so we can revert the commit safely. This patch is also preparation
> to use MSI chip framework instead of weak arch MSI functions in all
> platforms. Because s390 MSI also use default_msi_mask_irq() functions
> introduced by commit 0e4ccb1505a9. We keep default_msi_mask_irq()
> and default_msix_mask_irq() temporarily. They will be removed in later
> patch.

Acked-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [Xen-devel] [PATCH 1/3] x86/xen: Introduce a global flag to fix the MSI mask bug

[Konrad Rzeszutek Wilk]

On Mon, Oct 27, 2014 at 11:09:42AM +0000, David Vrabel wrote:
> On 27/10/14 02:44, Yijing Wang wrote:
> > Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
> > fixed MSI mask bug which may cause kernel crash. But the commit
> > made MSI code complex. Introduce a new global flag "pci_msi_ignore_mask"
> > to ignore MSI/MSI-X to fix this issue, it's a cleaner solution.
> > And the commit 0e4ccb1505a9 will be reverted in the later patch.
> 
> Reviewed-by: David Vrabel <david.vrabel@citrix.com>
> 
> In the sense that it keeps the odd Xen behaviour.  But...
> 
> Konrad, why was this fixed like this in the first place?  IMO, it would

As 0e4ccb1505a9 explains:
    PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()
    
    Certain platforms do not allow writes in the MSI-X BARs to setup or tear
    down vector values.  To combat against the generic code trying to write to
    that and either silently being ignored or crashing due to the pagetables
    being marked R/O this patch introduces a platform override.
    
    Note that we keep two separate, non-weak, functions default_mask_msi_irqs()
    and default_mask_msix_irqs() for the behavior of the arch_mask_msi_irqs()
    and arch_mask_msix_irqs(), as the default behavior is needed by x86 PCI
    code.
    
    For Xen, which does not allow the guest to write to MSI-X tables - as the
    hypervisor is solely responsible for setting the vector values - we
    implement two nops.
    
    This fixes a Xen guest crash when passing a PCI device with MSI-X to the
    guest.  See the bugzilla for more details.
    
    [bhelgaas: add bugzilla info]
    Reference: https://bugzilla.kernel.org/show_bug.cgi?id=64581

> have been better to get Xen to trap-and-emulate accesses to the relevant
> MSI/MSI-X registers.  The mask/unmask on setup/teardown isn't
> performance critical.
> 
> David
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/3] x86/xen: Introduce a global flag to fix the MSI mask bug

[David Vrabel]

On 27/10/14 19:45, Konrad Rzeszutek Wilk wrote:
> On Mon, Oct 27, 2014 at 11:09:42AM +0000, David Vrabel wrote:
>> On 27/10/14 02:44, Yijing Wang wrote:
>>> Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
>>> fixed MSI mask bug which may cause kernel crash. But the commit
>>> made MSI code complex. Introduce a new global flag "pci_msi_ignore_mask"
>>> to ignore MSI/MSI-X to fix this issue, it's a cleaner solution.
>>> And the commit 0e4ccb1505a9 will be reverted in the later patch.
>>
>> Reviewed-by: David Vrabel <david.vrabel@citrix.com>
>>
>> In the sense that it keeps the odd Xen behaviour.  But...
>>
>> Konrad, why was this fixed like this in the first place?  IMO, it would
> 
> As 0e4ccb1505a9 explains:
>     PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()
>     
>     Certain platforms do not allow writes in the MSI-X BARs to setup or tear
>     down vector values.  To combat against the generic code trying to write to
>     that and either silently being ignored or crashing due to the pagetables
>     being marked R/O this patch introduces a platform override.
>     
>     Note that we keep two separate, non-weak, functions default_mask_msi_irqs()
>     and default_mask_msix_irqs() for the behavior of the arch_mask_msi_irqs()
>     and arch_mask_msix_irqs(), as the default behavior is needed by x86 PCI
>     code.
>     
>     For Xen, which does not allow the guest to write to MSI-X tables - as the
>     hypervisor is solely responsible for setting the vector values - we
>     implement two nops.

My question specifically was: could this be fixed by allowing writes to
set/clear the mask bit?  If so, why was this not done and could we do
this now?

David

Re: [Xen-devel] [PATCH 1/3] x86/xen: Introduce a global flag to fix the MSI mask bug

[Konrad Rzeszutek Wilk]

On Tue, Oct 28, 2014 at 04:44:44PM +0000, David Vrabel wrote:
> On 27/10/14 19:45, Konrad Rzeszutek Wilk wrote:
> > On Mon, Oct 27, 2014 at 11:09:42AM +0000, David Vrabel wrote:
> >> On 27/10/14 02:44, Yijing Wang wrote:
> >>> Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
> >>> fixed MSI mask bug which may cause kernel crash. But the commit
> >>> made MSI code complex. Introduce a new global flag "pci_msi_ignore_mask"
> >>> to ignore MSI/MSI-X to fix this issue, it's a cleaner solution.
> >>> And the commit 0e4ccb1505a9 will be reverted in the later patch.
> >>
> >> Reviewed-by: David Vrabel <david.vrabel@citrix.com>
> >>
> >> In the sense that it keeps the odd Xen behaviour.  But...
> >>
> >> Konrad, why was this fixed like this in the first place?  IMO, it would
> > 
> > As 0e4ccb1505a9 explains:
> >     PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()
> >     
> >     Certain platforms do not allow writes in the MSI-X BARs to setup or tear
> >     down vector values.  To combat against the generic code trying to write to
> >     that and either silently being ignored or crashing due to the pagetables
> >     being marked R/O this patch introduces a platform override.
> >     
> >     Note that we keep two separate, non-weak, functions default_mask_msi_irqs()
> >     and default_mask_msix_irqs() for the behavior of the arch_mask_msi_irqs()
> >     and arch_mask_msix_irqs(), as the default behavior is needed by x86 PCI
> >     code.
> >     
> >     For Xen, which does not allow the guest to write to MSI-X tables - as the
> >     hypervisor is solely responsible for setting the vector values - we
> >     implement two nops.
> 
> My question specifically was: could this be fixed by allowing writes to
> set/clear the mask bit?  If so, why was this not done and could we do

No.
> this now?
> 
> David

Re: [PATCH 1/3] x86/xen: Introduce a global flag to fix the MSI mask bug

[Bjorn Helgaas]

On Mon, Oct 27, 2014 at 10:44:36AM +0800, Yijing Wang wrote:
> Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
> fixed MSI mask bug which may cause kernel crash. But the commit
> made MSI code complex. Introduce a new global flag "pci_msi_ignore_mask"
> to ignore MSI/MSI-X to fix this issue, it's a cleaner solution.
> And the commit 0e4ccb1505a9 will be reverted in the later patch.

The 0e4ccb1505a9 changelog says Xen guests can't write to MSI-X BARs.
But it makes mask_irq a no-op for both MSI-X and MSI.  The MSI mask bit is
in config space, not in memory space.  So why does mask_irq need to be a
no-op for MSI as well?  Are Xen guests prohibited from writing to config
space, too?  (It's fine if they are; it's just that the changelog
specifically mentioned MSI-X memory space tables, and it didn't mention
config space at all.)

And I *assume* there's some Xen mechanism that accomplishes the mask_irq in
a different way, since the actual mask_irq interface does nothing?  (This
is really a question for 0e4ccb1505a9, since I don't think this particular
patch changes anything in that respect.)

> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> CC: David Vrabel <david.vrabel@citrix.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: xen-devel@lists.xenproject.org
> ---
>  arch/x86/pci/xen.c  |    2 ++
>  drivers/pci/msi.c   |    7 ++++++-
>  include/linux/msi.h |    1 +
>  3 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 093f5f4..5ef62ed 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -427,6 +427,7 @@ int __init pci_xen_init(void)
>  	x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
>  	x86_msi.msi_mask_irq = xen_nop_msi_mask_irq;
>  	x86_msi.msix_mask_irq = xen_nop_msix_mask_irq;
> +	pci_msi_ignore_mask = 1;
>  #endif
>  	return 0;
>  }
> @@ -508,6 +509,7 @@ int __init pci_xen_initial_domain(void)
>  	x86_msi.restore_msi_irqs = xen_initdom_restore_msi_irqs;
>  	x86_msi.msi_mask_irq = xen_nop_msi_mask_irq;
>  	x86_msi.msix_mask_irq = xen_nop_msix_mask_irq;
> +	pci_msi_ignore_mask = 1;
>  #endif
>  	xen_setup_acpi_sci();
>  	__acpi_register_gsi = acpi_register_gsi_xen;
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 38511d9..ecb5f54 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -23,6 +23,7 @@
>  #include "pci.h"
>  
>  static int pci_msi_enable = 1;
> +int pci_msi_ignore_mask;
>  
>  #define msix_table_size(flags)	((flags & PCI_MSIX_FLAGS_QSIZE) + 1)
>  
> @@ -166,7 +167,7 @@ u32 default_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
>  {
>  	u32 mask_bits = desc->masked;
>  
> -	if (!desc->msi_attrib.maskbit)
> +	if (pci_msi_ignore_mask || !desc->msi_attrib.maskbit)
>  		return 0;
>  
>  	mask_bits &= ~mask;
> @@ -198,6 +199,10 @@ u32 default_msix_mask_irq(struct msi_desc *desc, u32 flag)
>  	u32 mask_bits = desc->masked;
>  	unsigned offset = desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
>  						PCI_MSIX_ENTRY_VECTOR_CTRL;
> +
> +	if (pci_msi_ignore_mask)
> +		return 0;
> +
>  	mask_bits &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
>  	if (flag)
>  		mask_bits |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 44f4746..86dc501 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -10,6 +10,7 @@ struct msi_msg {
>  	u32	data;		/* 16 bits of msi message data */
>  };
>  
> +extern int pci_msi_ignore_mask;
>  /* Helper functions */
>  struct irq_data;
>  struct msi_desc;
> -- 
> 1.7.1
> 

Re: [PATCH 1/3] x86/xen: Introduce a global flag to fix the MSI mask bug

[Yijing Wang]

On 2014/11/11 8:04, Bjorn Helgaas wrote:
> On Mon, Oct 27, 2014 at 10:44:36AM +0800, Yijing Wang wrote:
>> Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
>> fixed MSI mask bug which may cause kernel crash. But the commit
>> made MSI code complex. Introduce a new global flag "pci_msi_ignore_mask"
>> to ignore MSI/MSI-X to fix this issue, it's a cleaner solution.
>> And the commit 0e4ccb1505a9 will be reverted in the later patch.
> 
> The 0e4ccb1505a9 changelog says Xen guests can't write to MSI-X BARs.
> But it makes mask_irq a no-op for both MSI-X and MSI.  The MSI mask bit is
> in config space, not in memory space.  So why does mask_irq need to be a
> no-op for MSI as well?  Are Xen guests prohibited from writing to config
> space, too?  (It's fine if they are; it's just that the changelog
> specifically mentioned MSI-X memory space tables, and it didn't mention
> config space at all.)
> 
> And I *assume* there's some Xen mechanism that accomplishes the mask_irq in
> a different way, since the actual mask_irq interface does nothing?  (This
> is really a question for 0e4ccb1505a9, since I don't think this particular
> patch changes anything in that respect.)

Yes, it's another history problem, maybe Konrad know the detail.

> 
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> CC: David Vrabel <david.vrabel@citrix.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: xen-devel@lists.xenproject.org
>> ---
>>  arch/x86/pci/xen.c  |    2 ++
>>  drivers/pci/msi.c   |    7 ++++++-
>>  include/linux/msi.h |    1 +
>>  3 files changed, 9 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
>> index 093f5f4..5ef62ed 100644
>> --- a/arch/x86/pci/xen.c
>> +++ b/arch/x86/pci/xen.c
>> @@ -427,6 +427,7 @@ int __init pci_xen_init(void)
>>  	x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
>>  	x86_msi.msi_mask_irq = xen_nop_msi_mask_irq;
>>  	x86_msi.msix_mask_irq = xen_nop_msix_mask_irq;
>> +	pci_msi_ignore_mask = 1;
>>  #endif
>>  	return 0;
>>  }
>> @@ -508,6 +509,7 @@ int __init pci_xen_initial_domain(void)
>>  	x86_msi.restore_msi_irqs = xen_initdom_restore_msi_irqs;
>>  	x86_msi.msi_mask_irq = xen_nop_msi_mask_irq;
>>  	x86_msi.msix_mask_irq = xen_nop_msix_mask_irq;
>> +	pci_msi_ignore_mask = 1;
>>  #endif
>>  	xen_setup_acpi_sci();
>>  	__acpi_register_gsi = acpi_register_gsi_xen;
>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index 38511d9..ecb5f54 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
>> @@ -23,6 +23,7 @@
>>  #include "pci.h"
>>  
>>  static int pci_msi_enable = 1;
>> +int pci_msi_ignore_mask;
>>  
>>  #define msix_table_size(flags)	((flags & PCI_MSIX_FLAGS_QSIZE) + 1)
>>  
>> @@ -166,7 +167,7 @@ u32 default_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
>>  {
>>  	u32 mask_bits = desc->masked;
>>  
>> -	if (!desc->msi_attrib.maskbit)
>> +	if (pci_msi_ignore_mask || !desc->msi_attrib.maskbit)
>>  		return 0;
>>  
>>  	mask_bits &= ~mask;
>> @@ -198,6 +199,10 @@ u32 default_msix_mask_irq(struct msi_desc *desc, u32 flag)
>>  	u32 mask_bits = desc->masked;
>>  	unsigned offset = desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
>>  						PCI_MSIX_ENTRY_VECTOR_CTRL;
>> +
>> +	if (pci_msi_ignore_mask)
>> +		return 0;
>> +
>>  	mask_bits &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
>>  	if (flag)
>>  		mask_bits |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
>> diff --git a/include/linux/msi.h b/include/linux/msi.h
>> index 44f4746..86dc501 100644
>> --- a/include/linux/msi.h
>> +++ b/include/linux/msi.h
>> @@ -10,6 +10,7 @@ struct msi_msg {
>>  	u32	data;		/* 16 bits of msi message data */
>>  };
>>  
>> +extern int pci_msi_ignore_mask;
>>  /* Helper functions */
>>  struct irq_data;
>>  struct msi_desc;
>> -- 
>> 1.7.1
>>
> 
> .
> 


-- 
Thanks!
Yijing

Re: [PATCH 1/3] x86/xen: Introduce a global flag to fix the MSI mask bug

[Konrad Rzeszutek Wilk]

On Mon, Nov 10, 2014 at 05:04:56PM -0700, Bjorn Helgaas wrote:
> On Mon, Oct 27, 2014 at 10:44:36AM +0800, Yijing Wang wrote:
> > Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
> > fixed MSI mask bug which may cause kernel crash. But the commit
> > made MSI code complex. Introduce a new global flag "pci_msi_ignore_mask"
> > to ignore MSI/MSI-X to fix this issue, it's a cleaner solution.
> > And the commit 0e4ccb1505a9 will be reverted in the later patch.
> 
> The 0e4ccb1505a9 changelog says Xen guests can't write to MSI-X BARs.
> But it makes mask_irq a no-op for both MSI-X and MSI.  The MSI mask bit is
> in config space, not in memory space.  So why does mask_irq need to be a
> no-op for MSI as well?  Are Xen guests prohibited from writing to config

The PV guests it refers do not do write to config space. They have
an PCI frontend and backend driver which communicates to the backend (the
trusted domain) to setup the MSI and MSI-X. The 'pci_xen_init' (arch/x86/pci/xen.c)
is the one that sets up the overrides. When an MSI or MSI-X is requested
it is done via the request_irq which ends up calling 'xen_setup_msi_irqs'.
If you look there you can see:

173         if (type == PCI_CAP_ID_MSIX)                                            
174                 ret = xen_pci_frontend_enable_msix(dev, v, nvec);               
175         else                                                                    
176                 ret = xen_pci_frontend_enable_msi(dev, v);                      
177         if (ret)                                                                
178                 goto error;                                                     

Which are the calls to the Xen PCI driver to communicate with the
backend to setup the MSI.

> space, too?  (It's fine if they are; it's just that the changelog
> specifically mentioned MSI-X memory space tables, and it didn't mention
> config space at all.)

Correct. The config space is accessible to the guest but if it writes
to it - all of those values are ignored by the hypervisor - and that
is because the backend is suppose to communicate to the hypervisor
whether the guest can indeed setup MSI or MSI-X.

> 
> And I *assume* there's some Xen mechanism that accomplishes the mask_irq in
> a different way, since the actual mask_irq interface does nothing?  (This
> is really a question for 0e4ccb1505a9, since I don't think this particular
> patch changes anything in that respect.)

Correct. 'request_irq' ends up doing that. Or rather it ends up
calling xen_setup_msi_irqs which takes care of that.

The Xen PV guests (not to be confused with Xen HVM guests) run without
any emulated devices. That means most of the x86 platform things - ioports,
VGA, etc - are removed. Instead that functionality is provided via 
frontend drivers that communicate to the backend via a ring.

Hopefully this clarifies it?
> 
> > Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> > CC: David Vrabel <david.vrabel@citrix.com>
> > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > CC: xen-devel@lists.xenproject.org
> > ---
> >  arch/x86/pci/xen.c  |    2 ++
> >  drivers/pci/msi.c   |    7 ++++++-
> >  include/linux/msi.h |    1 +
> >  3 files changed, 9 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> > index 093f5f4..5ef62ed 100644
> > --- a/arch/x86/pci/xen.c
> > +++ b/arch/x86/pci/xen.c
> > @@ -427,6 +427,7 @@ int __init pci_xen_init(void)
> >  	x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
> >  	x86_msi.msi_mask_irq = xen_nop_msi_mask_irq;
> >  	x86_msi.msix_mask_irq = xen_nop_msix_mask_irq;
> > +	pci_msi_ignore_mask = 1;
> >  #endif
> >  	return 0;
> >  }
> > @@ -508,6 +509,7 @@ int __init pci_xen_initial_domain(void)
> >  	x86_msi.restore_msi_irqs = xen_initdom_restore_msi_irqs;
> >  	x86_msi.msi_mask_irq = xen_nop_msi_mask_irq;
> >  	x86_msi.msix_mask_irq = xen_nop_msix_mask_irq;
> > +	pci_msi_ignore_mask = 1;
> >  #endif
> >  	xen_setup_acpi_sci();
> >  	__acpi_register_gsi = acpi_register_gsi_xen;
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 38511d9..ecb5f54 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -23,6 +23,7 @@
> >  #include "pci.h"
> >  
> >  static int pci_msi_enable = 1;
> > +int pci_msi_ignore_mask;
> >  
> >  #define msix_table_size(flags)	((flags & PCI_MSIX_FLAGS_QSIZE) + 1)
> >  
> > @@ -166,7 +167,7 @@ u32 default_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
> >  {
> >  	u32 mask_bits = desc->masked;
> >  
> > -	if (!desc->msi_attrib.maskbit)
> > +	if (pci_msi_ignore_mask || !desc->msi_attrib.maskbit)
> >  		return 0;
> >  
> >  	mask_bits &= ~mask;
> > @@ -198,6 +199,10 @@ u32 default_msix_mask_irq(struct msi_desc *desc, u32 flag)
> >  	u32 mask_bits = desc->masked;
> >  	unsigned offset = desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
> >  						PCI_MSIX_ENTRY_VECTOR_CTRL;
> > +
> > +	if (pci_msi_ignore_mask)
> > +		return 0;
> > +
> >  	mask_bits &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
> >  	if (flag)
> >  		mask_bits |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > diff --git a/include/linux/msi.h b/include/linux/msi.h
> > index 44f4746..86dc501 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -10,6 +10,7 @@ struct msi_msg {
> >  	u32	data;		/* 16 bits of msi message data */
> >  };
> >  
> > +extern int pci_msi_ignore_mask;
> >  /* Helper functions */
> >  struct irq_data;
> >  struct msi_desc;
> > -- 
> > 1.7.1
> > 

Re: [PATCH 1/3] x86/xen: Introduce a global flag to fix the MSI mask bug

[Bjorn Helgaas]

On Tue, Nov 11, 2014 at 10:45:38AM -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Nov 10, 2014 at 05:04:56PM -0700, Bjorn Helgaas wrote:
> > On Mon, Oct 27, 2014 at 10:44:36AM +0800, Yijing Wang wrote:
> > > Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
> > > fixed MSI mask bug which may cause kernel crash. But the commit
> > > made MSI code complex. Introduce a new global flag "pci_msi_ignore_mask"
> > > to ignore MSI/MSI-X to fix this issue, it's a cleaner solution.
> > > And the commit 0e4ccb1505a9 will be reverted in the later patch.
> > 
> > The 0e4ccb1505a9 changelog says Xen guests can't write to MSI-X BARs.
> > But it makes mask_irq a no-op for both MSI-X and MSI.  The MSI mask bit is
> > in config space, not in memory space.  So why does mask_irq need to be a
> > no-op for MSI as well?  Are Xen guests prohibited from writing to config
> 
> The PV guests it refers do not do write to config space. They have
> an PCI frontend and backend driver which communicates to the backend (the
> trusted domain) to setup the MSI and MSI-X. The 'pci_xen_init' (arch/x86/pci/xen.c)
> is the one that sets up the overrides. When an MSI or MSI-X is requested
> it is done via the request_irq which ends up calling 'xen_setup_msi_irqs'.
> If you look there you can see:
> 
> 173         if (type == PCI_CAP_ID_MSIX)
> 174                 ret = xen_pci_frontend_enable_msix(dev, v, nvec);
> 175         else
> 176                 ret = xen_pci_frontend_enable_msi(dev, v);
> 177         if (ret)
> 178                 goto error;
> 
> Which are the calls to the Xen PCI driver to communicate with the
> backend to setup the MSI.
> 
> > space, too?  (It's fine if they are; it's just that the changelog
> > specifically mentioned MSI-X memory space tables, and it didn't mention
> > config space at all.)
> 
> Correct. The config space is accessible to the guest but if it writes
> to it - all of those values are ignored by the hypervisor - and that
> is because the backend is suppose to communicate to the hypervisor
> whether the guest can indeed setup MSI or MSI-X.
> 
> > 
> > And I *assume* there's some Xen mechanism that accomplishes the mask_irq in
> > a different way, since the actual mask_irq interface does nothing?  (This
> > is really a question for 0e4ccb1505a9, since I don't think this particular
> > patch changes anything in that respect.)
> 
> Correct. 'request_irq' ends up doing that. Or rather it ends up
> calling xen_setup_msi_irqs which takes care of that.
> 
> The Xen PV guests (not to be confused with Xen HVM guests) run without
> any emulated devices. That means most of the x86 platform things - ioports,
> VGA, etc - are removed. Instead that functionality is provided via 
> frontend drivers that communicate to the backend via a ring.
> 
> Hopefully this clarifies it?

I think so.  I propose the following changelog.  Let me know if it's still
inaccurate:

  PCI/MSI: Add pci_msi_ignore_mask to prevent MSI/MSI-X BAR writes

  MSI-X vector Mask Bits are in MSI-X Tables in PCI memory space.  Xen guests
  can't write to those tables.  MSI vector Mask Bits are in PCI configuration
  space.  Xen guests can write to config space, but those writes are ignored.

  Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and
  msix_mask_irq()") added a way to override default_mask_msi_irqs() and
  default_mask_msix_irqs() so they can be no-ops in Xen guests, but this is
  more complicated than necessary.

  Add "pci_msi_ignore_mask" in the core PCI MSI code.  If set,
  default_mask_msi_irqs() and default_mask_msix_irqs() return without doing
  anything.  This is less flexible, but much simpler.

I guess you mentioned PV and HVM guests, and it sounds like all this only
applies to HVM guests.

Bjorn

Re: [PATCH 0/3] xen MSI code clean up

[Bjorn Helgaas]

[+cc Sebastian, linux-s390, David, Konrad, xen-devel]

On Mon, Oct 27, 2014 at 10:44:35AM +0800, Yijing Wang wrote:
> 
> Yijing Wang (3):
>   x86/xen: Introduce a global flag to fix the MSI mask bug
>   x86/xen: Revert "PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()"
>   s390/MSI: Use __msi_mask_irq() instead of default_msi_mask_irq()

I applied these with David's reviewed-by and ack to pci/msi for v3.19.

I'd like to see an ack from Sebastian for the s390 part, but I guess that
one is pretty trivial.

>  arch/s390/pci/pci.c             |    4 ++--
>  arch/x86/include/asm/x86_init.h |    3 ---
>  arch/x86/kernel/x86_init.c      |   10 ----------
>  arch/x86/pci/xen.c              |   15 +++------------
>  drivers/pci/msi.c               |   29 ++++++++++++-----------------
>  include/linux/msi.h             |    5 +++--
>  6 files changed, 20 insertions(+), 46 deletions(-)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] x86/xen: Introduce a global flag to fix the MSI mask bug

[Konrad Rzeszutek Wilk]

On Tue, Nov 11, 2014 at 01:24:30PM -0700, Bjorn Helgaas wrote:
> On Tue, Nov 11, 2014 at 10:45:38AM -0500, Konrad Rzeszutek Wilk wrote:
> > On Mon, Nov 10, 2014 at 05:04:56PM -0700, Bjorn Helgaas wrote:
> > > On Mon, Oct 27, 2014 at 10:44:36AM +0800, Yijing Wang wrote:
> > > > Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
> > > > fixed MSI mask bug which may cause kernel crash. But the commit
> > > > made MSI code complex. Introduce a new global flag "pci_msi_ignore_mask"
> > > > to ignore MSI/MSI-X to fix this issue, it's a cleaner solution.
> > > > And the commit 0e4ccb1505a9 will be reverted in the later patch.
> > > 
> > > The 0e4ccb1505a9 changelog says Xen guests can't write to MSI-X BARs.
> > > But it makes mask_irq a no-op for both MSI-X and MSI.  The MSI mask bit is
> > > in config space, not in memory space.  So why does mask_irq need to be a
> > > no-op for MSI as well?  Are Xen guests prohibited from writing to config
> > 
> > The PV guests it refers do not do write to config space. They have
> > an PCI frontend and backend driver which communicates to the backend (the
> > trusted domain) to setup the MSI and MSI-X. The 'pci_xen_init' (arch/x86/pci/xen.c)
> > is the one that sets up the overrides. When an MSI or MSI-X is requested
> > it is done via the request_irq which ends up calling 'xen_setup_msi_irqs'.
> > If you look there you can see:
> > 
> > 173         if (type == PCI_CAP_ID_MSIX)
> > 174                 ret = xen_pci_frontend_enable_msix(dev, v, nvec);
> > 175         else
> > 176                 ret = xen_pci_frontend_enable_msi(dev, v);
> > 177         if (ret)
> > 178                 goto error;
> > 
> > Which are the calls to the Xen PCI driver to communicate with the
> > backend to setup the MSI.
> > 
> > > space, too?  (It's fine if they are; it's just that the changelog
> > > specifically mentioned MSI-X memory space tables, and it didn't mention
> > > config space at all.)
> > 
> > Correct. The config space is accessible to the guest but if it writes
> > to it - all of those values are ignored by the hypervisor - and that
> > is because the backend is suppose to communicate to the hypervisor
> > whether the guest can indeed setup MSI or MSI-X.
> > 
> > > 
> > > And I *assume* there's some Xen mechanism that accomplishes the mask_irq in
> > > a different way, since the actual mask_irq interface does nothing?  (This
> > > is really a question for 0e4ccb1505a9, since I don't think this particular
> > > patch changes anything in that respect.)
> > 
> > Correct. 'request_irq' ends up doing that. Or rather it ends up
> > calling xen_setup_msi_irqs which takes care of that.
> > 
> > The Xen PV guests (not to be confused with Xen HVM guests) run without
> > any emulated devices. That means most of the x86 platform things - ioports,
> > VGA, etc - are removed. Instead that functionality is provided via 
> > frontend drivers that communicate to the backend via a ring.
> > 
> > Hopefully this clarifies it?
> 
> I think so.  I propose the following changelog.  Let me know if it's still
> inaccurate:
> 
>   PCI/MSI: Add pci_msi_ignore_mask to prevent MSI/MSI-X BAR writes
> 
>   MSI-X vector Mask Bits are in MSI-X Tables in PCI memory space.  Xen guests

Xen PV
>   can't write to those tables.  MSI vector Mask Bits are in PCI configuration
>   space.  Xen guests can write to config space, but those writes are ignored.
> 
>   Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and
>   msix_mask_irq()") added a way to override default_mask_msi_irqs() and
>   default_mask_msix_irqs() so they can be no-ops in Xen guests, but this is

Xen PV guests
>   more complicated than necessary.
> 
>   Add "pci_msi_ignore_mask" in the core PCI MSI code.  If set,
>   default_mask_msi_irqs() and default_mask_msix_irqs() return without doing
>   anything.  This is less flexible, but much simpler.
> 
> I guess you mentioned PV and HVM guests, and it sounds like all this only
> applies to HVM guests.

No, PV guests :-)

HVM guests will do the normal x86 type machinery.
> 
> Bjorn

Re: [PATCH 0/3] xen MSI code clean up

[Sebastian Ott]

On Tue, 11 Nov 2014, Bjorn Helgaas wrote:
> [+cc Sebastian, linux-s390, David, Konrad, xen-devel]
> 
> On Mon, Oct 27, 2014 at 10:44:35AM +0800, Yijing Wang wrote:
> > 
> > Yijing Wang (3):
> >   x86/xen: Introduce a global flag to fix the MSI mask bug
> >   x86/xen: Revert "PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()"
> >   s390/MSI: Use __msi_mask_irq() instead of default_msi_mask_irq()
> 
> I applied these with David's reviewed-by and ack to pci/msi for v3.19.
> 
> I'd like to see an ack from Sebastian for the s390 part, but I guess that
> one is pretty trivial.

> >   s390/MSI: Use __msi_mask_irq() instead of default_msi_mask_irq()

Acked-by: Sebastian Ott <sebott@linux.vnet.ibm.com>

Regards,
Sebastian

> 
> >  arch/s390/pci/pci.c             |    4 ++--
> >  arch/x86/include/asm/x86_init.h |    3 ---
> >  arch/x86/kernel/x86_init.c      |   10 ----------
> >  arch/x86/pci/xen.c              |   15 +++------------
> >  drivers/pci/msi.c               |   29 ++++++++++++-----------------
> >  include/linux/msi.h             |    5 +++--
> >  6 files changed, 20 insertions(+), 46 deletions(-)
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>