[PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R

[PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R

[Leandro Lupori]

When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
offset, causing the first byte of the adjacent PTE to be corrupted.
This caused a panic when booting FreeBSD, using the Hash MMU.

Fixes: a2dd4e83e76b ("ppc/hash64: Rework R and C bit updates")
Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
---
Changes from v3:
- rename defines
---
 hw/ppc/spapr.c          | 8 ++++----
 hw/ppc/spapr_softmmu.c  | 2 +-
 target/ppc/mmu-hash64.c | 4 ++--
 target/ppc/mmu-hash64.h | 5 +++++
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 163c90388a..3b5fd749be 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1414,7 +1414,7 @@ void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
         kvmppc_write_hpte(ptex, pte0, pte1);
     } else {
         if (pte0 & HPTE64_V_VALID) {
-            stq_p(spapr->htab + offset + HASH_PTE_SIZE_64 / 2, pte1);
+            stq_p(spapr->htab + offset + HPTE64_DW1, pte1);
             /*
              * When setting valid, we write PTE1 first. This ensures
              * proper synchronization with the reading code in
@@ -1430,7 +1430,7 @@ void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
              * ppc_hash64_pteg_search()
              */
             smp_wmb();
-            stq_p(spapr->htab + offset + HASH_PTE_SIZE_64 / 2, pte1);
+            stq_p(spapr->htab + offset + HPTE64_DW1, pte1);
         }
     }
 }
