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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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.

Heya!

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.
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.


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.
   
 - 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.

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

 - 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?

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

Thanks!

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel