On 01/22/2017 03:26 PM, Nadav Har'El wrote:
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()?
I wasn't sure how to do it correctly.
arch_early_console is of abstract class console_driver, so can't be
instantiated.
I could use console_driver *arch_early_console, but in this case had to correct
all the usage throughout the code to use pointer, plus point it to either
PL011_Console or XEN_Console object.
But this objects are created at "console" init phase, while early console must
be available before inittab gets executed.
But anyway, thank for your comment, I'll try to come up with smth. better.
[..]
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.
Agree, the better is to do as x64 code already does.
+ /* 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.
Agree, that's a good point. Probably if xen is detected the console option
should be ignored altogether, probably with a warning.
} 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.
I'll try to find out how to implement it better in C++
--
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
