Skip to contentSkip to conversation

hw/ppc/spapr: Add "Processor Register Hypervisor Resource Access" H-calls

PATCH Superseded Thomas Huth 2 participants 11 messages Feb 10, 2016 – Feb 11, 2016

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover hw/ppc/spapr: Add "Processor Register Hypervisor Resource Access" H-calls

While we were recently debugging a problem with the H_SET_DABR call [1], I noticed that some hypercalls from the chapter 14.5.4.3 ("Processor Register Hypervisor Resource Access") from the LoPAPR spec [2] are still missing in QEMU.
So here's are some patches that implement these hypercalls. Linux apparently does not depend on these hypercalls yet (otherwise somebody would have noticed this earlier), but the hypercalls are rather simple, so I think the implementations are quite straight-forward and easy to read.

[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=760a7364f27d974d

[2] https://members.openpowerfoundation.org/document/dl/469

Thomas Huth (4):
hw/ppc/spapr: Add h_set_sprg0 hypercall hw/ppc/spapr: Implement h_set_dabr hw/ppc/spapr: Implement the h_set_xdabr hypercall hw/ppc/spapr: Implement the h_page_init hypercall

 hw/ppc/spapr_hcall.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 98 insertions(+), 7 deletions(-)
Signature · 1 line
1.8.3.1
#1 lore raw

PATCH 1/4 hw/ppc/spapr: Add h_set_sprg0 hypercall

This is a very simple hypercall that only sets up the SPRG0 register for the guest (since writing to SPRG0 was only permitted to the hypervisor in older versions of the PowerISA).

---
 hw/ppc/spapr_hcall.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
hw/ppc/spapr_hcall.c +13 -2
index 12f8c33..58103ef 100644
@@ -332,6 +332,15 @@ static target_ulong h_read(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     return H_SUCCESS;
 }
 
+static target_ulong h_set_sprg0(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+                                target_ulong opcode, target_ulong *args)
+{
+    CPUState *cs = CPU(cpu);
+
+    set_spr(cs, SPR_SPRG0, args[0], -1L);
+    return H_SUCCESS;
+}
+
 static target_ulong h_set_dabr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                                target_ulong opcode, target_ulong *args)
 {
@@ -997,6 +1006,10 @@ static void hypercall_register_types(void)
     spapr_register_hypercall(H_REGISTER_VPA, h_register_vpa);
     spapr_register_hypercall(H_CEDE, h_cede);
 
+    /* processor register resource access h-calls */
+    spapr_register_hypercall(H_SET_SPRG0, h_set_sprg0);
+    spapr_register_hypercall(H_SET_MODE, h_set_mode);
+
     /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate
      * here between the "CI" and the "CACHE" variants, they will use whatever
      * mapping attributes qemu is using. When using KVM, the kernel will
@@ -1013,8 +1026,6 @@ static void hypercall_register_types(void)
     /* qemu/KVM-PPC specific hcalls */
     spapr_register_hypercall(KVMPPC_H_RTAS, h_rtas);
 
-    spapr_register_hypercall(H_SET_MODE, h_set_mode);
-
     /* ibm,client-architecture-support support */
     spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
 }
Signature · 1 line
1.8.3.1
#2 lore raw

Re: [Qemu-devel] [PATCH 1/4] hw/ppc/spapr: Add h_set_sprg0 hypercall

Quoted 14 lines · Thomas Huth

This is a very simple hypercall that only sets up the SPRG0 register for the guest (since writing to SPRG0 was only permitted to the hypervisor in older versions of the PowerISA).

---
 hw/ppc/spapr_hcall.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
hw/ppc/spapr_hcall.c +0 -0
index 12f8c33..58103ef 100644

