diff mbox series

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

Message ID 1486404548-15062-2-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series xen/arm: Add pl011 uart support in Xen for guest domains | expand

Commit Message

Bhupinder Thakur Feb. 6, 2017, 6:09 p.m. UTC
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.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
 tools/console/daemon/io.c       | 170 ++++++++++++++---
 tools/console/daemon/main.c     |  10 +
 tools/libxc/include/xc_dom.h    |   3 +
 tools/libxc/xc_dom_arm.c        |   7 +-
 tools/libxc/xc_dom_boot.c       |   3 +
 tools/libxc/xc_domain.c         |   3 -
 tools/libxl/libxl.c             |   4 +
 tools/libxl/libxl_arm.c         |  50 ++++-
 tools/libxl/libxl_dom.c         |   9 +-
 tools/libxl/libxl_internal.h    |   2 +
 xen/arch/arm/Makefile           |   1 +
 xen/arch/arm/domain.c           |   7 +
 xen/arch/arm/hvm.c              |  22 ++-
 xen/arch/arm/setup.c            |   1 +
 xen/arch/arm/vgic.c             |   7 +
 xen/arch/arm/vpl011.c           | 391 ++++++++++++++++++++++++++++++++++++++++
 xen/common/event_channel.c      |  11 +-
 xen/include/asm-arm/domain.h    |  14 ++
 xen/include/public/arch-arm.h   |   5 +
 xen/include/public/hvm/params.h |  10 +-
 xen/include/xen/vpl011.h        | 146 +++++++++++++++
 21 files changed, 832 insertions(+), 44 deletions(-)
 create mode 100644 xen/arch/arm/vpl011.c
 create mode 100644 xen/include/xen/vpl011.h

Comments

Julien Grall Feb. 6, 2017, 6:30 p.m. UTC | #1
Hi Bhupinder,

Thank you for the patch.

On 06/02/17 18:09, 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.
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  tools/console/daemon/io.c       | 170 ++++++++++++++---
>  tools/console/daemon/main.c     |  10 +
>  tools/libxc/include/xc_dom.h    |   3 +
>  tools/libxc/xc_dom_arm.c        |   7 +-
>  tools/libxc/xc_dom_boot.c       |   3 +
>  tools/libxc/xc_domain.c         |   3 -
>  tools/libxl/libxl.c             |   4 +
>  tools/libxl/libxl_arm.c         |  50 ++++-
>  tools/libxl/libxl_dom.c         |   9 +-
>  tools/libxl/libxl_internal.h    |   2 +
>  xen/arch/arm/Makefile           |   1 +
>  xen/arch/arm/domain.c           |   7 +
>  xen/arch/arm/hvm.c              |  22 ++-
>  xen/arch/arm/setup.c            |   1 +
>  xen/arch/arm/vgic.c             |   7 +
>  xen/arch/arm/vpl011.c           | 391 ++++++++++++++++++++++++++++++++++++++++
>  xen/common/event_channel.c      |  11 +-
>  xen/include/asm-arm/domain.h    |  14 ++
>  xen/include/public/arch-arm.h   |   5 +
>  xen/include/public/hvm/params.h |  10 +-
>  xen/include/xen/vpl011.h        | 146 +++++++++++++++
>  21 files changed, 832 insertions(+), 44 deletions(-)

A such big patch should be broke down in multiple small patches to ease 
the review. I will wait the next version before doing any review.

I would recommend to read on Xen wiki the following page:
https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches

It will give you advice on how to prepare a patch series.

Cheers,
Konrad Rzeszutek Wilk Feb. 6, 2017, 6:35 p.m. UTC | #2
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!
Bhupinder Thakur Feb. 15, 2017, 7:23 a.m. UTC | #3
Hi Konrad,

Thanks for the feedback.

On 7 February 2017 at 00:05, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> 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
Konrad Rzeszutek Wilk Feb. 15, 2017, 3:38 p.m. UTC | #4
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@oracle.com> 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
Julien Grall Feb. 16, 2017, 2:34 p.m. UTC | #5
Hi,

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@oracle.com> 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.

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

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

The design was discussed in the thread "Xen ARM - Exposing a PL011 to 
the guest" <86800697-5057-3f14-c19f-151e81315133@arm.com>. 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 implements?
>
> 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.

Cheers,
Bhupinder Thakur Feb. 17, 2017, 12:35 p.m. UTC | #6
Hi,

On 16 February 2017 at 20:04, Julien Grall <julien.grall@arm.com> wrote:
> Hi,
>
>
> 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@oracle.com> 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.
>
>
> 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.
>
Ok. I have put the code accessing these HVM params under arm specific
#ifdef flag.

