| Message ID | 20210106153958.584169-2-dwmw2@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Xen INTX/GSI event channel delivery fixes | expand |
On 06.01.21 16:39, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > For a while, event channel notification via the PCI platform device > has been broken, because we attempt to communicate with xenstore before > we even have notifications working, with the xs_reset_watches() call > in xs_init(). > > We tend to get away with this on Xen versions below 4.0 because we avoid > calling xs_reset_watches() anyway, because xenstore might not cope with > reading a non-existent key. And newer Xen *does* have the vector > callback support, so we rarely fall back to INTX/GSI delivery. > > To fix it, clean up a bit of the mess of xs_init() and xenbus_probe() > startup. Call xs_init() directly from xenbus_init() only in the !XS_HVM > case, deferring it to be called from xenbus_probe() in the XS_HVM case > instead. > > Then fix up the invocation of xenbus_probe() to happen either from its > device_initcall if the callback is available early enough, or when the > callback is finally set up. This means that the hack of calling > xenbus_probe() from a workqueue after the first interrupt, or directly > from the PCI platform device setup, is no longer needed. > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Building for arm I get: /home/gross/korg/src/drivers/xen/xenbus/xenbus_probe.c: In function 'xenbus_probe_initcall': /home/gross/korg/src/drivers/xen/xenbus/xenbus_probe.c:711:41: error: 'xen_have_vector_callback' undeclared (first use in this function); did you mean 'em_data_callback'? (!IS_ENABLED(CONFIG_XEN_PVHVM) || xen_have_vector_callback))) Juergen > --- > arch/arm/xen/enlighten.c | 2 +- > drivers/xen/events/events_base.c | 10 ----- > drivers/xen/platform-pci.c | 1 - > drivers/xen/xenbus/xenbus.h | 1 + > drivers/xen/xenbus/xenbus_comms.c | 8 ---- > drivers/xen/xenbus/xenbus_probe.c | 68 ++++++++++++++++++++++++------- > include/xen/xenbus.h | 2 +- > 7 files changed, 57 insertions(+), 35 deletions(-) > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c > index 60e901cd0de6..5a957a9a0984 100644 > --- a/arch/arm/xen/enlighten.c > +++ b/arch/arm/xen/enlighten.c > @@ -371,7 +371,7 @@ static int __init xen_guest_init(void) > } > gnttab_init(); > if (!xen_initial_domain()) > - xenbus_probe(NULL); > + xenbus_probe(); > > /* > * Making sure board specific code will not set up ops for > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c > index a8030332a191..e850f79351cb 100644 > --- a/drivers/xen/events/events_base.c > +++ b/drivers/xen/events/events_base.c > @@ -2060,16 +2060,6 @@ static struct irq_chip xen_percpu_chip __read_mostly = { > .irq_ack = ack_dynirq, > }; > > -int xen_set_callback_via(uint64_t via) > -{ > - struct xen_hvm_param a; > - a.domid = DOMID_SELF; > - a.index = HVM_PARAM_CALLBACK_IRQ; > - a.value = via; > - return HYPERVISOR_hvm_op(HVMOP_set_param, &a); > -} > -EXPORT_SYMBOL_GPL(xen_set_callback_via); > - > #ifdef CONFIG_XEN_PVHVM > /* Vector callbacks are better than PCI interrupts to receive event > * channel notifications because we can receive vector callbacks on any > diff --git a/drivers/xen/platform-pci.c b/drivers/xen/platform-pci.c > index dd911e1ff782..9db557b76511 100644 > --- a/drivers/xen/platform-pci.c > +++ b/drivers/xen/platform-pci.c > @@ -149,7 +149,6 @@ static int platform_pci_probe(struct pci_dev *pdev, > ret = gnttab_init(); > if (ret) > goto grant_out; > - xenbus_probe(NULL); > return 0; > grant_out: > gnttab_free_auto_xlat_frames(); > diff --git a/drivers/xen/xenbus/xenbus.h b/drivers/xen/xenbus/xenbus.h > index 2a93b7c9c159..dc1537335414 100644 > --- a/drivers/xen/xenbus/xenbus.h > +++ b/drivers/xen/xenbus/xenbus.h > @@ -115,6 +115,7 @@ int xenbus_probe_node(struct xen_bus_type *bus, > const char *type, > const char *nodename); > int xenbus_probe_devices(struct xen_bus_type *bus); > +void xenbus_probe(void); > > void xenbus_dev_changed(const char *node, struct xen_bus_type *bus); > > diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c > index eb5151fc8efa..e5fda0256feb 100644 > --- a/drivers/xen/xenbus/xenbus_comms.c > +++ b/drivers/xen/xenbus/xenbus_comms.c > @@ -57,16 +57,8 @@ DEFINE_MUTEX(xs_response_mutex); > static int xenbus_irq; > static struct task_struct *xenbus_task; > > -static DECLARE_WORK(probe_work, xenbus_probe); > - > - > static irqreturn_t wake_waiting(int irq, void *unused) > { > - if (unlikely(xenstored_ready == 0)) { > - xenstored_ready = 1; > - schedule_work(&probe_work); > - } > - > wake_up(&xb_waitq); > return IRQ_HANDLED; > } > diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c > index 44634d970a5c..b1b5b6fe9b52 100644 > --- a/drivers/xen/xenbus/xenbus_probe.c > +++ b/drivers/xen/xenbus/xenbus_probe.c > @@ -683,29 +683,63 @@ void unregister_xenstore_notifier(struct notifier_block *nb) > } > EXPORT_SYMBOL_GPL(unregister_xenstore_notifier); > > -void xenbus_probe(struct work_struct *unused) > +void xenbus_probe(void) > { > xenstored_ready = 1; > > + /* > + * In the HVM case, xenbus_init() deferred its call to > + * xs_init() in case callbacks were not operational yet. > + * So do it now. > + */ > + if (xen_store_domain_type == XS_HVM) > + xs_init(); > + > /* Notify others that xenstore is up */ > blocking_notifier_call_chain(&xenstore_chain, 0, NULL); > } > -EXPORT_SYMBOL_GPL(xenbus_probe); > > static int __init xenbus_probe_initcall(void) > { > - if (!xen_domain()) > - return -ENODEV; > - > - if (xen_initial_domain() || xen_hvm_domain()) > - return 0; > + /* > + * Probe XenBus here in the XS_PV case, and also XS_HVM unless we > + * need to wait for the platform PCI device to come up, which is > + * the (XEN_PVPVM && !xen_have_vector_callback) case. > + */ > + if (xen_store_domain_type == XS_PV || > + (xen_store_domain_type == XS_HVM && > + (!IS_ENABLED(CONFIG_XEN_PVHVM) || xen_have_vector_callback))) > + xenbus_probe(); > > - xenbus_probe(NULL); > return 0; > } > - > device_initcall(xenbus_probe_initcall); > > +int xen_set_callback_via(uint64_t via) > +{ > + struct xen_hvm_param a; > + int ret; > + > + a.domid = DOMID_SELF; > + a.index = HVM_PARAM_CALLBACK_IRQ; > + a.value = via; > + > + ret = HYPERVISOR_hvm_op(HVMOP_set_param, &a); > + if (ret) > + return ret; > + > + /* > + * If xenbus_probe_initcall() deferred the xenbus_probe() > + * due to the callback not functioning yet, we can do it now. > + */ > + if (!xenstored_ready && xen_store_domain_type == XS_HVM && > + IS_ENABLED(CONFIG_XEN_PVHVM) && !xen_have_vector_callback) > + xenbus_probe(); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(xen_set_callback_via); > + > /* Set up event channel for xenstored which is run as a local process > * (this is normally used only in dom0) > */ > @@ -818,11 +852,17 @@ static int __init xenbus_init(void) > break; > } > > - /* Initialize the interface to xenstore. */ > - err = xs_init(); > - if (err) { > - pr_warn("Error initializing xenstore comms: %i\n", err); > - goto out_error; > + /* > + * HVM domains may not have a functional callback yet. In that > + * case let xs_init() be called from xenbus_probe(), which will > + * get invoked at an appropriate time. > + */ > + if (xen_store_domain_type != XS_HVM) { > + err = xs_init(); > + if (err) { > + pr_warn("Error initializing xenstore comms: %i\n", err); > + goto out_error; > + } > } > > if ((xen_store_domain_type != XS_LOCAL) && > diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h > index 00c7235ae93e..2c43b0ef1e4d 100644 > --- a/include/xen/xenbus.h > +++ b/include/xen/xenbus.h > @@ -192,7 +192,7 @@ void xs_suspend_cancel(void); > > struct work_struct; > > -void xenbus_probe(struct work_struct *); > +void xenbus_probe(void); > > #define XENBUS_IS_ERR_READ(str) ({ \ > if (!IS_ERR(str) && strlen(str) == 0) { \ >
On Wed, 2021-01-13 at 12:20 +0100, Jürgen Groß wrote: > > /home/gross/korg/src/drivers/xen/xenbus/xenbus_probe.c: In function > 'xenbus_probe_initcall': > /home/gross/korg/src/drivers/xen/xenbus/xenbus_probe.c:711:41: > error: > 'xen_have_vector_callback' undeclared (first use in this function); > did > you mean 'em_data_callback'? > (!IS_ENABLED(CONFIG_XEN_PVHVM) || xen_have_vector_callback))) > Oops. I think this should fix it; will retest and post the series again with this folded into the offending commit. Thanks. diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index b1b5b6fe9b52..f3ef23ebcd94 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -699,16 +699,30 @@ void xenbus_probe(void) blocking_notifier_call_chain(&xenstore_chain, 0, NULL); } +/* + * Returns true when XenStore init must be deferred in order to + * allow the PCI platform device to be initialised, before we + * can actually have event channel interrupts working. + */ +static bool xs_hvm_defer_init_for_callback(void) +{ +#ifdef CONFIG_XEN_PVHVM + return xen_store_domain_type == XS_HVM && + !xen_have_vector_callback; +#else + return false; +#endif +} + static int __init xenbus_probe_initcall(void) { /* * Probe XenBus here in the XS_PV case, and also XS_HVM unless we - * need to wait for the platform PCI device to come up, which is - * the (XEN_PVPVM && !xen_have_vector_callback) case. + * need to wait for the platform PCI device to come up. */ if (xen_store_domain_type == XS_PV || - (xen_store_domain_type == XS_HVM && - (!IS_ENABLED(CONFIG_XEN_PVHVM) || xen_have_vector_callback))) + (xen_store_domain_type == XS_HVM && + !xs_hvm_defer_init_for_callback()) xenbus_probe(); return 0; @@ -732,8 +746,7 @@ int xen_set_callback_via(uint64_t via) * If xenbus_probe_initcall() deferred the xenbus_probe() * due to the callback not functioning yet, we can do it now. */ - if (!xenstored_ready && xen_store_domain_type == XS_HVM && - IS_ENABLED(CONFIG_XEN_PVHVM) && !xen_have_vector_callback) + if (!xenstored_ready && xs_hvm_defer_init_for_callback()) xenbus_probe(); return ret;
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index 60e901cd0de6..5a957a9a0984 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -371,7 +371,7 @@ static int __init xen_guest_init(void) } gnttab_init(); if (!xen_initial_domain()) - xenbus_probe(NULL); + xenbus_probe(); /* * Making sure board specific code will not set up ops for diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c index a8030332a191..e850f79351cb 100644 --- a/drivers/xen/events/events_base.c +++ b/drivers/xen/events/events_base.c @@ -2060,16 +2060,6 @@ static struct irq_chip xen_percpu_chip __read_mostly = { .irq_ack = ack_dynirq, }; -int xen_set_callback_via(uint64_t via) -{ - struct xen_hvm_param a; - a.domid = DOMID_SELF; - a.index = HVM_PARAM_CALLBACK_IRQ; - a.value = via; - return HYPERVISOR_hvm_op(HVMOP_set_param, &a); -} -EXPORT_SYMBOL_GPL(xen_set_callback_via); - #ifdef CONFIG_XEN_PVHVM /* Vector callbacks are better than PCI interrupts to receive event * channel notifications because we can receive vector callbacks on any diff --git a/drivers/xen/platform-pci.c b/drivers/xen/platform-pci.c index dd911e1ff782..9db557b76511 100644 --- a/drivers/xen/platform-pci.c +++ b/drivers/xen/platform-pci.c @@ -149,7 +149,6 @@ static int platform_pci_probe(struct pci_dev *pdev, ret = gnttab_init(); if (ret) goto grant_out; - xenbus_probe(NULL); return 0; grant_out: gnttab_free_auto_xlat_frames(); diff --git a/drivers/xen/xenbus/xenbus.h b/drivers/xen/xenbus/xenbus.h index 2a93b7c9c159..dc1537335414 100644 --- a/drivers/xen/xenbus/xenbus.h +++ b/drivers/xen/xenbus/xenbus.h @@ -115,6 +115,7 @@ int xenbus_probe_node(struct xen_bus_type *bus, const char *type, const char *nodename); int xenbus_probe_devices(struct xen_bus_type *bus); +void xenbus_probe(void); void xenbus_dev_changed(const char *node, struct xen_bus_type *bus); diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c index eb5151fc8efa..e5fda0256feb 100644 --- a/drivers/xen/xenbus/xenbus_comms.c +++ b/drivers/xen/xenbus/xenbus_comms.c @@ -57,16 +57,8 @@ DEFINE_MUTEX(xs_response_mutex); static int xenbus_irq; static struct task_struct *xenbus_task; -static DECLARE_WORK(probe_work, xenbus_probe); - - static irqreturn_t wake_waiting(int irq, void *unused) { - if (unlikely(xenstored_ready == 0)) { - xenstored_ready = 1; - schedule_work(&probe_work); - } - wake_up(&xb_waitq); return IRQ_HANDLED; } diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index 44634d970a5c..b1b5b6fe9b52 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -683,29 +683,63 @@ void unregister_xenstore_notifier(struct notifier_block *nb) } EXPORT_SYMBOL_GPL(unregister_xenstore_notifier); -void xenbus_probe(struct work_struct *unused) +void xenbus_probe(void) { xenstored_ready = 1; + /* + * In the HVM case, xenbus_init() deferred its call to + * xs_init() in case callbacks were not operational yet. + * So do it now. + */ + if (xen_store_domain_type == XS_HVM) + xs_init(); + /* Notify others that xenstore is up */ blocking_notifier_call_chain(&xenstore_chain, 0, NULL); } -EXPORT_SYMBOL_GPL(xenbus_probe); static int __init xenbus_probe_initcall(void) { - if (!xen_domain()) - return -ENODEV; - - if (xen_initial_domain() || xen_hvm_domain()) - return 0; + /* + * Probe XenBus here in the XS_PV case, and also XS_HVM unless we + * need to wait for the platform PCI device to come up, which is + * the (XEN_PVPVM && !xen_have_vector_callback) case. + */ + if (xen_store_domain_type == XS_PV || + (xen_store_domain_type == XS_HVM && + (!IS_ENABLED(CONFIG_XEN_PVHVM) || xen_have_vector_callback))) + xenbus_probe(); - xenbus_probe(NULL); return 0; } - device_initcall(xenbus_probe_initcall); +int xen_set_callback_via(uint64_t via) +{ + struct xen_hvm_param a; + int ret; + + a.domid = DOMID_SELF; + a.index = HVM_PARAM_CALLBACK_IRQ; + a.value = via; + + ret = HYPERVISOR_hvm_op(HVMOP_set_param, &a); + if (ret) + return ret; + + /* + * If xenbus_probe_initcall() deferred the xenbus_probe() + * due to the callback not functioning yet, we can do it now. + */ + if (!xenstored_ready && xen_store_domain_type == XS_HVM && + IS_ENABLED(CONFIG_XEN_PVHVM) && !xen_have_vector_callback) + xenbus_probe(); + + return ret; +} +EXPORT_SYMBOL_GPL(xen_set_callback_via); + /* Set up event channel for xenstored which is run as a local process * (this is normally used only in dom0) */ @@ -818,11 +852,17 @@ static int __init xenbus_init(void) break; } - /* Initialize the interface to xenstore. */ - err = xs_init(); - if (err) { - pr_warn("Error initializing xenstore comms: %i\n", err); - goto out_error; + /* + * HVM domains may not have a functional callback yet. In that + * case let xs_init() be called from xenbus_probe(), which will + * get invoked at an appropriate time. + */ + if (xen_store_domain_type != XS_HVM) { + err = xs_init(); + if (err) { + pr_warn("Error initializing xenstore comms: %i\n", err); + goto out_error; + } } if ((xen_store_domain_type != XS_LOCAL) && diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h index 00c7235ae93e..2c43b0ef1e4d 100644 --- a/include/xen/xenbus.h +++ b/include/xen/xenbus.h @@ -192,7 +192,7 @@ void xs_suspend_cancel(void); struct work_struct; -void xenbus_probe(struct work_struct *); +void xenbus_probe(void); #define XENBUS_IS_ERR_READ(str) ({ \ if (!IS_ERR(str) && strlen(str) == 0) { \