Re: [Xen-devel] [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Oct 15, 2018 at 11:36:20AM +0100, Andrew Cooper wrote:
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 115ddf6..cc85395 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1576,8 +1576,11 @@ void arch_get_info_guest(struct vcpu *v, 
> vcpu_guest_context_u c)
>          }
>      }
>  
> -    for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
> -        c(debugreg[i] = v->arch.debugreg[i]);
> +    for ( i = 0; i < ARRAY_SIZE(v->arch.dr); ++i )
> +        c(debugreg[i] = v->arch.dr[i]);
> +    c(debugreg[6] = v->arch.dr6);
> +    c(debugreg[7] = v->arch.dr7 |
> +      (is_pv_domain(d) ? v->arch.pv.dr7_emul : 0));

Wouldn't it be clearer to use c(debugreg[6]) = v->arch.dr6;? I know
existing code does as above, but I find it more difficult to
understand.

> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index cdb43e4..6c0887d 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -549,6 +549,12 @@ struct pv_vcpu
>      spinlock_t shadow_ldt_lock;
>  #endif
>  
> +    /*
> +     * %dr7 bits the guest has set, but aren't loaded into hardware, and are
> +     * completely emulated.
> +     */
> +    uint32_t dr7_emul;

Just to match the size of the actual register shouldn't this be
unsigned long?

I assume this is not very relevant because the high bits are not
actually used, but a comment might be appropriate here.

> +
>      /* data breakpoint extension MSRs */
>      uint32_t dr_mask[4];
>  
> @@ -567,7 +573,10 @@ struct arch_vcpu
>      void              *fpu_ctxt;
>      unsigned long      vgc_flags;
>      struct cpu_user_regs user_regs;
> -    unsigned long      debugreg[8];
> +
> +    /* Debug registers. */
> +    unsigned long dr[4], dr7;
> +    unsigned int dr6;

I'm likely missing some information here because I don't get why dr6
is 32bits while dr7 is 64bits wide. According to the spec the high
part (bits 63-32) on both registers is reserved, so I think it would
make more sense to use the same type for both.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel