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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

Hi Bhupinder,

Thanks for trying, and I would have done the same thing as you did:

ifeq ($(CONFIG_SBSA_VUART_CONSOLE),y)
CONFIG_VUART_CONSOLE := y
endif

However, we don't want to introduce a dependency between "make tools"
and "make xen", meaning that one should be able to do "make tools"
successfully even without having done "make xen" before.

I think this is a generic tools building issue that will need to be
fixed, probably by always generating the xen .config, even when building
tools. But I don't think it's fair to ask you to do it as part of this
series. So feel free to disregard my little request for now and always
set CONFIG_VUART_CONSOLE := y.


On Tue, 25 Jul 2017, Bhupinder Thakur wrote:
> Hi Stefano,
> 
> Can we make CONFIG_VUART_CONSOLE dependent on CONFIG_SBSA_VUART_CONSOLE?
> 
> CONFIG_SBSA_VUART_CONSOLE is a Kconfig option while
> CONFIG_VUART_CONSOLE is an option defined in the .mk file which is
> used while compiling the toolstack.
> 
> So if I try to do something like this in arm64.mk/arm32.mk file, I am
> not sure if CONFIG_SBSA_VUART_CONSOLE definition will be available
> (since .config would not be generated) if I have not compiled Xen
> hypervisor code first:
> 
> ifeq ($(CONFIG_SBSA_VUART_CONSOLE),y)
> CONFIG_VUART_CONSOLE := y
> endif
> 
> Regards,
> Bhupinder
> 
> On 22 July 2017 at 01:14, Stefano Stabellini <sstabellini@xxxxxxxxxx> 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?
> 

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