* [PATCH v1 01/02] HVM firmware passthrough libxl support
@ 2013-01-18 21:40 Ross Philipson
2013-01-21 9:57 ` Ian Campbell
0 siblings, 1 reply; 7+ messages in thread
From: Ross Philipson @ 2013-01-18 21:40 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1: Type: text/plain, Size: 559 bytes --]
This patch introduces support for two new parameters in libxl:
smbios_firmware=<path_to_smbios_structures_file>
acpi_firmware=<path_to_acpi_tables_file>
The changes are primarily in the domain building code where the firmware files
are read and passed to libxc for loading into the new guest. After the domain
building call to libxc, the addresses for the loaded blobs are returned and
written to xenstore.
Additionally, this patch switches to using the new xc_hvm_build() routine.
Signed-off-by: Ross Philipson <ross.philipson@citrix.com>
[-- Attachment #2: hvm-firmware-passthrough-libxl-v1-01.patch --]
[-- Type: application/octet-stream, Size: 11017 bytes --]
This patch introduces support for two new parameters in libxl:
smbios_firmware=<path_to_smbios_structures_file>
acpi_firmware=<path_to_acpi_tables_file>
The changes are primarily in the domain building code where the firmware files
are read and passed to libxc for loading into the new guest. After the domain
building call to libxc, the addresses for the loaded blobs are returned and
written to xenstore.
Additionally, this patch switches to using the new xc_hvm_build() routine.
Signed-off-by: Ross Philipson <ross.philipson@citrix.com>
diff -r 35a0556a7f76 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c Thu Jan 10 17:32:10 2013 +0000
+++ b/tools/libxl/libxl_dom.c Fri Jan 18 15:21:50 2013 -0500
@@ -21,6 +21,7 @@
#include <xc_dom.h>
#include <xen/hvm/hvm_info_table.h>
+#include <xen/hvm/hvm_xs_strings.h>
libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
{
@@ -462,6 +463,7 @@ static unsigned long timer_mode(const li
mode <= LIBXL_TIMER_MODE_ONE_MISSED_TICK_PENDING);
return ((unsigned long)mode);
}
+
static int hvm_build_set_params(xc_interface *handle, uint32_t domid,
libxl_domain_build_info *info,
int store_evtchn, unsigned long *store_mfn,
@@ -510,11 +512,79 @@ static int hvm_build_set_params(xc_inter
return 0;
}
-static const char *libxl__domain_firmware(libxl__gc *gc,
- libxl_domain_build_info *info)
+static int hvm_build_set_xs_values(libxl__gc *gc,
+ uint32_t domid,
+ struct xc_hvm_build_args *args)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ char *path = NULL;
+ int ret = 0;
+
+ if (args->smbios_module.guest_addr_out) {
+ path = libxl__sprintf(gc, "/local/domain/%d/"HVM_XS_SMBIOS_PT_ADDRESS,
+ domid);
+ if (!path)
+ goto str_err;
+
+ ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64,
+ args->smbios_module.guest_addr_out);
+ if (ret)
+ goto xs_err;
+
+ path = libxl__sprintf(gc, "/local/domain/%d/"HVM_XS_SMBIOS_PT_LENGTH,
+ domid);
+ if (!path)
+ goto str_err;
+
+ ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x",
+ args->smbios_module.length);
+ if (ret)
+ goto xs_err;
+ }
+
+ if (args->acpi_module.guest_addr_out) {
+ path = libxl__sprintf(gc, "/local/domain/%d/"HVM_XS_ACPI_PT_ADDRESS,
+ domid);
+ if (!path)
+ goto str_err;
+
+ ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64,
+ args->acpi_module.guest_addr_out);
+ if (ret)
+ goto xs_err;
+
+ path = libxl__sprintf(gc, "/local/domain/%d/"HVM_XS_ACPI_PT_LENGTH,
+ domid);
+ if (!path)
+ goto str_err;
+
+ ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x",
+ args->acpi_module.length);
+ if (ret)
+ goto xs_err;
+ }
+
+ return 0;
+
+xs_err:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "failed to write firmware xenstore value, err: %d", ret);
+ return -1;
+str_err:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "failed to get firmware xenstore path");
+ return -1;
+}
+
+static int libxl__domain_firmware(libxl__gc *gc,
+ libxl_domain_build_info *info,
+ struct xc_hvm_build_args *args)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
const char *firmware;
+ int e, rc = ERROR_FAIL;
+ int datalen = 0;
+ void *data = 0;
if (info->u.hvm.firmware)
firmware = info->u.hvm.firmware;
@@ -530,11 +600,68 @@ static const char *libxl__domain_firmwar
default:
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "invalid device model version %d",
info->device_model_version);
- return NULL;
+ return -1;
break;
}
}
- return libxl__abs_path(gc, firmware, libxl__xenfirmwaredir_path());
+ args->image_file_name = libxl__abs_path(gc, firmware,
+ libxl__xenfirmwaredir_path());
+ if (!args->image_file_name) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "invalid firmware path");
+ return -1;
+ }
+
+ if (info->u.hvm.smbios_firmware) {
+ e = libxl_read_file_contents(ctx, info->u.hvm.smbios_firmware,
+ &data, &datalen);
+ if (e) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "failed to read SMBIOS firmware file %s",
+ info->u.hvm.smbios_firmware);
+ goto err;
+ }
+ if (!datalen) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "SMBIOS firmware file %s is empty",
+ info->u.hvm.smbios_firmware);
+ goto err;
+ }
+ args->smbios_module.data = data;
+ args->smbios_module.length = (uint32_t)datalen;
+ }
+
+ if (info->u.hvm.acpi_firmware) {
+ e = libxl_read_file_contents(ctx, info->u.hvm.acpi_firmware,
+ &data, &datalen);
+ if (e) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "failed to read ACPI firmware file %s",
+ info->u.hvm.acpi_firmware);
+ goto err;
+ }
+ if (!datalen) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "ACPI firmware file %s is empty",
+ info->u.hvm.acpi_firmware);
+ goto err;
+ }
+ args->acpi_module.data = data;
+ args->acpi_module.length = (uint32_t)datalen;
+ }
+
+ rc = 0;
+ goto out;
+err:
+ if (args->smbios_module.data) {
+ free(args->smbios_module.data);
+ args->smbios_module.data = NULL;
+ }
+ if (args->acpi_module.data) {
+ free(args->acpi_module.data);
+ args->acpi_module.data = NULL;
+ }
+out:
+ return rc;
}
int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
@@ -542,31 +669,54 @@ int libxl__build_hvm(libxl__gc *gc, uint
libxl__domain_build_state *state)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
+ struct xc_hvm_build_args args = {};
int ret, rc = ERROR_FAIL;
- const char *firmware = libxl__domain_firmware(gc, info);
- if (!firmware)
+ memset(&args, 0, sizeof(struct xc_hvm_build_args));
+ /* The memory size names are misleading. The params are in Mb then
+ * multiplied by 1 Kb. This was then divided off when calling
+ * the old xc_hvm_build_target_mem() which then turned them to bytes.
+ * Do all this in one step here...
+ */
+ args.mem_size = (uint64_t)(info->max_memkb - info->video_memkb) << 10;
+ args.mem_target = (uint64_t)(info->target_memkb - info->video_memkb) << 10;
+
+ if (libxl__domain_firmware(gc, info, &args)) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "initializing domain firmware failed");
goto out;
- ret = xc_hvm_build_target_mem(
- ctx->xch,
- domid,
- (info->max_memkb - info->video_memkb) / 1024,
- (info->target_memkb - info->video_memkb) / 1024,
- firmware);
+ }
+
+ ret = xc_hvm_build(ctx->xch, domid, &args);
if (ret) {
LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "hvm building failed");
goto out;
}
+
ret = hvm_build_set_params(ctx->xch, domid, info, state->store_port,
&state->store_mfn, state->console_port,
&state->console_mfn, state->store_domid,
state->console_domid);
if (ret) {
- LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "hvm build set params failed");
+ LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret,
+ "hvm build set params failed");
goto out;
}
+
+ ret = hvm_build_set_xs_values(gc, domid, &args);
+ if (ret) {
+ LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret,
+ "hvm build set xenstore values failed");
+ goto out;
+ }
+
rc = 0;
out:
+ if (args.smbios_module.data)
+ free(args.smbios_module.data);
+ if (args.acpi_module.data)
+ free(args.acpi_module.data);
+
return rc;
}
@@ -627,7 +777,7 @@ int libxl__toolstack_restore(uint32_t do
memcpy(&count, ptr, sizeof(count));
ptr += sizeof(count);
-
+
if (size < sizeof(version) + sizeof(count) +
count * (sizeof(struct libxl__physmap_info))) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "wrong size");
@@ -841,7 +991,7 @@ static void switch_logdirty_xswatch(libx
rc = libxl__xs_rm_checked(gc, t, lds->ret_path);
if (rc) goto out;
- rc = libxl__xs_transaction_commit(gc, &t);
+ rc = libxl__xs_transaction_commit(gc, &t);
if (!rc) break;
if (rc<0) goto out;
}
@@ -1313,7 +1463,7 @@ void libxl__xc_domain_save_done(libxl__e
if (type == LIBXL_DOMAIN_TYPE_HVM) {
rc = libxl__domain_suspend_device_model(gc, dss);
if (rc) goto out;
-
+
libxl__domain_save_device_model(egc, dss, domain_suspend_done);
return;
}
diff -r 35a0556a7f76 tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl Thu Jan 10 17:32:10 2013 +0000
+++ b/tools/libxl/libxl_types.idl Fri Jan 18 15:21:50 2013 -0500
@@ -308,6 +308,8 @@ libxl_domain_build_info = Struct("domain
("vpt_align", libxl_defbool),
("timer_mode", libxl_timer_mode),
("nested_hvm", libxl_defbool),
+ ("smbios_firmware", string),
+ ("acpi_firmware", string),
("nographic", libxl_defbool),
("vga", libxl_vga_interface_info),
("vnc", libxl_vnc_info),
diff -r 35a0556a7f76 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Thu Jan 10 17:32:10 2013 +0000
+++ b/tools/libxl/xl_cmdimpl.c Fri Jan 18 15:21:50 2013 -0500
@@ -882,6 +882,11 @@ static void parse_config_data(const char
}
xlu_cfg_get_defbool(config, "nestedhvm", &b_info->u.hvm.nested_hvm, 0);
+
+ xlu_cfg_replace_string (config, "smbios_firmware",
+ &b_info->u.hvm.smbios_firmware, 0);
+ xlu_cfg_replace_string (config, "acpi_firmware",
+ &b_info->u.hvm.acpi_firmware, 0);
break;
case LIBXL_DOMAIN_TYPE_PV:
{
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 01/02] HVM firmware passthrough libxl support
2013-01-18 21:40 [PATCH v1 01/02] HVM firmware passthrough libxl support Ross Philipson
@ 2013-01-21 9:57 ` Ian Campbell
2013-01-21 17:05 ` Ross Philipson
0 siblings, 1 reply; 7+ messages in thread
From: Ian Campbell @ 2013-01-21 9:57 UTC (permalink / raw)
To: Ross Philipson; +Cc: xen-devel
On Fri, 2013-01-18 at 21:40 +0000, Ross Philipson wrote:
> This patch introduces support for two new parameters in libxl:
>
> smbios_firmware=<path_to_smbios_structures_file>
> acpi_firmware=<path_to_acpi_tables_file>
>
> The changes are primarily in the domain building code where the firmware files
> are read and passed to libxc for loading into the new guest. After the domain
> building call to libxc, the addresses for the loaded blobs are returned and
> written to xenstore.
>
> Additionally, this patch switches to using the new xc_hvm_build() routine.
>
> Signed-off-by: Ross Philipson <ross.philipson@citrix.com>
>
>
> diff -r 35a0556a7f76 tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c Thu Jan 10 17:32:10 2013 +0000
> +++ b/tools/libxl/libxl_dom.c Fri Jan 18 15:21:50 2013 -0500
> @@ -21,6 +21,7 @@
>
> #include <xc_dom.h>
> #include <xen/hvm/hvm_info_table.h>
> +#include <xen/hvm/hvm_xs_strings.h>
>
> libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
> {
> @@ -462,6 +463,7 @@ static unsigned long timer_mode(const li
> mode <= LIBXL_TIMER_MODE_ONE_MISSED_TICK_PENDING);
> return ((unsigned long)mode);
> }
> +
> static int hvm_build_set_params(xc_interface *handle, uint32_t domid,
> libxl_domain_build_info *info,
> int store_evtchn, unsigned long *store_mfn,
> @@ -510,11 +512,79 @@ static int hvm_build_set_params(xc_inter
> return 0;
> }
>
> -static const char *libxl__domain_firmware(libxl__gc *gc,
> - libxl_domain_build_info *info)
> +static int hvm_build_set_xs_values(libxl__gc *gc,
> + uint32_t domid,
> + struct xc_hvm_build_args *args)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + char *path = NULL;
> + int ret = 0;
> +
> + if (args->smbios_module.guest_addr_out) {
> + path = libxl__sprintf(gc, "/local/domain/%d/"HVM_XS_SMBIOS_PT_ADDRESS,
> + domid);
> + if (!path)
> + goto str_err;
xl will panic on memory allocation failure so you don't need to handle
it yourself, which removes a lot of error handling.
You could also use GCSPRINTF to shorten the line if you wanted
> +
> + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64,
> + args->smbios_module.guest_addr_out);
> + if (ret)
> + goto xs_err;
> +
> + path = libxl__sprintf(gc, "/local/domain/%d/"HVM_XS_SMBIOS_PT_LENGTH,
> + domid);
> + if (!path)
> + goto str_err;
> +
> + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x",
> + args->smbios_module.length);
> + if (ret)
> + goto xs_err;
> + }
> +
> + if (args->acpi_module.guest_addr_out) {
> + path = libxl__sprintf(gc, "/local/domain/%d/"HVM_XS_ACPI_PT_ADDRESS,
> + domid);
> + if (!path)
> + goto str_err;
> +
> + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64,
> + args->acpi_module.guest_addr_out);
> + if (ret)
> + goto xs_err;
> +
> + path = libxl__sprintf(gc, "/local/domain/%d/"HVM_XS_ACPI_PT_LENGTH,
> + domid);
> + if (!path)
> + goto str_err;
> +
> + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x",
> + args->acpi_module.length);
> + if (ret)
> + goto xs_err;
> + }
> +
> + return 0;
> +
> +xs_err:
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "failed to write firmware xenstore value, err: %d", ret);
You can use the LOG macro to shorten these long lines too.
> + return -1;
> +str_err:
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "failed to get firmware xenstore path");
> + return -1;
> +}
> +
> +static int libxl__domain_firmware(libxl__gc *gc,
> + libxl_domain_build_info *info,
> + struct xc_hvm_build_args *args)
> {
> libxl_ctx *ctx = libxl__gc_owner(gc);
> const char *firmware;
> + int e, rc = ERROR_FAIL;
> + int datalen = 0;
> + void *data = 0;
>
> if (info->u.hvm.firmware)
> firmware = info->u.hvm.firmware;
> @@ -530,11 +600,68 @@ static const char *libxl__domain_firmwar
> default:
> LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "invalid device model version %d",
> info->device_model_version);
> - return NULL;
> + return -1;
> break;
> }
> }
> - return libxl__abs_path(gc, firmware, libxl__xenfirmwaredir_path());
> + args->image_file_name = libxl__abs_path(gc, firmware,
> + libxl__xenfirmwaredir_path());
> + if (!args->image_file_name) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "invalid firmware path");
> + return -1;
> + }
> +
> + if (info->u.hvm.smbios_firmware) {
> + e = libxl_read_file_contents(ctx, info->u.hvm.smbios_firmware,
> + &data, &datalen);
> + if (e) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "failed to read SMBIOS firmware file %s",
> + info->u.hvm.smbios_firmware);
> + goto err;
> + }
> + if (!datalen) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "SMBIOS firmware file %s is empty",
> + info->u.hvm.smbios_firmware);
> + goto err;
> + }
> + args->smbios_module.data = data;
> + args->smbios_module.length = (uint32_t)datalen;
> + }
> +
> + if (info->u.hvm.acpi_firmware) {
> + e = libxl_read_file_contents(ctx, info->u.hvm.acpi_firmware,
> + &data, &datalen);
> + if (e) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "failed to read ACPI firmware file %s",
> + info->u.hvm.acpi_firmware);
> + goto err;
> + }
> + if (!datalen) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "ACPI firmware file %s is empty",
> + info->u.hvm.acpi_firmware);
> + goto err;
> + }
> + args->acpi_module.data = data;
> + args->acpi_module.length = (uint32_t)datalen;
> + }
> +
> + rc = 0;
> + goto out;
Might as well return 0?
> +err:
> + if (args->smbios_module.data) {
> + free(args->smbios_module.data);
> + args->smbios_module.data = NULL;
> + }
> + if (args->acpi_module.data) {
> + free(args->acpi_module.data);
> + args->acpi_module.data = NULL;
> + }
DO you leak args->image_file_name here?
> +out:
> + return rc;
> }
>
> int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
> diff -r 35a0556a7f76 tools/libxl/libxl_types.idl
> --- a/tools/libxl/libxl_types.idl Thu Jan 10 17:32:10 2013 +0000
> +++ b/tools/libxl/libxl_types.idl Fri Jan 18 15:21:50 2013 -0500
> @@ -308,6 +308,8 @@ libxl_domain_build_info = Struct("domain
> ("vpt_align", libxl_defbool),
> ("timer_mode", libxl_timer_mode),
> ("nested_hvm", libxl_defbool),
> + ("smbios_firmware", string),
> + ("acpi_firmware", string),
> ("nographic", libxl_defbool),
> ("vga", libxl_vga_interface_info),
> ("vnc", libxl_vnc_info),
I think we ought to have a LIBXL_HAVE_FOO define for these two, in
libxl.h I suppose since the IDL doesn't support that sort of thing (not
sure if it should or not).
> diff -r 35a0556a7f76 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c Thu Jan 10 17:32:10 2013 +0000
> +++ b/tools/libxl/xl_cmdimpl.c Fri Jan 18 15:21:50 2013 -0500
> @@ -882,6 +882,11 @@ static void parse_config_data(const char
> }
>
> xlu_cfg_get_defbool(config, "nestedhvm", &b_info->u.hvm.nested_hvm, 0);
> +
> + xlu_cfg_replace_string (config, "smbios_firmware",
> + &b_info->u.hvm.smbios_firmware, 0);
> + xlu_cfg_replace_string (config, "acpi_firmware",
> + &b_info->u.hvm.acpi_firmware, 0);
> break;
> case LIBXL_DOMAIN_TYPE_PV:
> {
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 01/02] HVM firmware passthrough libxl support
2013-01-21 9:57 ` Ian Campbell
@ 2013-01-21 17:05 ` Ross Philipson
2013-01-21 17:12 ` Ian Campbell
0 siblings, 1 reply; 7+ messages in thread
From: Ross Philipson @ 2013-01-21 17:05 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
> -----Original Message-----
> From: Ian Campbell
> Sent: Monday, January 21, 2013 4:57 AM
> To: Ross Philipson
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH v1 01/02] HVM firmware passthrough libxl
> support
>
> On Fri, 2013-01-18 at 21:40 +0000, Ross Philipson wrote:
> > This patch introduces support for two new parameters in libxl:
> >
> > smbios_firmware=<path_to_smbios_structures_file>
> > acpi_firmware=<path_to_acpi_tables_file>
> >
> > The changes are primarily in the domain building code where the
> firmware files
> > are read and passed to libxc for loading into the new guest. After the
> domain
> > building call to libxc, the addresses for the loaded blobs are
> returned and
> > written to xenstore.
> >
> > Additionally, this patch switches to using the new xc_hvm_build()
> routine.
> >
> > Signed-off-by: Ross Philipson <ross.philipson@citrix.com>
> >
> >
> > diff -r 35a0556a7f76 tools/libxl/libxl_dom.c
> > --- a/tools/libxl/libxl_dom.c Thu Jan 10 17:32:10 2013 +0000
> > +++ b/tools/libxl/libxl_dom.c Fri Jan 18 15:21:50 2013 -0500
> > @@ -21,6 +21,7 @@
> >
> > #include <xc_dom.h>
> > #include <xen/hvm/hvm_info_table.h>
> > +#include <xen/hvm/hvm_xs_strings.h>
> >
> > libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
> > {
> > @@ -462,6 +463,7 @@ static unsigned long timer_mode(const li
> > mode <= LIBXL_TIMER_MODE_ONE_MISSED_TICK_PENDING);
> > return ((unsigned long)mode);
> > }
> > +
> > static int hvm_build_set_params(xc_interface *handle, uint32_t domid,
> > libxl_domain_build_info *info,
> > int store_evtchn, unsigned long
> *store_mfn,
> > @@ -510,11 +512,79 @@ static int hvm_build_set_params(xc_inter
> > return 0;
> > }
> >
> > -static const char *libxl__domain_firmware(libxl__gc *gc,
> > - libxl_domain_build_info
> *info)
> > +static int hvm_build_set_xs_values(libxl__gc *gc,
> > + uint32_t domid,
> > + struct xc_hvm_build_args *args)
> > +{
> > + libxl_ctx *ctx = libxl__gc_owner(gc);
> > + char *path = NULL;
> > + int ret = 0;
> > +
> > + if (args->smbios_module.guest_addr_out) {
> > + path = libxl__sprintf(gc,
> "/local/domain/%d/"HVM_XS_SMBIOS_PT_ADDRESS,
> > + domid);
> > + if (!path)
> > + goto str_err;
>
> xl will panic on memory allocation failure so you don't need to handle
> it yourself, which removes a lot of error handling.
>
> You could also use GCSPRINTF to shorten the line if you wanted
I will change that and look at using GCSPRINTF. I sort of just copied the
logging from elsewhere in that code.
>
> > +
> > + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64,
> > + args->smbios_module.guest_addr_out);
> > + if (ret)
> > + goto xs_err;
> > +
> > + path = libxl__sprintf(gc,
> "/local/domain/%d/"HVM_XS_SMBIOS_PT_LENGTH,
> > + domid);
> > + if (!path)
> > + goto str_err;
> > +
> > + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x",
> > + args->smbios_module.length);
> > + if (ret)
> > + goto xs_err;
> > + }
> > +
> > + if (args->acpi_module.guest_addr_out) {
> > + path = libxl__sprintf(gc,
> "/local/domain/%d/"HVM_XS_ACPI_PT_ADDRESS,
> > + domid);
> > + if (!path)
> > + goto str_err;
> > +
> > + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64,
> > + args->acpi_module.guest_addr_out);
> > + if (ret)
> > + goto xs_err;
> > +
> > + path = libxl__sprintf(gc,
> "/local/domain/%d/"HVM_XS_ACPI_PT_LENGTH,
> > + domid);
> > + if (!path)
> > + goto str_err;
> > +
> > + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x",
> > + args->acpi_module.length);
> > + if (ret)
> > + goto xs_err;
> > + }
> > +
> > + return 0;
> > +
> > +xs_err:
> > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > + "failed to write firmware xenstore value, err: %d",
> ret);
>
> You can use the LOG macro to shorten these long lines too.
Will look into it.
>
> > + return -1;
> > +str_err:
> > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > + "failed to get firmware xenstore path");
> > + return -1;
> > +}
> > +
> > +static int libxl__domain_firmware(libxl__gc *gc,
> > + libxl_domain_build_info *info,
> > + struct xc_hvm_build_args *args)
> > {
> > libxl_ctx *ctx = libxl__gc_owner(gc);
> > const char *firmware;
> > + int e, rc = ERROR_FAIL;
> > + int datalen = 0;
> > + void *data = 0;
> >
> > if (info->u.hvm.firmware)
> > firmware = info->u.hvm.firmware;
> > @@ -530,11 +600,68 @@ static const char *libxl__domain_firmwar
> > default:
> > LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "invalid device model
> version %d",
> > info->device_model_version);
> > - return NULL;
> > + return -1;
> > break;
> > }
> > }
> > - return libxl__abs_path(gc, firmware,
> libxl__xenfirmwaredir_path());
> > + args->image_file_name = libxl__abs_path(gc, firmware,
> > +
> libxl__xenfirmwaredir_path());
> > + if (!args->image_file_name) {
> > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "invalid firmware path");
> > + return -1;
> > + }
> > +
> > + if (info->u.hvm.smbios_firmware) {
> > + e = libxl_read_file_contents(ctx, info-
> >u.hvm.smbios_firmware,
> > + &data, &datalen);
> > + if (e) {
> > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > + "failed to read SMBIOS firmware file %s",
> > + info->u.hvm.smbios_firmware);
> > + goto err;
> > + }
> > + if (!datalen) {
> > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > + "SMBIOS firmware file %s is empty",
> > + info->u.hvm.smbios_firmware);
> > + goto err;
> > + }
> > + args->smbios_module.data = data;
> > + args->smbios_module.length = (uint32_t)datalen;
> > + }
> > +
> > + if (info->u.hvm.acpi_firmware) {
> > + e = libxl_read_file_contents(ctx, info->u.hvm.acpi_firmware,
> > + &data, &datalen);
> > + if (e) {
> > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > + "failed to read ACPI firmware file %s",
> > + info->u.hvm.acpi_firmware);
> > + goto err;
> > + }
> > + if (!datalen) {
> > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > + "ACPI firmware file %s is empty",
> > + info->u.hvm.acpi_firmware);
> > + goto err;
> > + }
> > + args->acpi_module.data = data;
> > + args->acpi_module.length = (uint32_t)datalen;
> > + }
> > +
> > + rc = 0;
> > + goto out;
>
> Might as well return 0?
Yup.
>
> > +err:
> > + if (args->smbios_module.data) {
> > + free(args->smbios_module.data);
> > + args->smbios_module.data = NULL;
> > + }
> > + if (args->acpi_module.data) {
> > + free(args->acpi_module.data);
> > + args->acpi_module.data = NULL;
> > + }
>
> DO you leak args->image_file_name here?
The previous code did not clean up the values returned from
libxl__abs_path() so I assumed it was like strings returned
from libxl__sprintf(), ie cleaned up by the GC.
>
> > +out:
> > + return rc;
> > }
> >
> > int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
>
> > diff -r 35a0556a7f76 tools/libxl/libxl_types.idl
> > --- a/tools/libxl/libxl_types.idl Thu Jan 10 17:32:10 2013 +0000
> > +++ b/tools/libxl/libxl_types.idl Fri Jan 18 15:21:50 2013 -0500
> > @@ -308,6 +308,8 @@ libxl_domain_build_info = Struct("domain
> > ("vpt_align",
> libxl_defbool),
> > ("timer_mode",
> libxl_timer_mode),
> > ("nested_hvm",
> libxl_defbool),
> > + ("smbios_firmware", string),
> > + ("acpi_firmware", string),
> > ("nographic",
> libxl_defbool),
> > ("vga",
> libxl_vga_interface_info),
> > ("vnc",
> libxl_vnc_info),
>
> I think we ought to have a LIBXL_HAVE_FOO define for these two, in
> libxl.h I suppose since the IDL doesn't support that sort of thing (not
> sure if it should or not).
I think I understand what you are suggesting (having read the comment
in libxl.h). I guess the thing that really breaks backward compat is
the code in libxl_dom.c that calls libxc. Should I introduce a
LIBXL_HAVE_FIRMWARE_PASSTHROUGH and have an additional block in
Libxl_dom.c with the old code that calls xc_hvm_build_target_mem()?
I guess this would be the first instance of a LIBXL_HAVE_* from the
looks of it.
>
> > diff -r 35a0556a7f76 tools/libxl/xl_cmdimpl.c
> > --- a/tools/libxl/xl_cmdimpl.c Thu Jan 10 17:32:10 2013 +0000
> > +++ b/tools/libxl/xl_cmdimpl.c Fri Jan 18 15:21:50 2013 -0500
> > @@ -882,6 +882,11 @@ static void parse_config_data(const char
> > }
> >
> > xlu_cfg_get_defbool(config, "nestedhvm", &b_info-
> >u.hvm.nested_hvm, 0);
> > +
> > + xlu_cfg_replace_string (config, "smbios_firmware",
> > + &b_info->u.hvm.smbios_firmware, 0);
> > + xlu_cfg_replace_string (config, "acpi_firmware",
> > + &b_info->u.hvm.acpi_firmware, 0);
> > break;
> > case LIBXL_DOMAIN_TYPE_PV:
> > {
> >
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 01/02] HVM firmware passthrough libxl support
2013-01-21 17:05 ` Ross Philipson
@ 2013-01-21 17:12 ` Ian Campbell
2013-01-21 18:35 ` Ross Philipson
0 siblings, 1 reply; 7+ messages in thread
From: Ian Campbell @ 2013-01-21 17:12 UTC (permalink / raw)
To: Ross Philipson; +Cc: xen-devel
On Mon, 2013-01-21 at 17:05 +0000, Ross Philipson wrote:
> I will change that and look at using GCSPRINTF. I sort of just copied the
> logging from elsewhere in that code.
The GC* things are pretty new, likewise the shortened LOG macros.
> >
> > > +err:
> > > + if (args->smbios_module.data) {
> > > + free(args->smbios_module.data);
> > > + args->smbios_module.data = NULL;
> > > + }
> > > + if (args->acpi_module.data) {
> > > + free(args->acpi_module.data);
> > > + args->acpi_module.data = NULL;
> > > + }
> >
> > DO you leak args->image_file_name here?
>
> The previous code did not clean up the values returned from
> libxl__abs_path() so I assumed it was like strings returned
> from libxl__sprintf(), ie cleaned up by the GC.
You are right, I missed that this was a gc allocation.
You could add the module data to the gc too btw and have it take care of
everything in that struct, arguably it is less confusing of each struct
only contains one "kind" of pointer. (the other option is to pass NOGC
to libxl__abs_path and manage that by hand too).
>
> >
> > > +out:
> > > + return rc;
> > > }
> > >
> > > int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
> >
> > > diff -r 35a0556a7f76 tools/libxl/libxl_types.idl
> > > --- a/tools/libxl/libxl_types.idl Thu Jan 10 17:32:10 2013 +0000
> > > +++ b/tools/libxl/libxl_types.idl Fri Jan 18 15:21:50 2013 -0500
> > > @@ -308,6 +308,8 @@ libxl_domain_build_info = Struct("domain
> > > ("vpt_align",
> > libxl_defbool),
> > > ("timer_mode",
> > libxl_timer_mode),
> > > ("nested_hvm",
> > libxl_defbool),
> > > + ("smbios_firmware", string),
> > > + ("acpi_firmware", string),
> > > ("nographic",
> > libxl_defbool),
> > > ("vga",
> > libxl_vga_interface_info),
> > > ("vnc",
> > libxl_vnc_info),
> >
> > I think we ought to have a LIBXL_HAVE_FOO define for these two, in
> > libxl.h I suppose since the IDL doesn't support that sort of thing (not
> > sure if it should or not).
>
> I think I understand what you are suggesting (having read the comment
> in libxl.h). I guess the thing that really breaks backward compat is
> the code in libxl_dom.c that calls libxc. Should I introduce a
> LIBXL_HAVE_FIRMWARE_PASSTHROUGH and have an additional block in
> Libxl_dom.c with the old code that calls xc_hvm_build_target_mem()?
I don't think that is necessary, all which is required is some way for
users of libxl (like libvirt, or xl if it weren't in tree) to tell if
they can try and use the new *_firmware fields or not. There's no need
to vary the libxl implementation based on it.
> I guess this would be the first instance of a LIBXL_HAVE_* from the
> looks of it.
Right.
>
> >
> > > diff -r 35a0556a7f76 tools/libxl/xl_cmdimpl.c
> > > --- a/tools/libxl/xl_cmdimpl.c Thu Jan 10 17:32:10 2013 +0000
> > > +++ b/tools/libxl/xl_cmdimpl.c Fri Jan 18 15:21:50 2013 -0500
> > > @@ -882,6 +882,11 @@ static void parse_config_data(const char
> > > }
> > >
> > > xlu_cfg_get_defbool(config, "nestedhvm", &b_info-
> > >u.hvm.nested_hvm, 0);
> > > +
> > > + xlu_cfg_replace_string (config, "smbios_firmware",
> > > + &b_info->u.hvm.smbios_firmware, 0);
> > > + xlu_cfg_replace_string (config, "acpi_firmware",
> > > + &b_info->u.hvm.acpi_firmware, 0);
> > > break;
> > > case LIBXL_DOMAIN_TYPE_PV:
> > > {
> > >
> > >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 01/02] HVM firmware passthrough libxl support
2013-01-21 17:12 ` Ian Campbell
@ 2013-01-21 18:35 ` Ross Philipson
2013-01-22 9:32 ` Ian Campbell
0 siblings, 1 reply; 7+ messages in thread
From: Ross Philipson @ 2013-01-21 18:35 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
> -----Original Message-----
> From: Ian Campbell
> Sent: Monday, January 21, 2013 12:12 PM
> To: Ross Philipson
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH v1 01/02] HVM firmware passthrough libxl
> support
>
> On Mon, 2013-01-21 at 17:05 +0000, Ross Philipson wrote:
>
> > I will change that and look at using GCSPRINTF. I sort of just copied
> the
> > logging from elsewhere in that code.
>
> The GC* things are pretty new, likewise the shortened LOG macros.
>
>
> > >
> > > > +err:
> > > > + if (args->smbios_module.data) {
> > > > + free(args->smbios_module.data);
> > > > + args->smbios_module.data = NULL;
> > > > + }
> > > > + if (args->acpi_module.data) {
> > > > + free(args->acpi_module.data);
> > > > + args->acpi_module.data = NULL;
> > > > + }
> > >
> > > DO you leak args->image_file_name here?
> >
> > The previous code did not clean up the values returned from
> > libxl__abs_path() so I assumed it was like strings returned
> > from libxl__sprintf(), ie cleaned up by the GC.
>
> You are right, I missed that this was a gc allocation.
>
> You could add the module data to the gc too btw and have it take care of
> everything in that struct, arguably it is less confusing of each struct
> only contains one "kind" of pointer. (the other option is to pass NOGC
> to libxl__abs_path and manage that by hand too).
Ok I can switch all the items in that struct to ones that are gc'ed. I
noticed there was not GC* macro for a straight allocation. Would adding
a GCZALLOC for consistency be ok?
>
> >
> > >
> > > > +out:
> > > > + return rc;
> > > > }
> > > >
> > > > int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
> > >
> > > > diff -r 35a0556a7f76 tools/libxl/libxl_types.idl
> > > > --- a/tools/libxl/libxl_types.idl Thu Jan 10 17:32:10 2013
> +0000
> > > > +++ b/tools/libxl/libxl_types.idl Fri Jan 18 15:21:50 2013 -
> 0500
> > > > @@ -308,6 +308,8 @@ libxl_domain_build_info = Struct("domain
> > > > ("vpt_align",
> > > libxl_defbool),
> > > > ("timer_mode",
> > > libxl_timer_mode),
> > > > ("nested_hvm",
> > > libxl_defbool),
> > > > + ("smbios_firmware",
> string),
> > > > + ("acpi_firmware",
> string),
> > > > ("nographic",
> > > libxl_defbool),
> > > > ("vga",
> > > libxl_vga_interface_info),
> > > > ("vnc",
> > > libxl_vnc_info),
> > >
> > > I think we ought to have a LIBXL_HAVE_FOO define for these two, in
> > > libxl.h I suppose since the IDL doesn't support that sort of thing
> (not
> > > sure if it should or not).
> >
> > I think I understand what you are suggesting (having read the comment
> > in libxl.h). I guess the thing that really breaks backward compat is
> > the code in libxl_dom.c that calls libxc. Should I introduce a
> > LIBXL_HAVE_FIRMWARE_PASSTHROUGH and have an additional block in
> > Libxl_dom.c with the old code that calls xc_hvm_build_target_mem()?
>
> I don't think that is necessary, all which is required is some way for
> users of libxl (like libvirt, or xl if it weren't in tree) to tell if
> they can try and use the new *_firmware fields or not. There's no need
> to vary the libxl implementation based on it.
>
> > I guess this would be the first instance of a LIBXL_HAVE_* from the
> > looks of it.
>
> Right.
I guess I misunderstood the use of the LIBXL_HAVE_* defines. So all I
really need is something like this in libxl.h with a comment about what
it is:
#define LIBXL_HAVE_FIRMWARE_PASSTHROUGH 1
Or am I still missing something?
>
> >
> > >
> > > > diff -r 35a0556a7f76 tools/libxl/xl_cmdimpl.c
> > > > --- a/tools/libxl/xl_cmdimpl.c Thu Jan 10 17:32:10 2013 +0000
> > > > +++ b/tools/libxl/xl_cmdimpl.c Fri Jan 18 15:21:50 2013 -0500
> > > > @@ -882,6 +882,11 @@ static void parse_config_data(const char
> > > > }
> > > >
> > > > xlu_cfg_get_defbool(config, "nestedhvm", &b_info-
> > > >u.hvm.nested_hvm, 0);
> > > > +
> > > > + xlu_cfg_replace_string (config, "smbios_firmware",
> > > > + &b_info->u.hvm.smbios_firmware,
> 0);
> > > > + xlu_cfg_replace_string (config, "acpi_firmware",
> > > > + &b_info->u.hvm.acpi_firmware, 0);
> > > > break;
> > > > case LIBXL_DOMAIN_TYPE_PV:
> > > > {
> > > >
> > > >
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 01/02] HVM firmware passthrough libxl support
2013-01-21 18:35 ` Ross Philipson
@ 2013-01-22 9:32 ` Ian Campbell
2013-01-22 15:47 ` Ross Philipson
0 siblings, 1 reply; 7+ messages in thread
From: Ian Campbell @ 2013-01-22 9:32 UTC (permalink / raw)
To: Ross Philipson; +Cc: xen-devel
On Mon, 2013-01-21 at 18:35 +0000, Ross Philipson wrote:
> > You could add the module data to the gc too btw and have it take care of
> > everything in that struct, arguably it is less confusing of each struct
> > only contains one "kind" of pointer. (the other option is to pass NOGC
> > to libxl__abs_path and manage that by hand too).
>
> Ok I can switch all the items in that struct to ones that are gc'ed. I
> noticed there was not GC* macro for a straight allocation. Would adding
> a GCZALLOC for consistency be ok?
I guest neither GCNEW nor GCNEW_ARRAY do what you need do they, so yes I
think that would be fine.
> I guess I misunderstood the use of the LIBXL_HAVE_* defines. So all I
> really need is something like this in libxl.h with a comment about what
> it is:
> #define LIBXL_HAVE_FIRMWARE_PASSTHROUGH 1
>
> Or am I still missing something?
Nope, that it. Just enough so that users can do:
if (opt = get_opt("firmware"))
{
#ifdef LIBXL_HAVE_FIRMWARE_PASSTHROUGH
d_config->....firmware = opt;
#else
fprintf(stderr, "Firmware not supported\n");
#endif
}
Ian.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 01/02] HVM firmware passthrough libxl support
2013-01-22 9:32 ` Ian Campbell
@ 2013-01-22 15:47 ` Ross Philipson
0 siblings, 0 replies; 7+ messages in thread
From: Ross Philipson @ 2013-01-22 15:47 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
-----Original Message-----
> From: Ian Campbell
> Sent: Tuesday, January 22, 2013 4:32 AM
> To: Ross Philipson
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH v1 01/02] HVM firmware passthrough libxl
> support
>
> On Mon, 2013-01-21 at 18:35 +0000, Ross Philipson wrote:
> > > You could add the module data to the gc too btw and have it take
> care of
> > > everything in that struct, arguably it is less confusing of each
> struct
> > > only contains one "kind" of pointer. (the other option is to pass
> NOGC
> > > to libxl__abs_path and manage that by hand too).
> >
> > Ok I can switch all the items in that struct to ones that are gc'ed. I
> > noticed there was not GC* macro for a straight allocation. Would
> adding
> > a GCZALLOC for consistency be ok?
>
> I guest neither GCNEW nor GCNEW_ARRAY do what you need do they, so yes I
> think that would be fine.
>
> > I guess I misunderstood the use of the LIBXL_HAVE_* defines. So all I
> > really need is something like this in libxl.h with a comment about
> what
> > it is:
> > #define LIBXL_HAVE_FIRMWARE_PASSTHROUGH 1
> >
> > Or am I still missing something?
>
> Nope, that it. Just enough so that users can do:
>
> if (opt = get_opt("firmware"))
> {
> #ifdef LIBXL_HAVE_FIRMWARE_PASSTHROUGH
> d_config->....firmware = opt;
> #else
> fprintf(stderr, "Firmware not supported\n");
> #endif
> }
>
> Ian.
Great, thanks. I should be able to resubmit this week.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-01-22 15:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-18 21:40 [PATCH v1 01/02] HVM firmware passthrough libxl support Ross Philipson
2013-01-21 9:57 ` Ian Campbell
2013-01-21 17:05 ` Ross Philipson
2013-01-21 17:12 ` Ian Campbell
2013-01-21 18:35 ` Ross Philipson
2013-01-22 9:32 ` Ian Campbell
2013-01-22 15:47 ` Ross Philipson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.