Re: [Xen-devel] [PATCH] svm: support VMCB cleanbits

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

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?

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.

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel