Re: [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately

["Jan Beulich" <JBeulich@suse.com>]

>>> On 06.07.16 at 17:50, <czuzu@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