| Message ID | 58E66C79020000780014E07A@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/04/17 15:27, Jan Beulich wrote: > Doing so may not only confuse them, but will - on VMX - lead to > VMRESUME failures. Add respective ASSERT()s where the fields get set > to guard against future similar issues (or - in the restore case - > fail the operation). In that latter code at once convert the mis-used > gdprintk() to dprintk(), as the vCPU of interest is not "current". > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I'd drop the unnecessary full stops from the printed text. Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> On 06.04.17 at 16:33, <andrew.cooper3@citrix.com> wrote: > On 06/04/17 15:27, Jan Beulich wrote: >> Doing so may not only confuse them, but will - on VMX - lead to >> VMRESUME failures. Add respective ASSERT()s where the fields get set >> to guard against future similar issues (or - in the restore case - >> fail the operation). In that latter code at once convert the mis-used >> gdprintk() to dprintk(), as the vCPU of interest is not "current". >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > I'd drop the unnecessary full stops from the printed text. Oops - I didn't pay attention (and even copied them). Thanks for spotting. > Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks, Jan
> --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -269,13 +269,23 @@ static int svm_vmcb_restore(struct vcpu > struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; > struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); > > - if ( c->pending_valid && > - ((c->pending_type == 1) || (c->pending_type > 6) || > - (c->pending_reserved != 0)) ) > + if ( c->pending_valid ) > { > - gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n", > - c->pending_event); > - return -EINVAL; > + if ( (c->pending_type == 1) || (c->pending_type > 6) || Shouldn't this be >=5? The only valid types are 0, 2, 3 and 4. -boris
On 06/04/17 15:59, Boris Ostrovsky wrote: >> --- a/xen/arch/x86/hvm/svm/svm.c >> +++ b/xen/arch/x86/hvm/svm/svm.c >> @@ -269,13 +269,23 @@ static int svm_vmcb_restore(struct vcpu >> struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; >> struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); >> >> - if ( c->pending_valid && >> - ((c->pending_type == 1) || (c->pending_type > 6) || >> - (c->pending_reserved != 0)) ) >> + if ( c->pending_valid ) >> { >> - gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n", >> - c->pending_event); >> - return -EINVAL; >> + if ( (c->pending_type == 1) || (c->pending_type > 6) || > Shouldn't this be >=5? The only valid types are 0, 2, 3 and 4. That is a good point. SVM has fewer valid types than VT-x. I guess this was a copy&paste mistake in the past. ~Andrew
>>> On 06.04.17 at 16:59, <boris.ostrovsky@oracle.com> wrote: >> --- a/xen/arch/x86/hvm/svm/svm.c >> +++ b/xen/arch/x86/hvm/svm/svm.c >> @@ -269,13 +269,23 @@ static int svm_vmcb_restore(struct vcpu >> struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; >> struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); >> >> - if ( c->pending_valid && >> - ((c->pending_type == 1) || (c->pending_type > 6) || >> - (c->pending_reserved != 0)) ) >> + if ( c->pending_valid ) >> { >> - gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n", >> - c->pending_event); >> - return -EINVAL; >> + if ( (c->pending_type == 1) || (c->pending_type > 6) || > > Shouldn't this be >=5? The only valid types are 0, 2, 3 and 4. I don't know, I'm only moving this code. Changes to the values should be a separate patch. Jan
On 04/06/2017 11:11 AM, Jan Beulich wrote: >>>> On 06.04.17 at 16:59, <boris.ostrovsky@oracle.com> wrote: >>> --- a/xen/arch/x86/hvm/svm/svm.c >>> +++ b/xen/arch/x86/hvm/svm/svm.c >>> @@ -269,13 +269,23 @@ static int svm_vmcb_restore(struct vcpu >>> struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; >>> struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); >>> >>> - if ( c->pending_valid && >>> - ((c->pending_type == 1) || (c->pending_type > 6) || >>> - (c->pending_reserved != 0)) ) >>> + if ( c->pending_valid ) >>> { >>> - gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n", >>> - c->pending_event); >>> - return -EINVAL; >>> + if ( (c->pending_type == 1) || (c->pending_type > 6) || >> Shouldn't this be >=5? The only valid types are 0, 2, 3 and 4. > I don't know, I'm only moving this code. Changes to the values > should be a separate patch. Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> I can send a patch to adjust type check once this is committed to staging unless you want to do it yourself. -boris
> From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Thursday, April 6, 2017 10:28 PM > > Doing so may not only confuse them, but will - on VMX - lead to > VMRESUME failures. Add respective ASSERT()s where the fields get set > to guard against future similar issues (or - in the restore case - > fail the operation). In that latter code at once convert the mis-used > gdprintk() to dprintk(), as the vCPU of interest is not "current". > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
--- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3111,7 +3111,7 @@ static enum hvm_copy_result __hvm_copy( if ( pfinfo ) { pfinfo->linear = addr; - pfinfo->ec = pfec; + pfinfo->ec = pfec & ~PFEC_implicit; } return HVMCOPY_bad_gva_to_gfn; } --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -269,13 +269,23 @@ static int svm_vmcb_restore(struct vcpu struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); - if ( c->pending_valid && - ((c->pending_type == 1) || (c->pending_type > 6) || - (c->pending_reserved != 0)) ) + if ( c->pending_valid ) { - gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n", - c->pending_event); - return -EINVAL; + if ( (c->pending_type == 1) || (c->pending_type > 6) || + (c->pending_reserved != 0) ) + { + dprintk(XENLOG_ERR, "%pv: Invalid pending event %#"PRIx32".\n", + v, c->pending_event); + return -EINVAL; + } + + if ( c->pending_error_valid && + c->error_code != (uint16_t)c->error_code ) + { + dprintk(XENLOG_ERR, "%pv: Invalid error code %#"PRIx32".\n", + v, c->error_code); + return -EINVAL; + } } if ( !paging_mode_hap(v->domain) ) @@ -1288,6 +1298,8 @@ static void svm_inject_event(const struc vmcb->nextrip = (uint32_t)vmcb->nextrip; } + ASSERT(!eventinj.fields.ev || + eventinj.fields.errorcode == (uint16_t)eventinj.fields.errorcode); vmcb->eventinj = eventinj; if ( _event.vector == TRAP_page_fault ) --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -731,13 +731,23 @@ static int vmx_vmcs_restore(struct vcpu { int rc; - if ( c->pending_valid && - ((c->pending_type == 1) || (c->pending_type > 6) || - (c->pending_reserved != 0)) ) + if ( c->pending_valid ) { - gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n", - c->pending_event); - return -EINVAL; + if ( (c->pending_type == 1) || (c->pending_type > 6) || + (c->pending_reserved != 0) ) + { + dprintk(XENLOG_ERR, "%pv: Invalid pending event %#"PRIx32".\n", + v, c->pending_event); + return -EINVAL; + } + + if ( c->pending_error_valid && + c->error_code != (uint16_t)c->error_code ) + { + dprintk(XENLOG_ERR, "%pv: Invalid error code %#"PRIx32".\n", + v, c->error_code); + return -EINVAL; + } } rc = vmx_restore_cr0_cr3(v, c->cr0, c->cr3); @@ -1660,6 +1670,7 @@ static void __vmx_inject_exception(int t MASK_INSR(trap, INTR_INFO_VECTOR_MASK); if ( error_code != X86_EVENT_NO_EC ) { + ASSERT(error_code == (uint16_t)error_code); __vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, error_code); intr_fields |= INTR_INFO_DELIVER_CODE_MASK; }