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

[Matt Evans <matt@ozlabs.org>]

Hi Scott,

On 07/12/11 05:03, Scott Wood wrote:
> 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?

I had anticipated this comment (though not the "28 patches" remark, easy now).
It is a fair comment, but you hit the nail on the head with your other mail
(regarding configuring in i8042, presumably to emulate crappy dev boards)
asking whether kvmtool has a config system.  It does not.

Since we currently lack any kind of build-time configuration (or any fancy
run-time -M <machine> a la QEMU) it's a bit hard to cater for multiple
platforms.  I'm aware that the PPC patches are painfully PPC64-with-SPAPR and I
don't present them as perfect, but I really think we need to attack the
configuration stuff before bifurcating.  Is this something you'd like to see to?

(Your comments on the #defines and magic accepted & fixed, thank you.)


Cheers,


Matt



> 
> 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