Re: [PATCH v3.1 3/6] libxl: add support for CPUID features policy

[Jim Fehlig <jfehlig@xxxxxxxx>]

On 12/19/2017 06:19 AM, Joao Martins wrote:
> On 12/13/2017 07:09 PM, Marek Marczykowski-Górecki wrote:
> > Convert CPU features policy into libxl cpuid policy settings. Use new
> > ("libxl") syntax, which allow to enable/disable specific bits, using
> > host CPU as a base. For this reason, only "host-passthrough" mode is
> > accepted.
> > Libxl do not have distinction between "force" and "required" policy
> > (there is only "force") and also between "forbid" and "disable" (there
> > is only "disable"). So, merge them appropriately. If anything, "require"
> > and "forbid" should be enforced outside of specific driver.
> >
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
>
> This is quite neat Marek :)
>
> I have one comment towards the end, suggesting an alternative to a static
> translation table.
>
> > ---
> > Changes since v3:
> >   - compile fix (one more s/libxlTranslateCPUFeature/xenTranslateCPUFeature/)
> > Changes since v2:
> >   - drop spurious changes
> >   - move libxlTranslateCPUFeature function to xen_xl.c, to be reused by
> >   xenconfig driver
> > Changes since v1:
> >   - use new ("libxl") syntax to set only bits explicitly mentioned in
> >   domain XML
> > ---
> >   src/libxl/libxl_conf.c | 35 +++++++++++++++++++++++++++++++++--
> >   src/xenconfig/xen_xl.c | 34 ++++++++++++++++++++++++++++++++++
> >   src/xenconfig/xen_xl.h |  2 ++
> >   3 files changed, 69 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> > index 184610966..5eae6d10f 100644
> > --- a/src/libxl/libxl_conf.c
> > +++ b/src/libxl/libxl_conf.c
> > @@ -50,6 +50,7 @@
> >   #include "secret_util.h"
> >   #include "cpu/cpu.h"
> >   #include "xen_common.h"
> > +#include "xen_xl.h"
> >   
> >   
> >   #define VIR_FROM_THIS VIR_FROM_LIBXL
> > @@ -357,6 +358,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
> >               bool hasHwVirt = false;
> >               int nested_hvm = -1;
> >               bool svm = false, vmx = false;
> > +            char xlCPU[32];
> >   
> >               if (def->cpu->mode != (VIR_CPU_MODE_HOST_PASSTHROUGH)) {
> >                   virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > @@ -379,15 +381,44 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
> >                           case VIR_CPU_FEATURE_DISABLE:
> >                           case VIR_CPU_FEATURE_FORBID:
> >                               if ((vmx && STREQ(def->cpu->features[i].name, "vmx")) ||
> > -                                (svm && STREQ(def->cpu->features[i].name, "svm")))
> > +                                (svm && STREQ(def->cpu->features[i].name, "svm"))) {
> >                                   nested_hvm = 0;
> > +                                continue;
> > +                            }
> > +
> > +                            snprintf(xlCPU,
> > +                                    sizeof(xlCPU),
> > +                                    "%s=0",
> > +                                    xenTranslateCPUFeature(
> > +                                        def->cpu->features[i].name,
> > +                                        false));
> > +                            if (libxl_cpuid_parse_config(&b_info->cpuid, xlCPU)) {
> > +                                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                                        _("unsupported cpu feature '%s'"),
> > +                                        def->cpu->features[i].name);
> > +                                return -1;
> > +                            }
> >                               break;
> >   
> >                           case VIR_CPU_FEATURE_FORCE:
> >                           case VIR_CPU_FEATURE_REQUIRE:
> >                               if ((vmx && STREQ(def->cpu->features[i].name, "vmx")) ||
> > -                                (svm && STREQ(def->cpu->features[i].name, "svm")))
> > +                                (svm && STREQ(def->cpu->features[i].name, "svm"))) {
> >                                   nested_hvm = 1;
> > +                                continue;
> > +                            }
> > +
> > +                            snprintf(xlCPU,
> > +                                    sizeof(xlCPU),
> > +                                    "%s=1",
> > +                                    xenTranslateCPUFeature(
> > +                                        def->cpu->features[i].name, false));
> > +                            if (libxl_cpuid_parse_config(&b_info->cpuid, xlCPU)) {
> > +                                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                                        _("unsupported cpu feature '%s'"),
> > +                                        def->cpu->features[i].name);
> > +                                return -1;
> > +                            }
> >                               break;
> >                           case VIR_CPU_FEATURE_OPTIONAL:
> >                           case VIR_CPU_FEATURE_LAST:
> > diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> > index 317c7a08d..356ca3a4b 100644
> > --- a/src/xenconfig/xen_xl.c
> > +++ b/src/xenconfig/xen_xl.c
> > @@ -225,6 +225,40 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
> >       return 0;
> >   }
> >   
> > +/*
> > + * Translate CPU feature name from libvirt to libxl (from_libxl=false) or from
> > + * libxl to libvirt (from_libxl=true).
> > + */
> > +const char *
> > +xenTranslateCPUFeature(const char *feature_name, bool from_libxl)
> > +{
> > +    static const char *translation_table[][2] = {
> > +        /* libvirt name, libxl name */
> > +        { "cx16", "cmpxchg16" },
> > +        { "cid", "cntxid" },
> > +        { "ds_cpl", "dscpl" },
> > +        { "pclmuldq", "pclmulqdq" },
> > +        { "pni", "sse3" },
> > +        { "ht", "htt" },
> > +        { "pn", "psn" },
> > +        { "clflush", "clfsh" },
> > +        { "sep", "sysenter" },
> > +        { "cx8", "cmpxchg8" },
> > +        { "nodeid_msr", "nodeid" },
> > +        { "cr8legacy", "altmovcr8" },
> > +        { "lahf_lm", "lahfsahf" },
> > +        { "cmp_legacy", "cmplegacy" },
> > +        { "fxsr_opt", "ffxsr" },
> > +        { "pdpe1gb", "page1gb" },
> > +    };
> > +    size_t i;
> > +
> > +    for (i = 0; i < ARRAY_CARDINALITY(translation_table); i++)
> > +        if (STREQ(translation_table[i][from_libxl], feature_name))
> > +            return translation_table[i][!from_libxl];
> > +    return feature_name;
> > +}
> > +
> >   
>
> Cc'ing Jim as he may have some thoughts on the direction of this.
>
> What happens if the admin decides to change /usr/share/libvirt/cpu_map.xml?

