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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.07.16 at 12:22, <czuzu@xxxxxxxxxxxxxxx> wrote:
> 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:

Again this sentence makes no sense to me: I can't see how
"code motions" can be "relocated", just like I don't see how you
could move a move. But maybe it's just me...

> 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).

Yes, that what I've guessed.

>>> +{
>>> +    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. */"?

Yes, as mandated by ./CODING_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.
> 
> The "vmx_update_guest_cr code motion for cr = 0", that's what's 
> referring to.

So I guess my problem really is that I don't understand what a
"code motion" is (other than the act of moving code from one
place to another).

> '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).
>           */
> 
> "
> ?

Not really: The condition checks whether paging is enabled and
whether it's an unrestricted guest. The comment talks about
protected mode being enabled.

>>> +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).

No use of fixed width types when fixed width types aren't really
required. Generally generated code is less efficient when having
to deal with fixed width types.

>>> +{
>>> +    /* 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. */"?

For example, yes.

>>> +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.

Ah, okay - I was under the impression that no VM events were
allowed under SVM.

Jan

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