Hi, Here I have doubts on whether this is the right approach -
If we have the pl011_console class and the xen_console class, why can't early_console simply being set to one of them, instead of needing to choose between them on every write()? On Fri, Jan 20, 2017 at 1:04 PM, 'Sergiy Kibrik' via OSv Development < [email protected]> wrote: > In case Xen is detected use xenconsole driver for output, otherwise > fallback to pl011 for backwards compatibility. > > Signed-off-by: Sergiy Kibrik <[email protected]> > --- > Makefile | 2 ++ > arch/aarch64/arch-setup.cc | 20 ++++++++++++++------ > arch/aarch64/early-console.cc | 22 ++++++++++++++++++++-- > arch/aarch64/early-console.hh | 24 ++++++++++++++++++++++-- > 4 files changed, 58 insertions(+), 10 deletions(-) > > diff --git a/Makefile b/Makefile > index 9d47ce3..2049fa7 100644 > --- a/Makefile > +++ b/Makefile > @@ -822,6 +822,7 @@ endif # x64 > > ifeq ($(arch),aarch64) > drivers += drivers/pl011.o > +drivers += drivers/xenconsole.o > drivers += drivers/virtio.o > drivers += drivers/virtio-vring.o > drivers += drivers/virtio-rng.o > @@ -859,6 +860,7 @@ objects += arch/$(arch)/arm-clock.o > objects += arch/$(arch)/gic.o > objects += arch/$(arch)/arch-dtb.o > objects += arch/$(arch)/xen.o > +objects += arch/$(arch)/hypercall.o > objects += $(libfdt) > endif > > diff --git a/arch/aarch64/arch-setup.cc b/arch/aarch64/arch-setup.cc > index 62d2295..070220e 100644 > --- a/arch/aarch64/arch-setup.cc > +++ b/arch/aarch64/arch-setup.cc > @@ -19,6 +19,7 @@ > > #include "arch-mmu.hh" > #include "arch-dtb.hh" > +#include "xen.hh" > > #include "drivers/console.hh" > #include "drivers/pl011.hh" > @@ -90,10 +91,12 @@ void arch_setup_free_memory() > mmu::linear_map((void *)mmu::mem_addr, (mmu::phys)mmu::mem_addr, > addr - mmu::mem_addr); > > - /* linear_map [TTBR0 - UART] */ > - addr = (mmu::phys)console::arch_early_console.get_base_addr(); > - mmu::linear_map((void *)addr, addr, 0x1000, mmu::page_size, > - mmu::mattr::dev); > + if (!xen::is_xen_found) { > Nitpick: I think it would be nicer if you use a function, e.g., is_xen() instead of a variable. If performance is very important (in some other user of is_xen()), you can make it an inline function. > + /* linear_map [TTBR0 - UART] */ > + addr = (mmu::phys)console::pl011_console.get_base_addr(); > + mmu::linear_map((void *)addr, addr, 0x1000, mmu::page_size, > + mmu::mattr::dev); > + } > > /* linear_map [TTBR0 - GIC DIST and GIC CPU] */ > u64 dist, cpu; > @@ -175,6 +178,9 @@ void arch_init_drivers() > > void arch_init_early_console() > { > + if (xen::is_xen_found) > + return; > + > int irqid; > u64 addr = dtb_get_uart(&irqid); > if (!addr) { > @@ -189,9 +195,11 @@ void arch_init_early_console() > bool arch_setup_console(std::string opt_console) > { > if (opt_console.compare("pl011") == 0) { > - console::console_driver_add(&console::arch_early_console); > + console::console_driver_add(&console::pl011_console); > + } else if (opt_console.compare("xen") == 0) { > + console::console_driver_add(&console::xen_console); > } else if (opt_console.compare("all") == 0) { > - console::console_driver_add(&console::arch_early_console); > + console::console_driver_add(&console::pl011_console); > I didn't understand why "all" defaults to pl011_console, even on Xen. "all" is supposed to write to all output devices if you have several (in normal x86, we have VGA and the serial port, and we want to write to both). Since I believe you only have one of the pl011 or xen devices at a time (?), would something like this work instead of all the above? if (opt_console.compare("pl011") == 0 || opt_console.compare("all") == 0) { console::console_driver_add(&console::arch_early_console); } Note that if I understand correctly, arch_early_console is already set to the right thing (xen or pl011) at that point. (Oh, on second reading, I see this isn't the case.. .More on this below). The if()s you did above allows someone to choose the "xen" console even when there is no Xen - that doesn't sound like a good idea. > } else { > return false; > } > diff --git a/arch/aarch64/early-console.cc b/arch/aarch64/early-console.cc > index 3014199..eda58f5 100644 > --- a/arch/aarch64/early-console.cc > +++ b/arch/aarch64/early-console.cc > @@ -5,11 +5,29 @@ > * BSD license as described in the LICENSE file in the top-level > directory. > */ > > -#include "drivers/pl011.hh" > #include <osv/prio.hh> > +#include <drivers/pl011.hh> > +#include <drivers/xenconsole.hh> > + > +#include "xen.hh" > +#include "early-console.hh" > > namespace console { > > -PL011_Console arch_early_console __attribute__((init_priority(( > int)init_prio::console))); > +PL011_Console pl011_console > + __attribute__((init_priority((int)init_prio::console))); > +XEN_Console xen_console > + __attribute__((init_priority((int)init_prio::console))); > + > +void AARCH64_Console::write(const char *str, size_t len) > +{ > + if (xen::is_xen_found) > + xen_console.write(str, len); > + else > + pl011_console.write(str, len); > +} > Why do we need to do this - create a new console type and check is_xen on every write? Couldn't we just put in early_console *either* xen_console or pl011_console? I think that was sort of the point of having the console type hierarchy. > + > +AARCH64_Console arch_early_console > + __attribute__((init_priority((int)init_prio::console))); > > } > diff --git a/arch/aarch64/early-console.hh b/arch/aarch64/early-console.hh > index 146ed53..614b0da 100644 > --- a/arch/aarch64/early-console.hh > +++ b/arch/aarch64/early-console.hh > @@ -8,11 +8,31 @@ > #ifndef EARLY_CONSOLE_HH > #define EARLY_CONSOLE_HH > > -#include "drivers/pl011.hh" > +#include <drivers/console-driver.hh> > +#include <drivers/pl011.hh> > +#include <drivers/xenconsole.hh> > > namespace console { > > -extern PL011_Console arch_early_console; > +class AARCH64_Console : public console_driver { > +public: > + virtual void write(const char *str, size_t len); > + virtual void flush() {} > + virtual bool input_ready() { return false; } > + virtual char readch() { return '\0'; } > + > + void set_base_addr(u64 addr) {} > + u64 get_base_addr() { return 0; } > + void set_irqid(int irqid) {} > + > +private: > + virtual void dev_start() {} > + virtual const char *thread_name() { return "aarch64-early"; } > +}; > + > +extern AARCH64_Console arch_early_console; > +extern PL011_Console pl011_console; > +extern XEN_Console xen_console; > > } > > -- > 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.
