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

From: Daniel Henrique Barboza <danielhb413@gmail.com>
To: "Cédric Le Goater" <clg@kaod.org>, qemu-devel@nongnu.org
Cc: qemu-ppc@nongnu.org
Subject: Re: [PATCH 2/9] target/ppc: add errp to kvmppc_read_int_cpu_dt()
Date: Sat, 2 Jul 2022 10:34:49 -0300	[thread overview]
Message-ID: <47277f4f-a6a5-85dc-4806-67df8e2fc153@gmail.com> (raw)
In-Reply-To: <55014e2a-a668-4843-8338-850abeb5ff04@kaod.org>

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