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

[Bhupinder Thakur <bhupinder.thakur@linaro.org>]

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

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