| Message ID | 20200821002947.667887608@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86, PCI, XEN, genirq ...: Prepare for device MSI | expand |
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
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
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
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.
--- 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; /*
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(-)