hw/ppc/spapr_hcall.c static target_ulong h_read(PowerPCCPU *cpu, sPAPRMachineState *spapr, · line 340

Quoted 9 lines
hw/ppc/spapr_hcall.c +6 -0
@@ -332,6 +332,15 @@ static target_ulong h_read(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     return H_SUCCESS;
 }
 
+static target_ulong h_set_sprg0(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+                                target_ulong opcode, target_ulong *args)
+{
+    CPUState *cs = CPU(cpu);
+
+    set_spr(cs, SPR_SPRG0, args[0], -1L);

This looks correct, but I think set_spr() is serious overkill here. It does some fancy synchronization designed for setting one cpu's SPR from an hcall executed on a different CPU. In this case the calling CPU is just setting its own SPRG0, so just

	cpu_synchronize_state()
	env->spr[SPR_SPRG0] = XXX

Should be sufficient.

Quoted 26 lines
hw/ppc/spapr_hcall.c +7 -2
+    return H_SUCCESS;
+}
+
 static target_ulong h_set_dabr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                                target_ulong opcode, target_ulong *args)
 {
@@ -997,6 +1006,10 @@ static void hypercall_register_types(void)
     spapr_register_hypercall(H_REGISTER_VPA, h_register_vpa);
     spapr_register_hypercall(H_CEDE, h_cede);
 
+    /* processor register resource access h-calls */
+    spapr_register_hypercall(H_SET_SPRG0, h_set_sprg0);
+    spapr_register_hypercall(H_SET_MODE, h_set_mode);
+
     /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate
      * here between the "CI" and the "CACHE" variants, they will use whatever
      * mapping attributes qemu is using. When using KVM, the kernel will
@@ -1013,8 +1026,6 @@ static void hypercall_register_types(void)
     /* qemu/KVM-PPC specific hcalls */
     spapr_register_hypercall(KVMPPC_H_RTAS, h_rtas);
 
-    spapr_register_hypercall(H_SET_MODE, h_set_mode);
-
     /* ibm,client-architecture-support support */
     spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
 }
Signature · 4 lines
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
#3 lore raw

Re: [Qemu-devel] [PATCH 1/4] hw/ppc/spapr: Add h_set_sprg0 hypercall

Quoted 29 lines · David Gibson
Quoted 14 lines · Thomas Huth

This is a very simple hypercall that only sets up the SPRG0 register for the guest (since writing to SPRG0 was only permitted to the hypervisor in older versions of the PowerISA).

---
 hw/ppc/spapr_hcall.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
hw/ppc/spapr_hcall.c +0 -0
index 12f8c33..58103ef 100644

hw/ppc/spapr_hcall.c static target_ulong h_read(PowerPCCPU *cpu, sPAPRMachineState *spapr, · line 340

Quoted 9 lines
hw/ppc/spapr_hcall.c +6 -0
@@ -332,6 +332,15 @@ static target_ulong h_read(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     return H_SUCCESS;
 }
 
+static target_ulong h_set_sprg0(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+                                target_ulong opcode, target_ulong *args)
+{
+    CPUState *cs = CPU(cpu);
+
+    set_spr(cs, SPR_SPRG0, args[0], -1L);

This looks correct, but I think set_spr() is serious overkill here. It does some fancy synchronization designed for setting one cpu's SPR from an hcall executed on a different CPU. In this case the calling CPU is just setting its own SPRG0, so just

Quoted 4 lines
	cpu_synchronize_state()
	env->spr[SPR_SPRG0] = XXX

Should be sufficient.

AFAIK the synchronization stuff is skipped when set_spr() runs already on the destination CPU, but ok, since h-calls should be fast, I can change this anyway to save some precious cycles.

Thomas

#4 lore raw

PATCH 2/4 hw/ppc/spapr: Implement h_set_dabr

In reply to Thomas Huth

According to LoPAPR, h_set_dabr should simply set DABRX to 3 (if the register is available), and load the parameter into DABR. If DABRX is not available, the hypervisor has to check the "Breakpoint Translation" bit of the DABR register first.

---
 hw/ppc/spapr_hcall.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)
hw/ppc/spapr_hcall.c +21 -5
index 58103ef..6b9d512 100644
@@ -38,6 +38,12 @@ static void set_spr(CPUState *cs, int spr, target_ulong value,
     run_on_cpu(cs, do_spr_sync, &s);
 }
 
+static bool has_spr(PowerPCCPU *cpu, int spr)
+{
+    /* We can test whether the SPR is defined by checking for a valid name */
+    return cpu->env.spr_cb[spr].name != NULL;
+}
+
 static inline bool valid_pte_index(CPUPPCState *env, target_ulong pte_index)
 {
     /*
@@ -344,8 +350,20 @@ static target_ulong h_set_sprg0(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static target_ulong h_set_dabr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                                target_ulong opcode, target_ulong *args)
 {
-    /* FIXME: actually implement this */
-    return H_HARDWARE;
+    CPUState *cs = CPU(cpu);
+
+    if (!has_spr(cpu, SPR_DABR)) {
+        return H_HARDWARE;          /* DABR register not available */
+    }
+
+    if (has_spr(cpu, SPR_DABRX)) {
+        set_spr(cs, SPR_DABRX, 0x3, -1L);
+    } else if (!(args[0] & 0x4)) {  /* Breakpoint Translation set? */
+        return H_RESERVED_DABR;
+    }
+
+    set_spr(cs, SPR_DABR, args[0], -1L);
+    return H_SUCCESS;
 }
 
 #define FLAGS_REGISTER_VPA         0x0000200000000000ULL
@@ -999,15 +1017,13 @@ static void hypercall_register_types(void)
     /* hcall-bulk */
     spapr_register_hypercall(H_BULK_REMOVE, h_bulk_remove);
 
-    /* hcall-dabr */
-    spapr_register_hypercall(H_SET_DABR, h_set_dabr);
-
     /* hcall-splpar */
     spapr_register_hypercall(H_REGISTER_VPA, h_register_vpa);
     spapr_register_hypercall(H_CEDE, h_cede);
 
     /* processor register resource access h-calls */
     spapr_register_hypercall(H_SET_SPRG0, h_set_sprg0);
+    spapr_register_hypercall(H_SET_DABR, h_set_dabr);
     spapr_register_hypercall(H_SET_MODE, h_set_mode);
 
     /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate
Signature · 1 line
1.8.3.1
#5 lore raw

Re: [Qemu-devel] [PATCH 2/4] hw/ppc/spapr: Implement h_set_dabr

Quoted 31 lines · Thomas Huth

According to LoPAPR, h_set_dabr should simply set DABRX to 3 (if the register is available), and load the parameter into DABR. If DABRX is not available, the hypervisor has to check the "Breakpoint Translation" bit of the DABR register first.

---
 hw/ppc/spapr_hcall.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)
