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

[Joao Martins]

On 01/04/2018 12:43 AM, Marek Marczykowski-Górecki wrote:
> On Wed, Jan 03, 2018 at 05:00:10PM -0700, Jim Fehlig wrote:
>> On 12/19/2017 06:19 AM, Joao Martins wrote:
>>> On 12/13/2017 07:09 PM, Marek Marczykowski-Górecki wrote:
>>>> +/*
>>>> + * 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? :-)
> 
> Probably... And worse thing about such table is it needs to contain all
> entries from said libxl_cpuid.c. My solution require only listing
> entries with mismatching names.
> 
/nods and it would be a gigantic table most likely.

> Alternative would be to not use "new libxl syntax", but "old xend
> syntax" (which is still supported by libxl). The later allow to list
> specific bits instead of feature names. That was implemented in v1 of
> this patch series[1]... The problem with that approach is currently libvirt
> does not expose API for lookup of individual features, but only to
> construct full CPU and then enumerate its CPUID.

I kinda liked your xend version, provided we could lookup the feature bits as
you are hinting here.

> That means it isn't
> feasible for the current approach (mode='host-passthrough', then
> enable/disable individual features). See discussion on v1.
> Adding such API to libvirt cpu driver is beyond my knowledge of libvirt
> code and available time :/
> 
The main reason I suggesting this out was because this patch was hardcoding
libvirt feature names. Maybe it's not an issue :)

Joao