|
[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 16 February 2017 at 20:04, Julien Grall <julien.grall@xxxxxxx> 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. > 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@xxxxxxx>. 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |