On 18.07.2013, at 17:15, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: [email protected] [mailto:[email protected]] On
>> Behalf Of Alexander Graf
>> Sent: Thursday, July 18, 2013 8:23 PM
>> To: Bhushan Bharat-R65777
>> Cc: [email protected]; [email protected]; Wood Scott-B07421; Bhushan
>> Bharat-R65777
>> Subject: Re: [PATCH 2/2 v2] kvm: powerpc: set cache coherency only for kernel
>> managed pages
>>
>>
>> On 18.07.2013, at 15:19, Bharat Bhushan wrote:
>>
>>> If there is a struct page for the requested mapping then it's normal
>>> RAM and the mapping is set to "M" bit (coherent, cacheable) otherwise
>>> this is treated as I/O and we set "I + G" (cache inhibited, guarded)
>>>
>>> This helps setting proper TLB mapping for direct assigned device
>>>
>>> Signed-off-by: Bharat Bhushan <[email protected]>
>>> ---
>>> v2: some cleanup and added comment
>>> -
>>> arch/powerpc/kvm/e500_mmu_host.c | 23 ++++++++++++++++++-----
>>> 1 files changed, 18 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/e500_mmu_host.c
>>> b/arch/powerpc/kvm/e500_mmu_host.c
>>> index 1c6a9d7..02eb973 100644
>>> --- a/arch/powerpc/kvm/e500_mmu_host.c
>>> +++ b/arch/powerpc/kvm/e500_mmu_host.c
>>> @@ -64,13 +64,26 @@ static inline u32 e500_shadow_mas3_attrib(u32 mas3, int
>> usermode)
>>> return mas3;
>>> }
>>>
>>> -static inline u32 e500_shadow_mas2_attrib(u32 mas2, int usermode)
>>> +static inline u32 e500_shadow_mas2_attrib(u32 mas2, pfn_t pfn)
>>> {
>>> + u32 mas2_attr;
>>> +
>>> + mas2_attr = mas2 & MAS2_ATTRIB_MASK;
>>> +
>>> + /*
>>> + * RAM is always mappable on e500 systems, so this is identical
>>> + * to kvm_is_mmio_pfn(), just without its overhead.
>>> + */
>>> + if (!pfn_valid(pfn)) {
>>> + /* Pages not managed by Kernel are treated as I/O, set I + G */
>>
>> Please also document the intermediate thought that I/O should be mapped non-
>> cached.
>
> I did not get what you mean to document?
Page snot managed by the kernel are treated as I/O. Map it
with disabled cache.
>
>>
>>> + mas2_attr |= MAS2_I | MAS2_G;
>>> #ifdef CONFIG_SMP
>>
>> Please separate the SMP case out of the branch.
>
> Really :) this was looking simple to me.
Two branches intertwined never look simple :).
>
>>
>>> - return (mas2 & MAS2_ATTRIB_MASK) | MAS2_M;
>>> -#else
>>> - return mas2 & MAS2_ATTRIB_MASK;
>>> + } else {
>>> + /* Kernel managed pages are actually RAM so set "M" */
>>
>> This comment doesn't tell me why M can be set ;).
>
> RAM in SMP, so setting coherent, is not that obvious?
No.
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
