msix restore code optimization for dom0

PHYSDEVOP_restore_msi is used to restore all msix entrys in one hypercall in dom0. But it is called multi times in current code.

This patch split arch_restore_msi_irqs into two functions.
Use arch_restore_msi_irq deal with one entry and avoid call hypercall multi times in __pci_restore_msix_state.

Signed-off-by Zhenzhong Duan <[email protected]>

---
 arch/x86/include/asm/pci.h      |    8 ++++----
 arch/x86/include/asm/x86_init.h |    2 +-
 arch/x86/pci/xen.c              |    2 +-
 drivers/pci/msi.c               |   17 ++++++++++++-----
 4 files changed, 18 insertions(+), 11 deletions(-)

arch/x86/include/asm/pci.h +4 -4

index d9e9e6c..40cbea4 100644
@@ -115,9 +115,9 @@ static inline void x86_teardown_msi_irq(unsigned int irq)
 {
 	x86_msi.teardown_msi_irq(irq);
 }
-static inline void x86_restore_msi_irqs(struct pci_dev *dev, int irq)
+static inline void x86_restore_msi_irqs(struct pci_dev *dev)
 {
-	x86_msi.restore_msi_irqs(dev, irq);
+	x86_msi.restore_msi_irqs(dev);
 }
 #define arch_setup_msi_irqs x86_setup_msi_irqs
 #define arch_teardown_msi_irqs x86_teardown_msi_irqs

@@ -127,14 +127,14 @@ static inline void x86_restore_msi_irqs(struct pci_dev *dev, int irq)
 struct msi_desc;
 int native_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
 void native_teardown_msi_irq(unsigned int irq);
-void native_restore_msi_irqs(struct pci_dev *dev, int irq);
+void native_restore_msi_irqs(struct pci_dev *dev);
 int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 		  unsigned int irq_base, unsigned int irq_offset);
 /* default to the implementation in drivers/lib/msi.c */
 #define HAVE_DEFAULT_MSI_TEARDOWN_IRQS
 #define HAVE_DEFAULT_MSI_RESTORE_IRQS
 void default_teardown_msi_irqs(struct pci_dev *dev);
-void default_restore_msi_irqs(struct pci_dev *dev, int irq);
+void default_restore_msi_irqs(struct pci_dev *dev);
 #else
 #define native_setup_msi_irqs		NULL
 #define native_teardown_msi_irq		NULL

arch/x86/include/asm/x86_init.h +1 -1

index 828a156..f58a9c7 100644
@@ -180,7 +180,7 @@ struct x86_msi_ops {
 			       u8 hpet_id);
 	void (*teardown_msi_irq)(unsigned int irq);
 	void (*teardown_msi_irqs)(struct pci_dev *dev);
-	void (*restore_msi_irqs)(struct pci_dev *dev, int irq);
+	void (*restore_msi_irqs)(struct pci_dev *dev);
 	int  (*setup_hpet_msi)(unsigned int irq, unsigned int id);
 };
 

arch/x86/pci/xen.c +1 -1

index 48e8461..cdd869f 100644
@@ -337,7 +337,7 @@ out:
 	return ret;
 }
 
-static void xen_initdom_restore_msi_irqs(struct pci_dev *dev, int irq)
+static void xen_initdom_restore_msi_irqs(struct pci_dev *dev)
 {
 	int ret = 0;
 

drivers/pci/msi.c +12 -5

index 922fb49..d4ccfeb 100644
@@ -214,7 +214,7 @@ void unmask_msi_irq(struct irq_data *data)
 #endif /* CONFIG_GENERIC_HARDIRQS */
 
 #ifdef HAVE_DEFAULT_MSI_RESTORE_IRQS
-void default_restore_msi_irqs(struct pci_dev *dev, int irq)
+static void default_restore_msi_irq(struct pci_dev *dev, int irq)
 {
 	int pos;
 	u16 control;

@@ -244,6 +244,15 @@ void default_restore_msi_irqs(struct pci_dev *dev, int irq)
 		}
 	}
 }
