[PATCH 0/9] cleanup error handling in kvmppc_read_int_cpu_dt()

[PATCH 0/9] cleanup error handling in kvmppc_read_int_cpu_dt()

[Daniel Henrique Barboza]

Hi,

This series is a cleanup that fixes the issues observed by Markus
Armbruster in the review of jianchunfu's patch:

https://lists.gnu.org/archive/html/qemu-devel/2022-06/msg05393.html

I've also included jianchunfu's patch, with the relevant bits, in the
series.


Daniel Henrique Barboza (8):
  target/ppc/kvm.c: do not return -1 on uint64_t return
  target/ppc: add errp to kvmppc_read_int_cpu_dt()
  target/ppc: use g_autofree in kvmppc_read_int_cpu_dt()
  target/ppc: use Error pointer in kvmppc_get_clockfreq()
  ppc440_bamboo.c: handle clock freq read error in load_device_tree
  sam460ex.c: use CPU_FREQ if unable to read DT clock
  e500.c: use PLATFORM_CLK_FREQ_HZ if unable to read clock freq from DT
  spapr.c: handle clock freq read errors in spapr_dt_cpu()

jianchunfu (1):
  target/ppc: Add error reporting when opening file fails

 hw/ppc/e500.c          |  8 +++++++-
 hw/ppc/ppc440_bamboo.c | 17 ++++++++++++++---
 hw/ppc/sam460ex.c      |  8 +++++++-
 hw/ppc/spapr.c         | 12 +++++++++++-
 include/hw/ppc/spapr.h |  1 +
 target/ppc/kvm.c       | 33 +++++++++++++++++----------------
 target/ppc/kvm_ppc.h   |  2 +-
 7 files changed, 58 insertions(+), 23 deletions(-)

-- 
2.36.1

[PATCH 1/9] target/ppc/kvm.c: do not return -1 on uint64_t return

[Daniel Henrique Barboza]

kvmppc_read_int_dt() and kvmppc_read_int_cpu_dt() return an uint64_t,
while returning -1 when an error occurs. kvmppc_read_int_cpu_dt() claims
that it will return 0 if anything wrong happens, but it's returning -1
if kmvppc_find_cpu_dt() fails.

The elephant in the room is that returning -1 while claiming that the
return is uint64_t, results in 18446744073709551615 for the callers.
This reason alone is enough to not return -1 under these circunstances.

We'll still have the problem of having to deal with a '0' return that
might, or might not be, an error. We'll make this distintion clear in
the next patches.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/kvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 6eed466f80..109823136d 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1907,7 +1907,7 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
 
     f = fopen(filename, "rb");
     if (!f) {
-        return -1;
+        return 0;
     }
 
     len = fread(&u, 1, sizeof(u), f);
@@ -1934,7 +1934,7 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
     uint64_t val;
 
     if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
-        return -1;
+        return 0;
     }
 
     tmp = g_strdup_printf("%s/%s", buf, propname);
-- 
2.36.1

[PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Daniel Henrique Barboza]

The function can't just return 0 whether an error happened and call it a
day. We must provide a way of letting callers know if the zero return is
legitimate or due to an error.

Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
with an appropriate error, if one occurs. Callers are then free to pass
an Error pointer and handle it.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/kvm.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 109823136d..bc17437097 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
 
 /*
  * Read a CPU node property from the host device tree that's a single
- * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
- * (can't find or open the property, or doesn't understand the format)
+ * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
+ * wrong (can't find or open the property, or doesn't understand the
+ * format)
  */
-static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
+static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
 {
     char buf[PATH_MAX], *tmp;
     uint64_t val;
 
     if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
+        error_setg(errp, "Failed to read CPU property %s", propname);
         return 0;
     }
 
