Re: [Xen-devel] [PATCH v4] x86/cpuid: AVX-512 Feature Detection

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.08.16 at 13:22, <luwei.kang@xxxxxxxxx> wrote:
>> > >>>>>>>>>> First of all - please don't top post.
>> > >>>>>>>>>>
>> > >>>>>>>>>>> What about remove the dependency between AVX2 and AVX512F
>> > >>>>>>>> ( AVX2:
>> > >>>>>>>>>> [AVX512F], ) ?
>> > >>>>>>>>>>
>> > >>>>>>>>>> Yes, that's what I think we want, but we need Andrew's 
>> > >>>>>>>>>> agreement here.
>> > >>>>>>>>>>
>> > >>>>>>>>> Hi Andrew,  what is your opinion ?
>> > >>>>>>>> You are in a better position to answer than me.
>> > >>>>>>>>
>> > >>>>>>>> For a specific instruction which may be VEX and EVEX encoded,
>> > >>>>>>>> is the circuitry for a specific instruction shared, or
>> > >>>>>>>> discrete?  Is there a plausible future where you might
>> > >>>>>>>> support only the EVEX variant of an instruction, and not the VEX 
>> > >>>>>>>> variant?
>> > >>>>>>>>
>> > >>>>>>>> These dependences are about what may be reasonably assumed
>> > >>>>>>>> about the way the environment is structured.  It doesn't look
>> > >>>>>>>> reasonable to advertise an AVX512 environment to guests while
>> > >>>>>>>> at the same time stating that AVX2 is not present.  If this
>> > >>>>>>>> is correct, then the dependency
>> > >>>>> should stay.
>> > >>>>>>>> If Intel plausibly things it might release hardware with
>> > >>>>>>>> !AVX2 but AVX512, then the dependency should be dropped.
>> > >>>>>>> Regarding the dependency between AVX2 and AVX512F, I have ask
>> > >>>>>>> some hardware
>> > >>>>> architecture engineer.
>> > >>>>>>> AVX512 is a superset of AVX2, the most important item being
>> > >>>>>>> the state. If
>> > >>>>> the state for AVX2 isn't enabled (in XCR0), then AVX512
>> > >>>>>> also can't function.
>> > >>>>>>> So if we want to use AVX512, AVX2 must be supported and enabled.
>> > >>>>>>> The
>> > >>>>> dependence between AVX512F and AVX2 may need be
>> > >>>>>> reserved.
>> > >>>>>>
>> > >>>>>> Ok, so AVX512 strictly depends on AVX2.
>> > >>>>>>
>> > >>>>>> In which case, the python code was correct.  The meaning of the
>> > >>>>>> key/value
>> > >>>>> relationship is "if the feature in the key is not present, all
>> > >>>>>> features in the value must also be disabled".
>> > >>>>> Hi jan, what is your opinion?
>> > >>>> There's no opinion to be had here, according to your earlier reply.
>> > >>>> I do,
>> > >>> however, continue to question that model: State and
>> > >>>> instruction set are independent items. Of course YMM state is a
>> > >>>> prereq to
>> > >>> ZMM state, but I do not buy AVX2 insn support being a
>> > >>>> prereq to AVX-512 insns. Yet to me the AVX2 and AVX-512F feature
>> > >>>> flags solely
>> > >>> represent the instructions, while the XSTATE leaf bits
>> > >>>> represent the states.
>> > >>>>
>> > >>> Hi jan,
>> > >>>     I get some information from hardware colleague.  There is no
>> > >>> dependence about AVX512 instructions and AVX2 instructions. It is
>> > >>> also not states in
>> > > the
>> > >>> official document.
>> > >> As I had assumed.
>> > >>
>> > >>>    But I want to know the meaning of the dependence "AVX2:
>> > >>> [AVX512F]"  in "gen-cpuid.py" file.
>> > >>>    If it is the feature dependence, I think the dependence between
>> > >>> AVX512 and AVX2  may need to add. As we talk before, Zmm is part of 
>> > >>> AVX512 
> feature.
>> > >
>> > >>> If the state for AVX2 isn't enabled (in XCR0), then AVX512 also
>> > >>> can't function. Apart from that, there is no processors with
>> > >>> AVX512 without AVX2 currently and it is safe to treat AVX2 as part
>> > >>> of the thermometer leading to
>> > >
>> > >>> AVX512. Regarding if will have a cpu support avx512 without avx2
>> > >>> in future, it is really hard to say.
>> > >>>     If it is the instructions dependence, I have no idea to delete
>> > >>> this dependence in "gen-cpuid.py" file.
>> > >>>     So, I want to know your advice.
>> > >> Well, the main issue here is that we have no feature flag
>> > >> representation for the xstate bits. If we had, the relevant parts
>> > >> of the dependencies should look like
>> > >>
>> > >>         XSTATE_YMM: [AVX, XSTATE_ZMM]
>> > >>         AVX: [AVX2]
>> > >>         XSTATE_ZMM: [AVX512F]
>> > >>         AVX512F: [AVX512CD, ...]
>> > >>
>> > >> But in their absence I guess keeping the AVX2 dependency as you
>> > >> have it is the only reasonable approach. Just that I'd like it to
>> > >> be accompanied by a comment explaining that this isn't the actual
>> > >> dependency (so that if XSTATE feature flags got introduced later,
>> > >> it would be clear how to adjust those parts).
>> > >>
>> > >> Andrew - I keep forgetting why you think having such XSTATE_*
>> > >> feature flags is not appropriate.
>> > >
>> > > This is all about providing a plausible cpuid layout to a guest.
>> > >
>> > > It is not plausible that we will ever see a processor with e.g.
>> > > AVX512F but not XSTATE_ZMM.
>> >
>> > This is a somewhat bogus argument considering we have
>> >
>> >         FXSR: [FFXSR, SSE],
>> >
>> > which, as you certainly realize, is slightly wrong nowadays:
>> > XSTATE_XMM would suffice as a prereq, without any FXSR. (But I
>> > certainly realize that lack of FXSR is unlikely, as that's considered
>> > part of the base x86-64 architecture, and I also realize that
>> > expressing alternative prereqs would make the deep dependency
>> > generation logic more complicated.)
>> >
>> 
>> According your advice, I change the comment of "AVX2: [AVX512F]," as below.
>> If the description is not accurately enough, please help do some corrected, 
> thank you.
>> 
>> diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py index 
> 7c45eca..e8b64ce 100755
>> --- a/xen/tools/gen-cpuid.py
>> +++ b/xen/tools/gen-cpuid.py
>> @@ -243,6 +243,17 @@ def crunch_numbers(state):
>>          # AMD K6-2+ and K6-III processors shipped with 3DNow+, beyond the
>>          # standard 3DNow in the earlier K6 processors.
>>          _3DNOW: [_3DNOWEXT],
>> +
>> +        # This is just the dependency between AVX512 and AVX2 of XSTATE 
> feature flag.
>> +        # If want to use AVX512, AVX2 must be supported and enabled.
>> +        AVX2: [AVX512F],
>> +
>> +        # AVX512F is taken to mean hardware support for EVEX encoded 
> instructions,
>> +        # 512bit registers, and the instructions themselves. All further 
> AVX512 features
>> +        # are built on top of AVX512F
>> +        AVX512F: [AVX512DQ, AVX512IFMA, AVX512PF, AVX512ER, AVX512CD,
>> +                    AVX512BW, AVX512VL, AVX512VBMI],
>>      }
>> 
>>      deep_features = tuple(sorted(deps.keys()))
>> 
> 
> Hi jan,
>     What do you think about modify as above.  If need any corrected?

Looks okay - I was actually awaiting v5 ...

Jan

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