xen/console: Introduce a new console driver for Xen guest

[julien.grall_citrix.com]

Summary

The current Xen console driver is crashing very quickly when using it on ARM
guest. This is because of the console lock is recursive which may lead to
recurse on the tty lock and/or corrupt the ring pointer.

Furthermore, the console lock is not always taken where it should be and has
to be released too early because of the way the console has been designed.

Over the year, code has been added to support various new feature but the
driver has not been reworked. This brings to have code related to the
hypervisor console in ring specific function...

This new driver has been rewritten with this idea to only
have a small set of specific function to write either via the ring or the
hypercall.

Note that HVM support has been left aside for now because it requires external
feature to be used on ARM which are not yet upstreamed. A follow-up patch will
be sent with the ARM guest support.

List of items that may be good to have but not mandatory:

• Avoid to flush for each character written when using the tty
• Support multiple console

Test Plan

I've tested it on ARM (with the HVM support patch) but it should
work fine on x86 PVH given that most of the code is common.

[royger]

I have not yet tested the code on x86, but overall it looks good. Most of the comments are related to style(9).

sys/dev/xen/console/xen_console.c

102

"no matter what it costs" is more correct I think.

116

I would make those static inline functions rather than macros, so that we have the type checking.

123

In order to avoid this declaration I would define xencons_intr just after the Debug functions section, adding a proper "Interrupt handler" section.

204

Please avoid doing initializations with declarations, it's not FreeBSD coding style, see:

https://www.freebsd.org/cgi/man.cgi?query=style&sektion=9

(I'm not going to comment on each of the declaration + initialization style fixes).

209

Second level indents are always 4 spaces, see style(9).

210

Since err is not a boolean this should be err != 0.

211

device_printf instead of plain printf.

213

Parentheses around return code.

239

Since I think we want to get rid of the global console_page variable, I would be tempted to change this to:

cons->intf = (struct xencons_interface *)
    pmap_mapdev_attr(ptoa(HYPERVISOR_start_info->console.domU.mfn), PAGE_SIZE, VM_MEMATTR_DEFAULT);

That would work for x86, not sure if it's correct on ARM though.

254

INTR_TYPE_TTY rather than INTR_TYPE_MISC.

285

Double ";;"

293

No need to initialize sent, it's going to be initialized in the for loop below.

352

Why do you need to use a spin mutex? The interrupt handler is not a filter, so you are never going to execute code in interrupt context.

386

Instead of having this kind of conditionals, why not have function pointers in xencons_priv that are setup inside of xencons_early_init_*, and then this just becomes:

cons->read(...);

522

I would rename this functions (and the ones below) to xencons_* in order to keep name consistency with the rest of the file.

575

xencons_tty_open would be a better name IMHO.

579

This is a boolean, so:

cons->opened = true

728

That's fine for now, but in the future we should also look at being able to attach more than one console by hooking up into the xenbusb_front bus.

[julien.grall_citrix.com]

sys/dev/xen/console/xen_console.c

116

We need to keep CN_LOCK_ASSERT a macro to use the correct file/line in the error message. Otherwise we would always get the file/line of the helper.

I will move the rest inline.

123

xencons_intr relies on xencons_tx_flush and xencons_write. So it's not possible to move before in the current state.

I've added a new parameter to the helper xencons_init for the interrupt handler.

239

It's not that trivial.

pmap_mapdev_attr doesn't exist on ARM. If it ever exists it will map the memory with device attribute which means uncached + all the access to the page should be aligned. Although the Xen console interface must be mapped cached. I have a patch which introduce xen_pmap but it rely on the VM subsystem which is not yet initialised when the low-level console is initialised. I need to figure out if we have early mapping for ARM64.

Furthermore, HYPERVISOR_start_info is PV specific. For HVM, the page and the event channel should be retrieved via the HVM parameter. Note that the console driver is not supported for HVM for x86, so it's not present in this series.

So adding support for HVM and ARM will require more work. For instance, the start info page doesn't exists on ARM and the public interface doesn't export it (see XEN_HAVE_PV_GUEST_ENTRY). The way forward would be to drop any usage of HYPERVISOR_start_info in the common code and use an internal representation.

I'd like to postpone a such changes in a follow-up.

352

The previous driver was using a spin mutex and I didn't know that the interrupt handler will never be executed in interrupt context.

Although based on [1], sleepable lock should be avoided in interrupt handler to avoid starving the interrupt thread. Note that I'm not sure if it's still true given that many interrupt handler seems to use mutex.

[1] https://www.freebsd.org/doc/en_US.ISO8859-1/books/arch-handbook/smp-design.html

386

It was in my TODO list :). I though it wasn't not necessary for a first draft. It will be added in the next version.

728

That's in my TODO list :). I wanted to first rework the console driver with exactly the same features.

[julien.grall_citrix.com]

Major changes:

• Fix coding style
• Introduce a set of operations per type of console
• Uniformize the function name

[royger]

Thanks for the update. Just some minor comments.

Have you run a tinderbox with ARM and x86 at least in order to test it?

sys/dev/xen/console/xen_console.c

51

Can you split the xen includes from the system includes? Most Xen code places all the system includes, a newline, and then the xen specific headers (you already did this with the interface headers).

236

INTR_TYPE_TTY | INTR_MPSAFE or the handler is going to be run with the Giant lock held (which is not needed AFAICT).

240

Sure, no problem. armv6 seems to have pmap_mapdev_attr but not aarch64. I'm not sure which function is used on aarch64 in order to map memory in the early boot process (or if there's even a function). Anyway, we can solve that later.

289

Same here, INTR_TYPE_TTY | INTR_MPSAFE.

496

I would add brackets in both sides of the expression for clarity.

506

declaration + initialization.

732

Why is the interrupt handler passed as a parameter to the init function? AFAICT the interrupt handler is always the same.

[julien.grall_citrix.com]

sys/dev/xen/console/xen_console.c

732

I did that to avoid the forward declaration I had in the previous version.

[julien.grall_citrix.com]

Major changes:

• Add INTR_MPSAFE when mapping the event channel
• Split xen include from the system includes
• Check if the console is full before getting the characters from the tty (see xencons_getc)

[julien.grall_citrix.com]

Fix coding style

[royger]

I have some minor style(9) comments, if you want I can fix them while committing, but it LGTM. I'm going to give this a spin on x86.

sys/dev/xen/console/xen_console.c

43

This should also be separated from the rest of the includes, just like xen specific includes.

202

I've forgot to mention it, but according to style(9), functions that don't have local variables should start with an empty newline. Sorry for not realizing this earlier, I often miss this too.

246

Same here, there should be an empty newline before the function call.