>>>
>>>> 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
>
>
> What do you mean by the second CONFIG? Is it the one in config/*.mk?
>
In the Xen hypervisor, the pl011 emulation code would be under a
specific CONFIG flag which would be defined only while compiling for
ARM. For x86, the emulation code will be compiled out.
>>>
>>>>
>>>> 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.
>
Ok. I will add a run-time knob (may be controlled though domU config
file) to enable/disable pl011 emulation.

>>
>>> 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?
>
>
> 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
>>>>    #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?
>
>
> The design was discussed in the thread "Xen ARM - Exposing a PL011 to the
> guest" <86800697-5057-3f14-c19f-151e81315133@arm.com>. I do agree a summary
> of the discussion would have been useful here. Bhupinder, can you add one in
> the next version?
>
Yes, I will add a design doc while sending the patch.

>>>>
>>>>    Should vpl011.h be in include/xen/public/ ? If so you need
>>>>    a different license for that file.
>>>>
I have moved the file from the public folder and keeping it in xen/arch/arm/

>>> 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?
>
>
> 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.
>
> Cheers,
>
> --
> Julien Grall
Konrad Rzeszutek Wilk Feb. 17, 2017, 3:27 p.m. UTC | #7
On Thu, Feb 16, 2017 at 02:34:30PM +0000, Julien Grall wrote:
> Hi,
> 
> 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@oracle.com> 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.
> 
> 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.

But why couldn't you have pl101 on x86? It is a just a device
driver?
> 
> > > 
> > > > 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
> 
> What do you mean by the second CONFIG? Is it the one in config/*.mk?

In the hypervisor (xen/) Kconfig files.

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

Pfff. I am thinking about naughty guests that ignore such
things (think: exploits)).

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

Ah right. Good, as long as that works properly.
> 
> > 
> > > 
> > > >  - 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?
> 
> The design was discussed in the thread "Xen ARM - Exposing a PL011 to the
> guest" <86800697-5057-3f14-c19f-151e81315133@arm.com>. 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 implements?
> > 
> > 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

'console backend' can be some OS (MiniOS)? So we still need an
Xen public header I think?

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

> 
> Cheers,
> 
> -- 
> Julien Grall
Konrad Rzeszutek Wilk Feb. 17, 2017, 3:29 p.m. UTC | #8
> >>>>    Should vpl011.h be in include/xen/public/ ? If so you need
> >>>>    a different license for that file.
> >>>>
> I have moved the file from the public folder and keeping it in xen/arch/arm/

Huh? But if this is a ring protocol that is used by an OS that is not
part of of Xen tree it needs to be in public/io/ location.

Otherwise you may run in problems with different licenses (public/io
has BSD license while arch/arm is GPL) - and it may be that your
'console backend' is proprietary.
Bhupinder Thakur Feb. 20, 2017, 3:23 p.m. UTC | #9
Hi,


On 17 February 2017 at 20:59, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>> >>>>    Should vpl011.h be in include/xen/public/ ? If so you need
>> >>>>    a different license for that file.
>> >>>>
>> I have moved the file from the public folder and keeping it in xen/arch/arm/
>
> Huh? But if this is a ring protocol that is used by an OS that is not
> part of of Xen tree it needs to be in public/io/ location.
>
> Otherwise you may run in problems with different licenses (public/io
> has BSD license while arch/arm is GPL) - and it may be that your
> 'console backend' is proprietary.

xen/include/public/console/io.h contains the definition of the ring
structure (xencons_interface) which is used by the xenconsoled running
on the dom0 OS. The definitions in vpl011.h are used locally by the
emulation code in Xen including one ring structure which is defined
same as the one defined in the public folder.

Regards,
Bhupinder
Julien Grall Feb. 26, 2017, 8:50 p.m. UTC | #10
Hi Konrad,

On 02/17/2017 03:27 PM, Konrad Rzeszutek Wilk wrote:
> On Thu, Feb 16, 2017 at 02:34:30PM +0000, Julien Grall wrote:
>> On 15/02/17 15:38, Konrad Rzeszutek Wilk wrote:
>>> On Wed, Feb 15, 2017 at 12:53:34PM +0530, Bhupinder Thakur wrote:
>> 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.
>
> But why couldn't you have pl101 on x86? It is a just a device
> driver?

I assumed that because it is an ARM IP it will not be used on x86. But 
maybe I am wrong here.

>>
>>>>
>>>>> 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
>>
>> What do you mean by the second CONFIG? Is it the one in config/*.mk?
>
> In the hypervisor (xen/) Kconfig files.

I don't think it hurts to get the pl011 built-in in Xen as it would be 
disabled by default for the guest.

Also, I am not comfortable yet another Kconfig option on ARM until we 
get an embedded .config in Xen. Without that it makes harder to know 
what has been enabled or not by the user.


[...]

>>>>>
>>>>>    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?
>>
>> 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
>
> 'console backend' can be some OS (MiniOS)? So we still need an
> Xen public header I think?

Hmmm. I agree for the ring, but anything related to the emulation of the 
PL011 itself should stay in include/{xen,asm-arm}/

However, the pl011 ring is similar to the console ring. So maybe we 
should re-use the console header rather than re-inventing the wheel.

Cheers,
Konrad Rzeszutek Wilk Feb. 28, 2017, 9:21 p.m. UTC | #11
On Mon, Feb 20, 2017 at 08:53:50PM +0530, Bhupinder Thakur wrote:
> Hi,
> 
> 
> On 17 February 2017 at 20:59, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> >> >>>>    Should vpl011.h be in include/xen/public/ ? If so you need
> >> >>>>    a different license for that file.
> >> >>>>
> >> I have moved the file from the public folder and keeping it in xen/arch/arm/
> >
> > Huh? But if this is a ring protocol that is used by an OS that is not
> > part of of Xen tree it needs to be in public/io/ location.
> >
> > Otherwise you may run in problems with different licenses (public/io
> > has BSD license while arch/arm is GPL) - and it may be that your
> > 'console backend' is proprietary.
> 
> xen/include/public/console/io.h contains the definition of the ring
> structure (xencons_interface) which is used by the xenconsoled running
> on the dom0 OS. The definitions in vpl011.h are used locally by the

..and also in the Linux code - as the PV frontend.

> emulation code in Xen including one ring structure which is defined
> same as the one defined in the public folder.

.. Aaah. Then it should still be accessible from the xenconsold and to
be in the common code.
> 
> Regards,
> Bhupinder
Konrad Rzeszutek Wilk Feb. 28, 2017, 9:22 p.m. UTC | #12
> However, the pl011 ring is similar to the console ring. So maybe we should
> re-use the console header rather than re-inventing the wheel.

That would be an excellent approach.
> 
> Cheers,
> 
> -- 
> Julien Grall
Bhupinder Thakur March 1, 2017, 10:51 a.m. UTC | #13
Hi Julien,

On 1 March 2017 at 02:52, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> However, the pl011 ring is similar to the console ring. So maybe we should
>> re-use the console header rather than re-inventing the wheel.
>
Can I include a xen public header file
(xen/include/public/io/console.h) in the xen code?

> That would be an excellent approach.
>>
>> Cheers,
>>
>> --
>> Julien Grall
Julien Grall March 1, 2017, 4:44 p.m. UTC | #14
On 01/03/17 10:51, Bhupinder Thakur wrote:
> Hi Julien,

Hi Bhupinder,

> On 1 March 2017 at 02:52, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>>> However, the pl011 ring is similar to the console ring. So maybe we should
>>> re-use the console header rather than re-inventing the wheel.
>>
> Can I include a xen public header file
> (xen/include/public/io/console.h) in the xen code?

Yes, you can include them in xen code.

Cheers,
diff mbox series

Patch

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 7e6a886..8ea1c13 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -68,6 +68,8 @@  extern int log_time_hv;
 extern int log_time_guest;
 extern char *log_dir;
 extern int discard_overflowed_data;
+extern int debug_fd;
+extern bool vpl011_console;
 
 static int log_time_hv_needts = 1;
 static int log_time_guest_needts = 1;
@@ -101,11 +103,15 @@  struct domain {
 	struct domain *next;
 	char *conspath;
 	int ring_ref;
+	int vpl011_ring_ref;
 	xenevtchn_port_or_error_t local_port;
 	xenevtchn_port_or_error_t remote_port;
+	xenevtchn_port_or_error_t vpl011_local_port;
+	xenevtchn_port_or_error_t vpl011_remote_port;
 	xenevtchn_handle *xce_handle;
 	int xce_pollfd_idx;
 	struct xencons_interface *interface;
+	struct xencons_interface *vpl011_interface;
 	int event_count;
 	long long next_period;
 };
@@ -158,11 +164,11 @@  static int write_with_timestamp(int fd, const char *data, size_t sz,
 	return 0;
 }
 
-static void buffer_append(struct domain *dom)
+static void buffer_append(struct domain *dom, struct xencons_interface *intf, int port)
 {
 	struct buffer *buffer = &dom->buffer;
 	XENCONS_RING_IDX cons, prod, size;
-	struct xencons_interface *intf = dom->interface;
+    char debug_buf[80];
 
 	cons = intf->out_cons;
 	prod = intf->out_prod;
@@ -187,7 +193,10 @@  static void buffer_append(struct domain *dom)
 
 	xen_mb();
 	intf->out_cons = cons;
-	xenevtchn_notify(dom->xce_handle, dom->local_port);
+    snprintf(debug_buf, 79, "cons=%d, prod=%d\n", cons, prod);
+    write (debug_fd, debug_buf, strlen(debug_buf));
+
+	xenevtchn_notify(dom->xce_handle, port);
 
 	/* Get the data to the logfile as early as possible because if
 	 * no one is listening on the console pty then it will fill up
@@ -529,14 +538,65 @@  static void domain_unmap_interface(struct domain *dom)
 	dom->ring_ref = -1;
 }
  
+static void domain_unmap_vpl011_interface(struct domain *dom)
+{
+	if ( dom->vpl011_interface == NULL )
+		return;
+	if ( xgt_handle && dom->vpl011_ring_ref == -1 )
+		xengnttab_unmap(xgt_handle, dom->vpl011_interface, 1);
+	else
+		munmap(dom->vpl011_interface, XC_PAGE_SIZE);
+	dom->vpl011_interface = NULL;
+	dom->vpl011_ring_ref = -1;
+}
+
+int bind_event_channel(struct domain *dom, int new_rport, int *lport, int *rport)
+{
+    int err = 0, rc;
+
+	/* Go no further if port has not changed and we are still bound. */
+	if ( new_rport == *rport ) {
+		xc_evtchn_status_t status = {
+			.dom = DOMID_SELF,
+			.port = *lport };
+		if ((xc_evtchn_status(xc, &status) == 0) &&
+		    (status.status == EVTCHNSTAT_interdomain))
+            goto out;
+	}
+
+    /* initialize the ports */
+	*lport = -1;
+	*rport = -1;
+ 
+    /* bind to new remote port */
+	rc = xenevtchn_bind_interdomain(dom->xce_handle,
+		dom->domid, new_rport);
+
+	if ( rc == -1 ) {
+		err = errno;
+		xenevtchn_close(dom->xce_handle);
+		dom->xce_handle = NULL;
+		goto out;
+	}
+
+    /* store new local and remote event channel ports */
+	*lport = rc;
+	*rport = new_rport;
+
+out:
+    return err;
+}
+
 static int domain_create_ring(struct domain *dom)
 {
-	int err, remote_port, ring_ref, rc;
+	int err, remote_port, ring_ref, vpl011_remote_port, vpl011_ring_ref;
 	char *type, path[PATH_MAX];
 
 	err = xs_gather(xs, dom->conspath,
 			"ring-ref", "%u", &ring_ref,
 			"port", "%i", &remote_port,
+			"vpl011-ring-ref", "%u", &vpl011_ring_ref,
+            "vpl011-port", "%i", &vpl011_remote_port,
 			NULL);
 	if (err)
 		goto out;
@@ -553,6 +613,10 @@  static int domain_create_ring(struct domain *dom)
 	if (ring_ref != dom->ring_ref && dom->ring_ref != -1)
 		domain_unmap_interface(dom);
 
+	/* If using vpl011 ring_ref and it has changed, remap */
+	if ( vpl011_ring_ref != dom->vpl011_ring_ref && dom->vpl011_ring_ref != -1 )
+		domain_unmap_vpl011_interface(dom);
+
 	if (!dom->interface && xgt_handle) {
 		/* Prefer using grant table */
 		dom->interface = xengnttab_map_grant_ref(xgt_handle,
@@ -560,6 +624,8 @@  static int domain_create_ring(struct domain *dom)
 			PROT_READ|PROT_WRITE);
 		dom->ring_ref = -1;
 	}
+
+    /* map PV console ring buffer */
 	if (!dom->interface) {
 		/* Fall back to xc_map_foreign_range */
 		dom->interface = xc_map_foreign_range(
@@ -573,18 +639,20 @@  static int domain_create_ring(struct domain *dom)
 		dom->ring_ref = ring_ref;
 	}
 
-	/* Go no further if port has not changed and we are still bound. */
-	if (remote_port == dom->remote_port) {
-		xc_evtchn_status_t status = {
-			.dom = DOMID_SELF,
-			.port = dom->local_port };
-		if ((xc_evtchn_status(xc, &status) == 0) &&
-		    (status.status == EVTCHNSTAT_interdomain))
+    /* map vpl011 console ring buffer */
+	if ( !dom->vpl011_interface ) {
+		/* Fall back to xc_map_foreign_range */
+		dom->vpl011_interface = xc_map_foreign_range(
+			xc, dom->domid, XC_PAGE_SIZE,
+			PROT_READ|PROT_WRITE,
+			(unsigned long)vpl011_ring_ref);
+		if ( dom->vpl011_interface == NULL ) {
+			err = EINVAL;
 			goto out;
+		}
+		dom->vpl011_ring_ref = vpl011_ring_ref;
 	}
 
-	dom->local_port = -1;
-	dom->remote_port = -1;
 	if (dom->xce_handle != NULL)
 		xenevtchn_close(dom->xce_handle);
 
@@ -595,18 +663,22 @@  static int domain_create_ring(struct domain *dom)
 		err = errno;
 		goto out;
 	}
- 
-	rc = xenevtchn_bind_interdomain(dom->xce_handle,
-		dom->domid, remote_port);
 
-	if (rc == -1) {
-		err = errno;
+    /* bind PV console channel */
+    err = bind_event_channel(dom, remote_port, &dom->local_port, &dom->remote_port);
+    if (err)
+    {
 		xenevtchn_close(dom->xce_handle);
-		dom->xce_handle = NULL;
-		goto out;
-	}
-	dom->local_port = rc;
-	dom->remote_port = remote_port;
+        goto out;
+    }
+
+    /* bind vpl011 console channel */
+    err = bind_event_channel(dom, vpl011_remote_port, &dom->vpl011_local_port, &dom->vpl011_remote_port);
+    if (err)
+    {
+		xenevtchn_close(dom->xce_handle);
+        goto out;
+    }
 
 	if (dom->master_fd == -1) {
 		if (!domain_create_tty(dom)) {
@@ -615,6 +687,8 @@  static int domain_create_ring(struct domain *dom)
 			dom->xce_handle = NULL;
 			dom->local_port = -1;
 			dom->remote_port = -1;
+			dom->vpl011_local_port = -1;
+			dom->vpl011_remote_port = -1;
 			goto out;
 		}
 	}
@@ -812,8 +886,9 @@  static void handle_tty_read(struct domain *dom)
 	ssize_t len = 0;
 	char msg[80];
 	int i;
-	struct xencons_interface *intf = dom->interface;
+	struct xencons_interface *intf;
 	XENCONS_RING_IDX prod;
+    xenevtchn_port_or_error_t port;
 
 	if (dom->is_dead)
 		return;
@@ -826,6 +901,21 @@  static void handle_tty_read(struct domain *dom)
 		len = sizeof(msg);
 
 	len = read(dom->master_fd, msg, len);
+
+    /* select the interface based on whether vpl011 console is 
+     * enabled or not
+     */
+    if ( vpl011_console )
+    {
+        intf = dom->vpl011_interface;
+        port = dom->vpl011_local_port;
+    }
+    else
+    {
+        intf = dom->interface;
+        port = dom->local_port;
+    }
+
 	/*
 	 * Note: on Solaris, len == 0 means the slave closed, and this
 	 * is no problem, but Linux can't handle this usefully, so we
@@ -835,13 +925,14 @@  static void handle_tty_read(struct domain *dom)
 		domain_handle_broken_tty(dom, domain_is_valid(dom->domid));
 	} else if (domain_is_valid(dom->domid)) {
 		prod = intf->in_prod;
+
 		for (i = 0; i < len; i++) {
 			intf->in[MASK_XENCONS_IDX(prod++, intf->in)] =
 				msg[i];
 		}
 		xen_wmb();
 		intf->in_prod = prod;
-		xenevtchn_notify(dom->xce_handle, dom->local_port);
+		xenevtchn_notify(dom->xce_handle, port);
 	} else {
 		domain_close_tty(dom);
 		shutdown_domain(dom);
@@ -869,6 +960,8 @@  static void handle_tty_write(struct domain *dom)
 static void handle_ring_read(struct domain *dom)
 {
 	xenevtchn_port_or_error_t port;
+    char debug_buf[80];
+    struct xencons_interface *intf;
 
 	if (dom->is_dead)
 		return;
@@ -876,12 +969,28 @@  static void handle_ring_read(struct domain *dom)
 	if ((port = xenevtchn_pending(dom->xce_handle)) == -1)
 		return;
 
-	dom->event_count++;
 
-	buffer_append(dom);
+    dom->event_count++;
+
+    /*
+     * select the interface based on the port which the event received
+     */
+    if ( port == dom->vpl011_local_port )
+    {
+        intf = dom->vpl011_interface;
+    }
+    else
+    {
+        intf = dom->interface;
+    }
+
+    snprintf (debug_buf, 79, "port=%d\n", port);
+    write (debug_fd, debug_buf, strlen(debug_buf));
+
+    buffer_append(dom, intf, port);
 
-	if (dom->event_count < RATE_LIMIT_ALLOWANCE)
-		(void)xenevtchn_unmask(dom->xce_handle, port);
+    if (dom->event_count < RATE_LIMIT_ALLOWANCE)
+        (void)xenevtchn_unmask(dom->xce_handle, port);
 }
 
 static void handle_xs(void)
@@ -1068,7 +1177,8 @@  void handle_io(void)
 			if ((now+5) > d->next_period) {
 				d->next_period = now + RATE_LIMIT_PERIOD;
 				if (d->event_count >= RATE_LIMIT_ALLOWANCE) {
-					(void)xenevtchn_unmask(d->xce_handle, d->local_port);
+					    (void)xenevtchn_unmask(d->xce_handle, d->local_port);
+					    (void)xenevtchn_unmask(d->xce_handle, d->vpl011_local_port);
 				}
 				d->event_count = 0;
 			}
diff --git a/tools/console/daemon/main.c b/tools/console/daemon/main.c
index 806d2fd..3d3e061 100644
--- a/tools/console/daemon/main.c
+++ b/tools/console/daemon/main.c
@@ -21,6 +21,7 @@ 
 #include <stdlib.h>
 #include <stdio.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <unistd.h>
 #include <string.h>
 #include <signal.h>
@@ -41,6 +42,13 @@  int log_time_guest = 0;
 char *log_dir = NULL;
 int discard_overflowed_data = 1;
 
+int debug_fd;
+/*
+ * For now pl011 console is enabled by default.
+ * Later it would be exported out as a user configurable option
+ */
+bool vpl011_console=true;
+
 static void handle_hup(int sig)
 {
         log_reload = 1;
@@ -190,6 +198,8 @@  int main(int argc, char **argv)
 	openlog("xenconsoled", syslog_option, LOG_DAEMON);
 	setlogmask(syslog_mask);
 
+	debug_fd = open("/var/log/xen/debug.log", O_WRONLY|O_CREAT|O_APPEND, 0644);
+
 	increase_fd_limit();
 
 	if (!is_interactive) {
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 608cbc2..7454ba9 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -218,6 +218,9 @@  struct xc_dom_image {
 
     /* Extra SMBIOS structures passed to HVMLOADER */
     struct xc_hvm_firmware_module smbios_module;
+
+    xen_pfn_t vpl011_console_pfn;
+    unsigned int vpl011_console_evtchn;
 };
 
 /* --- pluggable kernel loader ------------------------------------- */
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index a7e839e..7060a22 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -26,10 +26,11 @@ 
 #include "xg_private.h"
 #include "xc_dom.h"
 
-#define NR_MAGIC_PAGES 3
+#define NR_MAGIC_PAGES 4
 #define CONSOLE_PFN_OFFSET 0
 #define XENSTORE_PFN_OFFSET 1
 #define MEMACCESS_PFN_OFFSET 2
+#define VPL011_CONSOLE_PFN_OFFSET 3
 
 #define LPAE_SHIFT 9
 
@@ -85,6 +86,7 @@  static int alloc_magic_pages(struct xc_dom_image *dom)
 
     dom->console_pfn = base + CONSOLE_PFN_OFFSET;
     dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
+    dom->vpl011_console_pfn = base + VPL011_CONSOLE_PFN_OFFSET;
 
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
@@ -95,6 +97,9 @@  static int alloc_magic_pages(struct xc_dom_image *dom)
             dom->xenstore_pfn);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
             base + MEMACCESS_PFN_OFFSET);
+    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_VPL011_CONSOLE_PFN,
+            base + VPL011_CONSOLE_PFN_OFFSET);
+
     /* allocated by toolstack */
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_EVTCHN,
             dom->console_evtchn);
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index 791041b..3420a08 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -227,6 +227,9 @@  int xc_dom_boot_image(struct xc_dom_image *dom)
     if ( (rc = clear_page(dom, dom->xenstore_pfn)) != 0 )
         return rc;
 
