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

[Julien Grall <julien.grall@arm.com>]

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.

>>
>>> 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@arm.com>. 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@lists.xen.org
https://lists.xen.org/xen-devel