On 15/10/18 14:56, Roger Pau Monné wrote: > 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.
Clearer => absolutely. However, your suggestion won't compile. domctl.c: In function ‘arch_get_info_guest’: domctl.c:1556:49: error: lvalue required as left operand of assignment c(flags) = v->arch.vgc_flags & ~(VGCF_i387_valid|VGCF_in_kernel); ^ which is why all the code is written like this. > >> 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. There is no point wasting storage for 4 bytes which are not used. After all, its 1/1000'th of struct vcpu. > >> + >> /* 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. dr7 would ideally be 32 bits wide, but __vmread() uses unsigned long *. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
