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

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

Commit Message

Bhupinder Thakur Aug. 28, 2017, 8:56 a.m.
This patch finally adds the support for vuart console. It adds
two new fields in the console initialization:

- optional
- use_gnttab

optional flag tells whether the console is optional.

use_gnttab tells whether the ring buffer should be allocated using
grant table.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>

Changes since v6:
- Renames prefer_gnttab to use_gnttab

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 | 30 ++++++++++++++++++++++++++++--
 4 files changed, 32 insertions(+), 3 deletions(-)

Comments

Jan Beulich Aug. 28, 2017, 9:11 a.m. | #1
>>> On 28.08.17 at 10:56, <bhupinder.thakur@linaro.org> wrote:
> --- 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

I think this wants to be solved better than by starting to again
introduce CONFIG_* values here.

Jan
Bhupinder Thakur Sept. 4, 2017, 4:28 p.m. | #2
Hi Jan,


On 28 August 2017 at 14:41, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 28.08.17 at 10:56, <bhupinder.thakur@linaro.org> wrote:
>> --- 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
>
> I think this wants to be solved better than by starting to again
> introduce CONFIG_* values here.

I think I can remove this flag from here since it is used currently
for xenconsole only to enable VUART console support for ARM. I can
directly define the flag in the tools/console Makefile based on
CONFIG_ARM option.

Regards,
Bhupinder
Jan Beulich Sept. 5, 2017, 5:29 a.m. | #3
>>> Bhupinder Thakur <bhupinder.thakur@linaro.org> 09/04/17 6:28 PM >>>
>On 28 August 2017 at 14:41, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 28.08.17 at 10:56, <bhupinder.thakur@linaro.org> wrote:
>>> --- 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
>>
>> I think this wants to be solved better than by starting to again
>> introduce CONFIG_* values here.
>
>I think I can remove this flag from here since it is used currently
>for xenconsole only to enable VUART console support for ARM. I can
>directly define the flag in the tools/console Makefile based on
>CONFIG_ARM option.

If it's unconditionally on for ARM, what's the extra CONFIG_* good for?
And if it was meant to be conditionally selectable, then a configure
based approach would be needed. But anyway, once this affects
tools/ only, the tools maintainers will know best.

Jan
Wei Liu Sept. 5, 2017, 9:31 a.m. | #4
On Mon, Sep 04, 2017 at 09:58:07PM +0530, Bhupinder Thakur wrote:
> Hi Jan,
> 
> 
> On 28 August 2017 at 14:41, Jan Beulich <JBeulich@suse.com> wrote:
> >>>> On 28.08.17 at 10:56, <bhupinder.thakur@linaro.org> wrote:
> >> --- 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
> >
> > I think this wants to be solved better than by starting to again
> > introduce CONFIG_* values here.
> 
> I think I can remove this flag from here since it is used currently
> for xenconsole only to enable VUART console support for ARM. I can
> directly define the flag in the tools/console Makefile based on
> CONFIG_ARM option.

Just use CONFIG_ARM directly?
Bhupinder Thakur Sept. 6, 2017, 5:29 p.m. | #5
On 5 September 2017 at 15:01, Wei Liu <wei.liu2@citrix.com> wrote:
> On Mon, Sep 04, 2017 at 09:58:07PM +0530, Bhupinder Thakur wrote:
>> Hi Jan,
>>
>>
>> On 28 August 2017 at 14:41, Jan Beulich <JBeulich@suse.com> wrote:
>> >>>> On 28.08.17 at 10:56, <bhupinder.thakur@linaro.org> wrote:
>> >> --- 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
>> >
>> > I think this wants to be solved better than by starting to again
>> > introduce CONFIG_* values here.
>>
>> I think I can remove this flag from here since it is used currently
>> for xenconsole only to enable VUART console support for ARM. I can
>> directly define the flag in the tools/console Makefile based on
>> CONFIG_ARM option.
>
> Just use CONFIG_ARM directly?

