diff mbox

[04/25,v6] xen/arm: vpl011: Add support for vuart in libxl

Message ID 1500296815-10243-5-git-send-email-bhupinder.thakur@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Bhupinder Thakur July 17, 2017, 1:06 p.m. UTC
An option is provided in libxl to enable/disable sbsa vuart while
creating a guest domain.

Libxl now suppots a generic vuart console and sbsa uart is a specific type.
In future support can be added for multiple vuart of different types.

User can enable sbsa vuart by adding the following line in the guest
configuration file:

vuart = "sbsa_uart"

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
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 "pl011" to "sbsa_uart".

Changes since v3:
- Added a new config option CONFIG_VUART_CONSOLE to enable/disable vuart console
  support.
- Moved libxl_vuart_type to arch-arm part of libxl_domain_build_info
- Updated xl command help to mention new console type - vuart.

Changes since v2:
- Defined vuart option as an enum instead of a string.
- Removed the domain creation flag defined for vuart and the related code
  to pass on the information while domain creation. Now vpl011 is initialized
  independent of domain creation through new DOMCTL APIs.

 tools/libxl/libxl.h          | 6 ++++++
 tools/libxl/libxl_console.c  | 3 +++
 tools/libxl/libxl_dom.c      | 1 +
 tools/libxl/libxl_internal.h | 3 +++
 tools/libxl/libxl_types.idl  | 7 +++++++
 tools/xl/xl_cmdtable.c       | 2 +-
 tools/xl/xl_console.c        | 5 ++++-
 tools/xl/xl_parse.c          | 8 ++++++++
 8 files changed, 33 insertions(+), 2 deletions(-)

Comments

Julien Grall July 18, 2017, 11:19 a.m. UTC | #1
Hi Bhupinder,

Sorry I am jumping a bit late in the discussion here.

On 17/07/17 14:06, Bhupinder Thakur wrote:
> An option is provided in libxl to enable/disable sbsa vuart while

s/sbsa/SBSA/

> creating a guest domain.
>
> Libxl now suppots a generic vuart console and sbsa uart is a specific type.

s/suppots/supports/

s/sbsa/SBSA/

> In future support can be added for multiple vuart of different types.
>
> User can enable sbsa vuart by adding the following line in the guest

ditto.

> configuration file:
>
> vuart = "sbsa_uart"
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
> 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 "pl011" to "sbsa_uart".
>
> Changes since v3:
> - Added a new config option CONFIG_VUART_CONSOLE to enable/disable vuart console
>   support.
> - Moved libxl_vuart_type to arch-arm part of libxl_domain_build_info
> - Updated xl command help to mention new console type - vuart.
>
> Changes since v2:
> - Defined vuart option as an enum instead of a string.
> - Removed the domain creation flag defined for vuart and the related code
>   to pass on the information while domain creation. Now vpl011 is initialized
>   independent of domain creation through new DOMCTL APIs.
>
>  tools/libxl/libxl.h          | 6 ++++++
>  tools/libxl/libxl_console.c  | 3 +++
>  tools/libxl/libxl_dom.c      | 1 +
>  tools/libxl/libxl_internal.h | 3 +++
>  tools/libxl/libxl_types.idl  | 7 +++++++
>  tools/xl/xl_cmdtable.c       | 2 +-
>  tools/xl/xl_console.c        | 5 ++++-
>  tools/xl/xl_parse.c          | 8 ++++++++
>  8 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 7cf0f31..892ed35 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -306,6 +306,12 @@
>  #define LIBXL_HAVE_BUILDINFO_HVM_ACPI_LAPTOP_SLATE 1
>
>  /*
> + * LIBXL_HAVE_VUART indicates that xenconsole/client supports
> + * virtual uart.

I am not sure why you mention about xenconsole/client supporting VUART. 
It does not really matter here, someone may use another backend for the 
PV console here. What matters is the existence or arm.vuart.

> + */
> +#define LIBXL_HAVE_VUART 1

Here you give the impression the virtual UART is supported for all the 
architectures but ...

[...]

