[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



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?

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

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

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

> Thanks!

Regards,
Bhupinder

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

 


Rackspace

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