+    if ( (rc = clear_page(dom, dom->vpl011_console_pfn)) != 0 )
+        return rc;
+
     /* start info page */
     if ( dom->arch_hooks->start_info )
         dom->arch_hooks->start_info(dom);
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index fa1daeb..e330738 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1337,9 +1337,6 @@  static inline int xc_hvm_param_deprecated_check(uint32_t param)
 {
     switch ( param )
     {
-        case HVM_PARAM_MEMORY_EVENT_CR0:
-        case HVM_PARAM_MEMORY_EVENT_CR3:
-        case HVM_PARAM_MEMORY_EVENT_CR4:
         case HVM_PARAM_MEMORY_EVENT_INT3:
         case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
         case HVM_PARAM_MEMORY_EVENT_MSR:
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d400fa2..4acd8c5 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3159,6 +3159,10 @@  int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
         flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
         flexarray_append(ro_front, "ring-ref");
         flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
+        flexarray_append(ro_front, "vpl011-port");
+        flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vpl011_console_port));
+        flexarray_append(ro_front, "vpl011-ring-ref");
+        flexarray_append(ro_front, GCSPRINTF("%lu", state->vpl011_console_mfn));
     } else {
         flexarray_append(front, "state");
         flexarray_append(front, GCSPRINTF("%d", XenbusStateInitialising));
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index d842d88..12eb2b2 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -130,9 +130,10 @@  static struct arch_info {
     const char *guest_type;
     const char *timer_compat;
     const char *cpu_compat;
+    const char *uart_compat;
 } arch_info[] = {
-    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15" },
-    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" },
+    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" },
+    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" },
 };
 
 /*
@@ -590,6 +591,48 @@  static int make_hypervisor_node(libxl__gc *gc, void *fdt,
     return 0;
 }
 
+static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
+                           const struct arch_info *ainfo,
+                           struct xc_dom_image *dom)
+{
+    int res;
+    gic_interrupt intr;
+    uint64_t    vpl011_irq;
+
+    res = fdt_begin_node(fdt, "sbsa-pl011");
+    if (res) return res;
+
+    res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat);
+    if (res) return res;
+
+    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
+                            1,
+                            GUEST_PL011_BASE, GUEST_PL011_SIZE);
+    if (res) 
+        return res;
+
+    /*
+     * Get the SPI VIRQ assigned to vpl011.
+     * The SPI VIRQ is assigned by Xen
+     */
+    res = xc_hvm_param_get(dom->xch, dom->guest_domid, HVM_PARAM_VPL011_VIRQ, 
+                                                                    &vpl011_irq);
+    if (res)
+        return res;
+
+    set_interrupt(intr, vpl011_irq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
+
+    res = fdt_property_interrupts(gc, fdt, &intr, 1);
+    if (res) return res;
+
+    fdt_property_u32(fdt, "current-speed", 115200);
+
+    res = fdt_end_node(fdt);
+    if (res) return res;
+
+    return 0;
+}
+
 static const struct arch_info *get_arch_info(libxl__gc *gc,
                                              const struct xc_dom_image *dom)
 {
@@ -888,6 +931,7 @@  next_resize:
 
         FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) );
         FDT( make_hypervisor_node(gc, fdt, vers) );