I believe I cannot use CONFIG_ARM directly in
tools/console/daemon/io.c as it is a makefile variable.

Currently, in tools/console/Makefile the VUART specifc flag is
included like this:

CFLAGS_vuart-$(CONFIG_VUART_CONSOLE) = -DCONFIG_VUART_CONSOLE

I will change it to:

CFLAGS_vuart-$(CONFIG_ARM) = -DCONFIG_VUART_CONSOLE

and remove CONFIG_VUART_CONSOLE variable from arm32.mk and arm64.mk.

Regards,
Bhupinder
Wei Liu Sept. 7, 2017, 9:08 a.m. | #6
On Wed, Sep 06, 2017 at 10:59:05PM +0530, Bhupinder Thakur wrote:
> On 5 September 2017 at 15:01, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Mon, Sep 04, 2017 at 09:58:07PM +0530, Bhupinder Thakur wrote:
> >> Hi Jan,
> >>
> >>
> >> On 28 August 2017 at 14:41, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>>> On 28.08.17 at 10:56, <bhupinder.thakur@linaro.org> wrote:
> >> >> --- 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
> >> >
> >> > I think this wants to be solved better than by starting to again
> >> > introduce CONFIG_* values here.
> >>
> >> I think I can remove this flag from here since it is used currently
> >> for xenconsole only to enable VUART console support for ARM. I can
> >> directly define the flag in the tools/console Makefile based on
> >> CONFIG_ARM option.
> >
> > Just use CONFIG_ARM directly?
> 
> I believe I cannot use CONFIG_ARM directly in
> tools/console/daemon/io.c as it is a makefile variable.
> 

You should be able to. I think CONFIG_ARM/X86 are passed on to gcc.

And I just tested with CONFIG_X86, which worked.
Bhupinder Thakur Sept. 12, 2017, 7:25 a.m. | #7
Hi Wei,

On 7 September 2017 at 14:38, Wei Liu <wei.liu2@citrix.com> wrote:
> On Wed, Sep 06, 2017 at 10:59:05PM +0530, Bhupinder Thakur wrote:
>> On 5 September 2017 at 15:01, Wei Liu <wei.liu2@citrix.com> wrote:
>> > On Mon, Sep 04, 2017 at 09:58:07PM +0530, Bhupinder Thakur wrote:
>> >> Hi Jan,
>> >>
>> >>
>> >> On 28 August 2017 at 14:41, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>>> On 28.08.17 at 10:56, <bhupinder.thakur@linaro.org> wrote:
>> >> >> --- 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
>> >> >
>> >> > I think this wants to be solved better than by starting to again
>> >> > introduce CONFIG_* values here.
>> >>
>> >> I think I can remove this flag from here since it is used currently
>> >> for xenconsole only to enable VUART console support for ARM. I can
>> >> directly define the flag in the tools/console Makefile based on
>> >> CONFIG_ARM option.
>> >
>> > Just use CONFIG_ARM directly?
>>
>> I believe I cannot use CONFIG_ARM directly in
>> tools/console/daemon/io.c as it is a makefile variable.
>>
>
> You should be able to. I think CONFIG_ARM/X86 are passed on to gcc.
>
> And I just tested with CONFIG_X86, which worked.

I tried using CONFIG_ARM or CONFIG_ARM_64 in daemon/io.c but these
flags are not defined.

I think I can define it the way, it is defined in libacpi/Makefile:

MKDSDT_CFLAGS-$(CONFIG_ARM_64) = -DCONFIG_ARM_64

Similarly I can define:

CONSOLE_CFLAGS-$(CONFIG_ARM) = -DCONFIG_ARM

