* [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream
@ 2017-11-16 19:15 Andrew Cooper
2017-11-17 10:48 ` Wei Liu
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Andrew Cooper @ 2017-11-16 19:15 UTC (permalink / raw)
To: Xen-devel
Cc: Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper, Julien Grall,
Jun Nakajima, Boris Ostrovsky, Suravee Suthikulpanit
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream
2017-11-16 19:15 [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream Andrew Cooper
@ 2017-11-17 10:48 ` Wei Liu
2017-11-17 12:10 ` Jan Beulich
2017-11-27 1:41 ` Tian, Kevin
2 siblings, 0 replies; 6+ messages in thread
From: Wei Liu @ 2017-11-17 10:48 UTC (permalink / raw)
To: Andrew Cooper
Cc: Kevin Tian, Wei Liu, Jan Beulich, Xen-devel, Julien Grall,
Jun Nakajima, Boris Ostrovsky, Suravee Suthikulpanit
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream
2017-11-16 19:15 [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream Andrew Cooper
2017-11-17 10:48 ` Wei Liu
@ 2017-11-17 12:10 ` Jan Beulich
2017-11-20 14:10 ` Andrew Cooper
2017-11-27 1:41 ` Tian, Kevin
2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-11-17 12:10 UTC (permalink / raw)
To: Andrew Cooper
Cc: Kevin Tian, Wei Liu, Suravee Suthikulpanit, Xen-devel,
Julien Grall, Jun Nakajima, Boris Ostrovsky
>>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream
2017-11-17 12:10 ` Jan Beulich
@ 2017-11-20 14:10 ` Andrew Cooper
2017-11-20 14:32 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2017-11-20 14:10 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Wei Liu, Suravee Suthikulpanit, Xen-devel,
Julien Grall, Jun Nakajima, Boris Ostrovsky
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream
2017-11-20 14:10 ` Andrew Cooper
@ 2017-11-20 14:32 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2017-11-20 14:32 UTC (permalink / raw)
To: Andrew Cooper
Cc: Kevin Tian, Wei Liu, Suravee Suthikulpanit, Xen-devel,
Julien Grall, Jun Nakajima, Boris Ostrovsky
>>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream
2017-11-16 19:15 [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream Andrew Cooper
2017-11-17 10:48 ` Wei Liu
2017-11-17 12:10 ` Jan Beulich
@ 2017-11-27 1:41 ` Tian, Kevin
2 siblings, 0 replies; 6+ messages in thread
From: Tian, Kevin @ 2017-11-27 1:41 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Wei Liu, Nakajima, Jun, Julien Grall, Jan Beulich,
Suravee Suthikulpanit, Boris Ostrovsky
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-27 1:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-16 19:15 [PATCH for-4.10] x86/hvm: Don't ignore unknown MSRs in the migration stream Andrew Cooper
2017-11-17 10:48 ` Wei Liu
2017-11-17 12:10 ` Jan Beulich
2017-11-20 14:10 ` Andrew Cooper
2017-11-20 14:32 ` Jan Beulich
2017-11-27 1:41 ` Tian, Kevin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).