[libvirt] [PATCH v3 2/3] xenconfig: add conversions for xen-xl
Wim ten Have
wim.ten.have at oracle.com
Fri Apr 21 15:53:48 UTC 2017
On Thu, 20 Apr 2017 15:40:22 -0600
Jim Fehlig <jfehlig at suse.com> wrote:
> Wim Ten Have wrote:
> > From: Wim ten Have <wim.ten.have at oracle.com>
> >
> > Per xen-xl conversions from and to native under host-passthrough
> > mode we take care for Xen (nestedhvm = mode) applied and inherited
> > settings generating or processing correct feature policy:
> >
> > [On Intel (VT-x) architectures]
> > <feature policy='disable' name='vmx'/>
> >
> > or
> >
> > [On AMD (AMD-V) architectures]
> > <feature policy='disable' name='svm'/>
> >
> > It will then generate (or parse) for nestedhvm=1 in/from xl format.
> >
> > Signed-off-by: Joao Martins <joao.m.martins at oracle.com>
> > Signed-off-by: Wim ten Have <wim.ten.have at oracle.com>
> > ---
> > cfg.mk | 2 +-
> > src/xenconfig/xen_xl.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 65 insertions(+), 1 deletion(-)
> >
> > diff --git a/cfg.mk b/cfg.mk
> > index 69e3f3a..32c3725 100644
> > --- a/cfg.mk
> > +++ b/cfg.mk
> > @@ -777,7 +777,7 @@ sc_prohibit_cross_inclusion:
> > locking/) safe="($$dir|util|conf|rpc)";; \
> > cpu/| network/| node_device/| rpc/| security/| storage/) \
> > safe="($$dir|util|conf|storage)";; \
> > - xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen)";; \
> > + xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen|cpu)";; \
>
> It would be nice to get another libvirt dev opinion on this change. As I said in
> the V2 thread, it seems we could remove xenconfig from this check.
>
> > *) safe="($$dir|$(mid_dirs)|util)";; \
>
> E.g. let it be handled in this case.
In that case we have to add 'xen' to "mid_dirs" above.
--- a/cfg.mk
+++ b/cfg.mk
@@ -768,7 +768,7 @@ sc_prohibit_gettext_markup:
# lower-level code must not include higher-level headers.
cross_dirs=$(patsubst $(srcdir)/src/%.,%,$(wildcard $(srcdir)/src/*/.))
cross_dirs_re=($(subst / ,/|,$(cross_dirs)))
-mid_dirs=access|conf|cpu|locking|logging|network|node_device|rpc|security|storage
+mid_dirs=access|conf|cpu|locking|logging|network|node_device|rpc|security|storage|xen
Otherwise there's various other complains. ... sound like this is a bit
deserted area. My selection to add cpu under xenapi/|xenconfig/) was to
have it at lease minimized to the world of xen arena.
> > esac; \
> > in_vc_files="^src/$$dir" \
> > diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> > index 74f68b3..62af8b8 100644
> > --- a/src/xenconfig/xen_xl.c
> > +++ b/src/xenconfig/xen_xl.c
> > @@ -34,6 +34,7 @@
> > #include "virstoragefile.h"
> > #include "xen_xl.h"
> > #include "libxl_capabilities.h"
> > +#include "cpu/cpu.h"
> >
> > #define VIR_FROM_THIS VIR_FROM_XENXL
> >
> > @@ -106,6 +107,7 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
> > if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
> > const char *bios;
> > const char *boot;
> > + int val = 0;
> >
> > if (xenConfigGetString(conf, "bios", &bios, NULL) < 0)
> > return -1;
> > @@ -164,6 +166,40 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
> > }
> > def->os.nBootDevs++;
> > }
> > +
> > + if (xenConfigGetBool(conf, "nestedhvm", &val, -1) < 0)
> > + return -1;
> > +
> > + if (val != -1) {
> > + virCPUDefPtr cpu = NULL;
> > +
> > + if (VIR_ALLOC(cpu) < 0)
> > + return -1;
> > +
> > + if (val == 0) {
> > + bool isVTx = true;
> > +
> > + if (VIR_ALLOC(cpu->features) < 0) {
> > + VIR_FREE(cpu);
> > + return -1;
> > + }
> > +
> > + if (caps && caps->host.cpu && ARCH_IS_X86(def->os.arch))
> > + isVTx = virCPUCheckFeature(caps->host.arch, caps->host.cpu, "vmx");
> > +
> > + if (VIR_STRDUP(cpu->features->name, isVTx ? "vmx" : "svm") < 0) {
> > + VIR_FREE(cpu->features);
> > + VIR_FREE(cpu);
> > + return -1;
>
> So if I understand this correctly, the feature would have the name "vmx" if arch
> != x86. If arch == x86 but feature "vmx" is not found, then the feature name
> would be "svm".
>
> IMO, it would be better to ignore <cpu> altogether if we can't find the name of
> the virt technology feature to disable. Without a <cpu> def, you'd get the libxl
> default, which is nestedhvm=disabled (and also the current behavior of
> libvirt+libxl). E.g. what do you think of the below diff to your patch?
Appreciate below insight and added change adding fixated cpuDefaultFeatures for
testsutils under xen.
Charm on below is that is saves us from the additional brought allocation under
VIR_STRDUP. Let me bring you PATCH v4 next Monday which also includes the signature
correction as suggested initially.
Regards,
-Wim.
> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> index c536e57a0..4f24d457c 100644
> --- a/src/xenconfig/xen_xl.c
> +++ b/src/xenconfig/xen_xl.c
> @@ -170,36 +170,48 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def,
> virCapsPtr caps)
> if (xenConfigGetBool(conf, "nestedhvm", &val, -1) < 0)
> return -1;
>
> - if (val != -1) {
> - virCPUDefPtr cpu = NULL;
> + if (val == 1) {
> + virCPUDefPtr cpu;
>
> if (VIR_ALLOC(cpu) < 0)
> return -1;
>
> - if (val == 0) {
> - bool isVTx = true;
> + cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
> + cpu->type = VIR_CPU_TYPE_GUEST;
> + def->cpu = cpu;
> + } else if (val == 0) {
> + const char *vtfeature = NULL;
> +
> + if (caps && caps->host.cpu && ARCH_IS_X86(def->os.arch)) {
> + if (virCPUCheckFeature(caps->host.arch, caps->host.cpu, "vmx"))
> + vtfeature = "vmx";
> + else if (virCPUCheckFeature(caps->host.arch, caps->host.cpu,
> "svm"))
> + vtfeature = "svm";
> + }
> +
> + if (vtfeature) {
> + virCPUDefPtr cpu;
> +
> + if (VIR_ALLOC(cpu) < 0)
> + return -1;
>
> if (VIR_ALLOC(cpu->features) < 0) {
> VIR_FREE(cpu);
> return -1;
> }
>
> - if (caps && caps->host.cpu && ARCH_IS_X86(def->os.arch))
> - isVTx = virCPUCheckFeature(caps->host.arch, caps->host.cpu,
> "vmx");
> -
> - if (VIR_STRDUP(cpu->features->name, isVTx ? "vmx" : "svm") < 0) {
> + if (VIR_STRDUP(cpu->features->name, vtfeature) < 0) {
> VIR_FREE(cpu->features);
> VIR_FREE(cpu);
> return -1;
> }
> cpu->features->policy = VIR_CPU_FEATURE_DISABLE;
> cpu->nfeatures = cpu->nfeatures_max = 1;
> + cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
> + cpu->type = VIR_CPU_TYPE_GUEST;
> + def->cpu = cpu;
> }
> - cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
> - cpu->type = VIR_CPU_TYPE_GUEST;
> - def->cpu = cpu;
> }
> -
More information about the libvir-list
mailing list