Regards,
Bhupinder
Wei Liu Sept. 12, 2017, 8:54 a.m. | #8
On Tue, Sep 12, 2017 at 12:55:23PM +0530, Bhupinder Thakur wrote:
> Hi Wei,
> 
> On 7 September 2017 at 14:38, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Wed, Sep 06, 2017 at 10:59:05PM +0530, Bhupinder Thakur wrote:
> >> On 5 September 2017 at 15:01, Wei Liu <wei.liu2@citrix.com> wrote:
> >> > On Mon, Sep 04, 2017 at 09:58:07PM +0530, Bhupinder Thakur wrote:
> >> >> Hi Jan,
> >> >>
> >> >>
> >> >> On 28 August 2017 at 14:41, Jan Beulich <JBeulich@suse.com> wrote:
> >> >> >>>> On 28.08.17 at 10:56, <bhupinder.thakur@linaro.org> wrote:
> >> >> >> --- 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
> >> >> >
> >> >> > I think this wants to be solved better than by starting to again
> >> >> > introduce CONFIG_* values here.
> >> >>
> >> >> I think I can remove this flag from here since it is used currently
> >> >> for xenconsole only to enable VUART console support for ARM. I can
> >> >> directly define the flag in the tools/console Makefile based on
> >> >> CONFIG_ARM option.
> >> >
> >> > Just use CONFIG_ARM directly?
> >>
> >> I believe I cannot use CONFIG_ARM directly in
> >> tools/console/daemon/io.c as it is a makefile variable.
> >>
> >
> > You should be able to. I think CONFIG_ARM/X86 are passed on to gcc.
> >
> > And I just tested with CONFIG_X86, which worked.
> 
> I tried using CONFIG_ARM or CONFIG_ARM_64 in daemon/io.c but these
> flags are not defined.
> 
> I think I can define it the way, it is defined in libacpi/Makefile:
> 
> MKDSDT_CFLAGS-$(CONFIG_ARM_64) = -DCONFIG_ARM_64
> 
> Similarly I can define:
> 
> CONSOLE_CFLAGS-$(CONFIG_ARM) = -DCONFIG_ARM
> 

Fine by me.

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 abe77b2..e7ff8ff 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 a198dbb..90aea8a 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 use_gnttab;
 };
 
 struct console_type {
 	char *xsname;
 	char *ttyname;
 	char *log_suffix;
+	bool optional;
+	bool use_gnttab;
 };
 
 static struct console_type console_type[] = {
@@ -120,7 +124,18 @@  static struct console_type console_type[] = {
 		.xsname = "/console",
 		.ttyname = "tty",
 		.log_suffix = "",
+		.optional = false,
+		.use_gnttab = true,
 	},
+#if defined(CONFIG_VUART_CONSOLE)
+	{
+		.xsname = "/vuart/0",
+		.ttyname = "tty",
+		.log_suffix = "-vuart0",
+		.optional = true,
+		.use_gnttab = false,
+	},
+#endif
 };
 
 #define NUM_CONSOLE_TYPE (sizeof(console_type)/sizeof(struct console_type))
@@ -654,8 +669,17 @@  static int console_create_ring(struct console *con)
 			"ring-ref", "%u", &ring_ref,
 			"port", "%i", &remote_port,
 			NULL);
-	if (err)
+
+	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);
@@ -669,7 +693,7 @@  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->use_gnttab) {
 		/* Prefer using grant table */
 		con->interface = xengnttab_map_grant_ref(xgt_handle,
 			dom->domid, GNTTAB_RESERVED_CONSOLE,
@@ -788,6 +812,8 @@  static int console_init(struct console *con, struct domain *dom, void **data)
 	con->d = dom;
 	con->ttyname = (*con_type)->ttyname;
 	con->log_suffix = (*con_type)->log_suffix;
+	con->optional = (*con_type)->optional;
+	con->use_gnttab = (*con_type)->use_gnttab;
 	xsname = (char *)(*con_type)->xsname;
 	xspath = xs_get_domain_path(xs, dom->domid);
 	s = realloc(xspath, strlen(xspath) +