| Message ID | 1500296815-10243-23-git-send-email-bhupinder.thakur@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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?
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
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,
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?
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?
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,
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
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? >
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,
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(-)