On 26/01/18 10:58, Jan Beulich wrote: >>>> On 26.01.18 at 11:49, <[email protected]> wrote: >> On 01/26/2018 12:27 PM, Jan Beulich wrote: >>>>>> On 26.01.18 at 10:39, <[email protected]> wrote: >>>> --- a/xen/arch/x86/hvm/monitor.c >>>> +++ b/xen/arch/x86/hvm/monitor.c >>>> @@ -36,6 +36,12 @@ bool hvm_monitor_cr(unsigned int index, unsigned long >>>> value, unsigned long old) >>>> struct arch_domain *ad = &curr->domain->arch; >>>> unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index); >>>> >>>> + if ( index == 3 && hvm_pcid_enabled(curr) ) /* Clear the noflush bit. >>>> */ >>>> + { >>>> + value &= ((1ull << 63) - 1); >>>> + old &= ((1ull << 63) - 1); >>> Why would "old" need clearing the bit? It being set during write >>> doesn't actually get stored into the register, aiui. >> v->arch.hvm_vcpu.guest_cr[3] (which is used for "old") does get set in a >> number of places: >> >> $ grep -IRn "hvm_vcpu.guest_cr\[3\] =" >> arch/x86/hvm/hvm.c:2343: v->arch.hvm_vcpu.guest_cr[3] = value; >> arch/x86/hvm/hvm.c:3914: v->arch.hvm_vcpu.guest_cr[3] = 0; >> arch/x86/hvm/vmx/vmx.c:790: v->arch.hvm_vcpu.guest_cr[3] = cr3; >> arch/x86/hvm/vmx/vmx.c:3522: v->arch.hvm_vcpu.guest_cr[3] = >> v->arch.hvm_vcpu.hw_cr[3]; >> arch/x86/hvm/domain.c:207: v->arch.hvm_vcpu.guest_cr[3] = regs->cr3; >> arch/x86/hvm/domain.c:258: v->arch.hvm_vcpu.guest_cr[3] = regs->cr3; >> arch/x86/hvm/svm/svm.c:316: v->arch.hvm_vcpu.guest_cr[3] = c->cr3; >> arch/x86/hvm/svm/svm.c:2476: v->arch.hvm_vcpu.guest_cr[3] = >> v->arch.hvm_vcpu.hw_cr[3] = >> >> and I thought it's better to be safe than sorry. > As can be implied from the bit not being stored into the actual > register, we should either consistently store the bit and mask > it where needed or - following your patch - mask the bit before > storing a value. This latter approach also has the advantage > of properly causing failure when e.g. the incoming migration > stream has the bit set.
Luckily, we won't ever see this bit set in migration. Introspection currently needs to detach from the domain around migration, which means we will never end up having a vcpu paused on its introspection-cr3 exit with the bit set. What is however a problem for migration is that we don't know that the incoming domain has PCID available when auditing %cr3's content, and there is still a large quantity of work required until we can fix this. ~Andrew _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
