diff mbox series

[RFC,24/38] x86/xen: Consolidate XEN-MSI init

Message ID 20200821002947.667887608@linutronix.de (mailing list archive)
State Superseded
Headers show
Series x86, PCI, XEN, genirq ...: Prepare for device MSI | expand

Commit Message

Thomas Gleixner Aug. 21, 2020, 12:24 a.m. UTC
X86 cannot store the irq domain pointer in struct device without breaking
XEN because the irq domain pointer takes precedence over arch_*_msi_irqs()
fallbacks.

To achieve this XEN MSI interrupt management needs to be wrapped into an
irq domain.

Move the x86_msi ops setup into a single function to prepare for this.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/pci/xen.c |   51 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 19 deletions(-)

Comments

Jürgen Groß Aug. 24, 2020, 4:59 a.m. UTC | #1
On 21.08.20 02:24, Thomas Gleixner wrote:
> X86 cannot store the irq domain pointer in struct device without breaking
> XEN because the irq domain pointer takes precedence over arch_*_msi_irqs()
> fallbacks.
> 
> To achieve this XEN MSI interrupt management needs to be wrapped into an
> irq domain.
> 
> Move the x86_msi ops setup into a single function to prepare for this.
> 
> Signed-off-by: Thomas Gleixner<tglx@linutronix.de>
> ---
>   arch/x86/pci/xen.c |   51 ++++++++++++++++++++++++++++++++-------------------
>   1 file changed, 32 insertions(+), 19 deletions(-)
> 
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -371,7 +371,10 @@ static void xen_initdom_restore_msi_irqs
>   		WARN(ret && ret != -ENOSYS, "restore_msi -> %d\n", ret);
>   	}
>   }
> -#endif
> +#else /* CONFIG_XEN_DOM0 */
> +#define xen_initdom_setup_msi_irqs	NULL
> +#define xen_initdom_restore_msi_irqs	NULL
> +#endif /* !CONFIG_XEN_DOM0 */
>   
>   static void xen_teardown_msi_irqs(struct pci_dev *dev)
>   {
> @@ -403,7 +406,31 @@ static void xen_teardown_msi_irq(unsigne
>   	WARN_ON_ONCE(1);
>   }
>   
> -#endif
> +static __init void xen_setup_pci_msi(void)
> +{
> +	if (xen_initial_domain()) {
> +		x86_msi.setup_msi_irqs = xen_initdom_setup_msi_irqs;
> +		x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
> +		x86_msi.restore_msi_irqs = xen_initdom_restore_msi_irqs;
> +		pci_msi_ignore_mask = 1;

This is wrong, as a PVH initial domain shouldn't do the pv settings.

The "if (xen_initial_domain())" should be inside the pv case, like:

if (xen_pv_domain()) {
	if (xen_initial_domain()) {
		...
	} else {
		...
	}
} else if (xen_hvm_domain()) {
	...

Juergen
Thomas Gleixner Aug. 24, 2020, 9:21 p.m. UTC | #2
On Mon, Aug 24 2020 at 06:59, Jürgen Groß wrote:
> On 21.08.20 02:24, Thomas Gleixner wrote:
>> +static __init void xen_setup_pci_msi(void)
>> +{
>> +	if (xen_initial_domain()) {
>> +		x86_msi.setup_msi_irqs = xen_initdom_setup_msi_irqs;
>> +		x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
>> +		x86_msi.restore_msi_irqs = xen_initdom_restore_msi_irqs;
>> +		pci_msi_ignore_mask = 1;
>
> This is wrong, as a PVH initial domain shouldn't do the pv settings.
>
> The "if (xen_initial_domain())" should be inside the pv case, like:
>
> if (xen_pv_domain()) {
> 	if (xen_initial_domain()) {
> 		...
> 	} else {
> 		...
> 	}
> } else if (xen_hvm_domain()) {
> 	...

I still think it does the right thing depending on the place it is
called from, but even if so, it's completely unreadable gunk. I'll fix
that proper.

Thanks,

        tglx
Jürgen Groß Aug. 25, 2020, 4:21 a.m. UTC | #3
On 24.08.20 23:21, Thomas Gleixner wrote:
> On Mon, Aug 24 2020 at 06:59, Jürgen Groß wrote:
>> On 21.08.20 02:24, Thomas Gleixner wrote:
>>> +static __init void xen_setup_pci_msi(void)
>>> +{
>>> +	if (xen_initial_domain()) {
>>> +		x86_msi.setup_msi_irqs = xen_initdom_setup_msi_irqs;
>>> +		x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
>>> +		x86_msi.restore_msi_irqs = xen_initdom_restore_msi_irqs;
>>> +		pci_msi_ignore_mask = 1;
>>
>> This is wrong, as a PVH initial domain shouldn't do the pv settings.
>>
>> The "if (xen_initial_domain())" should be inside the pv case, like:
>>
>> if (xen_pv_domain()) {
>> 	if (xen_initial_domain()) {
>> 		...
>> 	} else {
>> 		...
>> 	}
>> } else if (xen_hvm_domain()) {
>> 	...
> 
> I still think it does the right thing depending on the place it is
> called from, but even if so, it's completely unreadable gunk. I'll fix
> that proper.

The main issue is that xen_initial_domain() and xen_pv_domain() are
orthogonal to each other. So xen_initial_domain() can either be true
for xen_pv_domain() (the "classic" pv dom0) or for xen_hvm_domain()
(the new PVH dom0).


Juergen
Thomas Gleixner Aug. 25, 2020, 9:51 a.m. UTC | #4
On Tue, Aug 25 2020 at 06:21, Jürgen Groß wrote:
> On 24.08.20 23:21, Thomas Gleixner wrote:
>> I still think it does the right thing depending on the place it is
>> called from, but even if so, it's completely unreadable gunk. I'll fix
>> that proper.
>
> The main issue is that xen_initial_domain() and xen_pv_domain() are
> orthogonal to each other. So xen_initial_domain() can either be true
> for xen_pv_domain() (the "classic" pv dom0) or for xen_hvm_domain()
> (the new PVH dom0).

Fair enough. My limited XENology striked again.
diff mbox series

Patch

--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -371,7 +371,10 @@  static void xen_initdom_restore_msi_irqs
 		WARN(ret && ret != -ENOSYS, "restore_msi -> %d\n", ret);
 	}
 }
-#endif
+#else /* CONFIG_XEN_DOM0 */
+#define xen_initdom_setup_msi_irqs	NULL
+#define xen_initdom_restore_msi_irqs	NULL
+#endif /* !CONFIG_XEN_DOM0 */
 
 static void xen_teardown_msi_irqs(struct pci_dev *dev)
 {
@@ -403,7 +406,31 @@  static void xen_teardown_msi_irq(unsigne
 	WARN_ON_ONCE(1);
 }
 
-#endif
+static __init void xen_setup_pci_msi(void)
+{
+	if (xen_initial_domain()) {
+		x86_msi.setup_msi_irqs = xen_initdom_setup_msi_irqs;
+		x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
+		x86_msi.restore_msi_irqs = xen_initdom_restore_msi_irqs;
+		pci_msi_ignore_mask = 1;
+	} else if (xen_pv_domain()) {
+		x86_msi.setup_msi_irqs = xen_setup_msi_irqs;
+		x86_msi.teardown_msi_irqs = xen_pv_teardown_msi_irqs;
+		pci_msi_ignore_mask = 1;
+	} else if (xen_hvm_domain()) {
+		x86_msi.setup_msi_irqs = xen_hvm_setup_msi_irqs;
+		x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
+	} else {
+		WARN_ON_ONCE(1);
+		return;
+	}
+
+	x86_msi.teardown_msi_irq = xen_teardown_msi_irq;
+}
+
+#else /* CONFIG_PCI_MSI */
+static inline void xen_setup_pci_msi(void) { }
+#endif /* CONFIG_PCI_MSI */
 
 int __init pci_xen_init(void)
 {
@@ -420,12 +447,7 @@  int __init pci_xen_init(void)
 	/* Keep ACPI out of the picture */
 	acpi_noirq_set();
 
-#ifdef CONFIG_PCI_MSI
-	x86_msi.setup_msi_irqs = xen_setup_msi_irqs;
-	x86_msi.teardown_msi_irq = xen_teardown_msi_irq;
-	x86_msi.teardown_msi_irqs = xen_pv_teardown_msi_irqs;
-	pci_msi_ignore_mask = 1;
-#endif
+	xen_setup_pci_msi();
 	return 0;
 }
 
@@ -445,10 +467,7 @@  static void __init xen_hvm_msi_init(void
 		    ((eax & XEN_HVM_CPUID_APIC_ACCESS_VIRT) && boot_cpu_has(X86_FEATURE_APIC)))
 			return;
 	}
-
-	x86_msi.setup_msi_irqs = xen_hvm_setup_msi_irqs;
-	x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
-	x86_msi.teardown_msi_irq = xen_teardown_msi_irq;
+	xen_setup_pci_msi();
 }
 #endif
 
@@ -481,13 +500,7 @@  int __init pci_xen_initial_domain(void)
 {
 	int irq;
 
-#ifdef CONFIG_PCI_MSI
-	x86_msi.setup_msi_irqs = xen_initdom_setup_msi_irqs;
-	x86_msi.teardown_msi_irq = xen_teardown_msi_irq;
-	x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
-	x86_msi.restore_msi_irqs = xen_initdom_restore_msi_irqs;
-	pci_msi_ignore_mask = 1;
-#endif
+	xen_setup_pci_msi();
 	__acpi_register_gsi = acpi_register_gsi_xen;
 	__acpi_unregister_gsi = NULL;
 	/*