[Qemu-devel] [PATCH 0/4] hw/ppc/spapr: Add "Processor Register Hypervisor Resource Access" H-calls

[Qemu-devel] [PATCH 0/4] hw/ppc/spapr: Add "Processor Register Hypervisor Resource Access" H-calls

[Thomas Huth]

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(-)

-- 
1.8.3.1

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

[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).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr_hcall.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 12f8c33..58103ef 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -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);
 }
-- 
1.8.3.1

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

[David Gibson]

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

On Wed, Feb 10, 2016 at 07:09:09PM +0100, Thomas Huth wrote:
> 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).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr_hcall.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 12f8c33..58103ef 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -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.

> +    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);
>  }

-- 
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: 819 bytes --]

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

[Thomas Huth]

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

On 11.02.2016 00:30, David Gibson wrote:
> On Wed, Feb 10, 2016 at 07:09:09PM +0100, Thomas Huth wrote:
>> 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).
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/ppc/spapr_hcall.c | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 12f8c33..58103ef 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -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.

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[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.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr_hcall.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 58103ef..6b9d512 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -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
-- 
1.8.3.1

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

[David Gibson]

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

On Wed, Feb 10, 2016 at 07:09:10PM +0100, Thomas Huth wrote:
> 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.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr_hcall.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 58103ef..6b9d512 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -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);

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.

> +    } 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

-- 
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: 819 bytes --]

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

[Thomas Huth]

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

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr_hcall.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 6b9d512..f14f849 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -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
-- 
1.8.3.1

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

[David Gibson]

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

On Wed, Feb 10, 2016 at 07:09:11PM +0100, Thomas Huth wrote:
> The H_SET_XDABR hypercall is similar to H_SET_DABR, but also sets
> the extended DABR (DABRX) register.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr_hcall.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 6b9d512..f14f849 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -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);

Same comments for set_spr().

> +
> +    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

-- 
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: 819 bytes --]

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

[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).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr_hcall.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index f14f849..91e703d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -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
-- 
1.8.3.1

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

[David Gibson]

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

On Wed, Feb 10, 2016 at 07:09:12PM +0100, Thomas Huth wrote:
> 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.

> 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).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr_hcall.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index f14f849..91e703d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -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);

This should return H_PARAMETER as well as logging, surely?

> +    }
> +
> +    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);
> +    }

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.

> +    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

-- 
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: 819 bytes --]

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

[Thomas Huth]

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

On 11.02.2016 00:47, David Gibson wrote:
> On Wed, Feb 10, 2016 at 07:09:12PM +0100, Thomas Huth wrote:
>> 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.

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

> 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?

>> 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).
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/ppc/spapr_hcall.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index f14f849..91e703d 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -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);
> 
> This should return H_PARAMETER as well as logging, surely?

Yes, that's likely better.

>> +    }
>> +
>> +    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);
>> +    }
> 
> 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...

>> +    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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]