hw/ppc/spapr_hcall.c +6 -0
index 58103ef..6b9d512 100644
@@ -38,6 +38,12 @@ static void set_spr(CPUState *cs, int spr, target_ulong value,
     run_on_cpu(cs, do_spr_sync, &s);
 }
 
+static bool has_spr(PowerPCCPU *cpu, int spr)
+{
+    /* We can test whether the SPR is defined by checking for a valid name */
+    return cpu->env.spr_cb[spr].name != NULL;
+}
+
 static inline bool valid_pte_index(CPUPPCState *env, target_ulong pte_index)
 {
     /*
@@ -344,8 +350,20 @@ static target_ulong h_set_sprg0(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static target_ulong h_set_dabr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                                target_ulong opcode, target_ulong *args)
 {

hw/ppc/spapr_hcall.c static target_ulong h_set_sprg0(PowerPCCPU *cpu, sPAPRMachineState *spapr, · line 360

Quoted 10 lines
hw/ppc/spapr_hcall.c +8 -2
-    /* FIXME: actually implement this */
-    return H_HARDWARE;
+    CPUState *cs = CPU(cpu);
+
+    if (!has_spr(cpu, SPR_DABR)) {
+        return H_HARDWARE;          /* DABR register not available */
+    }
+
+    if (has_spr(cpu, SPR_DABRX)) {
+        set_spr(cs, SPR_DABRX, 0x3, -1L);

As in the previous patch, we shouldn't need the fancy run_on_cpu stuff that set_spr() uses, since this is just a cpu local register update.

Quoted 26 lines
hw/ppc/spapr_hcall.c +7 -3
+    } else if (!(args[0] & 0x4)) {  /* Breakpoint Translation set? */
+        return H_RESERVED_DABR;
+    }
+
+    set_spr(cs, SPR_DABR, args[0], -1L);
+    return H_SUCCESS;
 }
 
 #define FLAGS_REGISTER_VPA         0x0000200000000000ULL
@@ -999,15 +1017,13 @@ static void hypercall_register_types(void)
     /* hcall-bulk */
     spapr_register_hypercall(H_BULK_REMOVE, h_bulk_remove);
 
-    /* hcall-dabr */
-    spapr_register_hypercall(H_SET_DABR, h_set_dabr);
-
     /* hcall-splpar */
     spapr_register_hypercall(H_REGISTER_VPA, h_register_vpa);
     spapr_register_hypercall(H_CEDE, h_cede);
 
     /* processor register resource access h-calls */
     spapr_register_hypercall(H_SET_SPRG0, h_set_sprg0);
