| Message ID | 1510859732-17588-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
>>> 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
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
>>> 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
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;
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(-)