[PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream

[PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream

[Andrew Cooper]

Doing so amounts to silent state corruption, and must be avoided.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Julien Grall <julien.grall@arm.com>

This wants backporting to all stable trees, so should also be considered for
inclusion into 4.10 at this point.
---
 xen/arch/x86/hvm/svm/svm.c | 2 +-
 xen/arch/x86/hvm/vmx/vmx.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index b9cf423..7b0e688 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -450,7 +450,7 @@ static int svm_load_msr(struct vcpu *v, struct hvm_msr *ctxt)
             break;
 
         default:
-            continue;
+            err = -ENXIO;
         }
         if ( err )
             break;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b18ccea..fdb65a5 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -954,8 +954,9 @@ static int vmx_load_msr(struct vcpu *v, struct hvm_msr *ctxt)
             else
                 err = -ENXIO;
             break;
+
         default:
-            continue;
+            err = -ENXIO;
         }
         if ( err )
             break;
-- 
2.1.4


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

Re: [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream

[Wei Liu]

On Thu, Nov 16, 2017 at 07:15:32PM +0000, Andrew Cooper wrote:
> Doing so amounts to silent state corruption, and must be avoided.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream

[Jan Beulich]

>>> On 16.11.17 at 20:15, <andrew.cooper3@citrix.com> wrote:
> Doing so amounts to silent state corruption, and must be avoided.

I think a little more explanation is needed on why the current code
is insufficient. Note specifically this

    for ( i = 0; !err && i < ctxt->count; ++i )
    {
        switch ( ctxt->msr[i].index )
        {
        default:
            if ( !ctxt->msr[i]._rsvd )
                err = -ENXIO;
            break;
        }
    }

in hvm_load_cpu_msrs(), intended to give vendor code a first
shot, but allowing for vendor independent MSRs to be handled
here.

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -450,7 +450,7 @@ static int svm_load_msr(struct vcpu *v, struct hvm_msr *ctxt)
>              break;
>  
>          default:
> -            continue;
> +            err = -ENXIO;
>          }

If the change was nevertheless needed, please add break
statements here (and in VMX code as well), despite this
currently being the last label in the switch() block.

Jan


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

Re: [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream

[Andrew Cooper]

On 17/11/17 12:10, Jan Beulich wrote:
>>>> On 16.11.17 at 20:15, <andrew.cooper3@citrix.com> wrote:
>> Doing so amounts to silent state corruption, and must be avoided.
> I think a little more explanation is needed on why the current code
> is insufficient. Note specifically this
>
>     for ( i = 0; !err && i < ctxt->count; ++i )
>     {
>         switch ( ctxt->msr[i].index )
>         {
>         default:
>             if ( !ctxt->msr[i]._rsvd )
>                 err = -ENXIO;
>             break;
>         }
>     }
>
> in hvm_load_cpu_msrs(), intended to give vendor code a first
> shot, but allowing for vendor independent MSRs to be handled
> here.

That is sufficiently subtle and non-obvious that I'm still having a hard
time convincing myself that its correct.  Also, this use of _rsvd really
should be document.

~Andrew

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

Re: [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream

[Jan Beulich]

>>> On 20.11.17 at 15:10, <andrew.cooper3@citrix.com> wrote:
> On 17/11/17 12:10, Jan Beulich wrote:
>>>>> On 16.11.17 at 20:15, <andrew.cooper3@citrix.com> wrote:
>>> Doing so amounts to silent state corruption, and must be avoided.
>> I think a little more explanation is needed on why the current code
>> is insufficient. Note specifically this
>>
>>     for ( i = 0; !err && i < ctxt->count; ++i )
>>     {
>>         switch ( ctxt->msr[i].index )
>>         {
>>         default:
>>             if ( !ctxt->msr[i]._rsvd )
>>                 err = -ENXIO;
>>             break;
>>         }
>>     }
>>
>> in hvm_load_cpu_msrs(), intended to give vendor code a first
>> shot, but allowing for vendor independent MSRs to be handled
>> here.
> 
> That is sufficiently subtle and non-obvious that I'm still having a hard
> time convincing myself that its correct.  Also, this use of _rsvd really
> should be document.

Well, from an abstract pov I agree. The field being defined in the
public interface though, I don't see a good place where to document
that - its point of declaration certainly isn't the right one in such a
case, as the public interface should not document implementation
details.

Jan


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

Re: [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Friday, November 17, 2017 3:16 AM
> 
> Doing so amounts to silent state corruption, and must be avoided.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
> This wants backporting to all stable trees, so should also be considered for
> inclusion into 4.10 at this point.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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