+    spapr_register_hypercall(H_SET_DABR, h_set_dabr);
     spapr_register_hypercall(H_SET_MODE, h_set_mode);
 
     /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate
Signature · 4 lines
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
#6 lore raw

PATCH 3/4 hw/ppc/spapr: Implement the h_set_xdabr hypercall

In reply to Thomas Huth

The H_SET_XDABR hypercall is similar to H_SET_DABR, but also sets the extended DABR (DABRX) register.

---
 hw/ppc/spapr_hcall.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
hw/ppc/spapr_hcall.c +22 -0
index 6b9d512..f14f849 100644
@@ -366,6 +366,27 @@ static target_ulong h_set_dabr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     return H_SUCCESS;
 }
 
+static target_ulong h_set_xdabr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+                                target_ulong opcode, target_ulong *args)
+{
+    CPUState *cs = CPU(cpu);
+    target_ulong dabrx = args[1];
+
+    if (!has_spr(cpu, SPR_DABR) || !has_spr(cpu, SPR_DABRX)) {
+        return H_HARDWARE;
+    }
+
+    if ((dabrx & ~0xfULL) != 0 || (dabrx & H_DABRX_HYPERVISOR) != 0
+        || (dabrx & (H_DABRX_KERNEL | H_DABRX_USER)) == 0) {
+        return H_PARAMETER;
+    }
+
+    set_spr(cs, SPR_DABRX, dabrx, -1L);
+    set_spr(cs, SPR_DABR, args[0], -1L);
+
+    return H_SUCCESS;
+}
+
 #define FLAGS_REGISTER_VPA         0x0000200000000000ULL
 #define FLAGS_REGISTER_DTL         0x0000400000000000ULL
 #define FLAGS_REGISTER_SLBSHADOW   0x0000600000000000ULL
@@ -1024,6 +1045,7 @@ static void hypercall_register_types(void)
     /* processor register resource access h-calls */
     spapr_register_hypercall(H_SET_SPRG0, h_set_sprg0);
     spapr_register_hypercall(H_SET_DABR, h_set_dabr);
+    spapr_register_hypercall(H_SET_XDABR, h_set_xdabr);
     spapr_register_hypercall(H_SET_MODE, h_set_mode);
 
     /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate
Signature · 1 line
1.8.3.1
#7 lore raw

Re: [Qemu-devel] [PATCH 3/4] hw/ppc/spapr: Implement the h_set_xdabr hypercall

Quoted 23 lines · Thomas Huth

The H_SET_XDABR hypercall is similar to H_SET_DABR, but also sets the extended DABR (DABRX) register.

---
 hw/ppc/spapr_hcall.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
hw/ppc/spapr_hcall.c +7 -0
index 6b9d512..f14f849 100644
@@ -366,6 +366,27 @@ static target_ulong h_set_dabr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     return H_SUCCESS;
 }
 
