Re: [Xen-devel] [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

Re: [Xen-devel] [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[Jan Beulich]

>>> On 30.05.12 at 15:10, Andre Przywara <andre.przywara@amd.com> wrote:
> Because we are behind a family check before tweaking the topology
> bit, we can use the standard rd/wrmsr variants for the CPUID feature
> register.
> This fixes a crash when using the kernel as a Xen Dom0 on affected
> Trinity systems. The wrmsrl_amd_safe is not properly paravirtualized
> yet (this will be fixed in another patch).

I'm not following: If the AMD variants (putting a special value into
%edi) can be freely replaced by the non-AMD variants, why did
the AMD special ones get used in the first place?

Further, I can't see how checking_wrmsrl() is being paravirtualized
any better than wrmsrl_amd_safe() - both have nothing but an
exception handling fixup attached to the wrmsr invocation. Care
to point out what actual crash it is that was seen?

Finally, I would question whether re-enabling the topology
extensions under Xen shouldn't be skipped altogether, perhaps
even on Dom0 (as the hypervisor is controlling this MSR, but in
any case on DomU - the hypervisor won't allow (read: ignore,
not fault on) the write anyway (and will log a message for each
(v)CPU that attempts this).

Jan

> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
> Cc: stable@vger.kernel.org # 3.4+
> ---
>  arch/x86/kernel/cpu/amd.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 146bb62..80ccd99 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -586,9 +586,9 @@ static void __cpuinit init_amd(struct cpuinfo_x86 *c)
>  	    !cpu_has(c, X86_FEATURE_TOPOEXT)) {
>  		u64 val;
>  
> -		if (!rdmsrl_amd_safe(0xc0011005, &val)) {
> +		if (!rdmsrl_safe(0xc0011005, &val)) {
>  			val |= 1ULL << 54;
> -			wrmsrl_amd_safe(0xc0011005, val);
> +			checking_wrmsrl(0xc0011005, val);
>  			rdmsrl(0xc0011005, val);
>  			if (val & (1ULL << 54)) {
>  				set_cpu_cap(c, X86_FEATURE_TOPOEXT);

Re: [Xen-devel] [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[Konrad Rzeszutek Wilk]

On Wed, May 30, 2012 at 03:10:02PM +0200, Andre Przywara wrote:
> Because we are behind a family check before tweaking the topology
> bit, we can use the standard rd/wrmsr variants for the CPUID feature
> register.
> This fixes a crash when using the kernel as a Xen Dom0 on affected
> Trinity systems. The wrmsrl_amd_safe is not properly paravirtualized
> yet (this will be fixed in another patch).

So with a rdmsrl_amd_safe and wrmsrl_amd_safe being implemented in
the pv_cpu_ops - would this patch even be neccessary?

> 
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
> Cc: stable@vger.kernel.org # 3.4+
> ---
>  arch/x86/kernel/cpu/amd.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 146bb62..80ccd99 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -586,9 +586,9 @@ static void __cpuinit init_amd(struct cpuinfo_x86 *c)
>  	    !cpu_has(c, X86_FEATURE_TOPOEXT)) {
>  		u64 val;
>  
> -		if (!rdmsrl_amd_safe(0xc0011005, &val)) {
> +		if (!rdmsrl_safe(0xc0011005, &val)) {
>  			val |= 1ULL << 54;
> -			wrmsrl_amd_safe(0xc0011005, val);
> +			checking_wrmsrl(0xc0011005, val);
>  			rdmsrl(0xc0011005, val);
>  			if (val & (1ULL << 54)) {
>  				set_cpu_cap(c, X86_FEATURE_TOPOEXT);
> -- 
> 1.7.4.4
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[H. Peter Anvin]

On 05/30/2012 06:10 AM, Andre Przywara wrote:
> Because we are behind a family check before tweaking the topology
> bit, we can use the standard rd/wrmsr variants for the CPUID feature
> register.

That is not what the *msr*_amd*() functions do.

NAK.  This is a totally bogus patch.

	-hpa

Re: [Xen-devel] [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[H. Peter Anvin]

On 05/30/2012 07:39 AM, Konrad Rzeszutek Wilk wrote:
> On Wed, May 30, 2012 at 03:10:02PM +0200, Andre Przywara wrote:
>> Because we are behind a family check before tweaking the topology
>> bit, we can use the standard rd/wrmsr variants for the CPUID feature
>> register.
>> This fixes a crash when using the kernel as a Xen Dom0 on affected
>> Trinity systems. The wrmsrl_amd_safe is not properly paravirtualized
>> yet (this will be fixed in another patch).
> 
> So with a rdmsrl_amd_safe and wrmsrl_amd_safe being implemented in
> the pv_cpu_ops - would this patch even be neccessary?
> 

That is still bogus;  a better thing would be to implement the _regs
interface.  Even better would be to trap and emulate rdmsr/wrmsr!

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [Xen-devel] [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[Konrad Rzeszutek Wilk]

On Wed, May 30, 2012 at 07:50:15AM -0700, H. Peter Anvin wrote:
> On 05/30/2012 07:39 AM, Konrad Rzeszutek Wilk wrote:
> > On Wed, May 30, 2012 at 03:10:02PM +0200, Andre Przywara wrote:
> >> Because we are behind a family check before tweaking the topology
> >> bit, we can use the standard rd/wrmsr variants for the CPUID feature
> >> register.
> >> This fixes a crash when using the kernel as a Xen Dom0 on affected
> >> Trinity systems. The wrmsrl_amd_safe is not properly paravirtualized
> >> yet (this will be fixed in another patch).
> > 
> > So with a rdmsrl_amd_safe and wrmsrl_amd_safe being implemented in
> > the pv_cpu_ops - would this patch even be neccessary?
> > 
> 
> That is still bogus;  a better thing would be to implement the _regs
> interface.  Even better would be to trap and emulate rdmsr/wrmsr!

That is what I meant - implement these two:

	rdmsr_regs = native_rdmsr_safe_regs,
        .wrmsr_regs = native_wrmsr_safe_regs,

Xen already traps the rdmsr/wrms - I believe it just didn't do
anything for this specific MSR.

Re: [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[Borislav Petkov]

On Wed, May 30, 2012 at 07:42:27AM -0700, H. Peter Anvin wrote:
> On 05/30/2012 06:10 AM, Andre Przywara wrote:
> > Because we are behind a family check before tweaking the topology
> > bit, we can use the standard rd/wrmsr variants for the CPUID feature
> > register.
> 
> That is not what the *msr*_amd*() functions do.
> 
> NAK.  This is a totally bogus patch.

The *msr*_amd*() variants were used instead of the normal *msrl_safe
variants although the AMD variants weren't needed there at all.

This has no issue on baremetal but breaks xen and this is how we caught
this.

So the patch corrects the original patch so that xen is happy too.

-- 
Regards/Gruss,
Boris.

Re: [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[H. Peter Anvin]

On 05/30/2012 07:55 AM, Borislav Petkov wrote:
> On Wed, May 30, 2012 at 07:42:27AM -0700, H. Peter Anvin wrote:
>> On 05/30/2012 06:10 AM, Andre Przywara wrote:
>>> Because we are behind a family check before tweaking the topology
>>> bit, we can use the standard rd/wrmsr variants for the CPUID feature
>>> register.
>>
>> That is not what the *msr*_amd*() functions do.
>>
>> NAK.  This is a totally bogus patch.
> 
> The *msr*_amd*() variants were used instead of the normal *msrl_safe
> variants although the AMD variants weren't needed there at all.
> 
> This has no issue on baremetal but breaks xen and this is how we caught
> this.
> 
> So the patch corrects the original patch so that xen is happy too.
> 

If so, then fix the description to match reality and we can take the patch.

	-hpa

Re: [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[Borislav Petkov]

On Wed, May 30, 2012 at 07:58:23AM -0700, H. Peter Anvin wrote:
> If so, then fix the description to match reality and we can take the patch.

Andre, would you please do that?

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[H. Peter Anvin]

On 05/30/2012 08:00 AM, Borislav Petkov wrote:
> On Wed, May 30, 2012 at 07:58:23AM -0700, H. Peter Anvin wrote:
>> If so, then fix the description to match reality and we can take the patch.
> 
> Andre, would you please do that?
> 

Ah, but now we get:

>> I'm not following: If the AMD variants (putting a special value into
>> %edi) can be freely replaced by the non-AMD variants, why did
>> the AMD special ones get used in the first place?

> Older CPUs (K8) needed the AMD variants, starting with family 10h we
> can use the normal versions.

So no, not correct.

	-hpa

Re: [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[Borislav Petkov]

On Wed, May 30, 2012 at 08:01:19AM -0700, H. Peter Anvin wrote:
> >> I'm not following: If the AMD variants (putting a special value into
> >> %edi) can be freely replaced by the non-AMD variants, why did
> >> the AMD special ones get used in the first place?
> 
> > Older CPUs (K8) needed the AMD variants, starting with family 10h we
> > can use the normal versions.
> 
> So no, not correct.

I don't see what the problem is: the amd* variants shouldn't have been
used at all in the first place. Fullstop. The patch should've been doing
*msrl_safe from the get-go. Regardless of family.

So what's the issue?

-- 
Regards/Gruss,
Boris.

Re: [Xen-devel] [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[Jan Beulich]

>>> On 30.05.12 at 16:50, "H. Peter Anvin" <hpa@zytor.com> wrote:
> On 05/30/2012 07:39 AM, Konrad Rzeszutek Wilk wrote:
>> On Wed, May 30, 2012 at 03:10:02PM +0200, Andre Przywara wrote:
>>> Because we are behind a family check before tweaking the topology
>>> bit, we can use the standard rd/wrmsr variants for the CPUID feature
>>> register.
>>> This fixes a crash when using the kernel as a Xen Dom0 on affected
>>> Trinity systems. The wrmsrl_amd_safe is not properly paravirtualized
>>> yet (this will be fixed in another patch).
>> 
>> So with a rdmsrl_amd_safe and wrmsrl_amd_safe being implemented in
>> the pv_cpu_ops - would this patch even be neccessary?
>> 
> 
> That is still bogus;  a better thing would be to implement the _regs
> interface.  Even better would be to trap and emulate rdmsr/wrmsr!

The crash is not on the wrmsr instruction, but on the paravirt
layer finding a NULL pointer in one of the methods. Xen does
trap and emulate (possibly just ignore) both instructions.

Jan

Re: [Xen-devel] [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[H. Peter Anvin]

On 05/30/2012 08:08 AM, Jan Beulich wrote:
> 
> Xen does trap and emulate (possibly just ignore) both instructions.
> 

Then why the fsck is there paravirt_crap on this?

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [Xen-devel] [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[Jan Beulich]

>>> On 30.05.12 at 17:15, "H. Peter Anvin" <hpa@zytor.com> wrote:
> On 05/30/2012 08:08 AM, Jan Beulich wrote:
>> 
>> Xen does trap and emulate (possibly just ignore) both instructions.
>> 
> 
> Then why the fsck is there paravirt_crap on this?

I have no clue. Jeremy, Konrad?

Jan

Re: [Xen-devel] [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[Konrad Rzeszutek Wilk]

On Wed, May 30, 2012 at 04:35:41PM +0100, Jan Beulich wrote:
> >>> On 30.05.12 at 17:15, "H. Peter Anvin" <hpa@zytor.com> wrote:
> > On 05/30/2012 08:08 AM, Jan Beulich wrote:
> >> 
> >> Xen does trap and emulate (possibly just ignore) both instructions.
> >> 
> > 
> > Then why the fsck is there paravirt_crap on this?
> 
> I have no clue. Jeremy, Konrad?

No idea. The git 132ec92f says:

Borislav Petkov <petkovbb@googlemail.com>
Date:   Mon Aug 31 09:50:09 2009 +0200

    x86, msr: Add rd/wrmsr interfaces with preset registers

    native_{rdmsr,wrmsr}_safe_regs are two new interfaces which allow
    presetting of a subset of eight x86 GPRs before executing the rd/wrmsr
    instructions. This is needed at least on AMD K8 for accessing an erratum
    workaround MSR.

    Originally based on an idea by H. Peter Anvin.

Re: [PATCH] x86/amd: fix crash as Xen Dom0 on AMD Trinity systems

[H. Peter Anvin]

On 05/30/2012 06:10 AM, Andre Przywara wrote:
> Because we are behind a family check before tweaking the topology
> bit, we can use the standard rd/wrmsr variants for the CPUID feature
> register.
> This fixes a crash when using the kernel as a Xen Dom0 on affected
> Trinity systems. The wrmsrl_amd_safe is not properly paravirtualized
> yet (this will be fixed in another patch).
> 
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
> Cc: stable@vger.kernel.org # 3.4+

With Konrad's patch, this is no longer relevant, correct?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.