[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 22/25 v6] xen/arm: vpl011: Add support for vuart console in xenconsole

Hi Stefano,

On 07/21/2017 08:44 PM, Stefano Stabellini wrote:
On Fri, 21 Jul 2017, Julien Grall wrote:

On 18/07/17 21:07, Stefano Stabellini wrote:
On Mon, 17 Jul 2017, Bhupinder Thakur wrote:
This patch finally adds the support for vuart console. It adds
two new fields in the console initialization:

- optional
- prefer_gnttab

optional flag tells whether the console is optional.

prefer_gnttab tells whether the ring buffer should be allocated using
grant table.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
CC: Julien Grall <julien.grall@xxxxxxx>

Changes since v4:
- Renamed VUART_CFLAGS- to CFLAGS_vuart- in the Makefile as per the

  config/arm32.mk           |  1 +
  config/arm64.mk           |  1 +
  tools/console/Makefile    |  3 ++-
  tools/console/daemon/io.c | 29 ++++++++++++++++++++++++++++-
  4 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/config/arm32.mk b/config/arm32.mk
index f95228e..b9f23fe 100644
--- a/config/arm32.mk
+++ b/config/arm32.mk
@@ -1,5 +1,6 @@
  CONFIG_ARM_32 := y


What about leaving this off for ARM32 by default?

Why? This will only disable xenconsole changes and not the hypervisor. The
changes are quite tiny, so I would even be in favor of enabling for all

Or are you suggesting to disable the VPL011 emulation in the hypervisor? But I
don't see the emulation AArch64 specific, and a user could disable it if he
doesn't want it...

I was thinking that the virtual pl011 is mostly useful for SBSA
compliance, which doesn't really apply to ARM32 (there are no ARM32 SBSA
compliant platforms as far as I am aware).

Given that we don't need vpl011 on ARM32, I thought we might as well
disable it. Less code the better. I wouldn't go as far as introducing
more #ifdefs to disable it, but I would make use of the existing config
options to turn it off by default on ARM32. Does it make sense?

That said, you are right that there is no point in disabling only
CONFIG_VUART_CONSOLE, which affects the tools only. We should really
disable SBSA_VUART_CONSOLE by default on ARM32. In fact, ideally
CONFIG_VUART_CONSOLE would be set dependning on the value of
SBSA_VUART_CONSOLE. What do you think?

Kconfig is only targeting the hypervisor and this is the tools. It is possible to build the tools separately from the hypervisor and therefore .config would not be generated.

Therefore your suggestion cannot work at the moment. However, imposing an #ifdef to require some work to support correctly for 29 lines does not seem very warrant.

So I think we should keep on by default.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.