[Xen-devel] [PATCH v2] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

[Xen-devel] [PATCH v2] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

[Igor Druzhinin]

If the feature is not present Xen will try to force X86_BUG_FPU_PTRS
feature at CPU identification time. This is especially noticeable in
PV-shim that usually hotplugs its vCPUs. We either need to restrict this
action for boot CPU only or allow secondary CPUs to modify
forced CPU capabilities at runtime. Choose the former since modifying
forced capabilities out of boot path leaves the system in potentially
inconsistent state.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
Changes in v2:
- pick the former approach instead of the latter
---
 xen/arch/x86/cpu/amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index fec2830..8b5f0f2 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -583,7 +583,7 @@ static void init_amd(struct cpuinfo_x86 *c)
 	 * Older AMD CPUs don't save/load FOP/FIP/FDP unless an FPU exception
 	 * is pending.  Xen works around this at (F)XRSTOR time.
 	 */
-	if (!cpu_has(c, X86_FEATURE_RSTR_FP_ERR_PTRS))
+	if (c == &boot_cpu_data && !cpu_has(c, X86_FEATURE_RSTR_FP_ERR_PTRS))
 		setup_force_cpu_cap(X86_BUG_FPU_PTRS);
 
 	/*
-- 
2.7.4


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

Re: [Xen-devel] [PATCH v2] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

[Jan Beulich]

On 04.12.2019 00:56, Igor Druzhinin wrote:
> If the feature is not present Xen will try to force X86_BUG_FPU_PTRS
> feature at CPU identification time. This is especially noticeable in
> PV-shim that usually hotplugs its vCPUs. We either need to restrict this
> action for boot CPU only or allow secondary CPUs to modify
> forced CPU capabilities at runtime. Choose the former since modifying
> forced capabilities out of boot path leaves the system in potentially
> inconsistent state.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Andrew,

please clarify explicitly whether you're okay with this going in
despite your earlier objection, or whether you want us to follow
the outlined alternative route (or yet some different approach).
FAOD I continue to insist that the __init not be dropped from the
two functions in question.

Jan

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

Re: [Xen-devel] [PATCH v2] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

[Jan Beulich]

On 04.12.2019 00:56, Igor Druzhinin wrote:
> If the feature is not present Xen will try to force X86_BUG_FPU_PTRS
> feature at CPU identification time. This is especially noticeable in
> PV-shim that usually hotplugs its vCPUs. We either need to restrict this
> action for boot CPU only or allow secondary CPUs to modify
> forced CPU capabilities at runtime. Choose the former since modifying
> forced capabilities out of boot path leaves the system in potentially
> inconsistent state.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

I've committed this to unstable, as per the outcome of the
community call. What about this for 4.13? Iirc the breakage was
introduced during this development cycle.

Jan

> ---
> Changes in v2:
> - pick the former approach instead of the latter
> ---
>  xen/arch/x86/cpu/amd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index fec2830..8b5f0f2 100644
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -583,7 +583,7 @@ static void init_amd(struct cpuinfo_x86 *c)
>  	 * Older AMD CPUs don't save/load FOP/FIP/FDP unless an FPU exception
>  	 * is pending.  Xen works around this at (F)XRSTOR time.
>  	 */
> -	if (!cpu_has(c, X86_FEATURE_RSTR_FP_ERR_PTRS))
> +	if (c == &boot_cpu_data && !cpu_has(c, X86_FEATURE_RSTR_FP_ERR_PTRS))
>  		setup_force_cpu_cap(X86_BUG_FPU_PTRS);
>  
>  	/*
> 


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

Re: [Xen-devel] [PATCH v2] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

[Jürgen Groß]

On 10.12.19 11:10, Jan Beulich wrote:
> On 04.12.2019 00:56, Igor Druzhinin wrote:
>> If the feature is not present Xen will try to force X86_BUG_FPU_PTRS
>> feature at CPU identification time. This is especially noticeable in
>> PV-shim that usually hotplugs its vCPUs. We either need to restrict this
>> action for boot CPU only or allow secondary CPUs to modify
>> forced CPU capabilities at runtime. Choose the former since modifying
>> forced capabilities out of boot path leaves the system in potentially
>> inconsistent state.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> 
> I've committed this to unstable, as per the outcome of the
> community call. What about this for 4.13? Iirc the breakage was
> introduced during this development cycle.

In this case:

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

Re: [Xen-devel] [PATCH v2] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

[Andrew Cooper]

On 10/12/2019 10:26, Jürgen Groß wrote:
> On 10.12.19 11:10, Jan Beulich wrote:
>> On 04.12.2019 00:56, Igor Druzhinin wrote:
>>> If the feature is not present Xen will try to force X86_BUG_FPU_PTRS
>>> feature at CPU identification time. This is especially noticeable in
>>> PV-shim that usually hotplugs its vCPUs. We either need to restrict
>>> this
>>> action for boot CPU only or allow secondary CPUs to modify
>>> forced CPU capabilities at runtime. Choose the former since modifying
>>> forced capabilities out of boot path leaves the system in potentially
>>> inconsistent state.
>>>
>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>
>> I've committed this to unstable, as per the outcome of the
>> community call.

What outcome?  Yes technically your R-by is sufficient to get the patch
in, but you know very well there are open objections against this
version of the patch.

Also, you're actually in a position where you are reviewing your own
work, which is not how R-by is intended to work.

Furthermore, you will observe that there is an action item on me from
the call to come up with a less broken alternative which I'm genuinely
attempting to do.

~Andrew

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

Re: [Xen-devel] [PATCH v2] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

[Jan Beulich]

On 10.12.2019 13:37, Andrew Cooper wrote:
> On 10/12/2019 10:26, Jürgen Groß wrote:
>> On 10.12.19 11:10, Jan Beulich wrote:
>>> On 04.12.2019 00:56, Igor Druzhinin wrote:
>>>> If the feature is not present Xen will try to force X86_BUG_FPU_PTRS
>>>> feature at CPU identification time. This is especially noticeable in
>>>> PV-shim that usually hotplugs its vCPUs. We either need to restrict
>>>> this
>>>> action for boot CPU only or allow secondary CPUs to modify
>>>> forced CPU capabilities at runtime. Choose the former since modifying
>>>> forced capabilities out of boot path leaves the system in potentially
>>>> inconsistent state.
>>>>
>>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>>
>>> I've committed this to unstable, as per the outcome of the
>>> community call.
> 
> What outcome?  Yes technically your R-by is sufficient to get the patch
> in, but you know very well there are open objections against this
> version of the patch.

My proposal on the call was to go with the existing patch, and improve
from there. There wasn't great enthusiasm, but there was agreement that
this is the most pragmatic route to follow. In particular I don't
recall you voicing any objection to this on the call (I do very well
recall the objection you voiced earlier on, which I had responded to
with a suggestion of a slightly different approach, taking care [I
think] of your wishes as well as mine).

> Also, you're actually in a position where you are reviewing your own
> work, which is not how R-by is intended to work.

Which own work? The patch doesn't even carry a Suggested-by. Iirc
Igor told me that what has gone in is how he had it initially. So
I'm pretty confused by this statement of yours.

Furthermore, as a recurring pattern, simply not responding to ongoing
discussions should not, in the common case, lead to no progress at
all. Iirc this was also mentioned on the call ("lazy consensus").

> Furthermore, you will observe that there is an action item on me from
> the call to come up with a less broken alternative which I'm genuinely
> attempting to do.

There's no indication towards this in the minutes, afaics. Or wait
- the same topic appears twice there (as both 4 and 6). I wasn't
even aware of such an action item. I'll be happy to revert if you
indicate so, and if a better fix is going to show up in time.

Jan

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

Re: [Xen-devel] [PATCH v2] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

[Igor Druzhinin]

On 10/12/2019 13:58, Jan Beulich wrote:
> On 10.12.2019 13:37, Andrew Cooper wrote:
>> Furthermore, you will observe that there is an action item on me from
>> the call to come up with a less broken alternative which I'm genuinely
>> attempting to do.
> 
> There's no indication towards this in the minutes, afaics. Or wait
> - the same topic appears twice there (as both 4 and 6). I wasn't
> even aware of such an action item. I'll be happy to revert if you
> indicate so, and if a better fix is going to show up in time.

I don't think reverting would make sense - the patch doesn't break
anything, even more - it actually fixes the problem we observed.
If there is an improvement to that coming - it should be just done
on top of this also potentially taking care of other places in the code
that might be affected.

Igor

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