Is changing existing content likely/practical?

> Also you can also add new leafs/features to cpu_map.xml

Right. And I suppose the table in libxl_cpuid.c can grow too. And in cases where 
they differ we'd need to update the static table, which we'll probably only 
remember to do when receiving bug reports. So I like the idea of making this 
more dynamic, but I apparently don't have enough brain power today to understand 
your suggestion :-).

> Maybe an idea instead is to have a table with all leafs that libxl has hardcoded
> (i.e. cpuid_flags array on
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/libxl_cpuid.c;hb=HEAD#l92).

Where would this table reside? In src/cpu/?

> Then adding a new cpu driver routine to look for cpuid data based on feature
> name (and the reverse too). Finally you would populate this translation table at
> driver load time, or you could even get away without a translation table (but
> would be a little more inefficient?).

I'm having difficulty understanding how this provides a dynamic solution. 
Wouldn't the table you mention above need extended anytime the hardcoded one in 
libxl was extended? Which would probably only happen after receiving a bug 
report? :-)

Hmm, the more I think about it, the more I convince myself that knowing libvirt 
and libxl use different names for a CPU feature is static information. I'm 
afraid I don't have any better ideas beyond Marek's neat trick.

Regards,
Jim

> >   static int
> >   xenParseXLSpice(virConfPtr conf, virDomainDefPtr def)
> > diff --git a/src/xenconfig/xen_xl.h b/src/xenconfig/xen_xl.h
> > index dd963268e..68f332aca 100644
> > --- a/src/xenconfig/xen_xl.h
> > +++ b/src/xenconfig/xen_xl.h
> > @@ -33,4 +33,6 @@ virDomainDefPtr xenParseXL(virConfPtr conn,
> >   
> >   virConfPtr xenFormatXL(virDomainDefPtr def, virConnectPtr);
> >   
> > +const char *xenTranslateCPUFeature(const char *feature_name, bool from_libxl);
> > +
> >   #endif /* __VIR_XEN_XL_H__ */

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list