* [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R
@ 2021-11-29 18:57 Leandro Lupori
2021-11-30 0:30 ` David Gibson
0 siblings, 1 reply; 7+ messages in thread
From: Leandro Lupori @ 2021-11-29 18:57 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: groug, danielhb413, Leandro Lupori, clg, david
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R
2021-11-29 18:57 [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R Leandro Lupori
@ 2021-11-30 0:30 ` David Gibson
2021-11-30 8:44 ` Cédric Le Goater
0 siblings, 1 reply; 7+ messages in thread
From: David Gibson @ 2021-11-30 0:30 UTC (permalink / raw)
To: Leandro Lupori; +Cc: groug, danielhb413, qemu-ppc, qemu-devel, clg
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R
2021-11-30 0:30 ` David Gibson
@ 2021-11-30 8:44 ` Cédric Le Goater
2021-11-30 20:00 ` Leandro Lupori
2021-11-30 23:42 ` David Gibson
0 siblings, 2 replies; 7+ messages in thread
From: Cédric Le Goater @ 2021-11-30 8:44 UTC (permalink / raw)
To: David Gibson, Leandro Lupori; +Cc: danielhb413, qemu-ppc, qemu-devel, groug
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R
2021-11-30 8:44 ` Cédric Le Goater
@ 2021-11-30 20:00 ` Leandro Lupori
2021-12-01 7:51 ` Cédric Le Goater
2021-11-30 23:42 ` David Gibson
1 sibling, 1 reply; 7+ messages in thread
From: Leandro Lupori @ 2021-11-30 20:00 UTC (permalink / raw)
To: Cédric Le Goater, David Gibson
Cc: alfredo.junior, danielhb413, qemu-ppc, qemu-devel, groug
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R
2021-11-30 8:44 ` Cédric Le Goater
2021-11-30 20:00 ` Leandro Lupori
@ 2021-11-30 23:42 ` David Gibson
1 sibling, 0 replies; 7+ messages in thread
From: David Gibson @ 2021-11-30 23:42 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, danielhb413, Leandro Lupori, qemu-devel, groug
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R
2021-11-30 20:00 ` Leandro Lupori
@ 2021-12-01 7:51 ` Cédric Le Goater
2021-12-01 12:37 ` Leandro Lupori
0 siblings, 1 reply; 7+ messages in thread
From: Cédric Le Goater @ 2021-12-01 7:51 UTC (permalink / raw)
To: Leandro Lupori, David Gibson
Cc: qemu-ppc, danielhb413, alfredo.junior, qemu-devel, groug
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.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R
2021-12-01 7:51 ` Cédric Le Goater
@ 2021-12-01 12:37 ` Leandro Lupori
0 siblings, 0 replies; 7+ messages in thread
From: Leandro Lupori @ 2021-12-01 12:37 UTC (permalink / raw)
To: Cédric Le Goater, David Gibson
Cc: qemu-ppc, danielhb413, alfredo.junior, qemu-devel, groug
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-12-01 12:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-29 18:57 [PATCH v4] target/ppc: fix Hash64 MMU update of PTE bit R Leandro Lupori
2021-11-30 0:30 ` David Gibson
2021-11-30 8:44 ` Cédric Le Goater
2021-11-30 20:00 ` Leandro Lupori
2021-12-01 7:51 ` Cédric Le Goater
2021-12-01 12:37 ` Leandro Lupori
2021-11-30 23:42 ` David Gibson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.