Re: [Xen-staging] [xen-unstable] Added some more fields to host_cpu.

[Hollis Blanchard <hollisb@us.ibm.com>]

Hi Keir, I see the patch you've committed for this issue
(http://xenbits2.xensource.com/staging/xen-unstable.hg?rev/ae203b55e7c8), but my concern goes a littler further.

What will this data be used for? Will higher-level tools expect to find
"stepping" and break if they don't? Will XenSource's management tools
(which I assume will make use of this data) function properly in this
case? I agree that hardware-specific data needs to be visible somehow to
higher-level tools, but it makes me nervous...

(I don't have any particular objection to your fix; just voicing my
concerns.)

On Fri, 2007-03-02 at 13:39 -0700, Alex Williamson wrote:
> Hi Ewan,
> 
>    There are a couple problems with this patch for non-x86 (and maybe
> even on x86), see below:
> 
> On Tue, 2007-02-27 at 01:56 +0000, Xen staging patchbot-unstable wrote:
> > diff -r e7b2a282c9e7 -r 50e0616fd012 tools/python/xen/xend/XendNode.py
> > --- a/tools/python/xen/xend/XendNode.py	Mon Feb 26 17:20:36 2007 +0000
> > +++ b/tools/python/xen/xend/XendNode.py	Tue Feb 27 00:37:27 2007 +0000
> > @@ -81,7 +81,7 @@ class XendNode:
> >          for cpu_uuid, cpu in saved_cpus.items():
> >              self.cpus[cpu_uuid] = cpu
> >  
> > -        # verify we have enough cpus here
> > +        cpuinfo = parse_proc_cpuinfo()
> >          physinfo = self.physinfo_dict()
> >          cpu_count = physinfo['nr_cpus']
> >          cpu_features = physinfo['hw_caps']
> > @@ -91,12 +91,23 @@ class XendNode:
> >          if cpu_count != len(self.cpus):
> >              self.cpus = {}
> >              for i in range(cpu_count):
> > -                cpu_uuid = uuid.createString()
> > -                cpu_info = {'uuid': cpu_uuid,
> > -                            'host': self.uuid,
> > -                            'number': i,
> > -                            'features': cpu_features}
> > -                self.cpus[cpu_uuid] = cpu_info
> > +                u = uuid.createString()
> > +                self.cpus[u] = {'uuid': u, 'number': i }
> > +
> > +        for u in self.cpus.keys():
> > +            log.error(self.cpus[u])
> > +            number = self.cpus[u]['number']
> > +            log.error(number)
> > +            log.error(cpuinfo)
> > +            self.cpus[u].update(
> > +                { 'host'     : self.uuid,
> > +                  'features' : cpu_features,
> > +                  'speed'    : int(float(cpuinfo[number]['cpu MHz'])),
> > +                  'vendor'   : cpuinfo[number]['vendor_id'],
> > +                  'modelname': cpuinfo[number]['model name'],
> > +                  'stepping' : cpuinfo[number]['stepping'],
> > +                  'flags'    : cpuinfo[number]['flags'],
> > +                })
> 
>    On ia64, dom0 doesn't automatically get vcpus for each physical cpu,
> so the first problem is that we're not going to have a /proc/cpuinfo
> entry for every cpu in self.cpus.keys.  I think it's likely x86 could
> run into this problem too if a cpu was hotplugged or booted with the
> dom0_max_vcpus options.
> 
>    The second problem is that /proc/cpuinfo fields are very architecture
> specific.  I'd suggest importing arch and having separate cases for x86,
> ia64, and powerpc.  For ia64, think the most appropriate mapping would
> be:
> 
>                 self.cpus[u].update(
>                     { 'host'     : self.uuid,
>                       'features' : cpu_features,
>                       'speed'    : int(float(cpuinfo[0]['cpu MHz'])),
>                       'vendor'   : cpuinfo[0]['vendor'],
>                       'modelname': cpuinfo[0]['family'],
>                       'stepping' : cpuinfo[0]['model'],
>                       'flags'    : cpuinfo[0]['features'],
>                     })
> 
> Hollis or Jimi might be able to chime in with identifiers that would
> work on powerpc.  Thanks,
> 
> 	Alex
-- 
Hollis Blanchard
IBM Linux Technology Center