>>> On 06.07.16 at 17:50, <cz...@bitdefender.com> wrote: The title of this patch keeps confusing me - which code motion is being relocated here?
> +void vmx_vm_event_update_cr3_traps(struct domain *d) Is there anything in this function preventing the parameter to be const? > +{ > + struct vcpu *v; > + struct arch_vmx_struct *avmx; > + unsigned int cr3_bitmask; > + bool_t cr3_vmevent, cr3_ldexit; > + > + /* domain must be paused */ > + ASSERT(atomic_read(&d->pause_count)); Comment style. > + /* non-hap domains trap CR3 writes unconditionally */ > + if ( !paging_mode_hap(d) ) > + { > +#ifndef NDEBUG > + for_each_vcpu ( d, v ) > + ASSERT(v->arch.hvm_vmx.exec_control & > CPU_BASED_CR3_LOAD_EXITING); > +#endif > + return; > + } > + > + cr3_bitmask = monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3); > + cr3_vmevent = !!(d->arch.monitor.write_ctrlreg_enabled & cr3_bitmask); > + > + for_each_vcpu ( d, v ) > + { > + avmx = &v->arch.hvm_vmx; > + cr3_ldexit = !!(avmx->exec_control & CPU_BASED_CR3_LOAD_EXITING); > + > + if ( cr3_vmevent == cr3_ldexit ) > + continue; > + > + /* > + * If CR0.PE=0, CR3 load exiting must remain enabled. > + * See vmx_update_guest_cr code motion for cr = 0. > + */ Same as for the title - what code motion is this referring to? In a code comment you clearly shouldn't be referring to anything the patch effects, only to its result. > + if ( cr3_ldexit && !hvm_paging_enabled(v) && > !vmx_unrestricted_guest(v) ) > + continue; The first sentence of the comment should be brought in line with this condition. > +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t > index) Unless there is a particular reason for this uint8_t, please convert to unsigned int. > +{ > + /* vmx only */ > + ASSERT(cpu_has_vmx); Comment style (more below). Should perhaps also get "for now" or some such added. > +static inline void write_ctrlreg_disable_traps(struct domain *d) > +{ > + unsigned int old = d->arch.monitor.write_ctrlreg_enabled; > + d->arch.monitor.write_ctrlreg_enabled = 0; > + > + if ( old ) > + { > + /* vmx only */ > + ASSERT(cpu_has_vmx); Wouldn't this better move ahead of the if()? > + /* was CR3 load-exiting enabled due to monitoring? */ > + if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) ) And then this if() alone would suffice. > @@ -36,11 +63,40 @@ int arch_monitor_init_domain(struct domain *d) > void arch_monitor_cleanup_domain(struct domain *d) > { > xfree(d->arch.monitor.msr_bitmap); > - > + write_ctrlreg_disable_traps(d); > memset(&d->arch.monitor, 0, sizeof(d->arch.monitor)); > memset(&d->monitor, 0, sizeof(d->monitor)); > } Rather than deleting the blank line, perhaps better to insert another one after your addition? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel