Re: kernel bug in "Drop WIMG in favour of new constants"?

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:

> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
>
>> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
>>
>>> "Darrick J. Wong" <darrick.wong@oracle.com> writes:
>>>
>>>> On Thu, Jun 16, 2016 at 03:23:47PM +1000, Michael Ellerman wrote:
>>>>> On Wed, 2016-06-15 at 21:33 -0700, Darrick J. Wong wrote:
>>>>> 
>>>>> > Hi Aneesh,
>>>>> > 
>>>>> > I noticed when trying out 4.7-rc3 on qemu-2.5 that the kernel no longer
>>>>> > boots.  4.6 booted just fine, so I bisected the kernel to the commit
>>>>> > 30bda41aba4efb2370c97e2cbe7385de93ccc372, which is "powerpc/mm: Drop WIMG in
>>>>> > favour of new constants".  The changelog suggests that the KVM changes need
>>>>> > closer review, and here's an actual crash:
>>>>> > 
>>>>> > (I can send libvirt's machine xml, .config, and full dmesg if that helps.)
>>>>> 
>>>>> Yes please.
>>>>> 
>>>>> I'm successfully booting 4.7-rc's on qemu (2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.1)).
>>>>
>>>> Ok, see attached.  I also sent along the dpkg --status output for qemu
>>>> and qemu-slof; looks like we're running the same Ubuntu packages...
>>>>
>>>> ...my host kernel is 4.6.0 on x64.
>>>
>>> So this is Qemu TCG mode right ? I will try some test and update later.
>>>
>>
>> I am able to reproduce this with
>>
>> qemu-system-ppc64 -kernel vmlinux -machine
>> type=pseries,usb=off -smp 1 -m 1G  -vga none -nographic    -device
>> usb-ehci -device usb-kbd -device usb-mouse
>>
>> Looks like enabling usb device is the issue.
>>
>
> Hmm Qemu  does
>
>         /* Looks like an IO address */
>         /* FIXME: What WIMG combinations could be sensible for IO?
>          * For now we allow WIMG=010x, but are there others? */
>         /* FIXME: Should we check against registered IO addresses? */
>         if ((ptel & (HPTE64_R_W | HPTE64_R_I | HPTE64_R_M)) != HPTE64_R_I) {
>             return H_PARAMETER;
>         }
>


Can you try this patch ?

Ben,

The pHyp part of the comment was added by you in 
3c726f8dee6f55e96475574e9f645327e461884c ([PATCH] ppc64: support 64k
page) really an old commit. I also remember we having the discussion and
concluding that it should be safe to assume that we can always enable
memory coherence. Should we do the below patch of get Qemu fixed up ?
like below

-        if ((ptel & (HPTE64_R_W | HPTE64_R_I | HPTE64_R_M)) != HPTE64_R_I) {
+        wimg_flags = (ptel & (HPTE64_R_W | HPTE64_R_I | HPTE64_R_M));
+
+        if (wimg_flags != HPTE64_R_I && wimg_flags != (HPTE64_R_I | HPTE64_R_M)) {
             return H_PARAMETER;
         }


commit 58f32706f64ee8fe21bbd7d6c211720cedec1292
Author: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Date:   Thu Jun 16 19:43:39 2016 +0530

    powerpc/mm/hash: Don't add memory coherence if cache inhibited is set
    
    H_ENTER hcall handling in qemu had assumptions that a cache inhibited
    hpte entry won't have memory conference set. Also older kernel
    mentioned that some version of pHyp required this (look at the comment
    removed by the commit mentioned below).
    
    Fixes: commit 30bda41aba4e("powerpc/mm: Drop WIMG in favour of new constant")
    
    Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
index fcf676f7f522..89c2791bc510 100644
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c
@@ -201,9 +201,8 @@ unsigned long htab_convert_pte_flags(unsigned long pteflags)
 	/*
 	 * We can't allow hardware to update hpte bits. Hence always
 	 * set 'R' bit and set 'C' if it is a write fault
-	 * Memory coherence is always enabled
 	 */
-	rflags |=  HPTE_R_R | HPTE_R_M;
+	rflags |=  HPTE_R_R;
 
 	if (pteflags & _PAGE_DIRTY)
 		rflags |= HPTE_R_C;
@@ -216,7 +215,12 @@ unsigned long htab_convert_pte_flags(unsigned long pteflags)
 	if ((pteflags & _PAGE_CACHE_CTL ) == _PAGE_NON_IDEMPOTENT)
 		rflags |= (HPTE_R_I | HPTE_R_G);
 	if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_SAO)
-		rflags |= (HPTE_R_I | HPTE_R_W);
+		rflags |= (HPTE_R_W | HPTE_R_I | HPTE_R_M);
+	/*
+	 * Add memory coherence if cache inhibited is not set
+	 */
+	if (!(rflags & HPTE_R_I))
+		rflags |= HPTE_R_M;
 
 	return rflags;
 }