+        FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
 
         if (pfdt)
             FDT( copy_partial_fdt(gc, fdt, pfdt) );
@@ -933,9 +977,11 @@  int libxl__arch_domain_init_hw_description(libxl__gc *gc,
     val |= GUEST_EVTCHN_PPI;
     rc = xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CALLBACK_IRQ,
                           val);
+
     if (rc)
         return rc;
 
+
     rc = libxl__prepare_dtb(gc, info, state, dom);
     if (rc) goto out;
 
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index d519c8d..45f99c2 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -302,7 +302,7 @@  int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *xs_domid, *con_domid;
     int rc;
-    uint64_t size;
+    uint64_t size, val;
 
     if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) {
         LOG(ERROR, "Couldn't set max vcpu count");
@@ -432,6 +432,11 @@  int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     state->store_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->store_domid);
     state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->console_domid);
 
+    /* get the vpl011 event channel from Xen */
+    xc_hvm_param_get(ctx->xch, domid, HVM_PARAM_VPL011_CONSOLE_EVTCHN,
+            &val);
+    state->vpl011_console_port = (int)val;
+
     if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         hvm_set_conf_params(ctx->xch, domid, info);
 #if defined(__i386__) || defined(__x86_64__)
@@ -727,6 +732,7 @@  int libxl__build_pv(libxl__gc *gc, uint32_t domid,
 
     dom->flags = flags;
     dom->console_evtchn = state->console_port;
+    dom->vpl011_console_evtchn = state->vpl011_console_port;
     dom->console_domid = state->console_domid;
     dom->xenstore_evtchn = state->store_port;
     dom->xenstore_domid = state->store_domid;
@@ -771,6 +777,7 @@  int libxl__build_pv(libxl__gc *gc, uint32_t domid,
     if (xc_dom_feature_translated(dom)) {
         state->console_mfn = dom->console_pfn;
         state->store_mfn = dom->xenstore_pfn;
+        state->vpl011_console_mfn = dom->vpl011_console_pfn;
     } else {
         state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
         state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5f46578..6103036 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1128,6 +1128,8 @@  typedef struct {
     uint32_t num_vmemranges;
 
     xc_domain_configuration_t config;
+    unsigned long vpl011_console_mfn;
+    uint32_t    vpl011_console_port;
 } libxl__domain_build_state;
 
 _hidden int libxl__build_pre(libxl__gc *gc, uint32_t domid,
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 7afb8a3..ba94852 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -49,6 +49,7 @@  obj-y += vm_event.o
 obj-y += vtimer.o
 obj-y += vpsci.o
 obj-y += vuart.o
+obj-y += vpl011.o
 
 #obj-bin-y += ....o
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 7e43691..a0d96b9 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -20,6 +20,7 @@ 
 #include <xen/errno.h>
 #include <xen/bitops.h>
 #include <xen/grant_table.h>
+#include <xen/vpl011.h>
 
 #include <asm/current.h>
 #include <asm/event.h>
@@ -626,6 +627,12 @@  int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( (rc = domain_vtimer_init(d, config)) != 0 )
         goto fail;
 
+    /*
+     * call vpl011 init only for guest domains
+     */
+    if ( d->domain_id && ((rc = domain_vpl011_init(d)) != 0) )
+        goto fail;
+
     update_domain_wallclock_time(d);
 
     /*
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index d999bde..a484ea1 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -23,8 +23,11 @@ 
 #include <xen/guest_access.h>
 #include <xen/sched.h>
 #include <xen/monitor.h>
+#include <xen/event.h>
+#include <xen/vmap.h>
 
 #include <xsm/xsm.h>
+#include <xen/vpl011.h>
 
 #include <public/xen.h>
 #include <public/hvm/params.h>
@@ -61,10 +64,27 @@  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( op == HVMOP_set_param )
         {
             d->arch.hvm_domain.params[a.index] = a.value;
+
+            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN )
+            {
+                vpl011_map_guest_page(d);
+                printk("HVM_PARAM_VPL011_CONSOLE_PFN param set val=0x%lx\n", a.value);
+            }
+            else if ( a.index == HVM_PARAM_VPL011_MMIO_ADDR )
+            {
+                printk("HVM_PARAM_VPL011_MMIO_ADDR param set val=0x%lx\n", a.value);
+            }
         }
         else
         {
-            a.value = d->arch.hvm_domain.params[a.index];
+            if ( a.index == HVM_PARAM_VPL011_MMIO_ADDR )
+            {
+                a.value = d->arch.hvm_domain.params[a.index];
+                printk("HVM_PARAM_PL011_MMIO_ADDR param get val=0x%lx\n", a.value);
+            }
+            else
+                a.value = d->arch.hvm_domain.params[a.index];
+
             rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         }
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 049e449..e347ca0 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -37,6 +37,7 @@ 
 #include <xen/virtual_region.h>
 #include <xen/vmap.h>
 #include <xen/libfdt/libfdt.h>
+#include <xen/vpl011.h>
 #include <xen/acpi.h>
 #include <asm/alternative.h>
 #include <asm/page.h>
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 364d5f0..f6ae7e2 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -118,6 +118,12 @@  int domain_vgic_init(struct domain *d, unsigned int nr_spis)
 
     d->arch.vgic.ctlr = 0;
 
+    /*
+     * Allocate atleast 1 for vpl011 SPI to guest 
+     */
+    if ( !nr_spis )
+        nr_spis = 1;
+
     /* Limit the number of virtual SPIs supported to (1020 - 32) = 988  */
     if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
         return -EINVAL;
@@ -586,6 +592,7 @@  int vgic_allocate_virq(struct domain *d, bool spi)
      */
     do
     {
+        printk("searching virq in < %d, %d >\n", first, end);
         virq = find_next_zero_bit(d->arch.vgic.allocated_irqs, end, first);
         if ( virq >= end )
             return -1;
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
new file mode 100644
index 0000000..5c4507e
--- /dev/null
+++ b/xen/arch/arm/vpl011.c
@@ -0,0 +1,391 @@ 
+/*
+ * arch/arm/vpl011.c
+ *
+ * Virtual PL011 UART 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/config.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/errno.h>
+#include <xen/guest_access.h>
+#include <xen/sched.h>
+#include <xen/monitor.h>
+#include <xen/event.h>
+#include <xen/vmap.h>
+
+#include <xsm/xsm.h>
+#include <xen/vpl011.h>
+
+#include <public/xen.h>
+#include <public/hvm/params.h>
+#include <public/hvm/hvm_op.h>
+
+#include <asm/hypercall.h>
+
+static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r, void *priv)
+{
+    unsigned char ch;
+
+    switch (info->gpa - GUEST_PL011_BASE)
+    {
+        case VPL011_UARTCR_OFFSET:
+            *r = v->domain->arch.vpl011.control;
+            break;
+        case VPL011_UARTDR_OFFSET:
+            vpl011_read_data(v->domain, &ch);
+            *r = ch;
+            break;
+        case VPL011_UARTFR_OFFSET:
+            *r = v->domain->arch.vpl011.flag;
+            if ( *r & (1<<VPL011_UARTFR_TXFF_BIT) )
+                printk ("TXFF in FR\n");
+            break;
+        case VPL011_UARTIMSC_OFFSET:
+            *r = v->domain->arch.vpl011.intr_mask;
+            break;
+        case VPL011_UARTICR_OFFSET: 
+            *r = 0;
+            break;
+        case VPL011_UARTRIS_OFFSET:
+            *r = v->domain->arch.vpl011.raw_intr_status;
+            break;
+        case VPL011_UARTMIS_OFFSET:
+            *r = v->domain->arch.vpl011.raw_intr_status &
+                                v->domain->arch.vpl011.intr_mask;
+            break;
+        case VPL011_UARTDMACR_OFFSET:
+            *r = 0; /* uart DMA is not supported. Here it always returns 0 */
+            break;
+        case VPL011_UARTRSR_OFFSET: 
+            *r = 0; /* it always returns 0 as there are no physical errors */
+            break;
+        default:
+            printk ("vpl011_mmio_read: invalid switch case %d\n", (int)(info->gpa - GUEST_PL011_BASE));
+            break;
+    }
+
+    return 1;
+}
+
+static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r, void *priv)
+{
+    unsigned char ch = r;
+
+    switch (info->gpa - GUEST_PL011_BASE)
+    {
+        case VPL011_UARTCR_OFFSET:
+            v->domain->arch.vpl011.control = r;
+            break;
+        case VPL011_UARTDR_OFFSET:
+            vpl011_write_data(v->domain, ch);
+            break;
+        case VPL011_UARTIMSC_OFFSET:
+            v->domain->arch.vpl011.intr_mask = r;
+            printk ("set intr mask=0x%x\n", v->domain->arch.vpl011.intr_mask);
+            break;
+        case VPL011_UARTICR_OFFSET: 
+            /*
+             * clear all bits which are set in the input
+             */
+            v->domain->arch.vpl011.raw_intr_status &= ~r;
+            break;
+        case VPL011_UARTRSR_OFFSET: // nothing to clear
+            break;
+        case VPL011_UARTFR_OFFSET: // these are all RO registers
+        case VPL011_UARTRIS_OFFSET:
+        case VPL011_UARTMIS_OFFSET:
+        case VPL011_UARTDMACR_OFFSET:
+            break;
+        default:
+            printk ("vpl011_mmio_write: switch case not handled %d\n", (int)(info->gpa - GUEST_PL011_BASE));
+            break;
+    }
+
+    return 1;
+}
+
+static const struct mmio_handler_ops vpl011_mmio_handler = {
+    .read = vpl011_mmio_read,
+    .write = vpl011_mmio_write,
+};
+
+
+
+int vpl011_map_guest_page(struct domain *d)
+{
+    int rc=0;
+
+    printk ("HVM_PARAM_VPL011_CONSOLE_PFN = 0x%lx\n", d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_PFN]);
+
+    /*
+     * map the guest PFN to Xen address space
+     */
+    rc = prepare_ring_for_helper(d, d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_PFN],
+                                        &d->arch.vpl011.ring_page, (void **)&d->arch.vpl011.vpl011_cons_intf);
+    if ( rc < 0 )
+    {
+        printk("Failed to map vpl011 guest PFN\n");
+    }
+
+    return rc;
+}
+
+static int vpl011_data_avail(struct domain *d)
+{
+    int rc=0;
+    unsigned long flags;
+
+    struct vpl011_cons_intf_s *intf = d->arch.vpl011.vpl011_cons_intf;
+
+    VPL011_LOCK(d, flags);
+
+    /*`
+     * check IN ring buffer
+     */
+    if ( !VPL011_IN_RING_EMPTY(intf) )
+    {
+        /*
+         * clear the RX FIFO empty flag as the ring is not empty
+         */
+        d->arch.vpl011.flag &= ~(1<<VPL011_UARTFR_RXFE_BIT);
+
+        /*
+         * if the buffer is full then set the RX FIFO FULL flag
+         */
+        if ( VPL011_IN_RING_FULL(intf) )
+            d->arch.vpl011.flag |= (1<<VPL011_UARTFR_RXFF_BIT);
+
+        /*
+         * set the RX interrupt status
+         */
+        d->arch.vpl011.raw_intr_status |= (1<<VPL011_UARTRIS_RXRIS_BIT);
+    }
+
+    /*
+     * check OUT ring buffer
+     */
+    if ( !VPL011_OUT_RING_FULL(intf) )
+    {
+        /*
+         * if the buffer is not full then clear the TX FIFO full flag
+         */
+        d->arch.vpl011.flag &= ~(1<<VPL011_UARTFR_TXFF_BIT);
+
+        /*
+         * set the TX interrupt status
+         */
+        d->arch.vpl011.raw_intr_status |= (1<<VPL011_UARTRIS_TXRIS_BIT);
+
+        if ( VPL011_OUT_RING_EMPTY(intf) )
+        {
+            /*
+             * clear the uart busy flag and set the TX FIFO empty flag
+             */
+            d->arch.vpl011.flag &= ~(1<<VPL011_UARTFR_BUSY_BIT);
+            d->arch.vpl011.flag |= (1<<VPL011_UARTFR_TXFE_BIT);
+        }
+    }
+    else
+        printk("OUT ring buffer FULL\n");
+
+    VPL011_UNLOCK(d, flags);
+
+#if 0
+    printk ("flags=0x%x, intr=0x%x, mask=0x%x, in_cons=%d, in_prod=%d, out_cons=%d, out_prod=%d\n", 
+                                d->arch.vpl011.flag, 
+                                d->arch.vpl011.raw_intr_status,
+                                d->arch.vpl011.intr_mask,
+                                intf->in_cons, 
+                                intf->in_prod, 
+                                intf->out_cons, 
+                                intf->out_prod);
+#endif
+
+    /*
+     * send an interrupt if it is not masked
+     */
+    if ( d->arch.vpl011.raw_intr_status & d->arch.vpl011.intr_mask )
+    {
+#if 0
+        printk("SENding guest pl011 intr %d with mask=0x%x, flag=0x%x, out_cons=%d, out_prod=%d, in_cons=%d, in_prod=%d...\n", 
+                (int)d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ],
+                d->arch.vpl011.raw_intr_status,
+                d->arch.vpl011.flag,
+                intf->out_cons,
+                intf->out_prod,
+                intf->in_cons,
+                intf->in_prod);
+#endif
+
+        vgic_vcpu_inject_spi(d, (int)d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]);
+    }
+
+    if ( !VPL011_OUT_RING_EMPTY(intf) )
+    {
+        /*
+         * raise an interrupt to dom0
+         */
+        //printk("sending Dom0 event 0x%lx\n", d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN]);
+        rc = evtchn_send(d, d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN]); 
+
+        if ( rc < 0 )
+            printk("Failed to send vpl011 interrupt to dom0\n");
+    }
+
+    return rc;
+}
+
+int vpl011_read_data(struct domain *d, unsigned char *data)
+{
+    int rc=0;
+    unsigned long flags;
+    struct vpl011_cons_intf_s *intf = d->arch.vpl011.vpl011_cons_intf;
+
+    *data = 0;
+
+    VPL011_LOCK(d, flags);
+
+    /*
+     * if there is data in the ring buffer then copy it to the output buffer
+     */
+    if ( !VPL011_IN_RING_EMPTY(intf) )
+    {
+        *data = intf->in[MASK_VPL011CONS_IDX(intf->in_cons++, intf->in)];
+    }
+
+    /*
+     * if the ring buffer is empty then set the RX FIFO empty flag
+     */
+    if ( VPL011_IN_RING_EMPTY(intf) )
+    {
+        d->arch.vpl011.flag |= (1<<VPL011_UARTFR_RXFE_BIT);
+        d->arch.vpl011.raw_intr_status &= ~(1<<VPL011_UARTRIS_RXRIS_BIT);
+    }
+
+    /*
+     * clear the RX FIFO full flag
+     */
+    d->arch.vpl011.flag &= ~(1<<VPL011_UARTFR_RXFF_BIT);
+
+    VPL011_UNLOCK(d, flags);
+
+    return rc;
+}
+
+int vpl011_write_data(struct domain *d, unsigned char data)
+{
+    int rc=0;
+    unsigned long flags;
+    struct vpl011_cons_intf_s *intf = d->arch.vpl011.vpl011_cons_intf;
+
+    //printk ("vpl011_write_data[%d][%d]=%c, full=%d...\n", intf->out_cons, intf->out_prod, data, VPL011_OUT_RING_FULL(intf));
+
+    VPL011_LOCK(d, flags);
+
+    /*
+     * if there is space in the ring buffer then write the data
+     */
+    if ( !VPL011_OUT_RING_FULL(intf) )
+    {
+        intf->out[MASK_VPL011CONS_IDX(intf->out_prod++, intf->out)] = data;
+        smp_wmb();
+    }
+
+    /*
+     * if there is no space in the ring buffer then set the 
+     * TX FIFO FULL flag
+     */
+    if ( VPL011_OUT_RING_FULL(intf) )
+    {
+        d->arch.vpl011.flag |= (1<<VPL011_UARTFR_TXFF_BIT);
+    }
+
+    /*
+     * set the uart busy status
+     */
+    d->arch.vpl011.flag |= (1<<VPL011_UARTFR_BUSY_BIT);
+
+    /*
+     * clear the TX FIFO empty flag
+     */
+    d->arch.vpl011.flag &= ~(1<<VPL011_UARTFR_TXFE_BIT);
+
+    VPL011_UNLOCK(d, flags);
+
+    /*
+     * raise an event to dom0 only if it is the first character in the buffer
+     */
+    if ( VPL011_RING_DEPTH(intf, out) == 1 )
+    {
+        //printk("sending dom0 event 0x%lx\n", d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN]);
+
+        rc = evtchn_send(d, d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN]); 
+
+        if ( rc < 0 )
+            printk("Failed to send vpl011 interrupt to dom0\n");
+    }
+
+    return rc;
+}
+
+static void vpl011_notification(struct vcpu *v, unsigned int port)
+{
+    vpl011_data_avail(v->domain);
+}
+
+int domain_vpl011_init(struct domain *d)
+{
+    int rc=0;
+
+    /*
+     * register xen event channel
+     */
+    rc = alloc_unbound_xen_event_channel(d, 0, current->domain->domain_id, 
+                                                        vpl011_notification);
+    if (rc < 0)
+    {
+        printk ("Failed to allocate vpl011 event channel\n");
+        return rc;
+    }
+    d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN] = rc;
+    
+    /*
+     * allocate an SPI VIRQ for the guest
+     */
+    d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ] = vgic_allocate_spi(d);
+
+    /*
+     * register mmio handler 
+     */
+    register_mmio_handler (d, &vpl011_mmio_handler, GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
+
+    /* 
+     * initialize the lock
+     */
+    spin_lock_init(&d->arch.vpl011.lock);
+
+    /* 
+     * clear the flag, control and interrupts registers
+     */
+    d->arch.vpl011.control = 0;
+    d->arch.vpl011.flag = 0;
+    d->arch.vpl011.intr_mask = 0;
+    d->arch.vpl011.intr_clear = 0;
+    d->arch.vpl011.raw_intr_status = 0;
+    d->arch.vpl011.masked_intr_status = 0;
+
+    return 0;
+}
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 638dc5e..07a09c2 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -27,6 +27,7 @@ 
 #include <xen/keyhandler.h>
 #include <xen/event_fifo.h>
 #include <asm/current.h>
