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

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

[Daniel Henrique Barboza]

Hi,

In this second version the biggest change is that I decided to
error_fatal inside kvmppc_get_clockfreq().

After the discussions in v1 [1] it became clear that falling back to a
default value when 'clock-frequency' is missing is a bad idea. Not because
of the default value per se, but for the absence of this root node
property in the userspace.

This assumption works fine with pSeries, the most used KVM platform we
have today. If this change causes a legitimate regression (i.e. a valid
use case where clock-frequency is missing from the DT and it's ok) in
other KVM-capable ppc boards, we can amend it case by case.

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-06/msg05751.html

Daniel Henrique Barboza (4):
  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: exit(1) on failure in kvmppc_get_clockfreq()

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

 target/ppc/kvm.c | 46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

-- 
2.36.1

[PATCH v2 1/5] 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 v2 2/5] 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 v2 3/5] 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>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
[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 v2 4/5] 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 v2 5/5] target/ppc: exit(1) on failure in kvmppc_get_clockfreq()

[Daniel Henrique Barboza]

When running under KVM accel it is expected to have 'clock-frequency' in
the DT. Not having this attribute is too risky for both the machine
emulation and userspace.

We have a way of telling whether this error scenario might happen or not
via kvmppc_read_int_cpu_dt() now being able to report errors. From now
on, when running KVM, we will assume that 'clock-frequency' will always
be present in the DT.

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

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index c218380eb7..d9febb2c63 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1945,9 +1945,24 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname, Error **errp)
     return kvmppc_read_int_dt(tmp, errp);
 }
 
+/*
+ * Read the clock-frequency from the DT. On error (e.g.
+ * 'clock-frequency' is not present in the DT) will
+ * report an error and exit(1).
+ */
 uint64_t kvmppc_get_clockfreq(void)
 {
-    return kvmppc_read_int_cpu_dt("clock-frequency", NULL);
+    Error *local_err = NULL;
+    int ret;
+
+    ret = kvmppc_read_int_cpu_dt("clock-frequency", &local_err);
+
+    if (local_err) {
+        error_report_err(local_err);
+        exit(1);
+    }
+
+    return ret;
 }
 
 static int kvmppc_get_dec_bits(void)
-- 
2.36.1

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

[Cédric Le Goater]

On 7/6/22 21:31, Daniel Henrique Barboza wrote:
> 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)

I would add an errp parameter and use error_setg_errno().

C.


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