[Xen-devel,04/10] xen/arm: vpl011: Provide a knob in libxl to enable/disable pl011 emulation

Message ID 1491212673-13476-5-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series
  • pl011 emulation support in Xen
Related show

Commit Message

Bhupinder Thakur April 3, 2017, 9:44 a.m.
An option is provided in libxl to enable/disable pl011 emulation while
creating a guest domain.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
 tools/libxl/libxl_create.c   | 12 ++++++++++++
 tools/libxl/libxl_internal.h |  5 +++++
 tools/libxl/libxl_types.idl  |  2 ++
 tools/libxl/xl_cmdimpl.c     |  4 ++++
 4 files changed, 23 insertions(+)

Comments

Wei Liu April 12, 2017, 4:32 p.m. | #1
On Mon, Apr 03, 2017 at 03:14:27PM +0530, Bhupinder Thakur wrote:
[...]
>  _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 a612d1f..fe7f795 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, "VCON"),
>      ])
>  

You need to add a LIBXL_HAVE macro to libxl.h.

>  libxl_disk_format = Enumeration("disk_format", [
> @@ -460,6 +461,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("disable_migrate", libxl_defbool),
>      ("cpuid",           libxl_cpuid_policy_list),
>      ("blkdev_start",    string),
> +    ("enable_pl011",    libxl_defbool),
>  
>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
>      
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 358757f..4f4d4e6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1433,6 +1433,8 @@ static void parse_config_data(const char *config_source,
>      if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
>          b_info->max_vcpus = l;
>  
> +    xlu_cfg_get_defbool(config, "pl011", &b_info->enable_pl011, 0);
> +

I'm not very keen on having "pl011" because it is ARM specific.

Is there a case in which you don't want this vconsole? Should we always
enable it in toolstack?

>      parse_vnuma_config(config, b_info);
>  
>      /* Set max_memkb to target_memkb and max_vcpus to avail_vcpus if
> @@ -3788,6 +3790,8 @@ 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, "vcon"))
> +            type = LIBXL_CONSOLE_TYPE_VCON;
>          else {
>              fprintf(stderr, "console type supported are: pv, serial\n");
>              return EXIT_FAILURE;

You also need to patch manpage etc.

It appears that your tree is not up to date -- xl was split out from
libxl at some point.

Wei.
Bhupinder Thakur April 13, 2017, 8:19 a.m. | #2
Hi Wei,

>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index a612d1f..fe7f795 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, "VCON"),
>>      ])
>>
>
> You need to add a LIBXL_HAVE macro to libxl.h.

I will add this macro. I am trying to understand the use of these
macros. Are the applications which are building against a particular
libxl version supposed to check this flag before using this feature?

>
>>  libxl_disk_format = Enumeration("disk_format", [
>> @@ -460,6 +461,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>      ("disable_migrate", libxl_defbool),
>>      ("cpuid",           libxl_cpuid_policy_list),
>>      ("blkdev_start",    string),
>> +    ("enable_pl011",    libxl_defbool),
>>
>>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
>>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 358757f..4f4d4e6 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1433,6 +1433,8 @@ static void parse_config_data(const char *config_source,
>>      if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
>>          b_info->max_vcpus = l;
>>
>> +    xlu_cfg_get_defbool(config, "pl011", &b_info->enable_pl011, 0);
>> +
>
> I'm not very keen on having "pl011" because it is ARM specific.
I will rename it to vconsole, which I am using through out the libxl code now.

>
> Is there a case in which you don't want this vconsole? Should we always
> enable it in toolstack?
There was a review comment from Julien that pl011 emulation should not
be enabled by default for domU. So based on this flag the pl011
emulation will be enabled/disabled at the time of domain creation.

>
>>      parse_vnuma_config(config, b_info);
>>
>>      /* Set max_memkb to target_memkb and max_vcpus to avail_vcpus if
>> @@ -3788,6 +3790,8 @@ 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, "vcon"))
>> +            type = LIBXL_CONSOLE_TYPE_VCON;
>>          else {
>>              fprintf(stderr, "console type supported are: pv, serial\n");
>>              return EXIT_FAILURE;
>
> You also need to patch manpage etc.
>
> It appears that your tree is not up to date -- xl was split out from
> libxl at some point.
I will rebase my patches on the latest master branch.

Regards,
Bhupinder
Wei Liu April 13, 2017, 8:37 a.m. | #3
On Thu, Apr 13, 2017 at 01:49:51PM +0530, Bhupinder Thakur wrote:
> Hi Wei,
> 
> >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> >> index a612d1f..fe7f795 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, "VCON"),
> >>      ])
> >>
> >
> > You need to add a LIBXL_HAVE macro to libxl.h.
> 
> I will add this macro. I am trying to understand the use of these
> macros. Are the applications which are building against a particular
> libxl version supposed to check this flag before using this feature?
> 

Yes. Think about a new application that is built against an older
version of libxl source code that doesn't have this flag.  It can use
this flag to check if such feature is available. 

> >
> >>  libxl_disk_format = Enumeration("disk_format", [ @@ -460,6 +461,7
> >>  @@ libxl_domain_build_info = Struct("domain_build_info",[
> >>  ("disable_migrate", libxl_defbool), ("cpuid",
> >>  libxl_cpuid_policy_list), ("blkdev_start",    string), +
> >>  ("enable_pl011",    libxl_defbool),
> >>
> >>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
> >>
> >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> >> index 358757f..4f4d4e6 100644 --- a/tools/libxl/xl_cmdimpl.c +++
> >> b/tools/libxl/xl_cmdimpl.c @@ -1433,6 +1433,8 @@ static void
> >> parse_config_data(const char *config_source, if (!xlu_cfg_get_long
> >> (config, "maxvcpus", &l, 0)) b_info->max_vcpus = l;
> >>
> >> +    xlu_cfg_get_defbool(config, "pl011", &b_info->enable_pl011,
> >> 0); +
> >
> > I'm not very keen on having "pl011" because it is ARM specific.
> I will rename it to vconsole, which I am using through out the libxl
> code now.

Before you go away and change your code, we need to discuss this a bit.

Since there is no "console" option in xl, I think using just "console"
should be fine. And then user can specify "console=pl011" in their
config file.

There will also need to be changes in the libxl idl struct. If you
really want to use enable_pl011 (I would suggest to just use pl011), it
should be part of the arm specific sub-struct in build_info.

Please also wait a bit for Ian to express his preference for the config
option etc.

Wei.
Stefano Stabellini April 19, 2017, 12:29 a.m. | #4
On Thu, 13 Apr 2017, Wei Liu wrote:
> On Thu, Apr 13, 2017 at 01:49:51PM +0530, Bhupinder Thakur wrote:
> > Hi Wei,
> > 
> > >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > >> index a612d1f..fe7f795 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, "VCON"),
> > >>      ])
> > >>
> > >
> > > You need to add a LIBXL_HAVE macro to libxl.h.
> > 
> > I will add this macro. I am trying to understand the use of these
> > macros. Are the applications which are building against a particular
> > libxl version supposed to check this flag before using this feature?
> > 
> 
> Yes. Think about a new application that is built against an older
> version of libxl source code that doesn't have this flag.  It can use
> this flag to check if such feature is available. 
> 
> > >
> > >>  libxl_disk_format = Enumeration("disk_format", [ @@ -460,6 +461,7
> > >>  @@ libxl_domain_build_info = Struct("domain_build_info",[
> > >>  ("disable_migrate", libxl_defbool), ("cpuid",
> > >>  libxl_cpuid_policy_list), ("blkdev_start",    string), +
> > >>  ("enable_pl011",    libxl_defbool),
> > >>
> > >>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
> > >>
> > >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > >> index 358757f..4f4d4e6 100644 --- a/tools/libxl/xl_cmdimpl.c +++
> > >> b/tools/libxl/xl_cmdimpl.c @@ -1433,6 +1433,8 @@ static void
> > >> parse_config_data(const char *config_source, if (!xlu_cfg_get_long
> > >> (config, "maxvcpus", &l, 0)) b_info->max_vcpus = l;
> > >>
> > >> +    xlu_cfg_get_defbool(config, "pl011", &b_info->enable_pl011,
> > >> 0); +
> > >
> > > I'm not very keen on having "pl011" because it is ARM specific.
> > I will rename it to vconsole, which I am using through out the libxl
> > code now.
> 
> Before you go away and change your code, we need to discuss this a bit.
> 
> Since there is no "console" option in xl, I think using just "console"
> should be fine. And then user can specify "console=pl011" in their
> config file.

I think "console" is confusing an emulated device with xenconsole.

I would use vuart=pl011 to clarify that we are specifying a new
component for emulation.


> There will also need to be changes in the libxl idl struct. If you
> really want to use enable_pl011 (I would suggest to just use pl011), it
> should be part of the arm specific sub-struct in build_info.
> 
> Please also wait a bit for Ian to express his preference for the config
> option etc.
Bhupinder Thakur April 19, 2017, 9:17 a.m. | #5
Hi,

On 19 April 2017 at 05:59, Stefano Stabellini <sstabellini@kernel.org> wrote:
> I think "console" is confusing an emulated device with xenconsole.
>
> I would use vuart=pl011 to clarify that we are specifying a new
> component for emulation.

Throughout the code, I am using "vconsole" or "vcon" as a prefix for
most of the code. Should I replace "vconsole" with "vuart" at all such
places? I did not use "vuart" initially as there was some existing
code in Xen under vuart.c.

Regards,
Bhupinder
Wei Liu April 19, 2017, 10:25 a.m. | #6
On Wed, Apr 19, 2017 at 02:47:08PM +0530, Bhupinder Thakur wrote:
> Hi,
> 
> On 19 April 2017 at 05:59, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > I think "console" is confusing an emulated device with xenconsole.
> >
> > I would use vuart=pl011 to clarify that we are specifying a new
> > component for emulation.
> 
> Throughout the code, I am using "vconsole" or "vcon" as a prefix for
> most of the code. Should I replace "vconsole" with "vuart" at all such
> places? I did not use "vuart" initially as there was some existing
> code in Xen under vuart.c.

That would be fine by me. But I don't know enough of ARM to make
judgement. Please wait for Stefano or Julien to reply.

Wei.

> 
> Regards,
> Bhupinder
Julien Grall April 19, 2017, 11:06 a.m. | #7
Hi,

On 19/04/17 11:25, Wei Liu wrote:
> On Wed, Apr 19, 2017 at 02:47:08PM +0530, Bhupinder Thakur wrote:
>> Hi,
>>
>> On 19 April 2017 at 05:59, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> I think "console" is confusing an emulated device with xenconsole.
>>>
>>> I would use vuart=pl011 to clarify that we are specifying a new
>>> component for emulation.
>>
>> Throughout the code, I am using "vconsole" or "vcon" as a prefix for
>> most of the code. Should I replace "vconsole" with "vuart" at all such
>> places? I did not use "vuart" initially as there was some existing
>> code in Xen under vuart.c.
>
> That would be fine by me. But I don't know enough of ARM to make
> judgement. Please wait for Stefano or Julien to reply.

The name "vuart" makes more sense to use for the user point of view.

Regarding vuart.c, this is just a name of a file that is used to expose 
a very limited UART to DOM0 that mimic the host UART. In the future we 
may want to expose something similar for guest running unmodified OS and 
target a specific hardware.

We can rename the file if it is too confusing.

Cheers,

Patch

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index e3bc257..9a59354 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -208,6 +208,8 @@  int libxl__domain_build_info_setdefault(libxl__gc *gc,
 
     libxl_defbool_setdefault(&b_info->disable_migrate, false);
 
+    libxl_defbool_setdefault(&b_info->enable_pl011, false);
+
     for (i = 0 ; i < b_info->num_iomem; i++)
         if (b_info->iomem[i].gfn == LIBXL_INVALID_GFN)
             b_info->iomem[i].gfn = b_info->iomem[i].start;
@@ -546,6 +548,9 @@  int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
         flags |= XEN_DOMCTL_CDF_hap;
     }
 
+    if (libxl_defbool_val(d_config->b_info.enable_pl011))
+        flags |= XEN_DOMCTL_VCONSOLE_enable;
+
     /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
     libxl_uuid_copy(ctx, (libxl_uuid *)handle, &info->uuid);
 
@@ -910,6 +915,11 @@  static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
+    if (libxl_defbool_val(d_config->b_info.enable_pl011))
+        state->vconsole_enabled = true;
+    else
+        state->vconsole_enabled = false;
+
     if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
         (libxl_defbool_val(d_config->b_info.u.hvm.nested_hvm) &&
          libxl_defbool_val(d_config->b_info.u.hvm.altp2m))) {
@@ -926,6 +936,8 @@  static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
+    state->config.console_domid = state->console_domid;
+
     ret = libxl__domain_make(gc, d_config, &domid, &state->config);
     if (ret) {
         LOGD(ERROR, domid, "cannot make domain: %d", ret);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5f46578..2406eaa 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1128,6 +1128,11 @@  typedef struct {
     uint32_t num_vmemranges;
 
     xc_domain_configuration_t config;
+
+    /* Virtual console mfn and port. */
+    unsigned long vconsole_mfn;
+    uint32_t    vconsole_port;
+    bool        vconsole_enabled;
 } 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 a612d1f..fe7f795 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, "VCON"),
     ])
 
 libxl_disk_format = Enumeration("disk_format", [
@@ -460,6 +461,7 @@  libxl_domain_build_info = Struct("domain_build_info",[
     ("disable_migrate", libxl_defbool),
     ("cpuid",           libxl_cpuid_policy_list),
     ("blkdev_start",    string),
+    ("enable_pl011",    libxl_defbool),
 
     ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
     
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 358757f..4f4d4e6 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1433,6 +1433,8 @@  static void parse_config_data(const char *config_source,
     if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
         b_info->max_vcpus = l;
 
+    xlu_cfg_get_defbool(config, "pl011", &b_info->enable_pl011, 0);
+
     parse_vnuma_config(config, b_info);
 
     /* Set max_memkb to target_memkb and max_vcpus to avail_vcpus if
@@ -3788,6 +3790,8 @@  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, "vcon"))
+            type = LIBXL_CONSOLE_TYPE_VCON;
         else {
             fprintf(stderr, "console type supported are: pv, serial\n");
             return EXIT_FAILURE;