+static target_ulong h_set_xdabr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+                                target_ulong opcode, target_ulong *args)
+{
+    CPUState *cs = CPU(cpu);
+    target_ulong dabrx = args[1];
+
+    if (!has_spr(cpu, SPR_DABR) || !has_spr(cpu, SPR_DABRX)) {

hw/ppc/spapr_hcall.c static target_ulong h_set_dabr(PowerPCCPU *cpu, sPAPRMachineState *spapr, · line 385

Quoted 10 lines
hw/ppc/spapr_hcall.c +10 -0
+        return H_HARDWARE;
+    }
+
+    if ((dabrx & ~0xfULL) != 0 || (dabrx & H_DABRX_HYPERVISOR) != 0
+        || (dabrx & (H_DABRX_KERNEL | H_DABRX_USER)) == 0) {
+        return H_PARAMETER;
+    }
+
+    set_spr(cs, SPR_DABRX, dabrx, -1L);
+    set_spr(cs, SPR_DABR, args[0], -1L);

Same comments for set_spr().

Quoted 15 lines
hw/ppc/spapr_hcall.c +5 -0
+
+    return H_SUCCESS;
+}
+
 #define FLAGS_REGISTER_VPA         0x0000200000000000ULL
 #define FLAGS_REGISTER_DTL         0x0000400000000000ULL
 #define FLAGS_REGISTER_SLBSHADOW   0x0000600000000000ULL
@@ -1024,6 +1045,7 @@ static void hypercall_register_types(void)
     /* processor register resource access h-calls */
     spapr_register_hypercall(H_SET_SPRG0, h_set_sprg0);
     spapr_register_hypercall(H_SET_DABR, h_set_dabr);
+    spapr_register_hypercall(H_SET_XDABR, h_set_xdabr);
     spapr_register_hypercall(H_SET_MODE, h_set_mode);
 
     /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate
Signature · 4 lines
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
#8 lore raw

PATCH 4/4 hw/ppc/spapr: Implement the h_page_init hypercall

In reply to Thomas Huth

This hypercall either initializes a page with zeros, or copies another page.
According to LoPAPR, the i-cache of the page should also be flushed when using H_ICACHE_INVALIDATE or H_ICACHE_SYNCHRONIZE, but this is currently only done when running with TCG - assuming the cache will be flushed with KVM anyway when switching back to kernel / VM context.
The code currently also does not explicitely flush the data cache with H_ICACHE_SYNCHRONIZE, since this either also should be done when switching back to kernel / VM context (with KVM), or not matter anyway (for TCG).

---
 hw/ppc/spapr_hcall.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
hw/ppc/spapr_hcall.c +42 -0
index f14f849..91e703d 100644
@@ -387,6 +387,47 @@ static target_ulong h_set_xdabr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     return H_SUCCESS;
 }
 
+static target_ulong h_page_init(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+                                target_ulong opcode, target_ulong *args)
+{
+    target_ulong flags = args[0];
+    target_ulong dest = args[1];
+    target_ulong src = args[2];
+    uint8_t buf[TARGET_PAGE_SIZE];
+
+    if (flags & ~(H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE
+                  | H_COPY_PAGE | H_ZERO_PAGE)) {
+        qemu_log_mask(LOG_UNIMP, "h_page_init: Bad flags (" TARGET_FMT_lx "\n",
+                      flags);
+    }
+
+    if (!is_ram_address(spapr, dest) || (dest & ~TARGET_PAGE_MASK) != 0) {
+        return H_PARAMETER;
+    }
+
+    if (flags & H_COPY_PAGE) {
+        if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) != 0) {
+            return H_PARAMETER;
+        }
+        cpu_physical_memory_read(src, buf, TARGET_PAGE_SIZE);
+    } else if (flags & H_ZERO_PAGE) {
+        memset(buf, 0, TARGET_PAGE_SIZE);
+    }
+
+    if (flags & (H_COPY_PAGE | H_ZERO_PAGE)) {
+        cpu_physical_memory_write(dest, buf, TARGET_PAGE_SIZE);
+    }
+
+    if (flags & (H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE)) {
+        if (tcg_enabled()) {
+            tb_flush(CPU(cpu));
+        }
+        /* XXX: Flush data cache for H_ICACHE_SYNCHRONIZE? */
+    }
+
+    return H_SUCCESS;
+}
+
 #define FLAGS_REGISTER_VPA         0x0000200000000000ULL
 #define FLAGS_REGISTER_DTL         0x0000400000000000ULL
 #define FLAGS_REGISTER_SLBSHADOW   0x0000600000000000ULL
@@ -1046,6 +1087,7 @@ static void hypercall_register_types(void)
     spapr_register_hypercall(H_SET_SPRG0, h_set_sprg0);
     spapr_register_hypercall(H_SET_DABR, h_set_dabr);
     spapr_register_hypercall(H_SET_XDABR, h_set_xdabr);
+    spapr_register_hypercall(H_PAGE_INIT, h_page_init);
     spapr_register_hypercall(H_SET_MODE, h_set_mode);
 
     /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate
Signature · 1 line
1.8.3.1
#9 lore raw

Re: [Qemu-devel] [PATCH 4/4] hw/ppc/spapr: Implement the h_page_init hypercall

Quoted 7 lines · Thomas Huth

This hypercall either initializes a page with zeros, or copies another page.
According to LoPAPR, the i-cache of the page should also be flushed when using H_ICACHE_INVALIDATE or H_ICACHE_SYNCHRONIZE, but this is currently only done when running with TCG - assuming the cache will be flushed with KVM anyway when switching back to kernel / VM context.