@@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
 
 uint64_t kvmppc_get_clockfreq(void)
 {
-    return kvmppc_read_int_cpu_dt("clock-frequency");
+    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
 }
 
 static int kvmppc_get_dec_bits(void)
 {
-    int nr_bits = kvmppc_read_int_cpu_dt("ibm,dec-bits");
+    int nr_bits = kvmppc_read_int_cpu_dt("ibm,dec-bits", NULL);
 
     if (nr_bits > 0) {
         return nr_bits;
@@ -2336,8 +2338,8 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
 static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
-    uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size");
-    uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size");
+    uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size", NULL);
+    uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size", NULL);
 
     /* Now fix up the class with information we can query from the host */
     pcc->pvr = mfpvr();
-- 
2.36.1

[PATCH 3/9] target/ppc: Add error reporting when opening file fails

[Daniel Henrique Barboza]

From: jianchunfu <jianchunfu@cmss.chinamobile.com>

Add error reporting before return when opening file fails in
kvmppc_read_int_dt().

Signed-off-by: jianchunfu <jianchunfu@cmss.chinamobile.com>
[danielhb: use error_setg() instead of fprintf]
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/kvm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index bc17437097..7611e9ccf6 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1896,7 +1896,7 @@ static int kvmppc_find_cpu_dt(char *buf, int buf_len)
     return 0;
 }
 
-static uint64_t kvmppc_read_int_dt(const char *filename)
+static uint64_t kvmppc_read_int_dt(const char *filename, Error **errp)
 {
     union {
         uint32_t v32;
@@ -1907,6 +1907,7 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
 
     f = fopen(filename, "rb");
     if (!f) {
+        error_setg(errp, "Error opening %s: %s", filename, strerror(errno));
         return 0;
     }
 
@@ -1940,7 +1941,7 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
     }
 
     tmp = g_strdup_printf("%s/%s", buf, propname);
-    val = kvmppc_read_int_dt(tmp);
+    val = kvmppc_read_int_dt(tmp, errp);
     g_free(tmp);
 
     return val;
-- 
2.36.1

[PATCH 4/9] target/ppc: use g_autofree in kvmppc_read_int_cpu_dt()

[Daniel Henrique Barboza]

This spares us a g_free() call. Let's also not use 'val' and return the
value of kvmppc_read_int_dt() directly.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/kvm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 7611e9ccf6..c218380eb7 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1932,8 +1932,8 @@ static uint64_t kvmppc_read_int_dt(const char *filename, Error **errp)
  */
 static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
 {
-    char buf[PATH_MAX], *tmp;
-    uint64_t val;
+    g_autofree char *tmp = NULL;
+    char buf[PATH_MAX];
 
     if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
         error_setg(errp, "Failed to read CPU property %s", propname);
@@ -1941,10 +1941,8 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
     }
 
     tmp = g_strdup_printf("%s/%s", buf, propname);
-    val = kvmppc_read_int_dt(tmp, errp);
-    g_free(tmp);
 
-    return val;
+    return kvmppc_read_int_dt(tmp, errp);
 }
 
 uint64_t kvmppc_get_clockfreq(void)
-- 
2.36.1

[PATCH 5/9] target/ppc: use Error pointer in kvmppc_get_clockfreq()

[Daniel Henrique Barboza]

Callers will then be able to handle any errors that might happen when
reading the clock frequency.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/e500.c          | 2 +-
 hw/ppc/ppc440_bamboo.c | 2 +-
 hw/ppc/sam460ex.c      | 2 +-
 hw/ppc/spapr.c         | 2 +-
 target/ppc/kvm.c       | 4 ++--
 target/ppc/kvm_ppc.h   | 2 +-
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 7f7f5b3452..4b4e99ef3c 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -405,7 +405,7 @@ static int ppce500_load_device_tree(PPCE500MachineState *pms,
 
     if (kvm_enabled()) {
         /* Read out host's frequencies */
-        clock_freq = kvmppc_get_clockfreq();
+        clock_freq = kvmppc_get_clockfreq(NULL);
         tb_freq = kvmppc_get_tbfreq();
 
         /* indicate KVM hypercall interface */
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index d5973f2484..d23f881d9d 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -107,7 +107,7 @@ static int bamboo_load_device_tree(hwaddr addr,
      * the correct frequencies. */
     if (kvm_enabled()) {
         tb_freq = kvmppc_get_tbfreq();
-        clock_freq = kvmppc_get_clockfreq();
+        clock_freq = kvmppc_get_clockfreq(NULL);
     }
 
     qemu_fdt_setprop_cell(fdt, "/cpus/cpu@0", "clock-frequency",
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 2f24598f55..4d25cb2c2e 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -179,7 +179,7 @@ static int sam460ex_load_device_tree(hwaddr addr,
      * the correct frequencies. */
     if (kvm_enabled()) {
         tb_freq = kvmppc_get_tbfreq();
-        clock_freq = kvmppc_get_clockfreq();
+        clock_freq = kvmppc_get_clockfreq(NULL);
     }
 
     qemu_fdt_setprop_cell(fdt, "/cpus/cpu@0", "clock-frequency",
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fd4942e881..f66e3cbe38 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -654,7 +654,7 @@ static void spapr_dt_cpu(CPUState *cs, void *fdt, int offset,
                        0xffffffff, 0xffffffff};
     uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq()
         : SPAPR_TIMEBASE_FREQ;
-    uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
+    uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq(NULL) : 1000000000;
     uint32_t page_sizes_prop[64];
     size_t page_sizes_prop_size;
     unsigned int smp_threads = ms->smp.threads;
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index c218380eb7..2accd1f946 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1945,9 +1945,9 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
     return kvmppc_read_int_dt(tmp, errp);
 }
 
-uint64_t kvmppc_get_clockfreq(void)
+uint64_t kvmppc_get_clockfreq(Error **errp)
 {
-    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
+    return kvmppc_read_int_cpu_dt("clock-frequency", errp);
 }
 
 static int kvmppc_get_dec_bits(void)
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index ee9325bf9a..b05aedb9f8 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -14,7 +14,7 @@
 #ifdef CONFIG_KVM
 
 uint32_t kvmppc_get_tbfreq(void);
-uint64_t kvmppc_get_clockfreq(void);
+uint64_t kvmppc_get_clockfreq(Error **errp);
 bool kvmppc_get_host_model(char **buf);
 bool kvmppc_get_host_serial(char **buf);
 int kvmppc_get_hasidle(CPUPPCState *env);
-- 
2.36.1

[PATCH 6/9] ppc440_bamboo.c: handle clock freq read error in load_device_tree

[Daniel Henrique Barboza]

Let's put the default clock and timebase freq value in macros for better
readability.  Use PPC440EP_CLOCK_FREQ as the default value of
'clock_freq' if kvmppc_get_clockfreq() throws an error.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/ppc440_bamboo.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index d23f881d9d..6318112393 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -50,6 +50,10 @@
 
 #define PPC440EP_SDRAM_NR_BANKS 4
 
+#define PPC440EP_TB_FREQ        400000000
+#define PPC440EP_CLOCK_FREQ     400000000
+
+
 static const ram_addr_t ppc440ep_sdram_bank_sizes[] = {
     256 * MiB, 128 * MiB, 64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 0
 };
@@ -67,8 +71,8 @@ static int bamboo_load_device_tree(hwaddr addr,
     char *filename;
     int fdt_size;
     void *fdt;
-    uint32_t tb_freq = 400000000;
-    uint32_t clock_freq = 400000000;
+    uint32_t tb_freq = PPC440EP_TB_FREQ;
+    uint32_t clock_freq = PPC440EP_CLOCK_FREQ;
 
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, BINARY_DEVICE_TREE_FILE);
     if (!filename) {
@@ -106,8 +110,15 @@ static int bamboo_load_device_tree(hwaddr addr,
      * directly access the timebase without host involvement, we must expose
      * the correct frequencies. */
     if (kvm_enabled()) {
+        Error *local_err = NULL;
+
         tb_freq = kvmppc_get_tbfreq();
-        clock_freq = kvmppc_get_clockfreq(NULL);
+        clock_freq = kvmppc_get_clockfreq(&local_err);
+
+        /* Use default clock if we're unable to read it from the DT */
+        if (local_err) {
+            clock_freq = PPC440EP_CLOCK_FREQ;
+        }
     }
 
     qemu_fdt_setprop_cell(fdt, "/cpus/cpu@0", "clock-frequency",
-- 
2.36.1

[PATCH 7/9] sam460ex.c: use CPU_FREQ if unable to read DT clock

[Daniel Henrique Barboza]

Fallback 'clock_freq' to CPU_FREQ if kvmppc_get_clockfreq() fails to
read the clock from the DT.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/sam460ex.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 4d25cb2c2e..0b3ce0cb17 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -178,8 +178,14 @@ static int sam460ex_load_device_tree(hwaddr addr,
      * directly access the timebase without host involvement, we must expose
      * the correct frequencies. */
     if (kvm_enabled()) {
+        Error *local_err = NULL;
+
         tb_freq = kvmppc_get_tbfreq();
-        clock_freq = kvmppc_get_clockfreq(NULL);
+        clock_freq = kvmppc_get_clockfreq(&local_err);
+
+        if (local_err) {
+            clock_freq = CPU_FREQ;
+        }
     }
 
     qemu_fdt_setprop_cell(fdt, "/cpus/cpu@0", "clock-frequency",
-- 
2.36.1

[PATCH 8/9] e500.c: use PLATFORM_CLK_FREQ_HZ if unable to read clock freq from DT

[Daniel Henrique Barboza]

Default 'clock_freq' to PLATFORM_CLK_FREQ_HZ if kvmppc_get_clockfreq()
fails to read the clock from the DT.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/e500.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 4b4e99ef3c..dc53d99b47 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -404,8 +404,14 @@ static int ppce500_load_device_tree(PPCE500MachineState *pms,
         fprintf(stderr, "couldn't set /chosen/bootargs\n");
 
     if (kvm_enabled()) {
+        Error *local_err = NULL;
+
         /* Read out host's frequencies */
-        clock_freq = kvmppc_get_clockfreq(NULL);
+        clock_freq = kvmppc_get_clockfreq(&local_err);
+        if (local_err) {
+            clock_freq = PLATFORM_CLK_FREQ_HZ;
+        }
+
         tb_freq = kvmppc_get_tbfreq();
 
         /* indicate KVM hypercall interface */
-- 
2.36.1

[PATCH 9/9] spapr.c: handle clock freq read errors in spapr_dt_cpu()

[Daniel Henrique Barboza]

Let's put the default spapr clock value in a SPAPR_CLOCK_FREQ for better
readability.

After that, make 'cpufreq' default to SPAPR_CLOCK_FREQ if
kvmppc_get_clockfreq() fails to read the clock from the DT.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c         | 12 +++++++++++-
 include/hw/ppc/spapr.h |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f66e3cbe38..80189c78be 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -654,7 +654,7 @@ static void spapr_dt_cpu(CPUState *cs, void *fdt, int offset,
                        0xffffffff, 0xffffffff};
     uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq()
         : SPAPR_TIMEBASE_FREQ;
-    uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq(NULL) : 1000000000;
+    uint32_t cpufreq = SPAPR_CLOCK_FREQ;
     uint32_t page_sizes_prop[64];
     size_t page_sizes_prop_size;
     unsigned int smp_threads = ms->smp.threads;
@@ -699,6 +699,16 @@ static void spapr_dt_cpu(CPUState *cs, void *fdt, int offset,
     }
 
     _FDT((fdt_setprop_cell(fdt, offset, "timebase-frequency", tbfreq)));
+
+    if (kvm_enabled()) {
+        Error *local_err = NULL;
+
+        cpufreq = kvmppc_get_clockfreq(&local_err);
+        if (local_err) {
+            cpufreq = SPAPR_CLOCK_FREQ;
+        }
+    }
+
     _FDT((fdt_setprop_cell(fdt, offset, "clock-frequency", cpufreq)));
     _FDT((fdt_setprop_cell(fdt, offset, "slb-size", cpu->hash64_opts->slb_size)));
     _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", cpu->hash64_opts->slb_size)));
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 072dda2c72..ed579635ca 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -26,6 +26,7 @@ typedef struct SpaprPendingHpt SpaprPendingHpt;
 #define SPAPR_ENTRY_POINT       0x100
 
 #define SPAPR_TIMEBASE_FREQ     512000000ULL
+#define SPAPR_CLOCK_FREQ        1000000000ULL
 
 #define TYPE_SPAPR_RTC "spapr-rtc"
 
-- 
2.36.1

Re: [PATCH 4/9] target/ppc: use g_autofree in kvmppc_read_int_cpu_dt()

[Cédric Le Goater]

On 6/30/22 21:42, Daniel Henrique Barboza wrote:
> This spares us a g_free() call. Let's also not use 'val' and return the
> value of kvmppc_read_int_dt() directly.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   target/ppc/kvm.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 7611e9ccf6..c218380eb7 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1932,8 +1932,8 @@ static uint64_t kvmppc_read_int_dt(const char *filename, Error **errp)
>    */
>   static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>   {
> -    char buf[PATH_MAX], *tmp;
> -    uint64_t val;
> +    g_autofree char *tmp = NULL;

I think you need to assign g_autofree variables where they are declared.

C.

> +    char buf[PATH_MAX];
>   
>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>           error_setg(errp, "Failed to read CPU property %s", propname);
> @@ -1941,10 +1941,8 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>       }
>   
>       tmp = g_strdup_printf("%s/%s", buf, propname);
> -    val = kvmppc_read_int_dt(tmp, errp);
> -    g_free(tmp);
>   
> -    return val;
> +    return kvmppc_read_int_dt(tmp, errp);
>   }
>   
>   uint64_t kvmppc_get_clockfreq(void)

Re: [PATCH 5/9] target/ppc: use Error pointer in kvmppc_get_clockfreq()

[Cédric Le Goater]

On 6/30/22 21:42, Daniel Henrique Barboza wrote:
> Callers will then be able to handle any errors that might happen when
> reading the clock frequency.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/ppc/e500.c          | 2 +-
>   hw/ppc/ppc440_bamboo.c | 2 +-
>   hw/ppc/sam460ex.c      | 2 +-
>   hw/ppc/spapr.c         | 2 +-
>   target/ppc/kvm.c       | 4 ++--
>   target/ppc/kvm_ppc.h   | 2 +-
>   6 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 7f7f5b3452..4b4e99ef3c 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -405,7 +405,7 @@ static int ppce500_load_device_tree(PPCE500MachineState *pms,
>   
>       if (kvm_enabled()) {
>           /* Read out host's frequencies */
> -        clock_freq = kvmppc_get_clockfreq();
> +        clock_freq = kvmppc_get_clockfreq(NULL);

Can't we use error_fatal instead of NULL ?

Thanks,

C.
>           tb_freq = kvmppc_get_tbfreq();
>   
>           /* indicate KVM hypercall interface */
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index d5973f2484..d23f881d9d 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -107,7 +107,7 @@ static int bamboo_load_device_tree(hwaddr addr,
>        * the correct frequencies. */
>       if (kvm_enabled()) {
>           tb_freq = kvmppc_get_tbfreq();
> -        clock_freq = kvmppc_get_clockfreq();
> +        clock_freq = kvmppc_get_clockfreq(NULL);
>       }
>   
>       qemu_fdt_setprop_cell(fdt, "/cpus/cpu@0", "clock-frequency",
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 2f24598f55..4d25cb2c2e 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -179,7 +179,7 @@ static int sam460ex_load_device_tree(hwaddr addr,
>        * the correct frequencies. */
>       if (kvm_enabled()) {
>           tb_freq = kvmppc_get_tbfreq();
> -        clock_freq = kvmppc_get_clockfreq();
> +        clock_freq = kvmppc_get_clockfreq(NULL);
>       }
>   
>       qemu_fdt_setprop_cell(fdt, "/cpus/cpu@0", "clock-frequency",
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fd4942e881..f66e3cbe38 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -654,7 +654,7 @@ static void spapr_dt_cpu(CPUState *cs, void *fdt, int offset,
>                          0xffffffff, 0xffffffff};
>       uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq()
>           : SPAPR_TIMEBASE_FREQ;
> -    uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
> +    uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq(NULL) : 1000000000;
>       uint32_t page_sizes_prop[64];
>       size_t page_sizes_prop_size;
>       unsigned int smp_threads = ms->smp.threads;
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index c218380eb7..2accd1f946 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1945,9 +1945,9 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>       return kvmppc_read_int_dt(tmp, errp);
>   }
>   
> -uint64_t kvmppc_get_clockfreq(void)
> +uint64_t kvmppc_get_clockfreq(Error **errp)
>   {
> -    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
> +    return kvmppc_read_int_cpu_dt("clock-frequency", errp);
>   }
>   
>   static int kvmppc_get_dec_bits(void)
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index ee9325bf9a..b05aedb9f8 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -14,7 +14,7 @@
>   #ifdef CONFIG_KVM
>   
>   uint32_t kvmppc_get_tbfreq(void);
> -uint64_t kvmppc_get_clockfreq(void);
> +uint64_t kvmppc_get_clockfreq(Error **errp);
>   bool kvmppc_get_host_model(char **buf);
>   bool kvmppc_get_host_serial(char **buf);
>   int kvmppc_get_hasidle(CPUPPCState *env);

Re: [PATCH 6/9] ppc440_bamboo.c: handle clock freq read error in load_device_tree

[Cédric Le Goater]

On 6/30/22 21:42, Daniel Henrique Barboza wrote:
> Let's put the default clock and timebase freq value in macros for better
> readability.  Use PPC440EP_CLOCK_FREQ as the default value of
> 'clock_freq' if kvmppc_get_clockfreq() throws an error.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/ppc/ppc440_bamboo.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index d23f881d9d..6318112393 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -50,6 +50,10 @@
>   
>   #define PPC440EP_SDRAM_NR_BANKS 4
>   
> +#define PPC440EP_TB_FREQ        400000000
> +#define PPC440EP_CLOCK_FREQ     400000000
> +
> +
>   static const ram_addr_t ppc440ep_sdram_bank_sizes[] = {
>       256 * MiB, 128 * MiB, 64 * MiB, 32 * MiB, 16 * MiB, 8 * MiB, 0
>   };
> @@ -67,8 +71,8 @@ static int bamboo_load_device_tree(hwaddr addr,
>       char *filename;
>       int fdt_size;
>       void *fdt;
> -    uint32_t tb_freq = 400000000;
> -    uint32_t clock_freq = 400000000;
> +    uint32_t tb_freq = PPC440EP_TB_FREQ;
> +    uint32_t clock_freq = PPC440EP_CLOCK_FREQ;
>   
>       filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, BINARY_DEVICE_TREE_FILE);
>       if (!filename) {
> @@ -106,8 +110,15 @@ static int bamboo_load_device_tree(hwaddr addr,
>        * directly access the timebase without host involvement, we must expose
>        * the correct frequencies. */
>       if (kvm_enabled()) {
> +        Error *local_err = NULL;
> +
>           tb_freq = kvmppc_get_tbfreq();
> -        clock_freq = kvmppc_get_clockfreq(NULL);
> +        clock_freq = kvmppc_get_clockfreq(&local_err);
> +
> +        /* Use default clock if we're unable to read it from the DT */
> +        if (local_err) {

may be report the error to the user ?

> +            clock_freq = PPC440EP_CLOCK_FREQ;
> +        }
>       }
>   
>       qemu_fdt_setprop_cell(fdt, "/cpus/cpu@0", "clock-frequency",

Re: [PATCH 3/9] target/ppc: Add error reporting when opening file fails

[Cédric Le Goater]

On 6/30/22 21:42, Daniel Henrique Barboza wrote:
> From: jianchunfu <jianchunfu@cmss.chinamobile.com>
> 
> Add error reporting before return when opening file fails in
> kvmppc_read_int_dt().
> 
> Signed-off-by: jianchunfu <jianchunfu@cmss.chinamobile.com>
> [danielhb: use error_setg() instead of fprintf]
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>   target/ppc/kvm.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index bc17437097..7611e9ccf6 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1896,7 +1896,7 @@ static int kvmppc_find_cpu_dt(char *buf, int buf_len)
>       return 0;
>   }
>   
> -static uint64_t kvmppc_read_int_dt(const char *filename)
> +static uint64_t kvmppc_read_int_dt(const char *filename, Error **errp)
>   {
>       union {
>           uint32_t v32;
> @@ -1907,6 +1907,7 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>   
>       f = fopen(filename, "rb");
>       if (!f) {
> +        error_setg(errp, "Error opening %s: %s", filename, strerror(errno));
>           return 0;
>       }
>   
> @@ -1940,7 +1941,7 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>       }
>   
>       tmp = g_strdup_printf("%s/%s", buf, propname);
> -    val = kvmppc_read_int_dt(tmp);
> +    val = kvmppc_read_int_dt(tmp, errp);
>       g_free(tmp);
>   
>       return val;

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Cédric Le Goater]

On 6/30/22 21:42, Daniel Henrique Barboza wrote:
> The function can't just return 0 whether an error happened and call it a
> day. We must provide a way of letting callers know if the zero return is
> legitimate or due to an error.
> 
> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
> with an appropriate error, if one occurs. Callers are then free to pass
> an Error pointer and handle it.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   target/ppc/kvm.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 109823136d..bc17437097 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>   
>   /*
>    * Read a CPU node property from the host device tree that's a single
> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
> - * (can't find or open the property, or doesn't understand the format)
> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
> + * wrong (can't find or open the property, or doesn't understand the
> + * format)
>    */
> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>   {
>       char buf[PATH_MAX], *tmp;
>       uint64_t val;
>   
>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
> +        error_setg(errp, "Failed to read CPU property %s", propname);
>           return 0;
>       }
>   
> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>   
>   uint64_t kvmppc_get_clockfreq(void)
>   {
> -    return kvmppc_read_int_cpu_dt("clock-frequency");
> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);


This should be fatal. no ?

C.


>   }
>   
>   static int kvmppc_get_dec_bits(void)
>   {
> -    int nr_bits = kvmppc_read_int_cpu_dt("ibm,dec-bits");
> +    int nr_bits = kvmppc_read_int_cpu_dt("ibm,dec-bits", NULL);
>   
>       if (nr_bits > 0) {
>           return nr_bits;
> @@ -2336,8 +2338,8 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
>   static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>   {
>       PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> -    uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size");
> -    uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size");
> +    uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size", NULL);
> +    uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size", NULL);
>   
>       /* Now fix up the class with information we can query from the host */
>       pcc->pvr = mfpvr();

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Daniel Henrique Barboza]

On 7/2/22 03:24, Cédric Le Goater wrote:
> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>> The function can't just return 0 whether an error happened and call it a
>> day. We must provide a way of letting callers know if the zero return is
>> legitimate or due to an error.
>>
>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
>> with an appropriate error, if one occurs. Callers are then free to pass
>> an Error pointer and handle it.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   target/ppc/kvm.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 109823136d..bc17437097 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>>   /*
>>    * Read a CPU node property from the host device tree that's a single
>> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
>> - * (can't find or open the property, or doesn't understand the format)
>> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
>> + * wrong (can't find or open the property, or doesn't understand the
>> + * format)
>>    */
>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>   {
>>       char buf[PATH_MAX], *tmp;
>>       uint64_t val;
>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>> +        error_setg(errp, "Failed to read CPU property %s", propname);
>>           return 0;
>>       }
>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>   uint64_t kvmppc_get_clockfreq(void)
>>   {
>> -    return kvmppc_read_int_cpu_dt("clock-frequency");
>> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
> 
> 
> This should be fatal. no ?


I'm not sure. I went under the assumption that there might be some weird
condition where 'clock-frequency' might be missing from the DT, and this
is why we're not exiting out immediately.

That said, the advantage of turning this into fatal is that we won't need
all the other patches that handles error on this function. We're going to
assume that if 'clock-frequency' is missing then we can't boot. I'm okay
with that.


Thanks,


Daniel



> 
> C.
> 
> 
>>   }
>>   static int kvmppc_get_dec_bits(void)
>>   {
>> -    int nr_bits = kvmppc_read_int_cpu_dt("ibm,dec-bits");
>> +    int nr_bits = kvmppc_read_int_cpu_dt("ibm,dec-bits", NULL);
>>       if (nr_bits > 0) {
>>           return nr_bits;
>> @@ -2336,8 +2338,8 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
>>   static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>>   {
>>       PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>> -    uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size");
>> -    uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size");
>> +    uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size", NULL);
>> +    uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size", NULL);
>>       /* Now fix up the class with information we can query from the host */
>>       pcc->pvr = mfpvr();
> 

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Cédric Le Goater]

On 7/2/22 15:34, Daniel Henrique Barboza wrote:
> 
> 
> On 7/2/22 03:24, Cédric Le Goater wrote:
>> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>>> The function can't just return 0 whether an error happened and call it a
>>> day. We must provide a way of letting callers know if the zero return is
>>> legitimate or due to an error.
>>>
>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
>>> with an appropriate error, if one occurs. Callers are then free to pass
>>> an Error pointer and handle it.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> ---
>>>   target/ppc/kvm.c | 16 +++++++++-------
>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>> index 109823136d..bc17437097 100644
>>> --- a/target/ppc/kvm.c
>>> +++ b/target/ppc/kvm.c
>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>>>   /*
>>>    * Read a CPU node property from the host device tree that's a single
>>> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
>>> - * (can't find or open the property, or doesn't understand the format)
>>> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
>>> + * wrong (can't find or open the property, or doesn't understand the
>>> + * format)
>>>    */
>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>>   {
>>>       char buf[PATH_MAX], *tmp;
>>>       uint64_t val;
>>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>>> +        error_setg(errp, "Failed to read CPU property %s", propname);
>>>           return 0;
>>>       }
>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>   uint64_t kvmppc_get_clockfreq(void)
>>>   {
>>> -    return kvmppc_read_int_cpu_dt("clock-frequency");
>>> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
>>
>>
>> This should be fatal. no ?
> 
> 
> I'm not sure. I went under the assumption that there might be some weird
> condition where 'clock-frequency' might be missing from the DT, and this
> is why we're not exiting out immediately.
> 
> That said, the advantage of turning this into fatal is that we won't need
> all the other patches that handles error on this function. We're going to
> assume that if 'clock-frequency' is missing then we can't boot. I'm okay
> with that.

I think so. Some machines behave badly when 'clock-frequency' is bogus,
division by zero, no console, etc. We could check easily with pseries
which is the only KVM PPC platform.

C.

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Daniel Henrique Barboza]

On 7/4/22 14:34, Cédric Le Goater wrote:
> On 7/2/22 15:34, Daniel Henrique Barboza wrote:
>>
>>
>> On 7/2/22 03:24, Cédric Le Goater wrote:
>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>>>> The function can't just return 0 whether an error happened and call it a
>>>> day. We must provide a way of letting callers know if the zero return is
>>>> legitimate or due to an error.
>>>>
>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
>>>> with an appropriate error, if one occurs. Callers are then free to pass
>>>> an Error pointer and handle it.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>>   target/ppc/kvm.c | 16 +++++++++-------
>>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>> index 109823136d..bc17437097 100644
>>>> --- a/target/ppc/kvm.c
>>>> +++ b/target/ppc/kvm.c
>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>>>>   /*
>>>>    * Read a CPU node property from the host device tree that's a single
>>>> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
>>>> - * (can't find or open the property, or doesn't understand the format)
>>>> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
>>>> + * wrong (can't find or open the property, or doesn't understand the
>>>> + * format)
>>>>    */
>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>>>   {
>>>>       char buf[PATH_MAX], *tmp;
>>>>       uint64_t val;
>>>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>>>> +        error_setg(errp, "Failed to read CPU property %s", propname);
>>>>           return 0;
>>>>       }
>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>   uint64_t kvmppc_get_clockfreq(void)
>>>>   {
>>>> -    return kvmppc_read_int_cpu_dt("clock-frequency");
>>>> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
>>>
>>>
>>> This should be fatal. no ?
>>
>>
>> I'm not sure. I went under the assumption that there might be some weird
>> condition where 'clock-frequency' might be missing from the DT, and this
>> is why we're not exiting out immediately.
>>
>> That said, the advantage of turning this into fatal is that we won't need
>> all the other patches that handles error on this function. We're going to
>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay
>> with that.
> 
> I think so. Some machines behave badly when 'clock-frequency' is bogus,
> division by zero, no console, etc. We could check easily with pseries
> which is the only KVM PPC platform.


I can assert that with pSeries we can safely error_fatal if the DT doesn't
contain 'clock-frequency' since it's on PAPR under this section:

"C.6.2 OF Root Note

This section defines additional properties and methods associated with PAPR
platforms that OSs expect to find in the root node."

I interpret this as "if there's no clock-frequency just bail out".


Thanks,


Daniel


> 
> C.
> 

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Mark Cave-Ayland]

On 04/07/2022 18:34, Cédric Le Goater wrote:

> On 7/2/22 15:34, Daniel Henrique Barboza wrote:
>>
>>
>> On 7/2/22 03:24, Cédric Le Goater wrote:
>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>>>> The function can't just return 0 whether an error happened and call it a
>>>> day. We must provide a way of letting callers know if the zero return is
>>>> legitimate or due to an error.
>>>>
>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
>>>> with an appropriate error, if one occurs. Callers are then free to pass
>>>> an Error pointer and handle it.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>>   target/ppc/kvm.c | 16 +++++++++-------
>>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>> index 109823136d..bc17437097 100644
>>>> --- a/target/ppc/kvm.c
>>>> +++ b/target/ppc/kvm.c
>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>>>>   /*
>>>>    * Read a CPU node property from the host device tree that's a single
>>>> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
>>>> - * (can't find or open the property, or doesn't understand the format)
>>>> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
>>>> + * wrong (can't find or open the property, or doesn't understand the
>>>> + * format)
>>>>    */
>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>>>   {
>>>>       char buf[PATH_MAX], *tmp;
>>>>       uint64_t val;
>>>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>>>> +        error_setg(errp, "Failed to read CPU property %s", propname);
>>>>           return 0;
>>>>       }
>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>   uint64_t kvmppc_get_clockfreq(void)
>>>>   {
>>>> -    return kvmppc_read_int_cpu_dt("clock-frequency");
>>>> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
>>>
>>>
>>> This should be fatal. no ?
>>
>>
>> I'm not sure. I went under the assumption that there might be some weird
>> condition where 'clock-frequency' might be missing from the DT, and this
>> is why we're not exiting out immediately.
>>
>> That said, the advantage of turning this into fatal is that we won't need
>> all the other patches that handles error on this function. We're going to
>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay
>> with that.
> 
> I think so. Some machines behave badly when 'clock-frequency' is bogus,
> division by zero, no console, etc. We could check easily with pseries
> which is the only KVM PPC platform.

Well not quite true ;)  I haven't tested it during the last release cycle, but the 
Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini last 
time I checked.


ATB,

Mark.

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Cédric Le Goater]

On 7/5/22 08:51, Mark Cave-Ayland wrote:
> On 04/07/2022 18:34, Cédric Le Goater wrote:
> 
>> On 7/2/22 15:34, Daniel Henrique Barboza wrote:
>>>
>>>
>>> On 7/2/22 03:24, Cédric Le Goater wrote:
>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>>>>> The function can't just return 0 whether an error happened and call it a
>>>>> day. We must provide a way of letting callers know if the zero return is
>>>>> legitimate or due to an error.
>>>>>
>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
>>>>> with an appropriate error, if one occurs. Callers are then free to pass
>>>>> an Error pointer and handle it.
>>>>>
>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>>> ---
>>>>>   target/ppc/kvm.c | 16 +++++++++-------
>>>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>>> index 109823136d..bc17437097 100644
>>>>> --- a/target/ppc/kvm.c
>>>>> +++ b/target/ppc/kvm.c
>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>>>>>   /*
>>>>>    * Read a CPU node property from the host device tree that's a single
>>>>> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
>>>>> - * (can't find or open the property, or doesn't understand the format)
>>>>> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
>>>>> + * wrong (can't find or open the property, or doesn't understand the
>>>>> + * format)
>>>>>    */
>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>>>>   {
>>>>>       char buf[PATH_MAX], *tmp;
>>>>>       uint64_t val;
>>>>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>>>>> +        error_setg(errp, "Failed to read CPU property %s", propname);
>>>>>           return 0;
>>>>>       }
>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>>   uint64_t kvmppc_get_clockfreq(void)
>>>>>   {
>>>>> -    return kvmppc_read_int_cpu_dt("clock-frequency");
>>>>> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
>>>>
>>>>
>>>> This should be fatal. no ?
>>>
>>>
>>> I'm not sure. I went under the assumption that there might be some weird
>>> condition where 'clock-frequency' might be missing from the DT, and this
>>> is why we're not exiting out immediately.
>>>
>>> That said, the advantage of turning this into fatal is that we won't need
>>> all the other patches that handles error on this function. We're going to
>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay
>>> with that.
>>
>> I think so. Some machines behave badly when 'clock-frequency' is bogus,
>> division by zero, no console, etc. We could check easily with pseries
>> which is the only KVM PPC platform.
> 
> Well not quite true ;)  I haven't tested it during the last release cycle, 
> but the Mac machines were still working fine to boot OS X with KVM-PR on 
> my G4 Mac Mini last time I checked.

Oh. Sorry. and I still have access to a real G5 running the latest debian.
I should give it a try one day.

Thanks

C.

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Daniel Henrique Barboza]

On 7/5/22 03:51, Mark Cave-Ayland wrote:
> On 04/07/2022 18:34, Cédric Le Goater wrote:
> 
>> On 7/2/22 15:34, Daniel Henrique Barboza wrote:
>>>
>>>
>>> On 7/2/22 03:24, Cédric Le Goater wrote:
>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>>>>> The function can't just return 0 whether an error happened and call it a
>>>>> day. We must provide a way of letting callers know if the zero return is
>>>>> legitimate or due to an error.
>>>>>
>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
>>>>> with an appropriate error, if one occurs. Callers are then free to pass
>>>>> an Error pointer and handle it.
>>>>>
>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>>> ---
>>>>>   target/ppc/kvm.c | 16 +++++++++-------
>>>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>>> index 109823136d..bc17437097 100644
>>>>> --- a/target/ppc/kvm.c
>>>>> +++ b/target/ppc/kvm.c
>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>>>>>   /*
>>>>>    * Read a CPU node property from the host device tree that's a single
>>>>> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
>>>>> - * (can't find or open the property, or doesn't understand the format)
>>>>> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
>>>>> + * wrong (can't find or open the property, or doesn't understand the
>>>>> + * format)
>>>>>    */
>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>>>>   {
>>>>>       char buf[PATH_MAX], *tmp;
>>>>>       uint64_t val;
>>>>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>>>>> +        error_setg(errp, "Failed to read CPU property %s", propname);
>>>>>           return 0;
>>>>>       }
>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>>   uint64_t kvmppc_get_clockfreq(void)
>>>>>   {
>>>>> -    return kvmppc_read_int_cpu_dt("clock-frequency");
>>>>> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
>>>>
>>>>
>>>> This should be fatal. no ?
>>>
>>>
>>> I'm not sure. I went under the assumption that there might be some weird
>>> condition where 'clock-frequency' might be missing from the DT, and this
>>> is why we're not exiting out immediately.
>>>
>>> That said, the advantage of turning this into fatal is that we won't need
>>> all the other patches that handles error on this function. We're going to
>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay
>>> with that.
>>
>> I think so. Some machines behave badly when 'clock-frequency' is bogus,
>> division by zero, no console, etc. We could check easily with pseries
>> which is the only KVM PPC platform.
> 
> Well not quite true ;)  I haven't tested it during the last release cycle, but the Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini last time I checked.


We can't just error_fatal by default then.

Each machine should be able to determine how to handle this missing DT
element. If I want to error_fatal for pseries then I can do so in patch
9/9, but other than that I'll keep the existing behavior.


Thanks,


Daniel

> 
> 
> ATB,
> 
> Mark.

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Mark Cave-Ayland]

On 05/07/2022 10:39, Daniel Henrique Barboza wrote:

> On 7/5/22 03:51, Mark Cave-Ayland wrote:
>> On 04/07/2022 18:34, Cédric Le Goater wrote:
>>
>>> On 7/2/22 15:34, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 7/2/22 03:24, Cédric Le Goater wrote:
>>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>>>>>> The function can't just return 0 whether an error happened and call it a
>>>>>> day. We must provide a way of letting callers know if the zero return is
>>>>>> legitimate or due to an error.
>>>>>>
>>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
>>>>>> with an appropriate error, if one occurs. Callers are then free to pass
>>>>>> an Error pointer and handle it.
>>>>>>
>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>>>> ---
>>>>>>   target/ppc/kvm.c | 16 +++++++++-------
>>>>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>>>> index 109823136d..bc17437097 100644
>>>>>> --- a/target/ppc/kvm.c
>>>>>> +++ b/target/ppc/kvm.c
>>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>>>>>>   /*
>>>>>>    * Read a CPU node property from the host device tree that's a single
>>>>>> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
>>>>>> - * (can't find or open the property, or doesn't understand the format)
>>>>>> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
>>>>>> + * wrong (can't find or open the property, or doesn't understand the
>>>>>> + * format)
>>>>>>    */
>>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>>>>>   {
>>>>>>       char buf[PATH_MAX], *tmp;
>>>>>>       uint64_t val;
>>>>>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>>>>>> +        error_setg(errp, "Failed to read CPU property %s", propname);
>>>>>>           return 0;
>>>>>>       }
>>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char 
>>>>>> *propname)
>>>>>>   uint64_t kvmppc_get_clockfreq(void)
>>>>>>   {
>>>>>> -    return kvmppc_read_int_cpu_dt("clock-frequency");
>>>>>> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
>>>>>
>>>>>
>>>>> This should be fatal. no ?
>>>>
>>>>
>>>> I'm not sure. I went under the assumption that there might be some weird
>>>> condition where 'clock-frequency' might be missing from the DT, and this
>>>> is why we're not exiting out immediately.
>>>>
>>>> That said, the advantage of turning this into fatal is that we won't need
>>>> all the other patches that handles error on this function. We're going to
>>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay
>>>> with that.
>>>
>>> I think so. Some machines behave badly when 'clock-frequency' is bogus,
>>> division by zero, no console, etc. We could check easily with pseries
>>> which is the only KVM PPC platform.
>>
>> Well not quite true ;)  I haven't tested it during the last release cycle, but the 
>> Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini 
>> last time I checked.
> 
> 
> We can't just error_fatal by default then.
> 
> Each machine should be able to determine how to handle this missing DT
> element. If I want to error_fatal for pseries then I can do so in patch
> 9/9, but other than that I'll keep the existing behavior.

This wouldn't be a problem for the Mac machines since they pass the clock frequency 
from QEMU to OpenBIOS using the fw_cfg interface which builds the tree itself rather 
than using FDT. So I think using error_fatal should still be fine.


ATB,

Mark.

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Cédric Le Goater]

On 7/5/22 11:39, Daniel Henrique Barboza wrote:
> 
> 
> On 7/5/22 03:51, Mark Cave-Ayland wrote:
>> On 04/07/2022 18:34, Cédric Le Goater wrote:
>>
>>> On 7/2/22 15:34, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 7/2/22 03:24, Cédric Le Goater wrote:
>>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>>>>>> The function can't just return 0 whether an error happened and call it a
>>>>>> day. We must provide a way of letting callers know if the zero return is
>>>>>> legitimate or due to an error.
>>>>>>
>>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
>>>>>> with an appropriate error, if one occurs. Callers are then free to pass
>>>>>> an Error pointer and handle it.
>>>>>>
>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>>>> ---
>>>>>>   target/ppc/kvm.c | 16 +++++++++-------
>>>>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>>>> index 109823136d..bc17437097 100644
>>>>>> --- a/target/ppc/kvm.c
>>>>>> +++ b/target/ppc/kvm.c
>>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>>>>>>   /*
>>>>>>    * Read a CPU node property from the host device tree that's a single
>>>>>> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
>>>>>> - * (can't find or open the property, or doesn't understand the format)
>>>>>> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
>>>>>> + * wrong (can't find or open the property, or doesn't understand the
>>>>>> + * format)
>>>>>>    */
>>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>>>>>   {
>>>>>>       char buf[PATH_MAX], *tmp;
>>>>>>       uint64_t val;
>>>>>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>>>>>> +        error_setg(errp, "Failed to read CPU property %s", propname);
>>>>>>           return 0;
>>>>>>       }
>>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>>>   uint64_t kvmppc_get_clockfreq(void)
>>>>>>   {
>>>>>> -    return kvmppc_read_int_cpu_dt("clock-frequency");
>>>>>> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
>>>>>
>>>>>
>>>>> This should be fatal. no ?
>>>>
>>>>
>>>> I'm not sure. I went under the assumption that there might be some weird
>>>> condition where 'clock-frequency' might be missing from the DT, and this
>>>> is why we're not exiting out immediately.
>>>>
>>>> That said, the advantage of turning this into fatal is that we won't need
>>>> all the other patches that handles error on this function. We're going to
>>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay
>>>> with that.
>>>
>>> I think so. Some machines behave badly when 'clock-frequency' is bogus,
>>> division by zero, no console, etc. We could check easily with pseries
>>> which is the only KVM PPC platform.
>>
>> Well not quite true ;)  I haven't tested it during the last release cycle, but the Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini last time I checked.
> 
> 
> We can't just error_fatal by default then.
> 
> Each machine should be able to determine how to handle this missing DT
> element. If I want to error_fatal for pseries then I can do so in patch
> 9/9, but other than that I'll keep the existing behavior.

Or add an errp here :

hw/ppc/e500.c:        clock_freq = kvmppc_get_clockfreq();
hw/ppc/sam460ex.c:        clock_freq = kvmppc_get_clockfreq();
hw/ppc/ppc440_bamboo.c:        clock_freq = kvmppc_get_clockfreq();
hw/ppc/spapr.c:    uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;

C.

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Daniel Henrique Barboza]

On 7/5/22 06:44, Mark Cave-Ayland wrote:
> On 05/07/2022 10:39, Daniel Henrique Barboza wrote:
> 
>> On 7/5/22 03:51, Mark Cave-Ayland wrote:
>>> On 04/07/2022 18:34, Cédric Le Goater wrote:
>>>
>>>> On 7/2/22 15:34, Daniel Henrique Barboza wrote:
>>>>>
>>>>>
>>>>> On 7/2/22 03:24, Cédric Le Goater wrote:
>>>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>>>>>>> The function can't just return 0 whether an error happened and call it a
>>>>>>> day. We must provide a way of letting callers know if the zero return is
>>>>>>> legitimate or due to an error.
>>>>>>>
>>>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
>>>>>>> with an appropriate error, if one occurs. Callers are then free to pass
>>>>>>> an Error pointer and handle it.
>>>>>>>
>>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>>>>> ---
>>>>>>>   target/ppc/kvm.c | 16 +++++++++-------
>>>>>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>>>>> index 109823136d..bc17437097 100644
>>>>>>> --- a/target/ppc/kvm.c
>>>>>>> +++ b/target/ppc/kvm.c
>>>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>>>>>>>   /*
>>>>>>>    * Read a CPU node property from the host device tree that's a single
>>>>>>> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
>>>>>>> - * (can't find or open the property, or doesn't understand the format)
>>>>>>> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
>>>>>>> + * wrong (can't find or open the property, or doesn't understand the
>>>>>>> + * format)
>>>>>>>    */
>>>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>>>>>>   {
>>>>>>>       char buf[PATH_MAX], *tmp;
>>>>>>>       uint64_t val;
>>>>>>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>>>>>>> +        error_setg(errp, "Failed to read CPU property %s", propname);
>>>>>>>           return 0;
>>>>>>>       }
>>>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>>>>   uint64_t kvmppc_get_clockfreq(void)
>>>>>>>   {
>>>>>>> -    return kvmppc_read_int_cpu_dt("clock-frequency");
>>>>>>> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
>>>>>>
>>>>>>
>>>>>> This should be fatal. no ?
>>>>>
>>>>>
>>>>> I'm not sure. I went under the assumption that there might be some weird
>>>>> condition where 'clock-frequency' might be missing from the DT, and this
>>>>> is why we're not exiting out immediately.
>>>>>
>>>>> That said, the advantage of turning this into fatal is that we won't need
>>>>> all the other patches that handles error on this function. We're going to
>>>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay
>>>>> with that.
>>>>
>>>> I think so. Some machines behave badly when 'clock-frequency' is bogus,
>>>> division by zero, no console, etc. We could check easily with pseries
>>>> which is the only KVM PPC platform.
>>>
>>> Well not quite true ;)  I haven't tested it during the last release cycle, but the Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini last time I checked.
>>
>>
>> We can't just error_fatal by default then.
>>
>> Each machine should be able to determine how to handle this missing DT
>> element. If I want to error_fatal for pseries then I can do so in patch
>> 9/9, but other than that I'll keep the existing behavior.
> 
> This wouldn't be a problem for the Mac machines since they pass the clock frequency from QEMU to OpenBIOS using the fw_cfg interface which builds the tree itself rather than using FDT. So I think using error_fatal should still be fine.

One less board that would break if we error_fatal in that condition then :)

In theory, for boards that doesn't care/doesn't support KVM, this function
shouldn't be even called. I guess I'll move the series into this direction
instead.


Daniel

> 
> 
> ATB,
> 
> Mark.

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Cédric Le Goater]

On 7/5/22 08:57, Cédric Le Goater wrote:
> On 7/5/22 08:51, Mark Cave-Ayland wrote:
>> On 04/07/2022 18:34, Cédric Le Goater wrote:
>>
>>> On 7/2/22 15:34, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 7/2/22 03:24, Cédric Le Goater wrote:
>>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>>>>>> The function can't just return 0 whether an error happened and call it a
>>>>>> day. We must provide a way of letting callers know if the zero return is
>>>>>> legitimate or due to an error.
>>>>>>
>>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
>>>>>> with an appropriate error, if one occurs. Callers are then free to pass
>>>>>> an Error pointer and handle it.
>>>>>>
>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>>>> ---
>>>>>>   target/ppc/kvm.c | 16 +++++++++-------
>>>>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>>>> index 109823136d..bc17437097 100644
>>>>>> --- a/target/ppc/kvm.c
>>>>>> +++ b/target/ppc/kvm.c
>>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>>>>>>   /*
>>>>>>    * Read a CPU node property from the host device tree that's a single
>>>>>> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
>>>>>> - * (can't find or open the property, or doesn't understand the format)
>>>>>> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
>>>>>> + * wrong (can't find or open the property, or doesn't understand the
>>>>>> + * format)
>>>>>>    */
>>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>>>>>   {
>>>>>>       char buf[PATH_MAX], *tmp;
>>>>>>       uint64_t val;
>>>>>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>>>>>> +        error_setg(errp, "Failed to read CPU property %s", propname);
>>>>>>           return 0;
>>>>>>       }
>>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>>>   uint64_t kvmppc_get_clockfreq(void)
>>>>>>   {
>>>>>> -    return kvmppc_read_int_cpu_dt("clock-frequency");
>>>>>> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
>>>>>
>>>>>
>>>>> This should be fatal. no ?
>>>>
>>>>
>>>> I'm not sure. I went under the assumption that there might be some weird
>>>> condition where 'clock-frequency' might be missing from the DT, and this
>>>> is why we're not exiting out immediately.
>>>>
>>>> That said, the advantage of turning this into fatal is that we won't need
>>>> all the other patches that handles error on this function. We're going to
>>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay
>>>> with that.
>>>
>>> I think so. Some machines behave badly when 'clock-frequency' is bogus,
>>> division by zero, no console, etc. We could check easily with pseries
>>> which is the only KVM PPC platform.
>>
>> Well not quite true ;)  I haven't tested it during the last release cycle, but the Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini last time I checked.
> 
> Oh. Sorry. and I still have access to a real G5 running the latest debian.
> I should give it a try one day.

I gave KVM a try on a :

     cpu		: PPC970MP, altivec supported
     clock	: 2000.000000MHz
     revision	: 1.0 (pvr 0044 0100)
     
     processor	: 1
     cpu		: PPC970MP, altivec supported
     clock	: 2000.000000MHz
     revision	: 1.0 (pvr 0044 0100)
     
     timebase	: 33333333
     platform	: PowerMac
     model	: PowerMac11,2
     machine	: PowerMac11,2
     motherboard	: PowerMac11,2 MacRISC4 Power Macintosh
     detected as	: 337 (PowerMac G5 Dual Core)
     pmac flags	: 00000000
     L2 cache	: 1024K unified
     pmac-generation	: NewWorld
     

running debian with kernel 5.18.0-2-powerpc64. With the installed QEMU 7.0.0,

     qemu-system-ppc64 -M mac99 -cpu host -accel kvm ...

doesn't go very far. Program exception is quickly reached and host says:

     [56450.118422] Couldn't emulate instruction 0x00000000 (op 0 xop 0)
     [56450.119060] kvmppc_exit_pr_progint: emulation at 100 failed (00000000)

Anything special I should know ?

Thanks,

C.

Re: [PATCH 4/9] target/ppc: use g_autofree in kvmppc_read_int_cpu_dt()

[Daniel Henrique Barboza]

On 7/2/22 03:20, Cédric Le Goater wrote:
> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>> This spares us a g_free() call. Let's also not use 'val' and return the
>> value of kvmppc_read_int_dt() directly.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   target/ppc/kvm.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 7611e9ccf6..c218380eb7 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -1932,8 +1932,8 @@ static uint64_t kvmppc_read_int_dt(const char *filename, Error **errp)
>>    */
>>   static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>   {
>> -    char buf[PATH_MAX], *tmp;
>> -    uint64_t val;
>> +    g_autofree char *tmp = NULL;
> 
> I think you need to assign g_autofree variables where they are declared.

We need to initialize the var with something, not necessarily with the value we're
going to use. Initializing with 'NULL' works.


Thanks,


Daniel

> 
> C.
> 
>> +    char buf[PATH_MAX];
>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>>           error_setg(errp, "Failed to read CPU property %s", propname);
>> @@ -1941,10 +1941,8 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>       }
>>       tmp = g_strdup_printf("%s/%s", buf, propname);
>> -    val = kvmppc_read_int_dt(tmp, errp);
>> -    g_free(tmp);
>> -    return val;
>> +    return kvmppc_read_int_dt(tmp, errp);
>>   }
>>   uint64_t kvmppc_get_clockfreq(void)
> 

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Mark Cave-Ayland]

On 06/07/2022 08:45, Cédric Le Goater wrote:

> On 7/5/22 08:57, Cédric Le Goater wrote:
>> On 7/5/22 08:51, Mark Cave-Ayland wrote:
>>> On 04/07/2022 18:34, Cédric Le Goater wrote:
>>>
>>>> On 7/2/22 15:34, Daniel Henrique Barboza wrote:
>>>>>
>>>>>
>>>>> On 7/2/22 03:24, Cédric Le Goater wrote:
>>>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>>>>>>> The function can't just return 0 whether an error happened and call it a
>>>>>>> day. We must provide a way of letting callers know if the zero return is
>>>>>>> legitimate or due to an error.
>>>>>>>
>>>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
>>>>>>> with an appropriate error, if one occurs. Callers are then free to pass
>>>>>>> an Error pointer and handle it.
>>>>>>>
>>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>>>>> ---
>>>>>>>   target/ppc/kvm.c | 16 +++++++++-------
>>>>>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>>>>> index 109823136d..bc17437097 100644
>>>>>>> --- a/target/ppc/kvm.c
>>>>>>> +++ b/target/ppc/kvm.c
>>>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>>>>>>>   /*
>>>>>>>    * Read a CPU node property from the host device tree that's a single
>>>>>>> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
>>>>>>> - * (can't find or open the property, or doesn't understand the format)
>>>>>>> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
>>>>>>> + * wrong (can't find or open the property, or doesn't understand the
>>>>>>> + * format)
>>>>>>>    */
>>>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>>>>>>   {
>>>>>>>       char buf[PATH_MAX], *tmp;
>>>>>>>       uint64_t val;
>>>>>>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>>>>>>> +        error_setg(errp, "Failed to read CPU property %s", propname);
>>>>>>>           return 0;
>>>>>>>       }
>>>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char 
>>>>>>> *propname)
>>>>>>>   uint64_t kvmppc_get_clockfreq(void)
>>>>>>>   {
>>>>>>> -    return kvmppc_read_int_cpu_dt("clock-frequency");
>>>>>>> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
>>>>>>
>>>>>>
>>>>>> This should be fatal. no ?
>>>>>
>>>>>
>>>>> I'm not sure. I went under the assumption that there might be some weird
>>>>> condition where 'clock-frequency' might be missing from the DT, and this
>>>>> is why we're not exiting out immediately.
>>>>>
>>>>> That said, the advantage of turning this into fatal is that we won't need
>>>>> all the other patches that handles error on this function. We're going to
>>>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay
>>>>> with that.
>>>>
>>>> I think so. Some machines behave badly when 'clock-frequency' is bogus,
>>>> division by zero, no console, etc. We could check easily with pseries
>>>> which is the only KVM PPC platform.
>>>
>>> Well not quite true ;)  I haven't tested it during the last release cycle, but the 
>>> Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini 
>>> last time I checked.
>>
>> Oh. Sorry. and I still have access to a real G5 running the latest debian.
>> I should give it a try one day.
> 
> I gave KVM a try on a :
> 
>      cpu        : PPC970MP, altivec supported
>      clock    : 2000.000000MHz
>      revision    : 1.0 (pvr 0044 0100)
>      processor    : 1
>      cpu        : PPC970MP, altivec supported
>      clock    : 2000.000000MHz
>      revision    : 1.0 (pvr 0044 0100)
>      timebase    : 33333333
>      platform    : PowerMac
>      model    : PowerMac11,2
>      machine    : PowerMac11,2
>      motherboard    : PowerMac11,2 MacRISC4 Power Macintosh
>      detected as    : 337 (PowerMac G5 Dual Core)
>      pmac flags    : 00000000
>      L2 cache    : 1024K unified
>      pmac-generation    : NewWorld
> 
> running debian with kernel 5.18.0-2-powerpc64. With the installed QEMU 7.0.0,
> 
>      qemu-system-ppc64 -M mac99 -cpu host -accel kvm ...
> 
> doesn't go very far. Program exception is quickly reached and host says:
> 
>      [56450.118422] Couldn't emulate instruction 0x00000000 (op 0 xop 0)
>      [56450.119060] kvmppc_exit_pr_progint: emulation at 100 failed (00000000)
> 
> Anything special I should know ?

As I don't have access to a G5 I've never tried that, however the qemu-system-ppc64 
mac99 is wired differently to the qemu-system-ppc mac99 machine so I wouldn't be 
surprised if something is broken there.

My normal test for MacOS is something like:

    qemu-system-ppc -M mac99 -accel kvm -hda macos104.img

Can you try qemu-system-ppc and see if it is any better? If not then I can fire up 
the G4 and get the git hashes for my last known working configuration.


ATB,

Mark.

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Cédric Le Goater]

On 7/11/22 09:37, Mark Cave-Ayland wrote:
> On 06/07/2022 08:45, Cédric Le Goater wrote:
> 
>> On 7/5/22 08:57, Cédric Le Goater wrote:
>>> On 7/5/22 08:51, Mark Cave-Ayland wrote:
>>>> On 04/07/2022 18:34, Cédric Le Goater wrote:
>>>>
>>>>> On 7/2/22 15:34, Daniel Henrique Barboza wrote:
>>>>>>
>>>>>>
>>>>>> On 7/2/22 03:24, Cédric Le Goater wrote:
>>>>>>> On 6/30/22 21:42, Daniel Henrique Barboza wrote:
>>>>>>>> The function can't just return 0 whether an error happened and call it a
>>>>>>>> day. We must provide a way of letting callers know if the zero return is
>>>>>>>> legitimate or due to an error.
>>>>>>>>
>>>>>>>> Add an Error pointer to kvmppc_read_int_cpu_dt() that will be filled
>>>>>>>> with an appropriate error, if one occurs. Callers are then free to pass
>>>>>>>> an Error pointer and handle it.
>>>>>>>>
>>>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>>>>>> ---
>>>>>>>>   target/ppc/kvm.c | 16 +++++++++-------
>>>>>>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>>>>>> index 109823136d..bc17437097 100644
>>>>>>>> --- a/target/ppc/kvm.c
>>>>>>>> +++ b/target/ppc/kvm.c
>>>>>>>> @@ -1925,15 +1925,17 @@ static uint64_t kvmppc_read_int_dt(const char *filename)
>>>>>>>>   /*
>>>>>>>>    * Read a CPU node property from the host device tree that's a single
>>>>>>>> - * integer (32-bit or 64-bit).  Returns 0 if anything goes wrong
>>>>>>>> - * (can't find or open the property, or doesn't understand the format)
>>>>>>>> + * integer (32-bit or 64-bit).  Returns 0 and set errp if anything goes
>>>>>>>> + * wrong (can't find or open the property, or doesn't understand the
>>>>>>>> + * format)
>>>>>>>>    */
>>>>>>>> -static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>>>>> +static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
>>>>>>>>   {
>>>>>>>>       char buf[PATH_MAX], *tmp;
>>>>>>>>       uint64_t val;
>>>>>>>>       if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
>>>>>>>> +        error_setg(errp, "Failed to read CPU property %s", propname);
>>>>>>>>           return 0;
>>>>>>>>       }
>>>>>>>> @@ -1946,12 +1948,12 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>>>>>>>   uint64_t kvmppc_get_clockfreq(void)
>>>>>>>>   {
>>>>>>>> -    return kvmppc_read_int_cpu_dt("clock-frequency");
>>>>>>>> +    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
>>>>>>>
>>>>>>>
>>>>>>> This should be fatal. no ?
>>>>>>
>>>>>>
>>>>>> I'm not sure. I went under the assumption that there might be some weird
>>>>>> condition where 'clock-frequency' might be missing from the DT, and this
>>>>>> is why we're not exiting out immediately.
>>>>>>
>>>>>> That said, the advantage of turning this into fatal is that we won't need
>>>>>> all the other patches that handles error on this function. We're going to
>>>>>> assume that if 'clock-frequency' is missing then we can't boot. I'm okay
>>>>>> with that.
>>>>>
>>>>> I think so. Some machines behave badly when 'clock-frequency' is bogus,
>>>>> division by zero, no console, etc. We could check easily with pseries
>>>>> which is the only KVM PPC platform.
>>>>
>>>> Well not quite true ;)  I haven't tested it during the last release cycle, but the Mac machines were still working fine to boot OS X with KVM-PR on my G4 Mac Mini last time I checked.
>>>
>>> Oh. Sorry. and I still have access to a real G5 running the latest debian.
>>> I should give it a try one day.
>>
>> I gave KVM a try on a :
>>
>>      cpu        : PPC970MP, altivec supported
>>      clock    : 2000.000000MHz
>>      revision    : 1.0 (pvr 0044 0100)
>>      processor    : 1
>>      cpu        : PPC970MP, altivec supported
>>      clock    : 2000.000000MHz
>>      revision    : 1.0 (pvr 0044 0100)
>>      timebase    : 33333333
>>      platform    : PowerMac
>>      model    : PowerMac11,2
>>      machine    : PowerMac11,2
>>      motherboard    : PowerMac11,2 MacRISC4 Power Macintosh
>>      detected as    : 337 (PowerMac G5 Dual Core)
>>      pmac flags    : 00000000
>>      L2 cache    : 1024K unified
>>      pmac-generation    : NewWorld
>>
>> running debian with kernel 5.18.0-2-powerpc64. With the installed QEMU 7.0.0,
>>
>>      qemu-system-ppc64 -M mac99 -cpu host -accel kvm ...
>>
>> doesn't go very far. Program exception is quickly reached and host says:
>>
>>      [56450.118422] Couldn't emulate instruction 0x00000000 (op 0 xop 0)
>>      [56450.119060] kvmppc_exit_pr_progint: emulation at 100 failed (00000000)
>>
>> Anything special I should know ?
> 
> As I don't have access to a G5 I've never tried that, however the qemu-system-ppc64 mac99 is wired differently to the qemu-system-ppc mac99 machine so I wouldn't be surprised if something is broken there.
> 
> My normal test for MacOS is something like:
> 
>     qemu-system-ppc -M mac99 -accel kvm -hda macos104.img
> 
> Can you try qemu-system-ppc and see if it is any better? If not then I can fire up the G4 and get the git hashes for my last known working configuration.

Same issue with 32bit.

Thanks,

C.

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[BALATON Zoltan]

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

On Mon, 11 Jul 2022, Mark Cave-Ayland wrote:
> On 06/07/2022 08:45, Cédric Le Goater wrote:
>> I gave KVM a try on a :
>>
>>      cpu        : PPC970MP, altivec supported
>>      clock    : 2000.000000MHz
>>      revision    : 1.0 (pvr 0044 0100)
>>      processor    : 1
>>      cpu        : PPC970MP, altivec supported
>>      clock    : 2000.000000MHz
>>      revision    : 1.0 (pvr 0044 0100)
>>      timebase    : 33333333
>>      platform    : PowerMac
>>      model    : PowerMac11,2
>>      machine    : PowerMac11,2
>>      motherboard    : PowerMac11,2 MacRISC4 Power Macintosh
>>      detected as    : 337 (PowerMac G5 Dual Core)
>>      pmac flags    : 00000000
>>      L2 cache    : 1024K unified
>>      pmac-generation    : NewWorld
>> 
>> running debian with kernel 5.18.0-2-powerpc64. With the installed QEMU 
>> 7.0.0,
>>
>>      qemu-system-ppc64 -M mac99 -cpu host -accel kvm ...
>> 
>> doesn't go very far. Program exception is quickly reached and host says:
>>
>>      [56450.118422] Couldn't emulate instruction 0x00000000 (op 0 xop 0)
>>      [56450.119060] kvmppc_exit_pr_progint: emulation at 100 failed 
>> (00000000)

Maybe try with -d unimp,guest_errors at least or some more debug options 
to find why it gets a 0 opcode. It probably takes a wrong exception 
somewhere? But with KVM maybe this does not give more info and you need to 
enable KVM tracing or run in a debgger instead?

In the past I've managed to run Linux on qemu-system-ppc64 -M mac99 
with TCG and trace KVM-PR guest within that but I forgot the details. If I 
remember correctly I've found there's some problem with emulated KVM and 
nobody replied on the list so I could not go further. It's also quite slow 
that way so not the best way to test.

>> Anything special I should know ?
>
> As I don't have access to a G5 I've never tried that, however the 
> qemu-system-ppc64 mac99 is wired differently to the qemu-system-ppc mac99 
> machine so I wouldn't be surprised if something is broken there.

I think you can get the 32 bit version with qemu-system-ppc64 by adding 
-cpu G4 as it decides based on CPU type what to emulate. By default -M 
mac99 with qemu-system-ppc64 is a G5 Mac but OpenBIOS still gives it the 
same device-tree as the G4 one so guests might be confused by this. Linux 
did not seem to care that much though.

> My normal test for MacOS is something like:
>
>   qemu-system-ppc -M mac99 -accel kvm -hda macos104.img

This should really be -M mac99,via=pmu as that is the closest to real 
hardware currently.

This mac99 machine is quite confusing and maybe the only reason we need 
separate ppc and ppc64 qemu executables. What do you think about 
deprecating it and splitting it into something like powermac3_1 for 
mac99,via=pmu with G4 CPU, powermac7_2 for the G5 one and maybe some 
powerbookX_Y for the mac99 with CUDA instead of PMU but I don't know if 
that actually exists in real hardware? Also rename g3beige to the actual 
model name at the same time. This would clear this up and avoid the 
confusing options that we have now because of everything emulated by the 
single mac99 machine.

Regards,
BALATON Zoltan

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Mark Cave-Ayland]

On 11/07/2022 08:42, Cédric Le Goater wrote:

>>> Anything special I should know ?
>>
>> As I don't have access to a G5 I've never tried that, however the qemu-system-ppc64 
>> mac99 is wired differently to the qemu-system-ppc mac99 machine so I wouldn't be 
>> surprised if something is broken there.
>>
>> My normal test for MacOS is something like:
>>
>>     qemu-system-ppc -M mac99 -accel kvm -hda macos104.img
>>
>> Can you try qemu-system-ppc and see if it is any better? If not then I can fire up 
>> the G4 and get the git hashes for my last known working configuration.
> 
> Same issue with 32bit.

I've just fired up my G4 to test this again, pulled the latest QEMU git master and 
confirmed that I have a working setup with the details below:

Host kernel: (5.1.0-rc2+)
commit a3ac7917b73070010c05b4485b8582a6c9cd69b6
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon Mar 25 14:49:00 2019 -0700

Guest kernel: (4.14.0-3-powerpc)
using Debian ports debian-9.0-powerpc-NETINST-1.iso

Command line:
./qemu-system-ppc [-M mac99] -accel kvm -cdrom 
/home/mca/images/debian-9.0-powerpc-NETINST-1.iso -boot d -nographic

However if I switch to using the latest Debian ports 
debian-10.0.0-powerpc-NETINST-1.iso then I get a failure:

[    0.198565] BUG: Unable to handle kernel data access on read at 0xbb0030d4
[    0.205152] Faulting instruction address: 0x0001b0c4
[    0.210175] Oops: Kernel access of bad area, sig: 11 [#1]
[    0.214933] BE PAGE_SIZE=4K MMU=Hash PowerMac
[    0.218226] Modules linked in:
[    0.220746] CPU: 0 PID: 0 Comm: swapper Not tainted 5.6.0-2-powerpc #1 Debian 5.6.14-1
[    0.226967] NIP:  0001b0c4 LR: 000030d4 CTR: 00000000
[    0.230869] REGS: c7fb5908 TRAP: 0300   Not tainted  (5.6.0-2-powerpc Debian 5.6.14-1)
[    0.236844] MSR:  00001012 <ME,DR,RI>  CR: 24002820  XER: 20000000
[    0.242096] DAR: bb0030d4 DSISR: 40000000
[    0.242096] GPR00: c0044e70 c7fb59c0 c0b26510 c7fb5f48 bb0030d4 40000000 00000000 
00000001
[    0.242096] GPR08: ff340038 bb0030d4 00001032 c7fb59c0 00000000 00000000 00000000 
00000004
[    0.242096] GPR16: 029c61f0 029c5d68 07c5cd08 00000001 029dd844 fffffffd fff55d10 
42000000
[    0.242096] GPR24: c0af6704 c0b20d94 00000000 00000000 c0bd862c 00000000 00000000 
0000000d
[    0.271138] NIP [0001b0c4] 0x1b0c4
[    0.273978] LR [000030d4] 0x30d4
[    0.276410] Call Trace:
[    0.278812] Instruction dump:
[    0.281219] 55290206 XXXXXXXX XXXXXXXX XXXXXXXX 4c00012c XXXXXXXX XXXXXXXX XXXXXXXX
[    0.287561] 419f0028 XXXXXXXX XXXXXXXX XXXXXXXX 81690000 XXXXXXXX XXXXXXXX XXXXXXXX
[    0.293922] ---[ end trace 3a9d775bab6f3340 ]---
[    0.297491]
[    1.284408] Kernel panic - not syncing: Attempted to kill the idle task!
[    1.290027] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---


Decompressing the initrd takes quite a long time, but I think this is the issue that 
was recently discussed on the mailing list?

I think the next step should be to move my host kernel forward to a more current 
version with the working debian-9.0-powerpc-NETINST-1.iso and see if it is able to 
boot without any problems.


ATB,

Mark.

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[BALATON Zoltan]

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

On Tue, 12 Jul 2022, Mark Cave-Ayland wrote:
> On 11/07/2022 08:42, Cédric Le Goater wrote:
>>>> Anything special I should know ?
>>> 
>>> As I don't have access to a G5 I've never tried that, however the 
>>> qemu-system-ppc64 mac99 is wired differently to the qemu-system-ppc mac99 
>>> machine so I wouldn't be surprised if something is broken there.
>>> 
>>> My normal test for MacOS is something like:
>>> 
>>>     qemu-system-ppc -M mac99 -accel kvm -hda macos104.img
>>> 
>>> Can you try qemu-system-ppc and see if it is any better? If not then I can 
>>> fire up the G4 and get the git hashes for my last known working 
>>> configuration.
>> 
>> Same issue with 32bit.
>
> I've just fired up my G4 to test this again, pulled the latest QEMU git 
> master and confirmed that I have a working setup with the details below:
>
> Host kernel: (5.1.0-rc2+)
> commit a3ac7917b73070010c05b4485b8582a6c9cd69b6
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Mon Mar 25 14:49:00 2019 -0700
>
> Guest kernel: (4.14.0-3-powerpc)
> using Debian ports debian-9.0-powerpc-NETINST-1.iso
>
> Command line:
> ./qemu-system-ppc [-M mac99] -accel kvm -cdrom 
> /home/mca/images/debian-9.0-powerpc-NETINST-1.iso -boot d -nographic
>
> However if I switch to using the latest Debian ports 
> debian-10.0.0-powerpc-NETINST-1.iso then I get a failure:
>
> [    0.198565] BUG: Unable to handle kernel data access on read at 0xbb0030d4

What is or should be at this address and why does the kernel access it? By 
default I see nothing mapped there. Do you need more RAM? Maybe the 
default 128 MB is not enough for newer kernels? I've seen such problem 
with other OSes before.

Regards,
BALATON Zoltan

> [    0.205152] Faulting instruction address: 0x0001b0c4
> [    0.210175] Oops: Kernel access of bad area, sig: 11 [#1]
> [    0.214933] BE PAGE_SIZE=4K MMU=Hash PowerMac
> [    0.218226] Modules linked in:
> [    0.220746] CPU: 0 PID: 0 Comm: swapper Not tainted 5.6.0-2-powerpc #1 
> Debian 5.6.14-1
> [    0.226967] NIP:  0001b0c4 LR: 000030d4 CTR: 00000000
> [    0.230869] REGS: c7fb5908 TRAP: 0300   Not tainted  (5.6.0-2-powerpc 
> Debian 5.6.14-1)
> [    0.236844] MSR:  00001012 <ME,DR,RI>  CR: 24002820  XER: 20000000
> [    0.242096] DAR: bb0030d4 DSISR: 40000000
> [    0.242096] GPR00: c0044e70 c7fb59c0 c0b26510 c7fb5f48 bb0030d4 40000000 
> 00000000 00000001
> [    0.242096] GPR08: ff340038 bb0030d4 00001032 c7fb59c0 00000000 00000000 
> 00000000 00000004
> [    0.242096] GPR16: 029c61f0 029c5d68 07c5cd08 00000001 029dd844 fffffffd 
> fff55d10 42000000
> [    0.242096] GPR24: c0af6704 c0b20d94 00000000 00000000 c0bd862c 00000000 
> 00000000 0000000d
> [    0.271138] NIP [0001b0c4] 0x1b0c4
> [    0.273978] LR [000030d4] 0x30d4
> [    0.276410] Call Trace:
> [    0.278812] Instruction dump:
> [    0.281219] 55290206 XXXXXXXX XXXXXXXX XXXXXXXX 4c00012c XXXXXXXX XXXXXXXX 
> XXXXXXXX
> [    0.287561] 419f0028 XXXXXXXX XXXXXXXX XXXXXXXX 81690000 XXXXXXXX XXXXXXXX 
> XXXXXXXX
> [    0.293922] ---[ end trace 3a9d775bab6f3340 ]---
> [    0.297491]
> [    1.284408] Kernel panic - not syncing: Attempted to kill the idle task!
> [    1.290027] ---[ end Kernel panic - not syncing: Attempted to kill the 
> idle task! ]---
>
>
> Decompressing the initrd takes quite a long time, but I think this is the 
> issue that was recently discussed on the mailing list?
>
> I think the next step should be to move my host kernel forward to a more 
> current version with the working debian-9.0-powerpc-NETINST-1.iso and see if 
> it is able to boot without any problems.
>
>
> ATB,
>
> Mark.
>
>

Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()

[Mark Cave-Ayland]

On 12/07/2022 15:54, BALATON Zoltan wrote:

> On Tue, 12 Jul 2022, Mark Cave-Ayland wrote:
>> On 11/07/2022 08:42, Cédric Le Goater wrote:
>>>>> Anything special I should know ?
>>>>
>>>> As I don't have access to a G5 I've never tried that, however the 
>>>> qemu-system-ppc64 mac99 is wired differently to the qemu-system-ppc mac99 machine 
>>>> so I wouldn't be surprised if something is broken there.
>>>>
>>>> My normal test for MacOS is something like:
>>>>
>>>>     qemu-system-ppc -M mac99 -accel kvm -hda macos104.img
>>>>
>>>> Can you try qemu-system-ppc and see if it is any better? If not then I can fire 
>>>> up the G4 and get the git hashes for my last known working configuration.
>>>
>>> Same issue with 32bit.
>>
>> I've just fired up my G4 to test this again, pulled the latest QEMU git master and 
>> confirmed that I have a working setup with the details below:
>>
>> Host kernel: (5.1.0-rc2+)
>> commit a3ac7917b73070010c05b4485b8582a6c9cd69b6
>> Author: Linus Torvalds <torvalds@linux-foundation.org>
>> Date:   Mon Mar 25 14:49:00 2019 -0700
>>
>> Guest kernel: (4.14.0-3-powerpc)
>> using Debian ports debian-9.0-powerpc-NETINST-1.iso
>>
>> Command line:
>> ./qemu-system-ppc [-M mac99] -accel kvm -cdrom 
>> /home/mca/images/debian-9.0-powerpc-NETINST-1.iso -boot d -nographic
>>
>> However if I switch to using the latest Debian ports 
>> debian-10.0.0-powerpc-NETINST-1.iso then I get a failure:
>>
>> [    0.198565] BUG: Unable to handle kernel data access on read at 0xbb0030d4
> 
> What is or should be at this address and why does the kernel access it? By default I 
> see nothing mapped there. Do you need more RAM? Maybe the default 128 MB is not 
> enough for newer kernels? I've seen such problem with other OSes before.

Yeah I've already tried increasing the RAM and it makes no difference. It wouldn't 
surprise me if it's a kernel issue since Christophe has done a lot of low-level work 
for 32-bit PPC including moving routines from asm to C and KASAN. I'm away for a few 
days but I will do a bisect when I get back, unless anyone beats me to it...


ATB,

Mark.