[libvirt] [RFC PATCH 2/4] powerpc: Add cpu driver for powerpc host

[Prerna Saxena]

On Mon, 10 Oct 2011 10:57:34 +0100
"Daniel P. Berrange" <berrange at redhat.com> wrote:

> On Sat, Oct 08, 2011 at 12:12:46AM +0530, Prerna Saxena wrote:
> > This part of code primarily compares host and guest CPUs of a given
> > architecture for feature compatibility. x86 makes this choice based on
> > CPUID comparison.
> > Presently the PowerPC code has stubs to just get a 'pseries' guest to
> > boot. It would be augmented going forward, to do a detailed feature
> > comparison between guest and host CPUs on powerpc.
> > This part of code is presently well-classified into different
> > architectures, and consequently does not need reorganizing.
> > 
> > ---
> >  src/Makefile.am       |    3 +-
> >  src/cpu/cpu.c         |    2 +
> >  src/cpu/cpu_powerpc.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/cpu/cpu_powerpc.h |   32 +++++++++++++++++++
> >  4 files changed, 117 insertions(+), 1 deletions(-)
> >  create mode 100644 src/cpu/cpu_powerpc.c
> >  create mode 100644 src/cpu/cpu_powerpc.h
> 
> The idea here looks fine.

Thanks.

> 
> > diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c
> > new file mode 100644
> > index 0000000..6ceedc3
> > --- /dev/null
> > +++ b/src/cpu/cpu_powerpc.c
> > +
> > +#define VIR_FROM_THIS VIR_FROM_CPU
> > +
> > +static const char *archs[] = { "ppc64" };
> 
> How about 'ppc' too ?
> 
> > +static union cpuData *
> > +PowerPCNodeData(void)
> > +{
> > +    union cpuData *data;
> > +
> > +    if (VIR_ALLOC(data) < 0) {
> > +        virReportOOMError();
> > +        return NULL;
> > +    }
> > +
> > +    return data;
> > +}
> > +
> > +
> > +static int
> > +PowerPCDecode(virCPUDefPtr cpu,
> > +              const union cpuData *data,
> > +              const char **models,
> > +              unsigned int nmodels,
> > +              const char *preferred)
> 
> Need to annotate these with 'ATTRIBUTE_UNUSED' to avoid compiler
> warnings.

Will do.

> 
> > +{
> > +	return 0;
> > +}
> > +
> > +static int
> 
> Should be 'void'

Thanks for pointing this out, will fix it.

> 
> > +PowerPCDataFree(union cpuData *data)
> > +{
> > +   if (data == NULL)
> > +       return 0;
> > +
> > +   VIR_FREE(data);
> > +}
> > +
> > +struct cpuArchDriver cpuDriverPowerPC = {
> > +    .name = "ppc64",
> > +    .arch = archs,
> > +    .narch = ARRAY_CARDINALITY(archs),
> > +    .compare    = NULL,
> > +    .decode     = PowerPCDecode,
> > +    .encode     = NULL,
> > +    .free       = PowerPCDataFree,
> > +    .nodeData   = PowerPCNodeData,
> > +    .guestData  = NULL,
> > +    .baseline   = NULL,
> > +    .update     = NULL,
> > +    .hasFeature = NULL,
> > +};
> 
> Should we have another copy for 'ppc' arch too ?
> 

At present, I've tested this with KVM for Power ISA Book3S machines,
which introduces a new machine type "pseries" based on the ppc64
architecture. Extending this to 'ppc' architecture should be a trivial
effort -- I'll add that as soon as I get to test it.

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India