[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 1/1] xen/arm: Add pl011 uart support in Xen for guest domains
Hi, 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, isHow 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
Lists.xenproject.org is hosted with RackSpace, monitoring our