Re: [Qemu-ppc] [Qemu-devel] [PATCH for-2.11 5/6] ppc: simplify cpu model lookup by PVR

[Igor Mammedov]

On Fri, 25 Aug 2017 19:32:29 +1000
David Gibson <address@hidden> wrote:

> On Fri, Aug 25, 2017 at 09:40:37AM +0200, Igor Mammedov wrote:
> > On Fri, 25 Aug 2017 14:16:44 +1000
> > David Gibson <address@hidden> wrote:
> >   
> > > On Thu, Aug 24, 2017 at 10:21:50AM +0200, Igor Mammedov wrote:  
> > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > ---
> > > >  target/ppc/translate_init.c | 27 +++++++++++----------------
> > > >  1 file changed, 11 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > > > index f1a559d..ca9f1e3 100644
> > > > --- a/target/ppc/translate_init.c
> > > > +++ b/target/ppc/translate_init.c
> > > > @@ -34,6 +34,7 @@
> > > >  #include "hw/ppc/ppc.h"
> > > >  #include "mmu-book3s-v3.h"
> > > >  #include "sysemu/qtest.h"
> > > > +#include "qemu/cutils.h"
> > > >  
> > > >  //#define PPC_DUMP_CPU
> > > >  //#define PPC_DEBUG_SPR
> > > > @@ -10203,22 +10204,16 @@ static ObjectClass 
> > > > *ppc_cpu_class_by_name(const char *name)
> > > >      char *cpu_model, *typename;
> > > >      ObjectClass *oc;
> > > >      const char *p;
> > > > -    int i, len;
> > > > -
> > > > -    /* Check if the given name is a PVR */
> > > > -    len = strlen(name);
> > > > -    if (len == 10 && name[0] == '0' && name[1] == 'x') {
> > > > -        p = name + 2;
> > > > -        goto check_pvr;
> > > > -    } else if (len == 8) {
> > > > -        p = name;
> > > > -    check_pvr:
> > > > -        for (i = 0; i < 8; i++) {
> > > > -            if (!qemu_isxdigit(*p++))
> > > > -                break;
> > > > -        }
> > > > -        if (i == 8) {
> > > > -            return OBJECT_CLASS(ppc_cpu_class_by_pvr(strtoul(name, 
> > > > NULL, 16)));
> > > > +    unsigned long pvr;
> > > > +
> > > > +    /* Lookup by PVR if cpu_model is valid 8 digit hex number
> > > > +     * (excl: 0x prefix if present)
> > > > +     */
> > > > +    if (!qemu_strtoul(name, &p, 16, &pvr)) {
> > > > +        int len = p - name;
> > > > +        len = (len == 10) && (name[1] == 'x') ? len - 2 : len;
> > > > +        if (len == 8) {
> > > > +            return OBJECT_CLASS(ppc_cpu_class_by_pvr(pvr));    
> > > 
> > > This doesn't look quite right.  A hex string of a PVR followed by
> > > other stuff isn't a valid option, so if p (endptr) doesn't point to
> > > the end of the string, then we shouldn't be using this path  
> > yep, my mistake, I've lost somewhere *p == '\0' check when cleaning up the 
> > patch  
> 
> Ok.
> 
> > >(from the
> > > looks of qemu_strtoul() we can simply omit the endptr parameter to get
> > > that behaviour).  I'm not sure what the messing around with len is
> > > for, either.  If it's a valid hex string we can try this, I don't see
> > > that we need further checks.  
> > I've tried to keep current limitation here i.e. 8 hex symbols,
> > but if any hex number is fine then doing
> >  qemu_strtoul(name, NULL, 16, &pvr)
> > is even better, so would you prefer to drop 8 symbol check altogether?  
> 
> Yeah, I don't see any value to the 8 character check.
there are cpu models consisting of pure numbers => valid hex number
so if check is dropped then it lookup will go PVR route,
that will fail there.
So check should be there to avoid regression.

I'll fix whole string consumption check that I've missed before
and respin with it.