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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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