+#include <xen/domain_page.h>
 
 #include <public/xen.h>
 #include <public/event_channel.h>
@@ -663,9 +664,11 @@  int evtchn_send(struct domain *ld, unsigned int lport)
 
     spin_lock(&lchn->lock);
 
-    /* Guest cannot send via a Xen-attached event channel. */
-    if ( unlikely(consumer_is_xen(lchn)) )
+    /* Guest cannot send via a Xen-attached event channel unless it is vpl011 channel */
+    if ( unlikely(consumer_is_xen(lchn) && 
+                (lport != ld->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN])) )
     {
+        printk("evtchn_send failed to send via xen event channel\n");
         ret = -EINVAL;
         goto out;
     }
@@ -681,7 +684,9 @@  int evtchn_send(struct domain *ld, unsigned int lport)
         rport = lchn->u.interdomain.remote_port;
         rchn  = evtchn_from_port(rd, rport);
         if ( consumer_is_xen(rchn) )
+        {
             xen_notification_fn(rchn)(rd->vcpu[rchn->notify_vcpu_id], rport);
+        }
         else
             evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
         break;
@@ -1202,6 +1207,7 @@  int alloc_unbound_xen_event_channel(
 
     chn->state = ECS_UNBOUND;
     chn->xen_consumer = get_xen_consumer(notification_fn);
+    printk ("allocated xen_consumer = %d for port = %d, remote dom=%d\n", chn->xen_consumer, port, remote_domid);
     chn->notify_vcpu_id = lvcpu;
     chn->u.unbound.remote_domid = remote_domid;
 
@@ -1217,6 +1223,7 @@  void free_xen_event_channel(struct domain *d, int port)
 {
     BUG_ON(!port_is_valid(d, port));
 
+    printk ("event channel freed for dom=%d, port=%d\n", d->domain_id, port); 
     evtchn_close(d, port, 0);
 }
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 2d6fbb1..f49606e 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -11,6 +11,7 @@ 
 #include <asm/gic.h>
 #include <public/hvm/params.h>
 #include <xen/serial.h>
+#include <xen/vpl011.h>
 
 struct hvm_domain
 {
@@ -40,6 +41,7 @@  struct vtimer {
         uint64_t cval;
 };
 
+
 struct arch_domain
 {
 #ifdef CONFIG_ARM_64
@@ -131,6 +133,18 @@  struct arch_domain
     struct {
         uint8_t privileged_call_enabled : 1;
     } monitor;
+
+    struct vpl011 {
+        struct vpl011_cons_intf_s *vpl011_cons_intf;
+        struct page_info *ring_page;
+        uint32_t    flag;               /* flag register */
+        uint32_t    control;            /* control register */
+        uint32_t    intr_mask;          /* interrupt mask register*/
+        uint32_t    intr_clear;         /* interrupt clear register */
+        uint32_t    raw_intr_status;    /* raw interrupt status register */
+        uint32_t    masked_intr_status; /* masked interrupt register */
+        spinlock_t  lock;
+    } vpl011;
 }  __cacheline_aligned;
 
 struct arch_vcpu
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index bd974fb..d1d031b 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -406,6 +406,10 @@  typedef uint64_t xen_callback_t;
 #define GUEST_GICV3_GICR0_BASE     xen_mk_ullong(0x03020000) /* vCPU0..127 */
 #define GUEST_GICV3_GICR0_SIZE     xen_mk_ullong(0x01000000)
 
+/* PL011 mappings */
+#define GUEST_PL011_BASE  xen_mk_ullong(0x04020000)
+#define GUEST_PL011_SIZE  xen_mk_ullong(0x00001000)
+
 /* ACPI tables physical address */
 #define GUEST_ACPI_BASE 0x20000000ULL
 #define GUEST_ACPI_SIZE 0x02000000ULL
@@ -420,6 +424,7 @@  typedef uint64_t xen_callback_t;
 #define GUEST_MAGIC_BASE  xen_mk_ullong(0x39000000)
 #define GUEST_MAGIC_SIZE  xen_mk_ullong(0x01000000)
 
+
 #define GUEST_RAM_BANKS   2
 
 #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 3f54a49..a9aa1d9 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -202,11 +202,12 @@ 
  * You can find these address definitions in <hvm/ioreq.h>
  */
 #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
+#define HVM_PARAM_VPL011_CONSOLE_PFN 19
+#define HVM_PARAM_VPL011_CONSOLE_EVTCHN 20
+#define HVM_PARAM_VPL011_VIRQ           21
+#define HVM_PARAM_VPL011_MMIO_ADDR 22
 
 /* Deprecated */
-#define HVM_PARAM_MEMORY_EVENT_CR0          20
-#define HVM_PARAM_MEMORY_EVENT_CR3          21
-#define HVM_PARAM_MEMORY_EVENT_CR4          22
 #define HVM_PARAM_MEMORY_EVENT_INT3         23
 #define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP  25
 #define HVM_PARAM_MEMORY_EVENT_MSR          30
@@ -253,6 +254,7 @@ 
  */
 #define HVM_PARAM_X87_FIP_WIDTH 36
 
-#define HVM_NR_PARAMS 37
+
+#define HVM_NR_PARAMS 38
 
 #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
diff --git a/xen/include/xen/vpl011.h b/xen/include/xen/vpl011.h
new file mode 100644
index 0000000..dd09d41
--- /dev/null
+++ b/xen/include/xen/vpl011.h
@@ -0,0 +1,146 @@ 
+/*
+ * include/xen/vpl011.h
+ *
+ * Virtual PL011 UART 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _VPL011_H_
+
+#define _VPL011_H_
+
+/*
+ * register offsets
+ */
+#define VPL011_UARTDR_OFFSET    0x0 // data register (RW)
+#define VPL011_UARTRSR_OFFSET   0x4 // receive status and error clear register (RW)
+#define VPL011_UARTFR_OFFSET    0x18 // flag register (RO)
+#define VPL011_UARTRIS_OFFSET   0x3c // raw interrupt status register (RO)
+#define VPL011_UARTMIS_OFFSET   0x40 // masked interrupt status register (RO)
+#define VPL011_UARTIMSC_OFFSET  0x38 // interrupt mask set/clear register (RW)
+#define VPL011_UARTICR_OFFSET   0x44 // interrupt clear register (WO)
+#define VPL011_UARTCR_OFFSET    0x30 // uart control register
+#define VPL011_UARTDMACR_OFFSET 0x48 // uart dma control register
+
+/*
+ * control register bits - RW
+ */
+#define VPL011_UARTCR_UARTEN_BIT 0
+#define VPL011_UARTCR_TXE_BIT    8
+#define VPL011_UARTCR_RXE_BIT    9
+
+/*
+ * Flag register bits - RO
+ */
+#define VPL011_UARTFR_CTS_BIT   0   // clear to send
+#define VPL011_UARTFR_DSR_BIT   1   // data set ready
+#define VPL011_UARTFR_DCD_BIT   2   // data carrier detect
+#define VPL011_UARTFR_BUSY_BIT  3   // uart busy
+#define VPL011_UARTFR_RXFE_BIT  4   // receive fifo empty
+#define VPL011_UARTFR_TXFF_BIT  5   // transmit fifo full
+#define VPL011_UARTFR_RXFF_BIT  6   // receive fifo full
+#define VPL011_UARTFR_TXFE_BIT  7   // transmit fifo empty
+#define VPL011_UARTFR_RI_BIT    8   // ring indicator
+
+/*
+ * UART raw interrupt status bits - RO
+ */
+#define VPL011_UARTRIS_RIRMIS_BIT  0
+#define VPL011_UARTRIS_CTSRMIS_BIT 1
+#define VPL011_UARTRIS_DCDRMIS_BIT 2
+#define VPL011_UARTRIS_DSRRMIS_BIT 3
+#define VPL011_UARTRIS_RXRIS_BIT   4
+#define VPL011_UARTRIS_TXRIS_BIT   5
+#define VPL011_UARTRIS_RTRIS_BIT   6
+#define VPL011_UARTRIS_FERIS_BIT   7
+#define VPL011_UARTRIS_PERIS_BIT   8
+#define VPL011_UARTRIS_BERIS_BIT   9
+#define VPL011_UARTRIS_OERIS_BIT   10
+
+/*
+ * UART masked interrupt status bits - RO
+ */
+#define VPL011_UARTMIS_RIMMIS_BIT  0
+#define VPL011_UARTMIS_CTSMMIS_BIT 1
+#define VPL011_UARTMIS_DCDMMIS_BIT 2
+#define VPL011_UARTMIS_DSRMMIS_BIT 3
+#define VPL011_UARTMIS_RXMIS_BIT   4
+#define VPL011_UARTMIS_TXMIS_BIT   5
+#define VPL011_UARTMIS_RTMIS_BIT   6
+#define VPL011_UARTMIS_FEMIS_BIT   7
+#define VPL011_UARTMIS_PEMIS_BIT   8
+#define VPL011_UARTMIS_BEMIS_BIT   9
+#define VPL011_UARTMIS_OEMIS_BIT   10
+
+/*
+ * UART  interrupt clear bits - WO
+ */
+#define VPL011_UARTICR_RIMIC_BIT   0
+#define VPL011_UARTICR_CTSMIC_BIT   1
+#define VPL011_UARTICR_DCDMIC_BIT   2
+#define VPL011_UARTICR_DSRMIC_BIT   3
+#define VPL011_UARTICR_RXIC_BIT   4
+#define VPL011_UARTICR_TXIC_BIT   5
+#define VPL011_UARTICR_RTIC_BIT   6
+#define VPL011_UARTICR_FEIC_BIT   7
+#define VPL011_UARTICR_PEIC_BIT   8
+#define VPL011_UARTICR_BEIC_BIT   9
+#define VPL011_UARTICR_OEIC_BIT   10
+
+/*
+ * UART interrupt mask set/clear bits - RW
+ */
+#define VPL011_UARTIMSC_RIMIM_BIT   0
+#define VPL011_UARTIMSC_CTSMIM_BIT   1
+#define VPL011_UARTIMSC_DCDMIM_BIT   2
+#define VPL011_UARTIMSC_DSRMIM_BIT   3
+#define VPL011_UARTIMSC_RXIM_BIT   4
+#define VPL011_UARTIMSC_TXIM_BIT   5
+#define VPL011_UARTIMSC_RTIM_BIT   6
+#define VPL011_UARTIMSC_FEIM_BIT   7
+#define VPL011_UARTIMSC_PEIM_BIT   8
+#define VPL011_UARTIMSC_BEIM_BIT   9
+#define VPL011_UARTIMSC_OEIM_BIT   10
+
+
+/*
+ * helper macros
+ */
+#define VPL011_RING_DEPTH(intf,dir) (((intf)->dir ## _prod - (intf)->dir ## _cons))
+#define VPL011_RING_MAX_DEPTH(intf,dir) (sizeof((intf)->dir)-1)
+
+#define VPL011_IN_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, in) == 0)
+
+#define VPL011_OUT_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, out) == 0)
+
+#define VPL011_IN_RING_FULL(intf) (VPL011_RING_DEPTH(intf, in) == VPL011_RING_MAX_DEPTH(intf, in))
+
+#define VPL011_OUT_RING_FULL(intf) (VPL011_RING_DEPTH(intf, out) == VPL011_RING_MAX_DEPTH(intf,out))
+
+#define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags)
+#define VPL011_UNLOCK(d,flags) spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
+
+int domain_vpl011_init(struct domain *d);
+int vpl011_map_guest_page(struct domain *d);
+int vpl011_read_data(struct domain *d, unsigned char *data);
+int vpl011_write_data(struct domain *d, unsigned char data);
+
+#define MASK_VPL011CONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1)) 
+struct vpl011_cons_intf_s {
+    char in[1024];
+    char out[2048];
+    uint32_t in_cons, in_prod;
+    uint32_t out_cons, out_prod;
+};
+#endif