X86_VENDOR_* aren't bit masks in the older trees.

Reported-by: James Dingwall <james@dingwall.me.uk>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -226,7 +226,8 @@ int guest_rdmsr(const struct vcpu *v, ui
          */
     case MSR_IA32_PERF_STATUS:
     case MSR_IA32_PERF_CTL:
-        if ( !(cp->x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR)) )
+        if ( cp->x86_vendor != X86_VENDOR_INTEL &&
+             cp->x86_vendor != X86_VENDOR_CENTAUR )
             goto gp_fault;
 
         *val = 0;

On Thu, Feb 04, 2021 at 10:36:06AM +0100, Jan Beulich wrote:
> X86_VENDOR_* aren't bit masks in the older trees.
> 
> Reported-by: James Dingwall <james@dingwall.me.uk>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Should this have a set of Fixes tag for the commit hashes on <= 4.12?

Thanks, Roger.

On 04.02.2021 10:40, Roger Pau Monné wrote:
> On Thu, Feb 04, 2021 at 10:36:06AM +0100, Jan Beulich wrote:
>> X86_VENDOR_* aren't bit masks in the older trees.
>>
>> Reported-by: James Dingwall <james@dingwall.me.uk>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> Should this have a set of Fixes tag for the commit hashes on <= 4.12?

I'd prefer Fixes: to only reference non-backports. The tag is
mainly meant to allow noticing what needs backporting, after all.

Jan

Hi Jan,

On Thu, Feb 04, 2021 at 10:36:06AM +0100, Jan Beulich wrote:
> X86_VENDOR_* aren't bit masks in the older trees.
> 
> Reported-by: James Dingwall <james@dingwall.me.uk>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -226,7 +226,8 @@ int guest_rdmsr(const struct vcpu *v, ui
>           */
>      case MSR_IA32_PERF_STATUS:
>      case MSR_IA32_PERF_CTL:
> -        if ( !(cp->x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR)) )
> +        if ( cp->x86_vendor != X86_VENDOR_INTEL &&
> +             cp->x86_vendor != X86_VENDOR_CENTAUR )
>              goto gp_fault;
>  
>          *val = 0;

Thanks for this patch, I've applied it and the Windows guest no longer crashes.

Regards,
James

On 04.02.2021 10:36, Jan Beulich wrote:
> X86_VENDOR_* aren't bit masks in the older trees.
> 
> Reported-by: James Dingwall <james@dingwall.me.uk>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -226,7 +226,8 @@ int guest_rdmsr(const struct vcpu *v, ui
>           */
>      case MSR_IA32_PERF_STATUS:
>      case MSR_IA32_PERF_CTL:
> -        if ( !(cp->x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR)) )
> +        if ( cp->x86_vendor != X86_VENDOR_INTEL &&
> +             cp->x86_vendor != X86_VENDOR_CENTAUR )
>              goto gp_fault;
>  
>          *val = 0;

Darn - this was only half of it. There's a similar construct
in guest_wrmsr() which also wants replacing.

Jan

On 04/02/2021 15:53, Jan Beulich wrote:
> On 04.02.2021 10:36, Jan Beulich wrote:
>> X86_VENDOR_* aren't bit masks in the older trees.
>>
>> Reported-by: James Dingwall <james@dingwall.me.uk>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -226,7 +226,8 @@ int guest_rdmsr(const struct vcpu *v, ui
>>           */
>>      case MSR_IA32_PERF_STATUS:
>>      case MSR_IA32_PERF_CTL:
>> -        if ( !(cp->x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR)) )
>> +        if ( cp->x86_vendor != X86_VENDOR_INTEL &&
>> +             cp->x86_vendor != X86_VENDOR_CENTAUR )
>>              goto gp_fault;
>>  
>>          *val = 0;
> Darn - this was only half of it. There's a similar construct
> in guest_wrmsr() which also wants replacing.

I really should have renamed the constants when I changed their layout...

My R-by stands in light of that change.

~Andrew

On 04/02/2021 09:36, Jan Beulich wrote:
> X86_VENDOR_* aren't bit masks in the older trees.
>
> Reported-by: James Dingwall <james@dingwall.me.uk>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>