On 06/06/18 16:00, Jan Beulich wrote: >>>> On 04.06.18 at 15:59, <[email protected]> wrote: >> @@ -981,9 +981,14 @@ int arch_set_info_guest( >> v->arch.pv_vcpu.ctrlreg[4] = cr4 ? pv_guest_cr4_fixup(v, cr4) : >> real_cr4_to_pv_guest_cr4(mmu_cr4_features); >> >> - memset(v->arch.debugreg, 0, sizeof(v->arch.debugreg)); >> - for ( i = 0; i < 8; i++ ) >> - (void)set_debugreg(v, i, c(debugreg[i])); >> + for ( i = 0; !rc && i < ARRAY_SIZE(v->arch.dr); i++ ) >> + rc = set_debugreg(v, i, c(debugreg[i])); >> + if ( !rc ) >> + rc = set_debugreg(v, 6, c(debugreg[6])); >> + if ( !rc ) >> + rc = set_debugreg(v, 7, c(debugreg[7])); >> + if ( rc ) >> + return rc; > There is certainly a good intention behind this change, but it treats > one problem for another: The function has a pre-existing partial- > update-and-then-fail problem, which you now widen. Proper > behavior would imo be to never update any state when returning > an error, at least as far as anything ahead of > >> if ( v->is_initialised ) >> goto out; > goes (for not yet initialized vCPU-s the function would need to be > called again anyway before the vCPU could be started). On the > whole, until said problem gets addressed, I think I'd prefer original > behavior over the new one you switch to.
Hmm ok. > >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -1861,8 +1861,8 @@ void do_debug(struct cpu_user_regs *regs) >> } >> >> /* Save debug status register where guest OS can peek at it */ >> - v->arch.debugreg[6] |= (dr6 & ~X86_DR6_DEFAULT); >> - v->arch.debugreg[6] &= (dr6 | ~X86_DR6_DEFAULT); >> + v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT); >> + v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT); > Shouldn't this have been changed by patch 4? This gets fixed by patch 9. > >> @@ -518,7 +524,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]; >> + unsigned long dr6, dr7; > Since you make the last two separate fields, and since their upper > 32 bits are reserved-zero, why not make them uint32_t, just like > dr7_emul is? Because __vmread() takes unsigned long *. I did initially make them uint32_t, then had the compiler complain at me. I indented to leave a note in the commit message, but I forgot. Sorry. ~Andrew _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
