Re: [Xen-devel] [PATCH v8 10/50] x86emul: support AVX512{F, BW, _VBMI} full permute insns

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/05/2019 07:55, Jan Beulich wrote:
>>>> On 17.05.19 at 18:50, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 15/03/2019 10:41, Jan Beulich wrote:
>>> Take the liberty and also correct the (public interface) name of the
>>> AVX512_VBMI feature flag, on the assumption that no external consumer
>>> has actually been using that flag so far.
>> I've been giving this some thought, and I think putting these in the
>> public interface was a mistake.
>>
>> They are a representation of other peoples stable ABI, and are only used
>> in tools interfaces as far as Xen is concerned (SYSCTL_get_featureset,
>> DOMCTL_get_cpu_policy)
>>
>> The only external representations are xen_cpuid_leaf_t/xen_msr_entry_t
>> which don't need constants to go with them.
> While I don't really mind removing them from the public interface
> again, I think you're missing to discuss one point of why they've
> been put there originally: We wanted them to also serve as
> documentation of the forward compatible nature of the A/S/H
> annotations, i.e. such that we wouldn't lightly remove something
> that was previously exposed.

I don't recall that being a consideration, but the file doesn't need to
be in xen/public/ for us to apply general "don't change this lightly"
rules to the annotations.

I don't see it being different to other areas of backwards compatibility
we maintain in the main codebase.

>
>>>  Furthermore make it have
>>> AVX512BW instead of AVX512F as a prerequisite, for requiring full
>>> 64-bit mask registers (the upper 48 bits of which can't be accessed
>>> other than through XSAVE/XRSTOR without AVX512BW support).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> As for the rest, Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> but
>> we perhaps want to sort out the position in the public interface before
>> changing AVX512VBMI.
> Thanks; I don't think the order of events matters much, but if this is
> something to happen soon, I don't mind waiting (and the re-basing).

I don't have any immediate plans - its not on the critical path to
DOMCTL_set_cpu_policy work.

OTOH, it doesn't look like it would be hard to do, and it would make it
clear that we are fine to rename constants.  I can look into this if
you'd like.

> There's an important point here though that you don't say any word
> on, despite me having mentioned it to you on irc already after you
> had given your ack to "x86/CPUID: support leaf 7 subleaf 1 /
> AVX512_BF16" making a similar dependency as the patch here upon
> AVX512BW rather than AVX512F. Recall that I had to split off the
> change below from another patch of mine, because of our
> disagreement on the intended dependencies. If we make the sub-
> features requiring wider than 16-bit mask registers dependent
> upon AVX512BW (as suggested by the patch here), then I think
> we also want to change the SSEn dependencies as done/suggested
> in the patch below (albeit of course there's no dependency on
> changed register width there, just one on permitted element types,
> so I could also see a basis to accept the dependency here as is, but
> view things differently for SSEn).

Reply below.

> Seeing your further acks (thanks!), just as a side note: Are you
> aware that you've skipped patch 9 in the series, without which the
> ones you've acked can't go in anyway (i.e. regardless of whether
> I'm to wait for the public interface adjustment above)?

Oops - let me go and see.

>
> Jan
>
> x86/cpuid: correct dependencies of post-SSE ISA extensions
>
> Move AESNI, PCLMULQDQ, and SHA to SSE2, as all of them act on vectors of
> integers, whereas plain SSE supports vectors of single precision floats
> only. This is in line with how e.g. binutils and gcc treat them.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

This isn't worth the effort its been arguing over, but I'd request
s/correct/adjust/ in the subject as "correctness" here is subjective.

> ---
> v?: Re-base over split out PCLMULQDQ addition.
> ---
> TBD: On the same basis, SSE3, SSSE3 and SSE4A should probably also
> depend on SSE2 rather than SSE. In fact making this a chain SSE -> SSE2
> -> SSE3 -> { SSSE3, SSE4A } would probably be best, and get us in line
> with both binutils and gcc. But I think I did suggest so when the
> dependencies were introduced, and this wasn't liked for a reason I
> forgot.

The code is only as it is because you insisted upon it being this way. 
I'll remind you that my first submission looked rather closer to this.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel