Re: [Xen-devel] [PATCH 2/2] x86/AMD: Fix handling of x87 exception pointers on Fam17h hardware

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/09/2019 15:50, Jan Beulich wrote:
>>>  I'm also not sure why you
>>> call them "unpredictable": If all (or most) cases match, the branch
>>> there could be pretty well predicted (subject of course to capacity).
>> Data-dependent branches which have no correlation to pattern history, of
>> which this is an example, are frequently mispredicted because they are
>> inherently unstable.
>>
>> In this case, you're trading off the fact that an unmasked exception is
>> basically never pending, against the cost of mispredicts in the context
>> switch path.
> For
>
>     if ( !(fpu_ctxt->fsw & ~fpu_ctxt->fcw & 0x003f) &&
>
> you're claiming it to be true most of the time. How could the
> predictor be mislead if whenever this is encountered the result
> of the double & is zero?

Because whether it is 0 or not is unrelated to previous history.

As this argument isn't getting anywhere, I'll leave it in for now and do
the perf work to demonstrate the problem at some point when I don't have
15 other things needing doing yesterday.

>>> But as said before, just like for synthetic
>>> features I strongly think we should use simple boolean variables
>>> when using them only in if()-s. Use of the feature(/bug) machinery
>>> is needed only to not further complicate alternatives patching.
>> ... b)
>>
>> I see I'm going to have to repeat myself, which is time I can't really
>> afford to waste.
>>
>> x86_capabilities is not, and has never been, "just for alternatives". 
>> It is also not how it is currently used in Xen.
> And I've not been claiming this.

You literally have, and it is quoted above.

>  Nevertheless my opinion is that it
> shouldn't be needlessly abused beyond its main purpose.

The purpose is to be a collection bits, stored in reasonably efficient
manner.  Synthetic features, as well as bugs are related information,
and very definitely capabilities of the CPU.

Alternatives use the x86_capabilities[] bitmap, which existed for 2
decades previously, because it happens to be in a convenient form.  The
fact that alternatives do use x86_capabilities[] has no bearing on what
is reasonable or appropriate data to store in the bitmap, and it
certainly doesn't mean that data-not-used-for-patching should be purged.

> I thought I had successfully convinced you of not adding synthetic
> feature (non-bug) flags either anymore, unless needed for alternatives
> patching.

No.

I don't think you realise how quite how infuriating it was trying to
meet the embargos for speculative issues.  We had series which were 10's
of patches long, being invasively rewritten leading up to the embargo. 
Some requests where legitimate - I'm not going to pretend otherwise, but
some really were minutia like this which really didn't help the situation.

There are two big series outstanding, MSR_VIRT_SPEC_CTRL and CPUID
Policy, which is getting to be reprehensibly late, and both of which had
proper embargos I was trying to meet. 

There was no way VIRT_SPEC_CTRL was going to meet the SSBD embargo
because of the delay getting the spec together, but running Xen on AMD
hardware is currently embarrassing and slow due to guests falling back
to native means and hitting:

(XEN) emul-priv-op.c:1113:d0v2 Domain attempted WRMSR c0011020 from
0x0006404000000000 to 0x0006404000000400

on their context switch path, and doing a good job of filling /var/log/
in minutes.

CPUID policy is even worse.  It's not currently safe to migrate VMs on
Intel hardware, because we can't level MSR_ARCH_CAPS.RSBA across the
migration pool, and this is something which really should have met the
L1TF embargo a year ago, but which was stopped dead in its tracks
because I couldn't even argue in public as to why it needed to be done
certain ways.  It also means that Xen is crippled on current-generation
Intel hardware.

The sad fact is that it is rather too easy to put off finishing that
work when there is other just-as-important work to do, and the thought
of arguing over further minutia on vN+1 is occasionally too exhausting
to contemplate.

> Anyway - in the interest of forward progress, yet without being
> convinced at all, I'll (as in so many earlier cases) give in here and
> see about acking patch 1 then.

Thankyou.

>
>> I also don't agree with the general suggestion because amongst other
>> things, there is a factor of 8 storage difference between one extra bit
>> in x86_capabilities[] and using scattered booleans.
> While a valid argument at the first glance, there's nothing keeping
> us from having a feature flag independent bitmap. Correct my if I'm
> wrong, but I've gained the impression that you want this mainly
> because Linux does it this way.

To a first approximation, yes - this is a construct we inherited from
Linux, and I'm continuing to use it in the way Linux uses it.

>
>>>  With this, keying the return to cpu_bug_* also doesn't
>>> look very nice, but I admit I can't suggest a better alternative
>>> (other than leaving the vendor check in place and checking the
>>> X86_FEATURE_RSTR_FP_ERR_PTRS bit).
>>>
>>> An option might be to give the construct a different name, without
>>> "leak" in it (NO_FP_ERR_PTRS?).
>> This name also isn't ideal, because its not always that there are no
>> error pointers.
>>
>> X86_BUG_FPU_PTRS might be best, as it is neutral as to precisely what is
>> buggy with them.
> Well, okay, let's use that one then and hope we won't learn of a 2nd
> FPU_PTRS bug later on.

Ok.

~Andrew

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