Re: [Xen-devel] [PATCH v2 05/30] xen/public: Export cpu featureset information in the public API

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On 02/20/2016 04:17 PM, Andrew Cooper wrote:
> On 19/02/16 22:03, Joao Martins wrote:
>> On 02/19/2016 05:55 PM, Andrew Cooper wrote:
>>> On 19/02/16 17:29, Joao Martins wrote:
>>>> On 02/05/2016 01:41 PM, Andrew Cooper wrote:
>>>>> For the featureset to be a useful object, it needs a stable 
>>>>> interpretation, a
>>>>> property which is missing from the current hw_caps interface.
>>>>>
>>>>> Additionly, introduce TSC_ADJUST, SHA, PREFETCHWT1, ITSC, EFRO and CLZERO
>>>>> which will be used by later changes.
>>>>>
>>>>> To maintain compilation, FSCAPINTS is currently hardcoded at 9.  Future
>>>>> changes will change this to being dynamically generated.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> Hey Andrew!
>>>>
>>>> There are a few word motions in this patch:
>>> Indeed there are.  They are in aid of getting a new clean interface.
>>>
>>>> [current]   [this series]
>>>> word 0   ->  word 0
>>>> word 4   ->  word 1
>>>> word 1,6 ->  word 2,3
>>>> word 2   ->  word 4
>>>> word 7,8 ->  word 5,6
>>>>          ->  word 7   (new leaf not previously described)
>>>>          ->  word 8   (new leaf not previously described)
>>>> word 3   ->  word 9   (linux defined mapping)
>>>>
>>>> Since you're proposing the stabilization of physinfo.hw_caps
>>> Stabilising of physinfo.hw_caps is a side effect, but it has shifted
>>> words in the past (c/s 4f4eec3, 6c421a1, 9c907c6).  It is not a stable
>>> interface from Xen, and cannot be relied upon.
>>>
>>> It has also never had a published ABI.
>>>
>> Thanks for the clarification! I thought that it was sort of a stable API 
>> because
>> it was exposed through libxl, but I got the wrong idea entirely.
> 
> Supposedly so.  In reality, a number of poor decisions were made when
> declaring certain things stable.
> 
>>
>>>>  and given that this
>>>> is exposed on both sysctl and libxl (through libxl_hwcap) shouldn't its 
>>>> size
>>>> match the real one (boot_cpu_data.x86_capability) i.e. NCAPINTS ? 
>>>> Additionally I
>>>> see that libxl_hwcap is also hardcoded to 8 alongside struct 
>>>> xen_sysctl_physinfo
>>>> when it should be 10 ?
>>> Hardcoding of the size in sysctl can be worked around. Fixing libxl is
>>> harder.
>>>
>>> The synthetic leaves are internal and should not be exposed.
>>>
>>>> libxl users could potentially make use of this hwcap field to see what 
>>>> features
>>>> the host CPU supports.
>>> The purpose of the new featureset interface is to have stable object
>>> which can be used by higher level toolstacks.
>>>
>>> This is done by pretending that hw_caps never existed, and replacing it
>>> wholesale with a bitmap, (specified as variable length and safe to
>>> zero-extend), with an ABI in the public header files detailing what each
>>> bit means.
>> Given that you introduce a new API for libxc (xc_get_cpu_featureset()) 
>> perhaps
>> an equivalent to libxl could also be added? That wat users of libxl could 
>> also
>> query about the host and guests supported features. I would be happy to 
>> produce
>> patches towards that.
> 
> In principle, this is fine.  Part of this is covered by the xen-cpuid
> utility in a later patch.
> 
OK.

> Despite my plans to further rework guest cpuid handling, the principle
> of the {raw,host,pv,hvm}_featuresets is expected to stay, and be usable
> in their current form.
That's great to hear. The reason I brought this up is because libvirt has the
idea of cpu model and features associated with it (similar to qemu -cpu
XXX,+feature,-feature stuff but in an hypervisor agnostic manner that other
architectures can also use). libvirt could do mostly everything on its own, but
it still needs to know what the host supports. Based on that it then calculates
the lowest common denominator of cpu features to be enabled or masked out for
guests when comparing to an older family in a pool of servers. Though PV/HVM
(with{,out} hap/shadow) have different feature sets as you mention. So libvirt
might be thrown into error since a certain feature isn't sure to be set/masked
for a certain type of guest. So knowing those (i.e {pv,hvm,...}_featuresets in
advance lets libxl users make more reliable usage of the libxl cpuid policies to
more correctly normalize the cpuid for each type of guest.

> 
> However, other details such as the hvm hap and shadow mask are expected
> to change moving forwards, so shouldn't be made into a stable API.
> 
> Note also that the current "inverted" mask is subject to some redesign,
> following the "x86: workaround inability to fully restore FPU stateâ"
> issue David was working on.
Ah, Thanks for the heads up!

Joao

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel