Re: [Xen-devel] [PATCH v3 2/3] x86/PV: support data breakpoint extension registers

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Mon, Apr 7, 2014 at 10:38 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> Introducing an extension to XEN_DOMCTL_[gs]et_ext_vcpucontext similar
> to the generic MSR save/restore logic recently added for HVM.
>
> This also moves some debug register related declarations/definition to
> the header intended for these.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Tested-by: Aravind Gopalakrishnan<aravind.gopalakrishnan@xxxxxxx>
> ---
> v3: Split off save/restore code to a separate patch, allowing this one
>     to be reviewed/applied independently.
>
> --- a/xen/arch/x86/acpi/suspend.c
> +++ b/xen/arch/x86/acpi/suspend.c
> @@ -9,6 +9,7 @@
>  #include <xen/smp.h>
>  #include <asm/processor.h>
>  #include <asm/msr.h>
> +#include <asm/debugreg.h>
>  #include <asm/flushtlb.h>
>  #include <asm/hvm/hvm.h>
>  #include <asm/hvm/support.h>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1341,14 +1341,7 @@ static void paravirt_ctxt_switch_to(stru
>          write_cr4(cr4);
>
>      if ( unlikely(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
> -    {
> -        write_debugreg(0, v->arch.debugreg[0]);
> -        write_debugreg(1, v->arch.debugreg[1]);
> -        write_debugreg(2, v->arch.debugreg[2]);
> -        write_debugreg(3, v->arch.debugreg[3]);
> -        write_debugreg(6, v->arch.debugreg[6]);
> -        write_debugreg(7, v->arch.debugreg[7]);
> -    }
> +        activate_debugregs(v);
>
>      if ( (v->domain->arch.tsc_mode ==  TSC_MODE_PVRDTSCP) &&
>           boot_cpu_has(X86_FEATURE_RDTSCP) )
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c

[snip]

> @@ -854,7 +854,42 @@ long arch_do_domctl(
>              evc->vmce.mci_ctl2_bank0 = v->arch.vmce.bank[0].mci_ctl2;
>              evc->vmce.mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2;
>
> -            ret = 0;
> +            i = ret = 0;
> +            if ( boot_cpu_has(X86_FEATURE_DBEXT) )
> +            {
> +                unsigned int j;
> +
> +                if ( v->arch.pv_vcpu.dr_mask[0] )
> +                {
> +                    if ( i < evc->msr_count && !ret )
> +                    {
> +                        msr.index = MSR_AMD64_DR0_ADDRESS_MASK;
> +                        msr.reserved = 0;
> +                        msr.value = v->arch.pv_vcpu.dr_mask[0];
> +                        if ( copy_to_guest_offset(evc->msrs, i, &msr, 1) )

Sorry if I missed something, but is there any bounds-checking on this
buffer?  I don't see any specification in the header file about how
big the buffer needs to be, nor any conceptual limit to the number of
elements which might be copied into it -- at least in this path.

> @@ -3628,7 +3660,27 @@ long do_set_trap_table(XEN_GUEST_HANDLE_
>      return rc;
>  }
>
> -long set_debugreg(struct vcpu *v, int reg, unsigned long value)
> +void activate_debugregs(const struct vcpu *curr)
> +{
> +    ASSERT(curr == current);
> +
> +    write_debugreg(0, curr->arch.debugreg[0]);
> +    write_debugreg(1, curr->arch.debugreg[1]);
> +    write_debugreg(2, curr->arch.debugreg[2]);
> +    write_debugreg(3, curr->arch.debugreg[3]);
> +    write_debugreg(6, curr->arch.debugreg[6]);
> +    write_debugreg(7, curr->arch.debugreg[7]);
> +
> +    if ( boot_cpu_has(X86_FEATURE_DBEXT) )
> +    {
> +        wrmsrl(MSR_AMD64_DR0_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[0]);
> +        wrmsrl(MSR_AMD64_DR1_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[1]);
> +        wrmsrl(MSR_AMD64_DR2_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[2]);
> +        wrmsrl(MSR_AMD64_DR3_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[3]);
> +    }
> +}
> +
> +long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
>  {
>      int i;
>      struct vcpu *curr = current;
> @@ -3709,11 +3761,8 @@ long set_debugreg(struct vcpu *v, int re
>              if ( (v == curr) &&
>                   !(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
>              {
> -                write_debugreg(0, v->arch.debugreg[0]);
> -                write_debugreg(1, v->arch.debugreg[1]);
> -                write_debugreg(2, v->arch.debugreg[2]);
> -                write_debugreg(3, v->arch.debugreg[3]);
> -                write_debugreg(6, v->arch.debugreg[6]);
> +                activate_debugregs(curr);

So in the other place you replaced with activate_debugregs(), it
writes DR7; but here it doesn't -- and yet the function will write
DR7.  Is that a problem?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel