| Message ID | 1491212673-13476-5-git-send-email-bhupinder.thakur@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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.
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
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
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,
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;
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(+)