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.