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

[Corneliu ZUZU <czuzu@bitdefender.com>]

On 7/8/2016 1:37 PM, Jan Beulich wrote:
>>>> On 08.07.16 at 12:22, <czuzu@bitdefender.com> wrote:
>> On 7/8/2016 10:21 AM, Jan Beulich wrote:
>>>>>> 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?
>> 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...

Hah, sorry, I'm not very good expressivity-wise, a weakness I'm aware of 
and which makes me pick up expressions I notice other people use (in 
this case those of maintainers).
I think you were the one I noticed using the expression back in an older 
patch-series I've sent and I thought by "code-motion" you meant simply 
"that which some code does, tries to accomplish and the code itself".

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

Again, sorry, will try to rephrase all of this properly :-).

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

Hah you're right, I only now notice, that comment has actually been 
adopted (although I don't remember from where, I wonder if it was 
meantime removed and I only now see), I always thought it said "CR0.PG = 
0"...
So...
"

         /*
          * If domain paging is disabled (CR0.PG=0) and
          * the domain is not in real-mode, then CR3 load-exiting
          * must remain enabled (see vmx_update_guest_cr(v, cr) 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).
> 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.

Strange, I would have thought the compiler would properly (and easily) 
deal with such efficiency issues.

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

Thanks,
Corneliu.

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