+
+void default_restore_msi_irqs(struct pci_dev *dev)
+{
+	struct msi_desc *entry;
+
+	list_for_each_entry(entry, &dev->msi_list, list) {
+		default_restore_msi_irq(dev, entry->irq);
+	}
+}
 #endif
 
 void __read_msi_msg(struct msi_desc *entry, struct msi_msg *msg)

@@ -416,7 +425,7 @@ static void __pci_restore_msi_state(struct pci_dev *dev)
 
 	pci_intx_for_msi(dev, 0);
 	msi_set_enable(dev, 0);
-	arch_restore_msi_irqs(dev, dev->irq);
+	arch_restore_msi_irqs(dev);
 
 	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
 	control &= ~PCI_MSI_FLAGS_QSIZE;

@@ -440,9 +449,7 @@ static void __pci_restore_msix_state(struct pci_dev *dev)
 	control |= PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL;
 	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, control);
 
-	list_for_each_entry(entry, &dev->msi_list, list) {
-		arch_restore_msi_irqs(dev, entry->irq);
-	}
+	arch_restore_msi_irqs(dev);
 
 	control &= ~PCI_MSIX_FLAGS_MASKALL;
 	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, control);

Signature · 1 line

1.7.3

PHYSDEVOP_restore_msi is used to restore all msix entrys in one hypercall in dom0. But it is called multi times in current code.

This patch split arch_restore_msi_irqs into two functions.
Use arch_restore_msi_irq deal with one entry and avoid call hypercall multi times in __pci_restore_msix_state.

Signed-off-by Zhenzhong Duan <[email protected]>

---
 arch/x86/include/asm/pci.h      |    8 ++++----
 arch/x86/include/asm/x86_init.h |    2 +-
 arch/x86/pci/xen.c              |    2 +-
 drivers/pci/msi.c               |   17 ++++++++++++-----
 4 files changed, 18 insertions(+), 11 deletions(-)

arch/x86/include/asm/pci.h +4 -4

index d9e9e6c..40cbea4 100644
@@ -115,9 +115,9 @@ static inline void x86_teardown_msi_irq(unsigned int irq)
 {
 	x86_msi.teardown_msi_irq(irq);
 }
-static inline void x86_restore_msi_irqs(struct pci_dev *dev, int irq)
+static inline void x86_restore_msi_irqs(struct pci_dev *dev)
 {
-	x86_msi.restore_msi_irqs(dev, irq);
+	x86_msi.restore_msi_irqs(dev);
 }
 #define arch_setup_msi_irqs x86_setup_msi_irqs
 #define arch_teardown_msi_irqs x86_teardown_msi_irqs

@@ -127,14 +127,14 @@ static inline void x86_restore_msi_irqs(struct pci_dev *dev, int irq)
 struct msi_desc;
 int native_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
 void native_teardown_msi_irq(unsigned int irq);
-void native_restore_msi_irqs(struct pci_dev *dev, int irq);
+void native_restore_msi_irqs(struct pci_dev *dev);
 int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 		  unsigned int irq_base, unsigned int irq_offset);
 /* default to the implementation in drivers/lib/msi.c */
 #define HAVE_DEFAULT_MSI_TEARDOWN_IRQS
 #define HAVE_DEFAULT_MSI_RESTORE_IRQS
 void default_teardown_msi_irqs(struct pci_dev *dev);
-void default_restore_msi_irqs(struct pci_dev *dev, int irq);
+void default_restore_msi_irqs(struct pci_dev *dev);
 #else
 #define native_setup_msi_irqs		NULL
 #define native_teardown_msi_irq		NULL

arch/x86/include/asm/x86_init.h +1 -1

index 828a156..f58a9c7 100644
@@ -180,7 +180,7 @@ struct x86_msi_ops {
 			       u8 hpet_id);
 	void (*teardown_msi_irq)(unsigned int irq);
 	void (*teardown_msi_irqs)(struct pci_dev *dev);
-	void (*restore_msi_irqs)(struct pci_dev *dev, int irq);
+	void (*restore_msi_irqs)(struct pci_dev *dev);
 	int  (*setup_hpet_msi)(unsigned int irq, unsigned int id);
 };
 

arch/x86/pci/xen.c +1 -1

index 48e8461..cdd869f 100644
@@ -337,7 +337,7 @@ out:
 	return ret;
 }
 
