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

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

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

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

'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

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