On Wednesday 15 December 2010 12:27:51 Tim Deegan wrote:
> Hi,
>
> Thanks for the patch!
>
> At 10:52 +0000 on 15 Dec (1292410374), Christoph Egger wrote:
> > @@ -660,16 +671,17 @@ static void svm_ctxt_switch_to(struct vc
> >
> > static void svm_do_resume(struct vcpu *v)
> > {
> > + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> > bool_t debug_state = v->domain->debugger_attached;
> >
> > if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) )
> > {
> > - uint32_t mask = (1U << TRAP_debug) | (1U << TRAP_int3);
> > v->arch.hvm_vcpu.debug_state_latch = debug_state;
> > if ( debug_state )
> > - v->arch.hvm_svm.vmcb->exception_intercepts |= mask;
> > - else
> > - v->arch.hvm_svm.vmcb->exception_intercepts &= ~mask;
> > + {
> > + svm_set_exception_intercept(vmcb, TRAP_debug);
> > + svm_set_exception_intercept(vmcb, TRAP_int3);
> > + }
>
> This seems to change the logic so it doesn't clear the intercepts if
> debug_state == 0. Is that OK?
No, that's not ok. I fixed that in the new patch.
> More generally, I'm not sure I like having all the VMCB accessor
> functions in files called "cleanbits" -- wouldn't it make sense to have
> all that in the vmcb files so people will see them and know to use them?
> You could rename the actual vmcb fields as well to catch anyone writing
> them directly, e.g. in forward-ported patches.
I renamed the 'svmcleanbits.[ch]' files to 'vmc_funcs.[ch]'
Thanks for your review.
Signed-off-by: Wei Huang <Wei.Huang2@xxxxxxx>
Signed-off-by: Christoph Egger <Christoph.Egger@xxxxxxx>
Christoph
--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
xen_vmcbcleanbits.diff
Description: xen_vmcbcleanbits.diff
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home