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

[Julien Grall <julien.grall@xxxxxxx>]

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

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