On 03/05/2017 11:08 AM, Nadav Har'El wrote:
On Fri, Mar 3, 2017 at 11:45 AM, 'Sergiy Kibrik' via OSv Development
<osv-dev@googlegroups.com <mailto:osv-dev@googlegroups.com>> 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.
hi Nadav,
I agree that it would be much clearer (and should be more robust also) to
explicitly specify priority level.
As this code expects interrupt_table to be already up, I'll put it into level
idt + 1 like this:
static __attribute__((init_priority((int)init_prio::idt + 1))) void
xen_init_irq()
Additional minor comments below.
Signed-off-by: Sergiy Kibrik <sergiy.kib...@globallogic.com
<mailto:sergiy.kib...@globallogic.com>>
---
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.
I'll just remove this check then.
--
regards,
Sergiy
--
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 osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