I don't think that's true. dcache and icache aren't usually flushed by kernel/user or even process context changes in Linux. Cache control instructions aren't priveleged so, I think to get this right you'd need a helper which does dcbst and icbi across the page.

I'm pretty sure libc needs to do this at several points, but alas I don't think there's an exported function to do it.

Quoted 20 lines

The code currently also does not explicitely flush the data cache with H_ICACHE_SYNCHRONIZE, since this either also should be done when switching back to kernel / VM context (with KVM), or not matter anyway (for TCG).

---
 hw/ppc/spapr_hcall.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
hw/ppc/spapr_hcall.c +2 -0
index f14f849..91e703d 100644
@@ -387,6 +387,47 @@ static target_ulong h_set_xdabr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     return H_SUCCESS;
 }
 
+static target_ulong h_page_init(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+                                target_ulong opcode, target_ulong *args)

hw/ppc/spapr_hcall.c static target_ulong h_set_xdabr(PowerPCCPU *cpu, sPAPRMachineState *spapr, · line 401

Quoted 10 lines
hw/ppc/spapr_hcall.c +10 -0
+{
+    target_ulong flags = args[0];
+    target_ulong dest = args[1];
+    target_ulong src = args[2];
+    uint8_t buf[TARGET_PAGE_SIZE];
+
+    if (flags & ~(H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE
+                  | H_COPY_PAGE | H_ZERO_PAGE)) {
+        qemu_log_mask(LOG_UNIMP, "h_page_init: Bad flags (" TARGET_FMT_lx "\n",
+                      flags);

This should return H_PARAMETER as well as logging, surely?

Quoted 8 lines
hw/ppc/spapr_hcall.c +8 -0
+    }
+
+    if (!is_ram_address(spapr, dest) || (dest & ~TARGET_PAGE_MASK) != 0) {
+        return H_PARAMETER;
+    }
+
+    if (flags & H_COPY_PAGE) {
+        if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) != 0) {

hw/ppc/spapr_hcall.c static target_ulong h_set_xdabr(PowerPCCPU *cpu, sPAPRMachineState *spapr, · line 419

Quoted 10 lines
hw/ppc/spapr_hcall.c +10 -0
+            return H_PARAMETER;
+        }
+        cpu_physical_memory_read(src, buf, TARGET_PAGE_SIZE);
+    } else if (flags & H_ZERO_PAGE) {
+        memset(buf, 0, TARGET_PAGE_SIZE);
+    }
+
+    if (flags & (H_COPY_PAGE | H_ZERO_PAGE)) {
+        cpu_physical_memory_write(dest, buf, TARGET_PAGE_SIZE);
+    }

Hmm, so this does 2 copies for an H_COPY_PAGE and a zero and a copy for H_ZERO_PAGE, which is going to be substantially slower than the caller might expect.

Quoted 21 lines
hw/ppc/spapr_hcall.c +11 -0
+    if (flags & (H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE)) {
+        if (tcg_enabled()) {
+            tb_flush(CPU(cpu));
+        }
+        /* XXX: Flush data cache for H_ICACHE_SYNCHRONIZE? */
+    }
+
+    return H_SUCCESS;
+}
+
 #define FLAGS_REGISTER_VPA         0x0000200000000000ULL
 #define FLAGS_REGISTER_DTL         0x0000400000000000ULL
 #define FLAGS_REGISTER_SLBSHADOW   0x0000600000000000ULL
@@ -1046,6 +1087,7 @@ static void hypercall_register_types(void)
     spapr_register_hypercall(H_SET_SPRG0, h_set_sprg0);
     spapr_register_hypercall(H_SET_DABR, h_set_dabr);
     spapr_register_hypercall(H_SET_XDABR, h_set_xdabr);
+    spapr_register_hypercall(H_PAGE_INIT, h_page_init);
     spapr_register_hypercall(H_SET_MODE, h_set_mode);
 
     /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate
Signature · 4 lines
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
#10 lore raw

Re: [Qemu-devel] [PATCH 4/4] hw/ppc/spapr: Implement the h_page_init hypercall

Quoted 9 lines · David Gibson
Quoted 7 lines · Thomas Huth

