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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 25 Jul 2017, Julien Grall wrote:
> 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.

It's incredible this problem hasn't been solved yet. I guess the right
way of fixing it would be to generate the Xen .config even when doing
"make tools".


> However, imposing an
> #ifdef to require some work to support correctly for 29 lines does not seem
> very warrant.

Yes, I don't want to feature-creep it.


> So I think we should keep on by default.

OK

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