Re: [Xen-devel] [For Xen-4.10 Resend PATCH 3/3] Avoid excess icache flushes in populate_physmap() before domain has been created

[Punit Agrawal <punit.agrawal@xxxxxxx>]

"Jan Beulich" <JBeulich@xxxxxxxx> writes:

>>>> On 17.05.17 at 18:15, <punit.agrawal@xxxxxxx> wrote:
>> Jan Beulich <JBeulich@xxxxxxxx> writes:
>> 
>>>>>> On 15.05.17 at 16:10, <punit.agrawal@xxxxxxx> wrote:
>>>> --- a/xen/include/asm-x86/page.h
>>>> +++ b/xen/include/asm-x86/page.h
>>>> @@ -375,6 +375,10 @@ perms_strictly_increased(uint32_t old_flags, uint32_t 
>>>> new_flags)
>>>>  
>>>>  #define PAGE_ALIGN(x) (((x) + PAGE_SIZE - 1) & PAGE_MASK)
>>>>  
>>>> +static inline void invalidate_icache(void)
>>>> +{
>>>> +}
>>>
>>> This function clearly does not what its name says, so there should
>>> be a brief comment saying why.
>> 
>> Ack. I've added the following comment block above the function
>> definition.
>> 
>> /*
>>  * While allocating memory for a domain, invalidate_icache() is called
>>  * to ensure that guest vcpu does not execute any stale instructions
>>  * from the recently allocated memory. There is nothing to be done
>>  * here as icaches are coherent on x86.
>>  */
>> 
>> My x86 foo is weak and I'd appreciate if somebody familiar with x86
>> could give this a once over.
>
> The text looks okay, but it ties the function to its _current_ single
> user. I'd like to ask you to rather use just the last sentence, and it's
> questionable (a matter of taste mostly) whether such a comment
> wouldn't better be placed inside the function. And perhaps it would
> be a good idea to insert "sufficiently", as they're not fully coherent
> (see the self modifying code constraints on MP systems).

I've updated the text and moved it inside the function.

Thanks for the feedback!

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

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