[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 1/1] xen/arm: Add pl011 uart support in Xen for guest domains


On 15/02/17 15:38, Konrad Rzeszutek Wilk wrote:
On Wed, Feb 15, 2017 at 12:53:34PM +0530, Bhupinder Thakur wrote:
Hi Konrad,

Thanks for the feedback.

On 7 February 2017 at 00:05, Konrad Rzeszutek Wilk
<konrad.wilk@xxxxxxxxxx> wrote:
On Mon, Feb 06, 2017 at 11:39:08PM +0530, Bhupinder Thakur wrote:
As per "VM System Specification for  ARM Processors", there is a requirement 
for Xen to support guest console
over pl011 UART, which is SBSA compliant. The changes in this patch implement 
the pl011 emulation in Xen
and a new pl011 console support in xenconsoled.


Got a couple of pointers for this RFC patch.

This patch should be broken up. That is the first patch
should be the one that brings in the HVM_PARAM changes along
with documentation on how this would work on non-ARM systems.

Since this feature is ARM specific, there are two options:

1. Define the HVM params only for ARM and keep the code which is using
these HVM params in the toolstack/xenconsoled under __arm__ flag
2. Define the HVM params independent of the architecture but
essentially return 0/NULL on get of these HVM params for x86
architecture. The code which is using these HVM params can then check
for the returned value and take the action accordingly. In this case,
the code is architecture agnostic.

Which is the preferred option?


We already have some HVM param that are x86 specific (see HVM_PARAM_VIRIDIAN) or interpreted differently whether Xen is compiled for ARM or x86 (see HVM_PARAM_CALLBACK_IRQ).

So I would keep the new HVM params ARM specific and enclosed in #ifdef.

The second patch would implement this in the generic
code (in xen/common/event_channel.c) - perhaps via an
secondary function that is NOP on x86 but not so on ARM?

Then another patch that fleshes out the emulation code in
the hypervisor, then the one in console code, and lastly
in libxl to turn this on/off.

Is it preferable to keep the pl011 emulation feature under its own
feature CONFIG flag so that it can be compiled off across

The CONFIG flag are only for hypervisor. I would add it as a

What do you mean by the second CONFIG? Is it the one in config/*.mk?

From a short glance I would recommend you also:
 - Include a doc which explains how pl011 UART works,
   or at least a link.

 - Remove the #if 0

 - Rip out the debug printk code.

 - Fix the tab/spaces alignment to match the code

 - Don't hardcode paths. They should be gathered from
   envionment variables (like the rest of xenconsoled does)

 - If you remove the VM_PARAM_MEMORY_EVENT_ you also need to
   rev up the version field.

I will incorporate these review comments.

 - Include a knob in libxl to define whether the guest has
   this emulation enabled or not. And if it is disabled
   then the code in hypervisor should not emulate it.

Since the guest  is unaware whether it is using an emulated pl011, is

How is it unaware? How did this work before?

In normal condition, a guest should not assume a specific set of devices available. It should discover them using the firmware table (ACPI or DT).

Furthermore, I agree with Konrad about adding a knob in libxl to turn on/off the PL011 available. We want to let the user choosing and it will likely be disabled by default at the beginning.

it required to have a knob to enable/disable this feature? I am
planning to add
a new console type "pl011" in addition to "pv" and "serial" and user
can connect to any of those consoles. So a guest, which has let us say
both HVC and pl011 consoles enabled, user can connect to any of the

What happens if a guest tries to use 'pl011' on a hypervisor
that does not have this?

It will receive a data abort if it is accessing a memory region not emulated or with underlying physical memory.

 - Return code for MMIO shouldn't be 1, but rather the proper

 - The vpl011_cons_intf_s looks very weird. It looks like it
   is missing an design document as well? That is should there
   be a header in include/xen/public/ file?

The design was discussed in the thread "Xen ARM - Exposing a PL011 to the guest" <86800697-5057-3f14-c19f-151e81315133@xxxxxxx>. I do agree a summary of the discussion would have been useful here. Bhupinder, can you add one in the next version?

   Should vpl011.h be in include/xen/public/ ? If so you need
   a different license for that file.

I will try to move this header file in the same folder where vpl011.c is.

Is the ring protocol suppose to be implemented by the Linux kernel? If so
it MUST be in the public/io directory.

And why does it have to be aring protocol? Why not whatever the pl011 

Why not emulate how pl011 works?

The ring protocol is between Xen (where the pl011 is emulated) and the console backend. The guest will use a normal pl011 driver and no Xen header should be required there.

For the protocol between Xen and the console backend, the ring was a natural choice because this is how works PV console. So there is no heavy change needed in the backend.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.