tools/libxl/libxl_dom.c | 183 ++---------------------------------------------- tools/libxl/libxl_x86.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 185 insertions(+), 179 deletions(-)
The call to xc_domain_disable_migrate() is made only from x86, while its
handling in Xen is common. Move it to the libxl__build_pre().
hvm_set_conf_params(), hvm_set_viridian_features(),
hvm_set_mca_capabilities(), and the altp2m logic is all in common
code (parts ifdef'd) but despite this, is all actually x86 specific.
Move it into x86 specific code, and fold all of the xc_hvm_param_set() calls
together into hvm_set_conf_params() in a far more coherent way.
Finally - ensure that all hypercalls have their return values checked.
No practical change in constructed domains. Fewer useless hypercalls now to
construct an ARM guest.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony Perard <anthony.perard@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
---
tools/libxl/libxl_dom.c | 183 ++----------------------------------------------
tools/libxl/libxl_x86.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 185 insertions(+), 179 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index cdb294ab8d..573c63692b 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -243,149 +243,6 @@ static int numa_place_domain(libxl__gc *gc, uint32_t domid,
return rc;
}
-static unsigned long timer_mode(const libxl_domain_build_info *info)
-{
- const libxl_timer_mode mode = info->timer_mode;
- assert(mode >= LIBXL_TIMER_MODE_DELAY_FOR_MISSED_TICKS &&
- mode <= LIBXL_TIMER_MODE_ONE_MISSED_TICK_PENDING);
- return ((unsigned long)mode);
-}
-
-#if defined(__i386__) || defined(__x86_64__)
-static int hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
- libxl_domain_build_info *const info)
-{
- libxl_bitmap enlightenments;
- libxl_viridian_enlightenment v;
- uint64_t mask = 0;
-
- libxl_bitmap_init(&enlightenments);
- libxl_bitmap_alloc(CTX, &enlightenments,
- LIBXL_BUILDINFO_HVM_VIRIDIAN_ENABLE_DISABLE_WIDTH);
-
- if (libxl_defbool_val(info->u.hvm.viridian)) {
- /* Enable defaults */
- libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE);
- libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ);
- libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_TIME_REF_COUNT);
- libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_APIC_ASSIST);
- libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_CRASH_CTL);
- }
-
- libxl_for_each_set_bit(v, info->u.hvm.viridian_enable) {
- if (libxl_bitmap_test(&info->u.hvm.viridian_disable, v)) {
- LOG(ERROR, "%s group both enabled and disabled",
- libxl_viridian_enlightenment_to_string(v));
- goto err;
- }
- if (libxl_viridian_enlightenment_to_string(v)) /* check validity */
- libxl_bitmap_set(&enlightenments, v);
- }
-
- libxl_for_each_set_bit(v, info->u.hvm.viridian_disable)
- if (libxl_viridian_enlightenment_to_string(v)) /* check validity */
- libxl_bitmap_reset(&enlightenments, v);
-
- /* The base set is a pre-requisite for all others */
- if (!libxl_bitmap_is_empty(&enlightenments) &&
- !libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE)) {
- LOG(ERROR, "base group not enabled");
- goto err;
- }
-
- libxl_for_each_set_bit(v, enlightenments)
- LOG(DETAIL, "%s group enabled", libxl_viridian_enlightenment_to_string(v));
-
- if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE)) {
- mask |= HVMPV_base_freq;
-
- if (!libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ))
- mask |= HVMPV_no_freq;
- }
-
- if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_TIME_REF_COUNT))
- mask |= HVMPV_time_ref_count;
-
- if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_REFERENCE_TSC))
- mask |= HVMPV_reference_tsc;
-
- if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_HCALL_REMOTE_TLB_FLUSH))
- mask |= HVMPV_hcall_remote_tlb_flush;
-
- if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_APIC_ASSIST))
- mask |= HVMPV_apic_assist;
-
- if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_CRASH_CTL))
- mask |= HVMPV_crash_ctl;
-
- if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_SYNIC))
- mask |= HVMPV_synic;
-
- if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_STIMER))
- mask |= HVMPV_time_ref_count | HVMPV_synic | HVMPV_stimer;
-
- if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_HCALL_IPI))
- mask |= HVMPV_hcall_ipi;
-
- if (mask != 0 &&
- xc_hvm_param_set(CTX->xch,
- domid,
- HVM_PARAM_VIRIDIAN,
- mask) != 0) {
- LOGE(ERROR, "Couldn't set viridian feature mask (0x%"PRIx64")", mask);
- goto err;
- }
-
- libxl_bitmap_dispose(&enlightenments);
- return 0;
-
-err:
- libxl_bitmap_dispose(&enlightenments);
- return ERROR_FAIL;
-}
-
-static int hvm_set_mca_capabilities(libxl__gc *gc, uint32_t domid,
- libxl_domain_build_info *const info)
-{
- unsigned long caps = info->u.hvm.mca_caps;
-
- if (!caps)
- return 0;
-
- return xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_MCA_CAP, caps);
-}
-#endif
-
-static void hvm_set_conf_params(xc_interface *handle, uint32_t domid,
- libxl_domain_build_info *const info)
-{
- switch(info->type) {
- case LIBXL_DOMAIN_TYPE_PVH:
- xc_hvm_param_set(handle, domid, HVM_PARAM_PAE_ENABLED, true);
- xc_hvm_param_set(handle, domid, HVM_PARAM_TIMER_MODE,
- timer_mode(info));
- xc_hvm_param_set(handle, domid, HVM_PARAM_NESTEDHVM,
- libxl_defbool_val(info->nested_hvm));
- break;
- case LIBXL_DOMAIN_TYPE_HVM:
- xc_hvm_param_set(handle, domid, HVM_PARAM_PAE_ENABLED,
- libxl_defbool_val(info->u.hvm.pae));
-#if defined(__i386__) || defined(__x86_64__)
- xc_hvm_param_set(handle, domid, HVM_PARAM_HPET_ENABLED,
- libxl_defbool_val(info->u.hvm.hpet));
-#endif
- xc_hvm_param_set(handle, domid, HVM_PARAM_TIMER_MODE,
- timer_mode(info));
- xc_hvm_param_set(handle, domid, HVM_PARAM_VPT_ALIGN,
- libxl_defbool_val(info->u.hvm.vpt_align));
- xc_hvm_param_set(handle, domid, HVM_PARAM_NESTEDHVM,
- libxl_defbool_val(info->nested_hvm));
- break;
- default:
- abort();
- }
-}
-
int libxl__build_pre(libxl__gc *gc, uint32_t domid,
libxl_domain_config *d_config, libxl__domain_build_state *state)
{
@@ -400,6 +257,12 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
return ERROR_FAIL;
}
+ if (libxl_defbool_val(d_config->b_info.disable_migrate) &&
+ xc_domain_disable_migrate(ctx->xch, domid) != 0) {
+ LOG(ERROR, "Couldn't set nomigrate");
+ return ERROR_FAIL;
+ }
+
/*
* Check if the domain has any CPU or node affinity already. If not, try
* to build up the latter via automatic NUMA placement. In fact, in case
@@ -522,40 +385,6 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
state->store_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->store_domid);
state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->console_domid);
- if (info->type != LIBXL_DOMAIN_TYPE_PV)
- hvm_set_conf_params(ctx->xch, domid, info);
-
-#if defined(__i386__) || defined(__x86_64__)
- if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
- rc = hvm_set_viridian_features(gc, domid, info);
- if (rc)
- return rc;
-
- rc = hvm_set_mca_capabilities(gc, domid, info);
- if (rc)
- return rc;
- }
-#endif
-
- /* Alternate p2m support on x86 is available only for PVH/HVM guests. */
- if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
- /* The config parameter "altp2m" replaces the parameter "altp2mhvm". For
- * legacy reasons, both parameters are accepted on x86 HVM guests.
- *
- * If the legacy field info->u.hvm.altp2m is set, activate altp2m.
- * Otherwise set altp2m based on the field info->altp2m. */
- if (info->altp2m == LIBXL_ALTP2M_MODE_DISABLED &&
- libxl_defbool_val(info->u.hvm.altp2m))
- xc_hvm_param_set(ctx->xch, domid, HVM_PARAM_ALTP2M,
- libxl_defbool_val(info->u.hvm.altp2m));
- else
- xc_hvm_param_set(ctx->xch, domid, HVM_PARAM_ALTP2M,
- info->altp2m);
- } else if (info->type == LIBXL_DOMAIN_TYPE_PVH) {
- xc_hvm_param_set(ctx->xch, domid, HVM_PARAM_ALTP2M,
- info->altp2m);
- }
-
rc = libxl__arch_domain_create(gc, d_config, domid);
return rc;
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 8b804537ba..1cae0e2b26 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -285,14 +285,193 @@ static int libxl__e820_alloc(libxl__gc *gc, uint32_t domid,
return 0;
}
+static unsigned long timer_mode(const libxl_domain_build_info *info)
+{
+ const libxl_timer_mode mode = info->timer_mode;
+ assert(mode >= LIBXL_TIMER_MODE_DELAY_FOR_MISSED_TICKS &&
+ mode <= LIBXL_TIMER_MODE_ONE_MISSED_TICK_PENDING);
+ return ((unsigned long)mode);
+}
+
+static int hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
+ const libxl_domain_build_info *info)
+{
+ libxl_bitmap enlightenments;
+ libxl_viridian_enlightenment v;
+ uint64_t mask = 0;
+
+ libxl_bitmap_init(&enlightenments);
+ libxl_bitmap_alloc(CTX, &enlightenments,
+ LIBXL_BUILDINFO_HVM_VIRIDIAN_ENABLE_DISABLE_WIDTH);
+
+ if (libxl_defbool_val(info->u.hvm.viridian)) {
+ /* Enable defaults */
+ libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE);
+ libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ);
+ libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_TIME_REF_COUNT);
+ libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_APIC_ASSIST);
+ libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_CRASH_CTL);
+ }
+
+ libxl_for_each_set_bit(v, info->u.hvm.viridian_enable) {
+ if (libxl_bitmap_test(&info->u.hvm.viridian_disable, v)) {
+ LOG(ERROR, "%s group both enabled and disabled",
+ libxl_viridian_enlightenment_to_string(v));
+ goto err;
+ }
+ if (libxl_viridian_enlightenment_to_string(v)) /* check validity */
+ libxl_bitmap_set(&enlightenments, v);
+ }
+
+ libxl_for_each_set_bit(v, info->u.hvm.viridian_disable)
+ if (libxl_viridian_enlightenment_to_string(v)) /* check validity */
+ libxl_bitmap_reset(&enlightenments, v);
+
+ /* The base set is a pre-requisite for all others */
+ if (!libxl_bitmap_is_empty(&enlightenments) &&
+ !libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE)) {
+ LOG(ERROR, "base group not enabled");
+ goto err;
+ }
+
+ libxl_for_each_set_bit(v, enlightenments)
+ LOG(DETAIL, "%s group enabled", libxl_viridian_enlightenment_to_string(v));
+
+ if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE)) {
+ mask |= HVMPV_base_freq;
+
+ if (!libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ))
+ mask |= HVMPV_no_freq;
+ }
+
+ if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_TIME_REF_COUNT))
+ mask |= HVMPV_time_ref_count;
+
+ if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_REFERENCE_TSC))
+ mask |= HVMPV_reference_tsc;
+
+ if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_HCALL_REMOTE_TLB_FLUSH))
+ mask |= HVMPV_hcall_remote_tlb_flush;
+
+ if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_APIC_ASSIST))
+ mask |= HVMPV_apic_assist;
+
+ if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_CRASH_CTL))
+ mask |= HVMPV_crash_ctl;
+
+ if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_SYNIC))
+ mask |= HVMPV_synic;
+
+ if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_STIMER))
+ mask |= HVMPV_time_ref_count | HVMPV_synic | HVMPV_stimer;
+
+ if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_HCALL_IPI))
+ mask |= HVMPV_hcall_ipi;
+
+ if (mask != 0 &&
+ xc_hvm_param_set(CTX->xch,
+ domid,
+ HVM_PARAM_VIRIDIAN,
+ mask) != 0) {
+ LOGE(ERROR, "Couldn't set viridian feature mask (0x%"PRIx64")", mask);
+ goto err;
+ }
+
+ libxl_bitmap_dispose(&enlightenments);
+ return 0;
+
+err:
+ libxl_bitmap_dispose(&enlightenments);
+ return ERROR_FAIL;
+}
+
+static int hvm_set_conf_params(libxl__gc *gc, uint32_t domid,
+ const libxl_domain_build_info *info)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ xc_interface *xch = ctx->xch;
+ int ret = ERROR_FAIL;
+ bool pae = true, altp2m = info->altp2m;
+
+ switch(info->type) {
+ case LIBXL_DOMAIN_TYPE_HVM:
+ pae = libxl_defbool_val(info->u.hvm.pae);
+
+ /* The config parameter "altp2m" replaces the parameter "altp2mhvm". For
+ * legacy reasons, both parameters are accepted on x86 HVM guests.
+ *
+ * If the legacy field info->u.hvm.altp2m is set, activate altp2m.
+ * Otherwise set altp2m based on the field info->altp2m. */
+ if (info->altp2m == LIBXL_ALTP2M_MODE_DISABLED &&
+ libxl_defbool_val(info->u.hvm.altp2m))
+ altp2m = libxl_defbool_val(info->u.hvm.altp2m);
+
+ if (xc_hvm_param_set(xch, domid, HVM_PARAM_HPET_ENABLED,
+ libxl_defbool_val(info->u.hvm.hpet))) {
+ LOG(ERROR, "Couldn't set HVM_PARAM_HPET_ENABLED");
+ goto out;
+ }
+ if (xc_hvm_param_set(xch, domid, HVM_PARAM_VPT_ALIGN,
+ libxl_defbool_val(info->u.hvm.vpt_align))) {
+ LOG(ERROR, "Couldn't set HVM_PARAM_VPT_ALIGN");
+ goto out;
+ }
+ if (info->u.hvm.mca_caps &&
+ xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_MCA_CAP,
+ info->u.hvm.mca_caps)) {
+ LOG(ERROR, "Couldn't set HVM_PARAM_MCA_CAP");
+ goto out;
+ }
+
+ /* Fallthrough */
+ case LIBXL_DOMAIN_TYPE_PVH:
+ if (xc_hvm_param_set(xch, domid, HVM_PARAM_PAE_ENABLED, pae)) {
+ LOG(ERROR, "Couldn't set HVM_PARAM_PAE_ENABLED");
+ goto out;
+ }
+ if (xc_hvm_param_set(xch, domid, HVM_PARAM_TIMER_MODE,
+ timer_mode(info))) {
+ LOG(ERROR, "Couldn't set HVM_PARAM_TIMER_MODE");
+ goto out;
+ }
+ if (xc_hvm_param_set(xch, domid, HVM_PARAM_NESTEDHVM,
+ libxl_defbool_val(info->nested_hvm))) {
+ LOG(ERROR, "Couldn't set HVM_PARAM_NESTEDHVM");
+ goto out;
+ }
+ if (xc_hvm_param_set(xch, domid, HVM_PARAM_ALTP2M, altp2m)) {
+ LOG(ERROR, "Couldn't set HVM_PARAM_ALTP2M");
+ goto out;
+ }
+ break;
+
+ default:
+ abort();
+ }
+
+ ret = 0;
+
+ out:
+ return ret;
+}
+
int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
uint32_t domid)
{
+ const libxl_domain_build_info *info = &d_config->b_info;
int ret = 0;
int tsc_mode;
uint32_t rtc_timeoffset;
libxl_ctx *ctx = libxl__gc_owner(gc);
+ if (info->type != LIBXL_DOMAIN_TYPE_PV &&
+ (ret = hvm_set_conf_params(gc, domid, info)) != 0)
+ goto out;
+
+ if (info->type == LIBXL_DOMAIN_TYPE_HVM &&
+ (ret = hvm_set_viridian_features(gc, domid, info)) != 0)
+ goto out;
+
if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_PV)
xc_domain_set_memmap_limit(ctx->xch, domid,
(d_config->b_info.max_memkb +
@@ -322,8 +501,6 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
goto out;
}
- if (libxl_defbool_val(d_config->b_info.disable_migrate))
- xc_domain_disable_migrate(ctx->xch, domid);
rtc_timeoffset = d_config->b_info.rtc_timeoffset;
if (libxl_defbool_val(d_config->b_info.localtime)) {
time_t t;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 27.12.2019 14:45, Andrew Cooper wrote: > The call to xc_domain_disable_migrate() is made only from x86, while its > handling in Xen is common. Move it to the libxl__build_pre(). > > hvm_set_conf_params(), hvm_set_viridian_features(), > hvm_set_mca_capabilities(), and the altp2m logic is all in common > code (parts ifdef'd) but despite this, is all actually x86 specific. > > Move it into x86 specific code, and fold all of the xc_hvm_param_set() calls > together into hvm_set_conf_params() in a far more coherent way. > > Finally - ensure that all hypercalls have their return values checked. > > No practical change in constructed domains. Fewer useless hypercalls now to > construct an ARM guest. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Ian Jackson <Ian.Jackson@citrix.com> > CC: Wei Liu <wl@xen.org> > CC: Anthony Perard <anthony.perard@citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wl@xen.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > --- > tools/libxl/libxl_dom.c | 183 ++---------------------------------------------- > tools/libxl/libxl_x86.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++- I'll defer to the tool stack maintainers here. Imo this file would better be split - one to contain stuff better falling under x86 maintainership, and the other for everything falling in the tool stack realm. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Fri, Dec 27, 2019 at 02:50:58PM +0100, Jan Beulich wrote: > On 27.12.2019 14:45, Andrew Cooper wrote: > > The call to xc_domain_disable_migrate() is made only from x86, while its > > handling in Xen is common. Move it to the libxl__build_pre(). > > > > hvm_set_conf_params(), hvm_set_viridian_features(), > > hvm_set_mca_capabilities(), and the altp2m logic is all in common > > code (parts ifdef'd) but despite this, is all actually x86 specific. > > > > Move it into x86 specific code, and fold all of the xc_hvm_param_set() calls > > together into hvm_set_conf_params() in a far more coherent way. > > > > Finally - ensure that all hypercalls have their return values checked. > > > > No practical change in constructed domains. Fewer useless hypercalls now to > > construct an ARM guest. > > > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > --- > > CC: Ian Jackson <Ian.Jackson@citrix.com> > > CC: Wei Liu <wl@xen.org> > > CC: Anthony Perard <anthony.perard@citrix.com> > > CC: Jan Beulich <JBeulich@suse.com> > > CC: Wei Liu <wl@xen.org> > > CC: Roger Pau Monné <roger.pau@citrix.com> > > CC: Stefano Stabellini <sstabellini@kernel.org> > > CC: Julien Grall <julien@xen.org> > > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > > --- > > tools/libxl/libxl_dom.c | 183 ++---------------------------------------------- > > tools/libxl/libxl_x86.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++- > > I'll defer to the tool stack maintainers here. Imo this file would > better be split - one to contain stuff better falling under x86 > maintainership, and the other for everything falling in the tool > stack realm. (Assuming you're talking about libxl_x86.c) That works for me. It is unclear to me how clean that split can be though... Wei. > > Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Fri, Dec 27, 2019 at 01:45:16PM +0000, Andrew Cooper wrote: > The call to xc_domain_disable_migrate() is made only from x86, while its > handling in Xen is common. Move it to the libxl__build_pre(). > > hvm_set_conf_params(), hvm_set_viridian_features(), > hvm_set_mca_capabilities(), and the altp2m logic is all in common > code (parts ifdef'd) but despite this, is all actually x86 specific. > > Move it into x86 specific code, and fold all of the xc_hvm_param_set() calls > together into hvm_set_conf_params() in a far more coherent way. > > Finally - ensure that all hypercalls have their return values checked. > > No practical change in constructed domains. Fewer useless hypercalls now to > construct an ARM guest. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> I'm fine with moving the code. AIUI Arm guests also need to set at least one hvm param for callback vector, but that's already handled in libxl_arm.c. As far as I can tell the code is correct, there is no code in between the moved code that depends on some of the fields being set in a specific order, so: Acked-by: Wei Liu <wl@xen.org> Wei. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Hi Andrew, On 27/12/2019 13:45, Andrew Cooper wrote: > The call to xc_domain_disable_migrate() is made only from x86, while its > handling in Xen is common. Move it to the libxl__build_pre(). > > hvm_set_conf_params(), hvm_set_viridian_features(), > hvm_set_mca_capabilities(), and the altp2m logic is all in common > code (parts ifdef'd) but despite this, is all actually x86 specific. While altp2m is only supported on x86, the concept is not x86-specific. I am actually aware of people using altp2m on Arm, althought the support is not upstream yet. > > Move it into x86 specific code, and fold all of the xc_hvm_param_set() calls > together into hvm_set_conf_params() in a far more coherent way. > > Finally - ensure that all hypercalls have their return values checked. > > No practical change in constructed domains. Fewer useless hypercalls now to > construct an ARM guest. I think it would be best to keep anything that we know can be used on arm (or new architecture) in common code. I am thinking about "nestedhvm" and "altp2m". Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 30/12/2019 13:15, Julien Grall wrote: > Hi Andrew, > > On 27/12/2019 13:45, Andrew Cooper wrote: >> The call to xc_domain_disable_migrate() is made only from x86, while its >> handling in Xen is common. Move it to the libxl__build_pre(). >> >> hvm_set_conf_params(), hvm_set_viridian_features(), >> hvm_set_mca_capabilities(), and the altp2m logic is all in common >> code (parts ifdef'd) but despite this, is all actually x86 specific. > > While altp2m is only supported on x86, the concept is not > x86-specific. I am actually aware of people using altp2m on Arm, > althought the support is not upstream yet. > >> >> Move it into x86 specific code, and fold all of the >> xc_hvm_param_set() calls >> together into hvm_set_conf_params() in a far more coherent way. >> >> Finally - ensure that all hypercalls have their return values checked. >> >> No practical change in constructed domains. Fewer useless hypercalls >> now to >> construct an ARM guest. > > I think it would be best to keep anything that we know can be used on > arm (or new architecture) in common code. I am thinking about > "nestedhvm" and "altp2m". Neither of those options are going to survive in this form. Also, the checks can't stay in common code. Currently, Xen doesn't reject bad parameters, and the toolstack doesn't check return values. Frankly, neither of these bugs should ever have got through code review, seeing as we were doing rather better code review by the time the ARM port came about. I've fixed the libxl code to check return values, but when the hypervisor has its "not-quite-an-XSA-because-the-guest-induced-damage-is-in-unsupported-subsystems" bugs fixed, these hypercalls will start failing. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Hi, On 30/12/2019 13:38, Andrew Cooper wrote: > On 30/12/2019 13:15, Julien Grall wrote: >> Hi Andrew, >> >> On 27/12/2019 13:45, Andrew Cooper wrote: >>> The call to xc_domain_disable_migrate() is made only from x86, while its >>> handling in Xen is common. Move it to the libxl__build_pre(). >>> >>> hvm_set_conf_params(), hvm_set_viridian_features(), >>> hvm_set_mca_capabilities(), and the altp2m logic is all in common >>> code (parts ifdef'd) but despite this, is all actually x86 specific. >> >> While altp2m is only supported on x86, the concept is not >> x86-specific. I am actually aware of people using altp2m on Arm, >> althought the support is not upstream yet. >> >>> >>> Move it into x86 specific code, and fold all of the >>> xc_hvm_param_set() calls >>> together into hvm_set_conf_params() in a far more coherent way. >>> >>> Finally - ensure that all hypercalls have their return values checked. >>> >>> No practical change in constructed domains. Fewer useless hypercalls >>> now to >>> construct an ARM guest. >> >> I think it would be best to keep anything that we know can be used on >> arm (or new architecture) in common code. I am thinking about >> "nestedhvm" and "altp2m". > > Neither of those options are going to survive in this form. Oh, it wasn't clear from the commit message. Would you mind to add a sentence in the commit message about it? > > Also, the checks can't stay in common code. Currently, Xen doesn't > reject bad parameters, and the toolstack doesn't check return values. > Frankly, neither of these bugs should ever have got through code review, > seeing as we were doing rather better code review by the time the ARM > port came about. The HVM_PARAM is not great on Arm :(. It would be nice to get this fixed properly. > > I've fixed the libxl code to check return values, but when the > hypervisor has its > "not-quite-an-XSA-because-the-guest-induced-damage-is-in-unsupported-subsystems" > bugs fixed, these hypercalls will start failing. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 30/12/2019 13:45, Julien Grall wrote: > Hi, > > On 30/12/2019 13:38, Andrew Cooper wrote: >> On 30/12/2019 13:15, Julien Grall wrote: >>> Hi Andrew, >>> >>> On 27/12/2019 13:45, Andrew Cooper wrote: >>>> The call to xc_domain_disable_migrate() is made only from x86, >>>> while its >>>> handling in Xen is common. Move it to the libxl__build_pre(). >>>> >>>> hvm_set_conf_params(), hvm_set_viridian_features(), >>>> hvm_set_mca_capabilities(), and the altp2m logic is all in common >>>> code (parts ifdef'd) but despite this, is all actually x86 specific. >>> >>> While altp2m is only supported on x86, the concept is not >>> x86-specific. I am actually aware of people using altp2m on Arm, >>> althought the support is not upstream yet. >>> >>>> >>>> Move it into x86 specific code, and fold all of the >>>> xc_hvm_param_set() calls >>>> together into hvm_set_conf_params() in a far more coherent way. >>>> >>>> Finally - ensure that all hypercalls have their return values checked. >>>> >>>> No practical change in constructed domains. Fewer useless hypercalls >>>> now to >>>> construct an ARM guest. >>> >>> I think it would be best to keep anything that we know can be used on >>> arm (or new architecture) in common code. I am thinking about >>> "nestedhvm" and "altp2m". >> >> Neither of those options are going to survive in this form. > > Oh, it wasn't clear from the commit message. Would you mind to add a > sentence in the commit message about it? Well - they aren't going to survive long-term in this form. Both need to become domain_create parameters. Whether or not they actually get changed before someone tries upstreaming the ARM Altp2m work is a different matter, if that affects your answer. > >> >> Also, the checks can't stay in common code. Currently, Xen doesn't >> reject bad parameters, and the toolstack doesn't check return values. >> Frankly, neither of these bugs should ever have got through code review, >> seeing as we were doing rather better code review by the time the ARM >> port came about. > > The HVM_PARAM is not great on Arm :(. It would be nice to get this > fixed properly. I looked back at my patch series doing just this, and despite being a year old, I'm still sufficiently irritated at the nitpicking and inability to read/interpret CODING_STYLE that I don't feel like wasting any more of my time right now. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Hi Andrew, On 02/01/2020 17:40, Andrew Cooper wrote: > On 30/12/2019 13:45, Julien Grall wrote: >> Hi, >> >> On 30/12/2019 13:38, Andrew Cooper wrote: >>> On 30/12/2019 13:15, Julien Grall wrote: >>>> Hi Andrew, >>>> >>>> On 27/12/2019 13:45, Andrew Cooper wrote: >>>>> The call to xc_domain_disable_migrate() is made only from x86, >>>>> while its >>>>> handling in Xen is common. Move it to the libxl__build_pre(). >>>>> >>>>> hvm_set_conf_params(), hvm_set_viridian_features(), >>>>> hvm_set_mca_capabilities(), and the altp2m logic is all in common >>>>> code (parts ifdef'd) but despite this, is all actually x86 specific. >>>> >>>> While altp2m is only supported on x86, the concept is not >>>> x86-specific. I am actually aware of people using altp2m on Arm, >>>> althought the support is not upstream yet. >>>> >>>>> >>>>> Move it into x86 specific code, and fold all of the >>>>> xc_hvm_param_set() calls >>>>> together into hvm_set_conf_params() in a far more coherent way. >>>>> >>>>> Finally - ensure that all hypercalls have their return values checked. >>>>> >>>>> No practical change in constructed domains. Fewer useless hypercalls >>>>> now to >>>>> construct an ARM guest. >>>> >>>> I think it would be best to keep anything that we know can be used on >>>> arm (or new architecture) in common code. I am thinking about >>>> "nestedhvm" and "altp2m". >>> >>> Neither of those options are going to survive in this form. >> >> Oh, it wasn't clear from the commit message. Would you mind to add a >> sentence in the commit message about it? > > Well - they aren't going to survive long-term in this form. Both need > to become domain_create parameters. > > Whether or not they actually get changed before someone tries > upstreaming the ARM Altp2m work is a different matter, if that affects > your answer. I am happy with "They should not survive". If the altp2m work is upstreamed first then we can have the discussion whether we should carry "legacy" interface if this wasn't reworked beforehand. > >> >>> >>> Also, the checks can't stay in common code. Currently, Xen doesn't >>> reject bad parameters, and the toolstack doesn't check return values. >>> Frankly, neither of these bugs should ever have got through code review, >>> seeing as we were doing rather better code review by the time the ARM >>> port came about. >> >> The HVM_PARAM is not great on Arm :(. It would be nice to get this >> fixed properly. > > I looked back at my patch series doing just this, and despite being a > year old, I'm still sufficiently irritated at the nitpicking and > inability to read/interpret CODING_STYLE that I don't feel like wasting > any more of my time right now. Sorry to hear that. I have put the work in my tracking list for Arm. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.