-static void xen_initdom_restore_msi_irqs(struct pci_dev *dev, int irq)
+static void xen_initdom_restore_msi_irqs(struct pci_dev *dev)
 {
 	int ret = 0;
 

drivers/pci/msi.c +12 -5

index 922fb49..d4ccfeb 100644
@@ -214,7 +214,7 @@ void unmask_msi_irq(struct irq_data *data)
 #endif /* CONFIG_GENERIC_HARDIRQS */
 
 #ifdef HAVE_DEFAULT_MSI_RESTORE_IRQS
-void default_restore_msi_irqs(struct pci_dev *dev, int irq)
+static void default_restore_msi_irq(struct pci_dev *dev, int irq)
 {
 	int pos;
 	u16 control;

@@ -244,6 +244,15 @@ void default_restore_msi_irqs(struct pci_dev *dev, int irq)
 		}
 	}
 }
+
+void default_restore_msi_irqs(struct pci_dev *dev)
+{
+	struct msi_desc *entry;
+
+	list_for_each_entry(entry, &dev->msi_list, list) {
+		default_restore_msi_irq(dev, entry->irq);
+	}
+}
 #endif
 
 void __read_msi_msg(struct msi_desc *entry, struct msi_msg *msg)

@@ -416,7 +425,7 @@ static void __pci_restore_msi_state(struct pci_dev *dev)
 
 	pci_intx_for_msi(dev, 0);
 	msi_set_enable(dev, 0);
-	arch_restore_msi_irqs(dev, dev->irq);
+	arch_restore_msi_irqs(dev);
 
 	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
 	control &= ~PCI_MSI_FLAGS_QSIZE;

@@ -440,9 +449,7 @@ static void __pci_restore_msix_state(struct pci_dev *dev)
 	control |= PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL;
 	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, control);
 
-	list_for_each_entry(entry, &dev->msi_list, list) {
-		arch_restore_msi_irqs(dev, entry->irq);
-	}
+	arch_restore_msi_irqs(dev);
 
 	control &= ~PCI_MSIX_FLAGS_MASKALL;
 	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, control);

Signature · 1 line

1.7.3

Quoted 2 lines · Zhenzhong Duan

PHYSDEVOP_restore_msi is used to restore all msix entrys in one hypercall in dom0. But it is called multi times in current code.

Couldn't the restore_msi_irqs instead check whether it has already made the hypercall (perhaps by seeing if the MSI-X's are enabled?) and if so don't do the hypercall?

I think you are addressing the problem from a different viewpoint.

The problem is not with the API (the x86_msi one). The problem is with the implementation (x86_msi.restore_msi_irq - specifically the Xen one) having an side effect.

Quoted 4 lines

This patch split arch_restore_msi_irqs into two functions.
Use arch_restore_msi_irq deal with one entry and avoid call hypercall multi times in __pci_restore_msix_state.

But irregardless of how you address the problem, this in the MSI code is a bit odd:

	list_for_each_entry(entry, &dev->msi_list, list) {
		arch_restore_msi_irqs(dev, entry->irq);
	}

and if you collapse that and make the 'arch_restore_msi_irqs' be responsible for doing all the heavy lifting.. That does seem an improvement on the API and will make it inline with 'teardown_msi_irqs'.

So from that view I think it would be nicer?

Quoted 2 lines · Zhenzhong Duan

PHYSDEVOP_restore_msi is used to restore all msix entrys in one hypercall in dom0. But it is called multi times in current code.

Couldn't the restore_msi_irqs instead check whether it has already made the hypercall (perhaps by seeing if the MSI-X's are enabled?) and if so don't do the hypercall?

I think you are addressing the problem from a different viewpoint.

The problem is not with the API (the x86_msi one). The problem is with the implementation (x86_msi.restore_msi_irq - specifically the Xen one) having an side effect.

Quoted 4 lines

This patch split arch_restore_msi_irqs into two functions.
Use arch_restore_msi_irq deal with one entry and avoid call hypercall multi times in __pci_restore_msix_state.

But irregardless of how you address the problem, this in the MSI code is a bit odd:

	list_for_each_entry(entry, &dev->msi_list, list) {
		arch_restore_msi_irqs(dev, entry->irq);
	}

