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.