@@ -1438,7 +1438,7 @@ void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
 static void spapr_hpte_set_c(PPCVirtualHypervisor *vhyp, hwaddr ptex,
                              uint64_t pte1)
 {
-    hwaddr offset = ptex * HASH_PTE_SIZE_64 + 15;
+    hwaddr offset = ptex * HASH_PTE_SIZE_64 + HPTE64_DW1_C;
     SpaprMachineState *spapr = SPAPR_MACHINE(vhyp);
 
     if (!spapr->htab) {
@@ -1454,7 +1454,7 @@ static void spapr_hpte_set_c(PPCVirtualHypervisor *vhyp, hwaddr ptex,
 static void spapr_hpte_set_r(PPCVirtualHypervisor *vhyp, hwaddr ptex,
                              uint64_t pte1)
 {
-    hwaddr offset = ptex * HASH_PTE_SIZE_64 + 14;
+    hwaddr offset = ptex * HASH_PTE_SIZE_64 + HPTE64_DW1_R;
     SpaprMachineState *spapr = SPAPR_MACHINE(vhyp);
 
     if (!spapr->htab) {
diff --git a/hw/ppc/spapr_softmmu.c b/hw/ppc/spapr_softmmu.c
index f8924270ef..4ee03c83e4 100644
--- a/hw/ppc/spapr_softmmu.c
+++ b/hw/ppc/spapr_softmmu.c
@@ -426,7 +426,7 @@ static void new_hpte_store(void *htab, uint64_t pteg, int slot,
     addr += slot * HASH_PTE_SIZE_64;
 
     stq_p(addr, pte0);
-    stq_p(addr + HASH_PTE_SIZE_64 / 2, pte1);
+    stq_p(addr + HPTE64_DW1, pte1);
 }
 
 static int rehash_hpte(PowerPCCPU *cpu,
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 19832c4b46..da9fe99ff8 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -786,7 +786,7 @@ static void ppc_hash64_set_dsi(CPUState *cs, int mmu_idx, uint64_t dar, uint64_t
 
 static void ppc_hash64_set_r(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
 {
-    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 16;
+    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + HPTE64_DW1_R;
 
     if (cpu->vhyp) {
         PPCVirtualHypervisorClass *vhc =
@@ -803,7 +803,7 @@ static void ppc_hash64_set_r(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
 
 static void ppc_hash64_set_c(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
 {
-    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 15;
+    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + HPTE64_DW1_C;
 
     if (cpu->vhyp) {
         PPCVirtualHypervisorClass *vhc =
diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
index c5b2f97ff7..1496955d38 100644
--- a/target/ppc/mmu-hash64.h
+++ b/target/ppc/mmu-hash64.h
@@ -97,6 +97,11 @@ void ppc_hash64_finalize(PowerPCCPU *cpu);
 #define HPTE64_V_1TB_SEG        0x4000000000000000ULL
 #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
 
+/* PTE offsets */
+#define HPTE64_DW1              (HASH_PTE_SIZE_64 / 2)
+#define HPTE64_DW1_R            (HPTE64_DW1 + 6)
+#define HPTE64_DW1_C            (HPTE64_DW1 + 7)
+
 /* Format changes for ARCH v3 */
 #define HPTE64_V_COMMON_BITS    0x000fffffffffffffULL
 #define HPTE64_R_3_0_SSIZE_SHIFT 58
-- 
2.25.1

Re: [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4894 bytes --]

On Mon, Nov 29, 2021 at 03:57:51PM -0300, Leandro Lupori wrote:
> When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
> offset, causing the first byte of the adjacent PTE to be corrupted.
> This caused a panic when booting FreeBSD, using the Hash MMU.
> 
> Fixes: a2dd4e83e76b ("ppc/hash64: Rework R and C bit updates")
> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Thanks for your patience with our nitpicking :).

> ---
> Changes from v3:
> - rename defines
> ---
>  hw/ppc/spapr.c          | 8 ++++----
>  hw/ppc/spapr_softmmu.c  | 2 +-
>  target/ppc/mmu-hash64.c | 4 ++--
>  target/ppc/mmu-hash64.h | 5 +++++
>  4 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 163c90388a..3b5fd749be 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1414,7 +1414,7 @@ void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
>          kvmppc_write_hpte(ptex, pte0, pte1);
>      } else {
>          if (pte0 & HPTE64_V_VALID) {
> -            stq_p(spapr->htab + offset + HASH_PTE_SIZE_64 / 2, pte1);
> +            stq_p(spapr->htab + offset + HPTE64_DW1, pte1);
>              /*
>               * When setting valid, we write PTE1 first. This ensures
>               * proper synchronization with the reading code in
> @@ -1430,7 +1430,7 @@ void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
>               * ppc_hash64_pteg_search()
>               */
>              smp_wmb();
> -            stq_p(spapr->htab + offset + HASH_PTE_SIZE_64 / 2, pte1);
> +            stq_p(spapr->htab + offset + HPTE64_DW1, pte1);
>          }
>      }
>  }
> @@ -1438,7 +1438,7 @@ void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
>  static void spapr_hpte_set_c(PPCVirtualHypervisor *vhyp, hwaddr ptex,
>                               uint64_t pte1)
>  {
> -    hwaddr offset = ptex * HASH_PTE_SIZE_64 + 15;
> +    hwaddr offset = ptex * HASH_PTE_SIZE_64 + HPTE64_DW1_C;
>      SpaprMachineState *spapr = SPAPR_MACHINE(vhyp);
>  
>      if (!spapr->htab) {
> @@ -1454,7 +1454,7 @@ static void spapr_hpte_set_c(PPCVirtualHypervisor *vhyp, hwaddr ptex,
>  static void spapr_hpte_set_r(PPCVirtualHypervisor *vhyp, hwaddr ptex,
>                               uint64_t pte1)
>  {
> -    hwaddr offset = ptex * HASH_PTE_SIZE_64 + 14;
> +    hwaddr offset = ptex * HASH_PTE_SIZE_64 + HPTE64_DW1_R;
>      SpaprMachineState *spapr = SPAPR_MACHINE(vhyp);
>  
>      if (!spapr->htab) {
> diff --git a/hw/ppc/spapr_softmmu.c b/hw/ppc/spapr_softmmu.c
> index f8924270ef..4ee03c83e4 100644
> --- a/hw/ppc/spapr_softmmu.c
> +++ b/hw/ppc/spapr_softmmu.c
> @@ -426,7 +426,7 @@ static void new_hpte_store(void *htab, uint64_t pteg, int slot,
>      addr += slot * HASH_PTE_SIZE_64;
>  
>      stq_p(addr, pte0);
> -    stq_p(addr + HASH_PTE_SIZE_64 / 2, pte1);
> +    stq_p(addr + HPTE64_DW1, pte1);
>  }
>  
>  static int rehash_hpte(PowerPCCPU *cpu,
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 19832c4b46..da9fe99ff8 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -786,7 +786,7 @@ static void ppc_hash64_set_dsi(CPUState *cs, int mmu_idx, uint64_t dar, uint64_t
>  
>  static void ppc_hash64_set_r(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
>  {
> -    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 16;
> +    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + HPTE64_DW1_R;
>  
>      if (cpu->vhyp) {
>          PPCVirtualHypervisorClass *vhc =
> @@ -803,7 +803,7 @@ static void ppc_hash64_set_r(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
>  
>  static void ppc_hash64_set_c(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
>  {
> -    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 15;
> +    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + HPTE64_DW1_C;
>  
>      if (cpu->vhyp) {
>          PPCVirtualHypervisorClass *vhc =
> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> index c5b2f97ff7..1496955d38 100644
> --- a/target/ppc/mmu-hash64.h
> +++ b/target/ppc/mmu-hash64.h
> @@ -97,6 +97,11 @@ void ppc_hash64_finalize(PowerPCCPU *cpu);
>  #define HPTE64_V_1TB_SEG        0x4000000000000000ULL
>  #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
>  
> +/* PTE offsets */
> +#define HPTE64_DW1              (HASH_PTE_SIZE_64 / 2)
> +#define HPTE64_DW1_R            (HPTE64_DW1 + 6)
> +#define HPTE64_DW1_C            (HPTE64_DW1 + 7)
> +
>  /* Format changes for ARCH v3 */
>  #define HPTE64_V_COMMON_BITS    0x000fffffffffffffULL
>  #define HPTE64_R_3_0_SSIZE_SHIFT 58

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R

[Cédric Le Goater]

On 11/30/21 01:30, David Gibson wrote:
> On Mon, Nov 29, 2021 at 03:57:51PM -0300, Leandro Lupori wrote:
>> When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
>> offset, causing the first byte of the adjacent PTE to be corrupted.
>> This caused a panic when booting FreeBSD, using the Hash MMU.
>>
>> Fixes: a2dd4e83e76b ("ppc/hash64: Rework R and C bit updates")
>> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Sorry, I didn't wait for your Rb because the patch was a good candidate
for -rc3. It is merged now.

> Thanks for your patience with our nitpicking :).

Yes,

Here is another QEMU bug found by FreeBSD :
  
https://lore.kernel.org/qemu-devel/427ef2ee-6871-0d27-f485-90ad142f6266@kaod.org/

It would be interesting to boot directly the PowerNV machine from a
FreeBSB kernel and a minimum inirtd without using the skiroot images
and an iso. Are images available ?

Thanks.

C.

Re: [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R

[Leandro Lupori]

On 30/11/2021 05:44, Cédric Le Goater wrote:
> It would be interesting to boot directly the PowerNV machine from a
> FreeBSB kernel and a minimum inirtd without using the skiroot images
> and an iso. Are images available ?

AFAIK there are no minimum initrd images available. The closest thing 
would be the "bootonly" ISOs that can be found in the links below:

https://download.freebsd.org/ftp/releases/powerpc/powerpc64/ISO-IMAGES/13.0/

https://download.freebsd.org/ftp/releases/powerpc/powerpc64le/ISO-IMAGES/13.0/

https://download.freebsd.org/ftp/snapshots/powerpc/powerpc64/ISO-IMAGES/14.0/

https://download.freebsd.org/ftp/snapshots/powerpc/powerpc64le/ISO-IMAGES/14.0/


But to avoid using skiroot, you would need to manually extract the 
kernel from the ISO and also specify the rootfs, using something like: 
"-append cd9660:cd0".
If you don't want to emulate a disk or cd, the ISO can be passed to 
-initrd and "-append cd9660:md0" may be used to tell FreeBSD where to 
find the root partition (it loads it as a memory disk).

There are also qcow2 snapshots available:

https://artifact.ci.freebsd.org/snapshot/14.0-CURRENT/latest/powerpc/powerpc64/

https://artifact.ci.freebsd.org/snapshot/14.0-CURRENT/latest/powerpc/powerpc64le/

But you'll also need to extract the kernel from the image or from 
kernel.txz to boot them.
As these images target pseries and lack a FAT32 partition, Petitboot 
won't be able to boot them.

Alfredo (cc'ed) was trying to make them Petitboot compatible.

Re: [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R

[Cédric Le Goater]

On 11/30/21 21:00, Leandro Lupori wrote:
> On 30/11/2021 05:44, Cédric Le Goater wrote:
>> It would be interesting to boot directly the PowerNV machine from a
>> FreeBSB kernel and a minimum inirtd without using the skiroot images
>> and an iso. Are images available ?
> 
> AFAIK there are no minimum initrd images available. The closest thing would be the "bootonly" ISOs that can be found in the links below:
> 
> https://download.freebsd.org/ftp/releases/powerpc/powerpc64/ISO-IMAGES/13.0/
> 
> https://download.freebsd.org/ftp/releases/powerpc/powerpc64le/ISO-IMAGES/13.0/
> 
> https://download.freebsd.org/ftp/snapshots/powerpc/powerpc64/ISO-IMAGES/14.0/
> 
> https://download.freebsd.org/ftp/snapshots/powerpc/powerpc64le/ISO-IMAGES/14.0/
> 
> 
> But to avoid using skiroot, you would need to manually extract the kernel from the ISO and also specify the rootfs, using something like: "-append cd9660:cd0".
> If you don't want to emulate a disk or cd, the ISO can be passed to -initrd and "-append cd9660:md0" may be used to tell FreeBSD where to find the root partition (it loads it as a memory disk).

The ISO is too big for quick tests. Isn't there a minimum initrd ? can we build a
builroot-like image for FreeBSD ?

> 
> There are also qcow2 snapshots available:
> 
> https://artifact.ci.freebsd.org/snapshot/14.0-CURRENT/latest/powerpc/powerpc64/
> 
> https://artifact.ci.freebsd.org/snapshot/14.0-CURRENT/latest/powerpc/powerpc64le/
> 
> But you'll also need to extract the kernel from the image or from kernel.txz to boot them.

ok. I will take a look.

> As these images target pseries and lack a FAT32 partition, Petitboot won't be able to boot them.

The idea would be to skip petitboot and load directly the FreeBSD kernel from skiboot
with a minimum initrd or disk image. See :

   https://github.com/legoater/qemu-ppc-boot/blob/main/buildroot/qemu_ppc64le_powernv-2021.11-rc1-199-g927444a04e-20211120/start-qemu.sh

or

   https://github.com/legoater/qemu-ppc-boot/blob/main/buildroot/qemu_ppc64le_pseries-2021.11-rc1-199-g927444a04e-20211120/start-qemu.sh

Thanks,

C.

> 
> Alfredo (cc'ed) was trying to make them Petitboot compatible.
> 

Re: [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R

[Leandro Lupori]

On 01/12/2021 04:51, Cédric Le Goater wrote:
> The ISO is too big for quick tests. Isn't there a minimum initrd ? can 
> we build a
> builroot-like image for FreeBSD ?
> 

FreeBSD doesn't use initrd. Its bootloader loads kernel modules directly 
from disk (unfortunately, it doesn't work on PowerNV).

But it's possible to build a minimum disk image and make FreeBSD load it 
as a ramdisk, by passing it through QEMU's -initrd (even though it's not 
an initrd). By default FreeBSD builds a program called rescue, that is 
similar to busybox, that would help when building a minimum image.

Re: [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

On Tue, Nov 30, 2021 at 09:44:58AM +0100, Cédric le Goater wrote:
> On 11/30/21 01:30, David Gibson wrote:
> > On Mon, Nov 29, 2021 at 03:57:51PM -0300, Leandro Lupori wrote:
> > > When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
> > > offset, causing the first byte of the adjacent PTE to be corrupted.
> > > This caused a panic when booting FreeBSD, using the Hash MMU.
> > > 
> > > Fixes: a2dd4e83e76b ("ppc/hash64: Rework R and C bit updates")
> > > Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Sorry, I didn't wait for your Rb because the patch was a good candidate
> for -rc3. It is merged now.

No worries.

> 
> > Thanks for your patience with our nitpicking :).
> 
> Yes,
> 
> Here is another QEMU bug found by FreeBSD :
> https://lore.kernel.org/qemu-devel/427ef2ee-6871-0d27-f485-90ad142f6266@kaod.org/
> 
> It would be interesting to boot directly the PowerNV machine from a
> FreeBSB kernel and a minimum inirtd without using the skiroot images
> and an iso. Are images available ?
> 
> Thanks.
> 
> C.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]