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 13:33, <czuzu@xxxxxxxxxxxxxxx> wrote:
> On 7/8/2016 1:37 PM, Jan Beulich wrote:
>>>>> 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:
>>>>> +        /*
>>>>> +         * If CR0.PE=0, CR3 load exiting must remain enabled.
>>>>> +         * See vmx_update_guest_cr code motion for cr = 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).
>           */
> "
> ?

Looks reasonable.

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

In this case the compiler may well do (as the function is static
inline), but in other cases it's simply not allowed to. In order to
not misguide people cloning existing code we should thus try to
limit the number of bad examples (which in particular mean not
introducing any new ones).

Jan


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