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

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

On 7/8/2016 10:21 AM, Jan Beulich wrote:
> > > > On 06.07.16 at 17:50, <czuzu@xxxxxxxxxxxxxxx> wrote:
> The title of this patch keeps confusing me - which code motion is
> being relocated here?
As the commit message clearly states, the code motions that are being 
relocated are:
1) handling of monitor_write_data @ hvm_do_resume
2) the code in vmx_update_guest_cr (when cr = 0) that deals with setting 
CR3 load-exiting for cr-write monitor vm-events, i.e. the comment:
    /* Trap CR3 updates if CR3 memory events are enabled. */
and what's removed from under it.

By 'relocation' I meant making that code vm-event specific (moving it to 
vm-event specific files).
> > +void vmx_vm_event_update_cr3_traps(struct domain *d)
> Is there anything in this function preventing the parameter to be
> const?
Nope, will do.

> > +{
> > +    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.
As in change to "/* Domain must be paused. */"?

> > +    /* 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.
The "vmx_update_guest_cr code motion for cr = 0", that's what's 
referring to.
'vmx_update_guest_cr()' is a function, 'cr' is one of its parameters.
In other words, see what's happening in the function 
'vmx_update_guest_cr() when you pass it cr = 0' and you'll understand 
why CR3 load-exiting must remain enabled when CR0.PE=0.
> > +        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.
Would this do (aligned with the above observation):

"

        /*
         * If CR3 load-exiting was enabled and CR0.PE=0, then it must remain
         * enabled (see vmx_update_guest_cr(v, cr) function when cr = 0).
         */

"
?

> > +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.
The particular reason is cr-indexes being uint8_t typed (see 
typeof(xen_domctl_monitor_op.mov_to_cr.index)).
But I will change it to unsigned int if you prefer (maybe you could 
explain the preference though).
> > +{
> > +    /* vmx only */
> > +    ASSERT(cpu_has_vmx);
> Comment style (more below). Should perhaps also get "for now" or
> some such added.
As in "/* For now, VMX only. */"?

> > +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.
No, it would be wrong because that ASSERT may not hold if "old == 0", 
i.e. we only ASSERT the implication "CR-write vm-events can be enabled 
-> vmx domain", but since the function is called by 
arch_monitor_cleanup_domain, putting the ASSERT before the if() would 
change that implication to "(any) monitor vm-events available -> vmx 
domain", assertion which wouldn't be proper TBD here.
> > @@ -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
Ack.

Thanks,
Corneliu.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel