Re: [PATCH 1/8] kvm tools: Add initial SPAPR PPC64 architecture support

[Scott Wood <scottwood@freescale.com>]

On 12/05/2011 10:05 PM, Matt Evans wrote:
> This patch adds a new arch directory, powerpc, basic file structure, register
> setup and where necessary stubs out arch-specific functions (e.g. interrupts,
> runloop exits) that later patches will provide.  The target is an
> SPAPR-compliant PPC64 machine (i.e. pSeries); there is no support for PPC32 or
> 'bare metal' PPC64 guests as yet.  Subsequent patches implement the hcalls and
> RTAS required to boot SPAPR pSeries kernels.

You just sent out 28 patches removing "everything is x86"
dependencies -- may I suggest that the PPC code be structured so that
there isn't an "everything on PPC (or even PPC64) is SPAPR" assumption,
even if SPAPR is initially the only sub-arch present?

E.g. this is SPAPR-specific despite being in generically-named
tools/kvm/powerpc/kvm-cpu.c:

> +static void kvm_cpu__setup_regs(struct kvm_cpu *vcpu)
> +{
> +	struct kvm_regs *r = &vcpu->regs;
> +
> +	if (vcpu->cpu_id == 0) {
> +		r->pc = KERNEL_START_ADDR;
> +		r->gpr[3] = vcpu->kvm->fdt_gra;
> +		r->gpr[5] = 0;
> +	} else {
> +		r->pc = KERNEL_SECONDARY_START_ADDR;
> +		r->gpr[3] = vcpu->cpu_id;
> +	}
> +	r->msr = 0x8000000000001000UL; /* 64bit, non-HV, ME */
> +
> +	if (ioctl(vcpu->vcpu_fd, KVM_SET_REGS, &vcpu->regs) < 0)
> +		die_perror("KVM_SET_REGS failed");
> +}

> diff --git a/tools/kvm/powerpc/include/kvm/kvm-arch.h b/tools/kvm/powerpc/include/kvm/kvm-arch.h
> new file mode 100644
> index 0000000..722d01c
> --- /dev/null
> +++ b/tools/kvm/powerpc/include/kvm/kvm-arch.h
> @@ -0,0 +1,70 @@
> +/*
> + * PPC64 architecture-specific definitions
> + *
> + * Copyright 2011 Matt Evans <matt@ozlabs.org>, IBM Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#ifndef KVM__KVM_ARCH_H
> +#define KVM__KVM_ARCH_H
[snip]
> +void ioport__setup_arch(void)
[snip]
> +int irq__register_device(u32 dev, u8 *num, u8 *pin, u8 *line)

I'm seeing a lot of double-underscores -- is this common style in KVM
tool?  It's reserved for use by the compiler and system library.  It's
common in the kernel (though not used like this for namespace
prefixes), but there's no system library involved there.

> diff --git a/tools/kvm/powerpc/kvm-cpu.c b/tools/kvm/powerpc/kvm-cpu.c
> new file mode 100644
> index 0000000..79422ff
> --- /dev/null
> +++ b/tools/kvm/powerpc/kvm-cpu.c
[snip]
> +#define MSR_SF		(1UL<<63)
> +#define MSR_HV		(1UL<<60)
> +#define MSR_VEC		(1UL<<25)
> +#define MSR_VSX		(1UL<<23)
> +#define MSR_POW		(1UL<<18)
> +#define MSR_EE		(1UL<<15)
> +#define MSR_PR		(1UL<<14)
> +#define MSR_FP		(1UL<<13)
> +#define MSR_ME		(1UL<<12)
> +#define MSR_FE0		(1UL<<11)
> +#define MSR_SE		(1UL<<10)
> +#define MSR_BE		(1UL<<9)
> +#define MSR_FE1		(1UL<<8)
> +#define MSR_IR		(1UL<<5)
> +#define MSR_DR		(1UL<<4)
> +#define MSR_PMM		(1UL<<2)
> +#define MSR_RI		(1UL<<1)
> +#define MSR_LE		(1UL<<0)

Shouldn't these go in a header?

> +#define HUGETLBFS_MAGIC       0x958458f6

#include <linux/magic.h>

?

-Scott