This hypercall either initializes a page with zeros, or copies another page.
According to LoPAPR, the i-cache of the page should also be flushed when using H_ICACHE_INVALIDATE or H_ICACHE_SYNCHRONIZE, but this is currently only done when running with TCG - assuming the cache will be flushed with KVM anyway when switching back to kernel / VM context.

Quoted 4 lines

I don't think that's true. dcache and icache aren't usually flushed by kernel/user or even process context changes in Linux. Cache control instructions aren't priveleged so, I think to get this right you'd need a helper which does dcbst and icbi across the page.

Oh, ok, didn't know/expect that ... I'll try to come up with a solution...

Quoted 2 lines

I'm pretty sure libc needs to do this at several points, but alas I don't think there's an exported function to do it.

There seems to be at least a __builtin___clear_cache() function for flushing the instruction cache (see
<https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html>).

I haven't found anything similar for the data cache yet ... in the worst case, I guess I need to write a wrapper with some inline assembly, similar to kvmppc_eieio() in kvm_ppc.h ... would that be acceptable?

Quoted 18 lines
Quoted 18 lines

The code currently also does not explicitely flush the data cache with H_ICACHE_SYNCHRONIZE, since this either also should be done when switching back to kernel / VM context (with KVM), or not matter anyway (for TCG).

---
 hw/ppc/spapr_hcall.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
hw/ppc/spapr_hcall.c +0 -0
index f14f849..91e703d 100644
@@ -387,6 +387,47 @@ static target_ulong h_set_xdabr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     return H_SUCCESS;
 }
 
Quoted 14 lines

hw/ppc/spapr_hcall.c static target_ulong h_set_xdabr(PowerPCCPU *cpu, sPAPRMachineState *spapr, · line 401

Quoted 12 lines
hw/ppc/spapr_hcall.c +12 -0
+static target_ulong h_page_init(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+                                target_ulong opcode, target_ulong *args)
+{
+    target_ulong flags = args[0];
+    target_ulong dest = args[1];
+    target_ulong src = args[2];
+    uint8_t buf[TARGET_PAGE_SIZE];
+
+    if (flags & ~(H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE
+                  | H_COPY_PAGE | H_ZERO_PAGE)) {
+        qemu_log_mask(LOG_UNIMP, "h_page_init: Bad flags (" TARGET_FMT_lx "\n",
+                      flags);

This should return H_PARAMETER as well as logging, surely?

Yes, that's likely better.

Quoted 14 lines
Quoted 14 lines
hw/ppc/spapr_hcall.c +14 -0
+    }
+
+    if (!is_ram_address(spapr, dest) || (dest & ~TARGET_PAGE_MASK) != 0) {
+        return H_PARAMETER;
+    }
+
+    if (flags & H_COPY_PAGE) {
+        if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) != 0) {
+            return H_PARAMETER;
+        }
+        cpu_physical_memory_read(src, buf, TARGET_PAGE_SIZE);
+    } else if (flags & H_ZERO_PAGE) {
+        memset(buf, 0, TARGET_PAGE_SIZE);
+    }
Quoted 8 lines

hw/ppc/spapr_hcall.c static target_ulong h_set_xdabr(PowerPCCPU *cpu, sPAPRMachineState *spapr, · line 419

Quoted 4 lines
hw/ppc/spapr_hcall.c +4 -0
+
+    if (flags & (H_COPY_PAGE | H_ZERO_PAGE)) {
+        cpu_physical_memory_write(dest, buf, TARGET_PAGE_SIZE);
+    }

Hmm, so this does 2 copies for an H_COPY_PAGE and a zero and a copy for H_ZERO_PAGE, which is going to be substantially slower than the caller might expect.

I guess I'd better use cpu_physical_memory_map and memset/memcpy. I likely need cpu_physical_memory_map now anyway to be able to flush the caches related to that page...

Quoted 9 lines

hw/ppc/spapr_hcall.c static target_ulong h_set_xdabr(PowerPCCPU *cpu, sPAPRMachineState *spapr, · line 429

Quoted 9 lines
hw/ppc/spapr_hcall.c +9 -0
+    if (flags & (H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE)) {
+        if (tcg_enabled()) {
+            tb_flush(CPU(cpu));
+        }
+        /* XXX: Flush data cache for H_ICACHE_SYNCHRONIZE? */
+    }
+
+    return H_SUCCESS;
+}

Thanks for the review!

Thomas