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

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

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

2.
> 
> > 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
> Xen/toolstack/console?

The CONFIG flag are only for hypervisor. I would add it as a
CONFIG
> 
> >
> > 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?

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

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

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

Why not emulate how pl011 works?

> 
> > Thanks!
> 
> Regards,
> Bhupinder

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