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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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.

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.

~Andrew

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