and if you collapse that and make the 'arch_restore_msi_irqs' be responsible for doing all the heavy lifting.. That does seem an improvement on the API and will make it inline with 'teardown_msi_irqs'.

So from that view I think it would be nicer?

Quoted 6 lines · Konrad Rzeszutek Wilk

Quoted 2 lines · Zhenzhong Duan

PHYSDEVOP_restore_msi is used to restore all msix entrys in one hypercall in dom0. But it is called multi times in current code.

Couldn't the restore_msi_irqs instead check whether it has already made the hypercall (perhaps by seeing if the MSI-X's are enabled?) and if so don't do the hypercall?

I think it's better to remove the for loop for dom0, also hard to know when to clear hypercall count.

Quoted 1 line

Quoted 5 lines

I think you are addressing the problem from a different viewpoint.

The problem is not with the API (the x86_msi one). The problem is with the implementation (x86_msi.restore_msi_irq - specifically the Xen one) having an side effect.

We have x86_msi.restore_msi_irqs and no x86_msi.restore_msi_irq,if want to add x86_msi.restore_msi_irq, same hypercall need to be added accordingly.

Quoted 11 lines

Quoted 4 lines

This patch split arch_restore_msi_irqs into two functions.
Use arch_restore_msi_irq deal with one entry and avoid call hypercall multi times in __pci_restore_msix_state.

But irregardless of how you address the problem, this in the MSI code is a bit odd:

	list_for_each_entry(entry, &dev->msi_list, list) {
		arch_restore_msi_irqs(dev, entry->irq);
	}

Quoted 5 lines

and if you collapse that and make the 'arch_restore_msi_irqs' be responsible for doing all the heavy lifting.. That does seem an improvement on the API and will make it inline with 'teardown_msi_irqs'.

So from that view I think it would be nicer?

Yes, This patch just did that. There is teardown_msi_irqs/teardown_msi_irq pair.
But in current code, arch_restore_msi_irqs is used for one msix entry, this is not consistent with
its naming tradiition. So I changed default_restore_msi_irqs to deal with all entrys and
default_restore_msi_irq to deal with one entry for baremetal. For dom0, we have
PHYSDEVOP_restore_msi to restore all msix entrys.

Quoted 6 lines · Konrad Rzeszutek Wilk

Quoted 2 lines · Zhenzhong Duan

PHYSDEVOP_restore_msi is used to restore all msix entrys in one hypercall in dom0. But it is called multi times in current code.

Couldn't the restore_msi_irqs instead check whether it has already made the hypercall (perhaps by seeing if the MSI-X's are enabled?) and if so don't do the hypercall?

I think it's better to remove the for loop for dom0, also hard to know when to clear hypercall count.

Quoted 1 line

Quoted 5 lines

I think you are addressing the problem from a different viewpoint.

The problem is not with the API (the x86_msi one). The problem is with the implementation (x86_msi.restore_msi_irq - specifically the Xen one) having an side effect.

We have x86_msi.restore_msi_irqs and no x86_msi.restore_msi_irq,if want to add x86_msi.restore_msi_irq, same hypercall need to be added accordingly.

Quoted 11 lines

Quoted 4 lines

This patch split arch_restore_msi_irqs into two functions.
Use arch_restore_msi_irq deal with one entry and avoid call hypercall multi times in __pci_restore_msix_state.

But irregardless of how you address the problem, this in the MSI code is a bit odd:

	list_for_each_entry(entry, &dev->msi_list, list) {
		arch_restore_msi_irqs(dev, entry->irq);
	}

Quoted 5 lines

and if you collapse that and make the 'arch_restore_msi_irqs' be responsible for doing all the heavy lifting.. That does seem an improvement on the API and will make it inline with 'teardown_msi_irqs'.

So from that view I think it would be nicer?

Yes, This patch just did that. There is teardown_msi_irqs/teardown_msi_irq pair.
But in current code, arch_restore_msi_irqs is used for one msix entry, this is not consistent with
its naming tradiition. So I changed default_restore_msi_irqs to deal with all entrys and
default_restore_msi_irq to deal with one entry for baremetal. For dom0, we have
PHYSDEVOP_restore_msi to restore all msix entrys.