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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 07/21/2017 08:44 PM, Stefano Stabellini wrote:
> On Fri, 21 Jul 2017, Julien Grall wrote:
> > Hi,
> >
> > 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
> > > > convention.
> > > >
> > > >   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 := y
> > > >   CONFIG_ARM_32 := y
> > > > +CONFIG_VUART_CONSOLE := y
> > > >   CONFIG_ARM_$(XEN_OS) := y
> > > >
> > > >   CONFIG_XEN_INSTALL_SUFFIX :=
> > > 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
> > architectures.
> >
> > 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.

Cheers,

--
Julien Grall

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