On Fri, Mar 3, 2017 at 11:45 AM, 'Sergiy Kibrik' via OSv Development < [email protected]> wrote:
> Initialize Xen event interrupt separately from xenbus driver, so that > events > can be served before xenbus is up, or even in case when xenbus not being > probed > at all or not required, e.g. in case of aarch64 Xen console. > Unfortunately I'm not familiar enough with this code to say much about the order of doing these things. You created a function that runs as a "constructor" but did not specify an init priority (see include/osv/prio.hh), so your constructor function may run after all other constructor functions. So, is it possible that you could call the initialization function explicitly in some existing function instead of having it be a constructor that runs at an unknown time? It is clearer when a reader can tell exactly when/where this function gets called. That being said, I see that before your patch, setup_xen_irq was already a priority-less constructor function - so I guess there is no harm if you use one too. Additional minor comments below. > > Signed-off-by: Sergiy Kibrik <[email protected]> > --- > arch/aarch64/xen.cc | 20 ++++++++++++++++++++ > arch/x64/xen.cc | 15 +++++++++++++++ > bsd/sys/xen/evtchn.h | 2 ++ > core/xen_intr.cc | 13 ++++++------- > drivers/xenfront-xenbus.cc | 9 +-------- > include/osv/xen.hh | 2 ++ > include/osv/xen_intr.hh | 4 +++- > 7 files changed, 49 insertions(+), 16 deletions(-) > > diff --git a/arch/aarch64/xen.cc b/arch/aarch64/xen.cc > index c08e8f3..2e4a5fb 100644 > --- a/arch/aarch64/xen.cc > +++ b/arch/aarch64/xen.cc > @@ -8,6 +8,9 @@ > #include <osv/types.h> > #include <osv/xen.hh> > #include <xen/interface/xen.h> > +#include <bsd/porting/netport.h> /* __dead2 defined here */ > +#include <machine/xen/xen-os.h> > +#include <xen/evtchn.h> > > #include "arch-dtb.hh" > > @@ -17,6 +20,23 @@ namespace xen { > > shared_info_t dummy_info; > struct xen_shared_info xen_shared_info __attribute__((aligned(4096))); > +constexpr int events_irq = 31; /*FIXME: get from FDT */ > + > +static __attribute__((constructor)) void xen_init_irq() > +{ > + if (!is_xen()) > + return; > + > + evtchn_init(NULL); > + > + auto intr = new spi_interrupt(gic::irq_type::IRQ_TYPE_LEVEL, > events_irq, > + xen::xen_ack_irq, > + xen::xen_handle_irq); > + if (!intr) > + abort("failed to allocate SPI interrupt"); > If you're using the "normal" C++ new (not the new(std::nothrow) version), new will throw an exception on allocation failure, NOT return a null pointer, so this checking is pointless. On OSv it's even more pointless because currently, allocation failures will cause a crash rather than failing gracefully. > + > + setup_xen_irq(intr); > +} > > } > > diff --git a/arch/x64/xen.cc b/arch/x64/xen.cc > index c1b7e77..3afface 100644 > --- a/arch/x64/xen.cc > +++ b/arch/x64/xen.cc > @@ -16,6 +16,7 @@ > #include <osv/sched.hh> > #include <bsd/porting/pcpu.h> > #include <machine/xen/xen-os.h> > +#include <xen/evtchn.h> > > shared_info_t *HYPERVISOR_shared_info; > uint8_t xen_features[XENFEAT_NR_SUBMAPS * 32]; > @@ -201,6 +202,20 @@ void xen_init(processor::features_type &features, > unsigned base) > HYPERVISOR_shared_info = reinterpret_cast<shared_info_t > *>(&xen_shared_info); > } > > +static __attribute__((constructor)) void xen_init_irq() > +{ > + if (!is_xen()) > + return; > + > + evtchn_init(NULL); > + > + /* if vector callback not supported, platform PCI driver will handle > that */ > + if (processor::features().xen_vector_callback) > + xen::xen_set_callback(); > + > + setup_xen_irq(NULL); > +} > + > extern "C" > void xen_init(struct start_info* si) > { > diff --git a/bsd/sys/xen/evtchn.h b/bsd/sys/xen/evtchn.h > index 0891a00..6d754b3 100644 > --- a/bsd/sys/xen/evtchn.h > +++ b/bsd/sys/xen/evtchn.h > @@ -32,6 +32,8 @@ void unmask_evtchn(int port); > > int evtchn_from_irq(int irq); > > +void evtchn_init(void *arg); > + > #ifdef SMP > void rebind_evtchn_to_cpu(int port, unsigned int cpu); > #else > diff --git a/core/xen_intr.cc b/core/xen_intr.cc > index 19131d3..685271e 100644 > --- a/core/xen_intr.cc > +++ b/core/xen_intr.cc > @@ -111,9 +111,11 @@ void xen_irq::_cpu_init(sched::cpu *c) > (*(_thread.for_cpu(c)))->start(); > } > > -xen_irq::xen_irq() > +xen_irq::xen_irq(interrupt *intr) > : _cpu_notifier([this] { cpu_init(); }) > { > + if (intr) > + _intr.reset(intr); > } > > static xen_irq *xen_irq_handlers; > @@ -129,13 +131,10 @@ bool xen_ack_irq() > return true; > } > > -static __attribute__((constructor)) void setup_xen_irq() > +void setup_xen_irq(interrupt *intr) > { > - if (!is_xen()) { > - return; > - } > - > - xen_irq_handlers = new xen_irq; > + assert(is_xen()); > + xen_irq_handlers = new xen_irq(intr); > } > } > > diff --git a/drivers/xenfront-xenbus.cc b/drivers/xenfront-xenbus.cc > index dbc0f00..3f6ab29 100644 > --- a/drivers/xenfront-xenbus.cc > +++ b/drivers/xenfront-xenbus.cc > @@ -26,7 +26,6 @@ > int xs_attach(struct device *); > > int xenpci_irq_init(device_t device, struct xenpci_softc *scp); > -void evtchn_init(void *arg); > > int xenbusb_front_probe(device_t dev); > int xenbusb_front_attach(device_t dev); > @@ -64,13 +63,7 @@ xenbus::xenbus(pci::device& pci_dev) > _dev.set_bus_master(true); > _driver_name = std::string("xenfront-xenbus"); > > - // From here on, all the OSV details are sorted, and we start the Xen > - // bringup > - evtchn_init(NULL); > - > - if (processor::features().xen_vector_callback) { > - xen::xen_set_callback(); > - } else { > + if (!processor::features().xen_vector_callback) { > _pgsi.reset(xen::xen_set_callback(irqno)); > } > > diff --git a/include/osv/xen.hh b/include/osv/xen.hh > index 504720b..3aa5d41 100644 > --- a/include/osv/xen.hh > +++ b/include/osv/xen.hh > @@ -16,6 +16,7 @@ > #include <xen/interface/version.h> > #include <xen/interface/hvm/hvm_op.h> > #include <osv/alternative.hh> > +#include <osv/interrupt.hh> > > extern char hypercall_page[]; > extern uint8_t xen_features[]; > @@ -53,6 +54,7 @@ extern struct xen_shared_info xen_shared_info; > void xen_set_callback(); > void xen_handle_irq(); > bool xen_ack_irq(); > +void setup_xen_irq(interrupt *intr); > > } > > diff --git a/include/osv/xen_intr.hh b/include/osv/xen_intr.hh > index d273615..1c8c6ca 100644 > --- a/include/osv/xen_intr.hh > +++ b/include/osv/xen_intr.hh > @@ -14,7 +14,7 @@ namespace xen { > > class xen_irq { > public: > - explicit xen_irq(); > + explicit xen_irq(interrupt *intr); > void wake() { (*_thread)->wake(); } > static void register_irq(int vector, driver_intr_t handler, void > *arg); > static void unregister_irq(int vector); > @@ -24,6 +24,8 @@ private: > sched::cpu::notifier _cpu_notifier; > void _cpu_init(sched::cpu *c); > > + std::unique_ptr<interrupt> _intr; > + > static percpu <sched::thread *> _thread; > }; > } > -- > 2.7.4 > > -- > You received this message because you are subscribed to the Google Groups > "OSv Development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
