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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3903 bytes --]

On Wed, Aug 30, 2017 at 12:50:33PM +0200, Igor Mammedov wrote:
> On Fri, 25 Aug 2017 19:32:29 +1000
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Thu, Aug 24, 2017 at 10:21:50AM +0200, Igor Mammedov wrote:  
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > >  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.

Ah.  Good point.

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]