> @@ -581,6 +587,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>
>
>      ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> +                               ("vuart", libxl_vuart_type),

... here it is ARM specific. I am not convinced that we should tie vuart 
to ARM only. I cannot see why x86 would not be able to use it in the 
future. Any opinions?

>                                ])),
>      # Alternate p2m is not bound to any architecture or guest type, as it is
>      # supported by x86 HVM and ARM support is planned.
> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> index 30eb93c..9f91651 100644
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -133,7 +133,7 @@ struct cmd_spec cmd_table[] = {
>        &main_console, 0, 0,
>        "Attach to domain's console",
>        "[options] <Domain>\n"
> -      "-t <type>       console type, pv or serial\n"
> +      "-t <type>       console type, pv , serial or vuart\n"
>        "-n <number>     console number"
>      },
>      { "vncviewer",
> diff --git a/tools/xl/xl_console.c b/tools/xl/xl_console.c
> index 0508dda..4e65d73 100644
> --- a/tools/xl/xl_console.c
> +++ b/tools/xl/xl_console.c
> @@ -27,6 +27,7 @@ int main_console(int argc, char **argv)
>      uint32_t domid;
>      int opt = 0, num = 0;
>      libxl_console_type type = 0;
> +    char *console_names = "pv, serial, vuart";
>
>      SWITCH_FOREACH_OPT(opt, "n:t:", NULL, "console", 1) {
>      case 't':
> @@ -34,8 +35,10 @@ int main_console(int argc, char **argv)
>              type = LIBXL_CONSOLE_TYPE_PV;
>          else if (!strcmp(optarg, "serial"))
>              type = LIBXL_CONSOLE_TYPE_SERIAL;
> +        else if (!strcmp(optarg, "vuart"))
> +            type = LIBXL_CONSOLE_TYPE_VUART;
>          else {
> -            fprintf(stderr, "console type supported are: pv, serial\n");
> +            fprintf(stderr, "console type supported are: %s\n", console_names);
>              return EXIT_FAILURE;
>          }
>          break;
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 5c2bf17..71588de 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -918,6 +918,14 @@ void parse_config_data(const char *config_source,
>      if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
>          b_info->max_vcpus = l;
>
> +    if (!xlu_cfg_get_string(config, "vuart", &buf, 0)) {
> +        if (libxl_vuart_type_from_string(buf, &b_info->arch_arm.vuart)) {
> +            fprintf(stderr, "ERROR: invalid value \"%s\" for \"vuart\"\n",
> +                    buf);
> +            exit(1);
> +        }
> +    }
> +
>      parse_vnuma_config(config, b_info);
>
>      /* Set max_memkb to target_memkb and max_vcpus to avail_vcpus if
>

Cheers,
Wei Liu July 18, 2017, 11:30 a.m. UTC | #2
CC x86 maintainers

On Tue, Jul 18, 2017 at 12:19:19PM +0100, Julien Grall wrote:
> > 
> >      ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> > +                               ("vuart", libxl_vuart_type),
> 
> ... here it is ARM specific. I am not convinced that we should tie vuart to
> ARM only. I cannot see why x86 would not be able to use it in the future.
> Any opinions?

I don't know. I asked Bhupinder to put it here because it looked arm
specific to me. I will let x86 maintainers to decide whether they want
such thing.
Bhupinder Thakur July 25, 2017, 5:38 p.m. UTC | #3
Hi,

On 18 July 2017 at 17:00, Wei Liu <wei.liu2@citrix.com> wrote:
> CC x86 maintainers
>
> On Tue, Jul 18, 2017 at 12:19:19PM +0100, Julien Grall wrote:
>> >
>> >      ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>> > +                               ("vuart", libxl_vuart_type),
>>
>> ... here it is ARM specific. I am not convinced that we should tie vuart to
>> ARM only. I cannot see why x86 would not be able to use it in the future.
>> Any opinions?
>
> I don't know. I asked Bhupinder to put it here because it looked arm
> specific to me. I will let x86 maintainers to decide whether they want
> such thing.

What is the decision on this?

I believe that since most of the vuart code added in libxl is arch
agnostic, it should be fine to keep the libxl_vuart_type as a generic
type.

Regards,
Bhupinder
Wei Liu July 28, 2017, 1:49 p.m. UTC | #4
On Tue, Jul 25, 2017 at 11:08:24PM +0530, Bhupinder Thakur wrote:
> Hi,
> 
> On 18 July 2017 at 17:00, Wei Liu <wei.liu2@citrix.com> wrote:
> > CC x86 maintainers
> >
> > On Tue, Jul 18, 2017 at 12:19:19PM +0100, Julien Grall wrote:
> >> >
> >> >      ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> >> > +                               ("vuart", libxl_vuart_type),
> >>
> >> ... here it is ARM specific. I am not convinced that we should tie vuart to
> >> ARM only. I cannot see why x86 would not be able to use it in the future.
> >> Any opinions?
> >
> > I don't know. I asked Bhupinder to put it here because it looked arm
> > specific to me. I will let x86 maintainers to decide whether they want
> > such thing.
> 
> What is the decision on this?
> 

Unfortunately this email probably slipped through the crack for Andrew
and Jan.

I've prodded Andrew on IRC so he might chime in.

> I believe that since most of the vuart code added in libxl is arch
> agnostic, it should be fine to keep the libxl_vuart_type as a generic
> type.
> 

What about the actual emulation code? Is that arch-agnostic? If not, I
personally don't see a chance of having vuart emulation for x86 in the
near future and I'm inclined to keep the code as-is.

There is always the option to lift the struct to common code in the
future.
Julien Grall July 28, 2017, 2:14 p.m. UTC | #5
Hi Wei,

On 07/28/2017 02:49 PM, Wei Liu wrote:
> On Tue, Jul 25, 2017 at 11:08:24PM +0530, Bhupinder Thakur wrote:
>> Hi,
>>
>> On 18 July 2017 at 17:00, Wei Liu <wei.liu2@citrix.com> wrote:
>>> CC x86 maintainers
>>>
>>> On Tue, Jul 18, 2017 at 12:19:19PM +0100, Julien Grall wrote:
>>>>>
>>>>>       ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>>>>> +                               ("vuart", libxl_vuart_type),
>>>>
>>>> ... here it is ARM specific. I am not convinced that we should tie vuart to
>>>> ARM only. I cannot see why x86 would not be able to use it in the future.
>>>> Any opinions?
>>>
>>> I don't know. I asked Bhupinder to put it here because it looked arm
>>> specific to me. I will let x86 maintainers to decide whether they want
>>> such thing.
>>
>> What is the decision on this?
>>
> 
> Unfortunately this email probably slipped through the crack for Andrew
> and Jan.
> 
> I've prodded Andrew on IRC so he might chime in.
> 
>> I believe that since most of the vuart code added in libxl is arch
>> agnostic, it should be fine to keep the libxl_vuart_type as a generic
>> type.
>>
> 
> What about the actual emulation code? Is that arch-agnostic? If not, I
> personally don't see a chance of having vuart emulation for x86 in the
> near future and I'm inclined to keep the code as-is.
> 
> There is always the option to lift the struct to common code in the
> future.

Lifting the struct to common will imply to add compatibility code, right?

Cheers,
Wei Liu July 28, 2017, 2:42 p.m. UTC | #6
On Fri, Jul 28, 2017 at 03:14:31PM +0100, Julien Grall wrote:
> Hi Wei,
> 
> On 07/28/2017 02:49 PM, Wei Liu wrote:
> > On Tue, Jul 25, 2017 at 11:08:24PM +0530, Bhupinder Thakur wrote:
> > > Hi,
> > > 
> > > On 18 July 2017 at 17:00, Wei Liu <wei.liu2@citrix.com> wrote:
> > > > CC x86 maintainers
> > > > 
> > > > On Tue, Jul 18, 2017 at 12:19:19PM +0100, Julien Grall wrote:
> > > > > > 
> > > > > >       ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> > > > > > +                               ("vuart", libxl_vuart_type),
> > > > > 
> > > > > ... here it is ARM specific. I am not convinced that we should tie vuart to
> > > > > ARM only. I cannot see why x86 would not be able to use it in the future.
> > > > > Any opinions?
> > > > 
> > > > I don't know. I asked Bhupinder to put it here because it looked arm
> > > > specific to me. I will let x86 maintainers to decide whether they want
> > > > such thing.
> > > 
> > > What is the decision on this?
> > > 
> > 
> > Unfortunately this email probably slipped through the crack for Andrew
> > and Jan.
> > 
> > I've prodded Andrew on IRC so he might chime in.
> > 
> > > I believe that since most of the vuart code added in libxl is arch
> > > agnostic, it should be fine to keep the libxl_vuart_type as a generic
> > > type.
> > > 
> > 
> > What about the actual emulation code? Is that arch-agnostic? If not, I
> > personally don't see a chance of having vuart emulation for x86 in the
> > near future and I'm inclined to keep the code as-is.
> > 
> > There is always the option to lift the struct to common code in the
> > future.
> 
> Lifting the struct to common will imply to add compatibility code, right?

Yes.
Julien Grall July 28, 2017, 2:46 p.m. UTC | #7
On 07/28/2017 03:42 PM, Wei Liu wrote:
> On Fri, Jul 28, 2017 at 03:14:31PM +0100, Julien Grall wrote:
>> Hi Wei,
>>
>> On 07/28/2017 02:49 PM, Wei Liu wrote:
>>> On Tue, Jul 25, 2017 at 11:08:24PM +0530, Bhupinder Thakur wrote:
>>>> Hi,
>>>>
>>>> On 18 July 2017 at 17:00, Wei Liu <wei.liu2@citrix.com> wrote:
>>>>> CC x86 maintainers
>>>>>
>>>>> On Tue, Jul 18, 2017 at 12:19:19PM +0100, Julien Grall wrote:
>>>>>>>
>>>>>>>        ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>>>>>>> +                               ("vuart", libxl_vuart_type),
>>>>>>
>>>>>> ... here it is ARM specific. I am not convinced that we should tie vuart to
>>>>>> ARM only. I cannot see why x86 would not be able to use it in the future.
>>>>>> Any opinions?
>>>>>
>>>>> I don't know. I asked Bhupinder to put it here because it looked arm
>>>>> specific to me. I will let x86 maintainers to decide whether they want
>>>>> such thing.
>>>>
>>>> What is the decision on this?
>>>>
>>>
>>> Unfortunately this email probably slipped through the crack for Andrew
>>> and Jan.
>>>
>>> I've prodded Andrew on IRC so he might chime in.
>>>
>>>> I believe that since most of the vuart code added in libxl is arch
>>>> agnostic, it should be fine to keep the libxl_vuart_type as a generic
>>>> type.
>>>>
>>>
>>> What about the actual emulation code? Is that arch-agnostic? If not, I
>>> personally don't see a chance of having vuart emulation for x86 in the
>>> near future and I'm inclined to keep the code as-is.
>>>
>>> There is always the option to lift the struct to common code in the
>>> future.
>>
>> Lifting the struct to common will imply to add compatibility code, right?
> 
> Yes.

I would try to avoid that. But you are the maintainer, so it is your 
call :).

Cheers,
diff mbox

Patch

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7cf0f31..892ed35 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -306,6 +306,12 @@ 
 #define LIBXL_HAVE_BUILDINFO_HVM_ACPI_LAPTOP_SLATE 1
 
 /*
+ * LIBXL_HAVE_VUART indicates that xenconsole/client supports
+ * virtual uart.
+ */
+#define LIBXL_HAVE_VUART 1
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
index 446e766..853be15 100644
--- a/tools/libxl/libxl_console.c
+++ b/tools/libxl/libxl_console.c
@@ -67,6 +67,9 @@  int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
     case LIBXL_CONSOLE_TYPE_SERIAL:
         cons_type_s = "serial";
         break;
+    case LIBXL_CONSOLE_TYPE_VUART:
+        cons_type_s = "vuart";
+        break;
     default:
         goto out;
     }
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index f54fd49..e0f0d78 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -803,6 +803,7 @@  int libxl__build_pv(libxl__gc *gc, uint32_t domid,
     if (xc_dom_translated(dom)) {
         state->console_mfn = dom->console_pfn;
         state->store_mfn = dom->xenstore_pfn;
+        state->vuart_gfn = dom->vuart_gfn;
     } else {
         state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
         state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index afe6652..d0d50c3 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1139,6 +1139,9 @@  typedef struct {
     uint32_t num_vmemranges;
 
     xc_domain_configuration_t config;
+
+    xen_pfn_t vuart_gfn;
+    evtchn_port_t vuart_port;
 } libxl__domain_build_state;
 
 _hidden int libxl__build_pre(libxl__gc *gc, uint32_t domid,
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 8a9849c..728cc56 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -105,6 +105,7 @@  libxl_console_type = Enumeration("console_type", [
     (0, "UNKNOWN"),
     (1, "SERIAL"),
     (2, "PV"),
+    (3, "VUART"),
     ])
 
 libxl_disk_format = Enumeration("disk_format", [
@@ -240,6 +241,11 @@  libxl_checkpointed_stream = Enumeration("checkpointed_stream", [
     (2, "COLO"),
     ])
 
+libxl_vuart_type = Enumeration("vuart_type", [
+    (0, "unknown"),
+    (1, "sbsa_uart"),
+    ])
+
 #
 # Complex libxl types
 #
@@ -581,6 +587,7 @@  libxl_domain_build_info = Struct("domain_build_info",[
 
 
     ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
+                               ("vuart", libxl_vuart_type),
                               ])),
     # Alternate p2m is not bound to any architecture or guest type, as it is
     # supported by x86 HVM and ARM support is planned.
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 30eb93c..9f91651 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -133,7 +133,7 @@  struct cmd_spec cmd_table[] = {
       &main_console, 0, 0,
       "Attach to domain's console",
       "[options] <Domain>\n"
-      "-t <type>       console type, pv or serial\n"
+      "-t <type>       console type, pv , serial or vuart\n"
       "-n <number>     console number"
     },
     { "vncviewer",
diff --git a/tools/xl/xl_console.c b/tools/xl/xl_console.c
index 0508dda..4e65d73 100644
--- a/tools/xl/xl_console.c
+++ b/tools/xl/xl_console.c
@@ -27,6 +27,7 @@  int main_console(int argc, char **argv)
     uint32_t domid;
     int opt = 0, num = 0;
     libxl_console_type type = 0;
+    char *console_names = "pv, serial, vuart";
 
     SWITCH_FOREACH_OPT(opt, "n:t:", NULL, "console", 1) {
     case 't':
@@ -34,8 +35,10 @@  int main_console(int argc, char **argv)
             type = LIBXL_CONSOLE_TYPE_PV;
         else if (!strcmp(optarg, "serial"))
             type = LIBXL_CONSOLE_TYPE_SERIAL;
+        else if (!strcmp(optarg, "vuart"))
+            type = LIBXL_CONSOLE_TYPE_VUART;
         else {
-            fprintf(stderr, "console type supported are: pv, serial\n");
+            fprintf(stderr, "console type supported are: %s\n", console_names);
             return EXIT_FAILURE;
         }
         break;
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 5c2bf17..71588de 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -918,6 +918,14 @@  void parse_config_data(const char *config_source,
     if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
         b_info->max_vcpus = l;
 
+    if (!xlu_cfg_get_string(config, "vuart", &buf, 0)) {
+        if (libxl_vuart_type_from_string(buf, &b_info->arch_arm.vuart)) {
+            fprintf(stderr, "ERROR: invalid value \"%s\" for \"vuart\"\n",
+                    buf);
+            exit(1);
+        }
+    }
+
     parse_vnuma_config(config, b_info);
 
     /* Set max_memkb to target_memkb and max_vcpus to avail_vcpus if