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

Message ID 1500296815-10243-23-git-send-email-bhupinder.thakur@linaro.org
State Superseded
Headers show
Series
  • SBSA UART emulation support in Xen
Related show

Commit Message

Bhupinder Thakur July 17, 2017, 1:06 p.m.
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@linaro.org>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

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(-)

Comments

Wei Liu July 18, 2017, 1:27 p.m. | #1
On Mon, Jul 17, 2017 at 06:36:52PM +0530, 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.
> 

Then use_gnttab is more appropriate.
Stefano Stabellini July 18, 2017, 8:07 p.m. | #2
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@linaro.org>
> ---
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> 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?


> diff --git a/config/arm64.mk b/config/arm64.mk
> index aa45772..861d0a4 100644
> --- a/config/arm64.mk
> +++ b/config/arm64.mk
> @@ -1,5 +1,6 @@
>  CONFIG_ARM := y
>  CONFIG_ARM_64 := y
> +CONFIG_VUART_CONSOLE := y
>  CONFIG_ARM_$(XEN_OS) := y
>  
>  CONFIG_XEN_INSTALL_SUFFIX :=
> diff --git a/tools/console/Makefile b/tools/console/Makefile
> index c8b0300..1cddb6e 100644
> --- a/tools/console/Makefile
> +++ b/tools/console/Makefile
> @@ -11,6 +11,7 @@ LDLIBS += $(SOCKET_LIBS)
>  
>  LDLIBS_xenconsoled += $(UTIL_LIBS)
>  LDLIBS_xenconsoled += -lrt
> +CFLAGS_vuart-$(CONFIG_VUART_CONSOLE) = -DCONFIG_VUART_CONSOLE
>  
>  BIN      = xenconsoled xenconsole
>  
> @@ -28,7 +29,7 @@ clean:
>  distclean: clean
>  
>  daemon/main.o: daemon/_paths.h
> -daemon/io.o: CFLAGS += $(CFLAGS_libxenevtchn) $(CFLAGS_libxengnttab)
> +daemon/io.o: CFLAGS += $(CFLAGS_libxenevtchn) $(CFLAGS_libxengnttab) $(CFLAGS_vuart-y)
>  xenconsoled: $(patsubst %.c,%.o,$(wildcard daemon/*.c))
>  	$(CC) $(LDFLAGS) $^ -o $@ $(LDLIBS) $(LDLIBS_libxenevtchn) $(LDLIBS_libxengnttab) $(LDLIBS_xenconsoled) $(APPEND_LDFLAGS)
>  
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 49f085c..c6d4cae 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -107,12 +107,16 @@ struct console {
>  	xenevtchn_port_or_error_t remote_port;
>  	struct xencons_interface *interface;
>  	struct domain *d;
> +	bool optional;
> +	bool prefer_gnttab;
>  };
>  
>  struct console_data {
>  	const char *const xsname;
>  	const char *const ttyname;
>  	const char *const log_suffix;
> +	bool optional;
> +	bool prefer_gnttab;
>  };
>  
>  static struct console_data console_data[] = {
> @@ -120,7 +124,18 @@ static struct console_data console_data[] = {
>  		.xsname = "/console",
>  		.ttyname = "tty",
>  		.log_suffix = "",
> +		.optional = false,
> +		.prefer_gnttab = true,
>  	},
> +#if defined(CONFIG_VUART_CONSOLE)
> +	{
> +		.xsname = "/vuart/0",
> +		.ttyname = "tty",
> +		.log_suffix = "-vuart0",
> +		.optional = true,
> +		.prefer_gnttab = false,
> +	},
> +#endif
>  };
>  
>  #define MAX_CONSOLE (sizeof(console_data)/sizeof(struct console_data))
> @@ -655,8 +670,18 @@ static int console_create_ring(struct console *con)
>  			"ring-ref", "%u", &ring_ref,
>  			"port", "%i", &remote_port,
>  			NULL);
> +
>  	if (err)
> +	{

wrong code style for tools/console


> +		/*
> +		 * This is a normal condition for optional consoles: they might not be
> +		 * present on xenstore at all. In that case, just return without error.
> +		*/
> +		if (con->optional)
> +			err = 0;
> +
>  		goto out;
> +	}
>  
>  	snprintf(path, sizeof(path), "%s/type", con->xspath);
>  	type = xs_read(xs, XBT_NULL, path, NULL);
> @@ -670,7 +695,9 @@ static int console_create_ring(struct console *con)
>  	if (ring_ref != con->ring_ref && con->ring_ref != -1)
>  		console_unmap_interface(con);
>  
> -	if (!con->interface && xgt_handle) {
> +	if (!con->interface &&
> +	    xgt_handle &&
> +	    con->prefer_gnttab) {
>  		/* Prefer using grant table */
>  		con->interface = xengnttab_map_grant_ref(xgt_handle,
>  			dom->domid, GNTTAB_RESERVED_CONSOLE,

I noticed that you removed the initialization of optional and
prefer_gnttab from console_init from this patch:

@@ -790,6 +817,8 @@ static int console_init(struct console *con, struct domain *dom, void **data)
      con->d = dom;
      con->ttyname = (*con_data)->ttyname;
      con->log_suffix = (*con_data)->log_suffix;
+     con->optional = (*con_data)->optional;
+     con->prefer_gnttab = (*con_data)->prefer_gnttab;
      xsname = (*con_data)->xsname;
      con->xspath = xs_get_domain_path(xs, dom->domid);
      s = realloc(con->xspath, strlen(con->xspath) +

Why? It that intended?
Bhupinder Thakur July 21, 2017, 6:02 a.m. | #3
Hi Stefano,


>>  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?

I will disable it for ARM32.


>> -     if (!con->interface && xgt_handle) {
>> +     if (!con->interface &&
>> +         xgt_handle &&
>> +         con->prefer_gnttab) {
>>               /* Prefer using grant table */
>>               con->interface = xengnttab_map_grant_ref(xgt_handle,
>>                       dom->domid, GNTTAB_RESERVED_CONSOLE,
>
> I noticed that you removed the initialization of optional and
> prefer_gnttab from console_init from this patch:
>
> @@ -790,6 +817,8 @@ static int console_init(struct console *con, struct domain *dom, void **data)
>       con->d = dom;
>       con->ttyname = (*con_data)->ttyname;
>       con->log_suffix = (*con_data)->log_suffix;
> +     con->optional = (*con_data)->optional;
> +     con->prefer_gnttab = (*con_data)->prefer_gnttab;
>       xsname = (*con_data)->xsname;
>       con->xspath = xs_get_domain_path(xs, dom->domid);
>       s = realloc(con->xspath, strlen(con->xspath) +
>
> Why? It that intended?

Thanks for noticing this. Actually while splitting the patch, this
change mistakenly moved to the previous patch. I will correct this.

Regards,
Bhupinder
Julien Grall July 21, 2017, 9:51 a.m. | #4
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@linaro.org>
>> ---
>> CC: Ian Jackson <ian.jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> 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...

Cheers,
Stefano Stabellini July 21, 2017, 7:44 p.m. | #5
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@linaro.org>
> > > ---
> > > CC: Ian Jackson <ian.jackson@eu.citrix.com>
> > > CC: Wei Liu <wei.liu2@citrix.com>
> > > CC: Stefano Stabellini <sstabellini@kernel.org>
> > > CC: Julien Grall <julien.grall@arm.com>
> > > 
> > > 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?
Bhupinder Thakur July 25, 2017, 9:06 a.m. | #6
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@kernel.org> 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@linaro.org>
>> > > ---
>> > > CC: Ian Jackson <ian.jackson@eu.citrix.com>
>> > > CC: Wei Liu <wei.liu2@citrix.com>
>> > > CC: Stefano Stabellini <sstabellini@kernel.org>
>> > > CC: Julien Grall <julien.grall@arm.com>
>> > >
>> > > 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?
Julien Grall July 25, 2017, 12:29 p.m. | #7
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@linaro.org>
>>>> ---
>>>> CC: Ian Jackson <ian.jackson@eu.citrix.com>
>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>> CC: Julien Grall <julien.grall@arm.com>
>>>>
>>>> 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,
Stefano Stabellini July 25, 2017, 5:39 p.m. | #8
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@linaro.org>
> > > > > ---
> > > > > CC: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > > CC: Wei Liu <wei.liu2@citrix.com>
> > > > > CC: Stefano Stabellini <sstabellini@kernel.org>
> > > > > CC: Julien Grall <julien.grall@arm.com>
> > > > > 
> > > > > 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
Stefano Stabellini July 25, 2017, 5:44 p.m. | #9
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@kernel.org> 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@linaro.org>
> >> > > ---
> >> > > CC: Ian Jackson <ian.jackson@eu.citrix.com>
> >> > > CC: Wei Liu <wei.liu2@citrix.com>
> >> > > CC: Stefano Stabellini <sstabellini@kernel.org>
> >> > > CC: Julien Grall <julien.grall@arm.com>
> >> > >
> >> > > 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?
>

Patch

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 :=
diff --git a/config/arm64.mk b/config/arm64.mk
index aa45772..861d0a4 100644
--- a/config/arm64.mk
+++ b/config/arm64.mk
@@ -1,5 +1,6 @@ 
 CONFIG_ARM := y
 CONFIG_ARM_64 := y
+CONFIG_VUART_CONSOLE := y
 CONFIG_ARM_$(XEN_OS) := y
 
 CONFIG_XEN_INSTALL_SUFFIX :=
diff --git a/tools/console/Makefile b/tools/console/Makefile
index c8b0300..1cddb6e 100644
--- a/tools/console/Makefile
+++ b/tools/console/Makefile
@@ -11,6 +11,7 @@  LDLIBS += $(SOCKET_LIBS)
 
 LDLIBS_xenconsoled += $(UTIL_LIBS)
 LDLIBS_xenconsoled += -lrt
+CFLAGS_vuart-$(CONFIG_VUART_CONSOLE) = -DCONFIG_VUART_CONSOLE
 
 BIN      = xenconsoled xenconsole
 
@@ -28,7 +29,7 @@  clean:
 distclean: clean
 
 daemon/main.o: daemon/_paths.h
-daemon/io.o: CFLAGS += $(CFLAGS_libxenevtchn) $(CFLAGS_libxengnttab)
+daemon/io.o: CFLAGS += $(CFLAGS_libxenevtchn) $(CFLAGS_libxengnttab) $(CFLAGS_vuart-y)
 xenconsoled: $(patsubst %.c,%.o,$(wildcard daemon/*.c))
 	$(CC) $(LDFLAGS) $^ -o $@ $(LDLIBS) $(LDLIBS_libxenevtchn) $(LDLIBS_libxengnttab) $(LDLIBS_xenconsoled) $(APPEND_LDFLAGS)
 
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 49f085c..c6d4cae 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -107,12 +107,16 @@  struct console {
 	xenevtchn_port_or_error_t remote_port;
 	struct xencons_interface *interface;
 	struct domain *d;
+	bool optional;
+	bool prefer_gnttab;
 };
 
 struct console_data {
 	const char *const xsname;
 	const char *const ttyname;
 	const char *const log_suffix;
+	bool optional;
+	bool prefer_gnttab;
 };
 
 static struct console_data console_data[] = {
@@ -120,7 +124,18 @@  static struct console_data console_data[] = {
 		.xsname = "/console",
 		.ttyname = "tty",
 		.log_suffix = "",
+		.optional = false,
+		.prefer_gnttab = true,
 	},
+#if defined(CONFIG_VUART_CONSOLE)
+	{
+		.xsname = "/vuart/0",
+		.ttyname = "tty",
+		.log_suffix = "-vuart0",
+		.optional = true,
+		.prefer_gnttab = false,
+	},
+#endif
 };
 
 #define MAX_CONSOLE (sizeof(console_data)/sizeof(struct console_data))
@@ -655,8 +670,18 @@  static int console_create_ring(struct console *con)
 			"ring-ref", "%u", &ring_ref,
 			"port", "%i", &remote_port,
 			NULL);
+
 	if (err)
+	{
+		/*
+		 * This is a normal condition for optional consoles: they might not be
+		 * present on xenstore at all. In that case, just return without error.
+		*/
+		if (con->optional)
+			err = 0;
+
 		goto out;
+	}
 
 	snprintf(path, sizeof(path), "%s/type", con->xspath);
 	type = xs_read(xs, XBT_NULL, path, NULL);
@@ -670,7 +695,9 @@  static int console_create_ring(struct console *con)
 	if (ring_ref != con->ring_ref && con->ring_ref != -1)
 		console_unmap_interface(con);
 
-	if (!con->interface && xgt_handle) {
+	if (!con->interface &&
+	    xgt_handle &&
+	    con->prefer_gnttab) {
 		/* Prefer using grant table */
 		con->interface = xengnttab_map_grant_ref(xgt_handle,
 			dom->domid, GNTTAB_RESERVED_CONSOLE,