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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

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