On Thu, Sep 27, 2012 at 10:13 AM, Will Deacon <will.deacon@xxxxxxx> wrote:
> On Wed, Sep 26, 2012 at 02:43:14AM +0100, Christoffer Dall wrote:
>> On 09/25/2012 11:20 AM, Will Deacon wrote:
>> >> +/* Multiprocessor Affinity Register */
>> >> +#define MPIDR_CPUID (0x3 << 0)
>> >
>> > I'm fairly sure we already have code under arch/arm/ for dealing with the
>> > mpidr. Let's re-use that rather than reinventing it here.
>> >
>>
>> I see some defines in topology.c - do you want some of these factored
>> out into a header file that we can then also use from kvm? If so, where?
>
> I guess either in topology.h or a new header (topology-bits.h).
>
>> >> +#define EXCEPTION_NONE 0
>> >> +#define EXCEPTION_RESET 0x80
>> >> +#define EXCEPTION_UNDEFINED 0x40
>> >> +#define EXCEPTION_SOFTWARE 0x20
>> >> +#define EXCEPTION_PREFETCH 0x10
>> >> +#define EXCEPTION_DATA 0x08
>> >> +#define EXCEPTION_IMPRECISE 0x04
>> >> +#define EXCEPTION_IRQ 0x02
>> >> +#define EXCEPTION_FIQ 0x01
>> >
>> > Why the noise?
>> >
>>
>> these are simply cruft from a previous life of KVM/ARM.
>
> Ok, then please get rid of them.
>
>> >> +static inline enum vcpu_mode vcpu_mode(struct kvm_vcpu *vcpu)
>> >> +{
>> >> + u8 mode = __vcpu_mode(vcpu->arch.regs.cpsr);
>> >> + BUG_ON(mode == 0xf);
>> >> + return mode;
>> >> +}
>> >
>> > I noticed that you have a fair few BUG_ONs throughout the series. Fair
>> > enough, but for hyp code is that really the right thing to do? Killing the
>> > guest could make more sense, perhaps?
>>
>> the idea is to have BUG_ONs that are indeed BUG_ONs that we want to
>> catch explicitly on the host. We have had a pass over the code to change
>> all the BUG_ONs that can be provoked by the guest and inject the proper
>> exceptions into the guest in this case. If you find places where this is
>> not the case, it should be changed, and do let me know.
>
> Ok, so are you saying that a BUG_ON due to some detected inconsistency with
> one guest may not necessarily terminate the other guests? BUG_ONs in the
> host seem like a bad idea if the host is able to continue with a subset of
> guests.
>
No, I'm saying a BUG_ON is an actual BUG, it should not happen and
there should be nowhere where a guest can cause a BUG_ON to occur in
the host, because that would be a bug.
We basically never kill a guest unless really extreme things happen
(like we cannot allocate a pte, in which case we return -ENOMEM). If a
guest does something really weird, that guest will receive the
appropriate exception (undefined, prefetch abort, ...)
>> >
>> >> +static inline u32 *vcpu_pc(struct kvm_vcpu *vcpu)
>> >> +{
>> >> + return vcpu_reg(vcpu, 15);
>> >> +}
>> >
>> > If you stick a struct pt_regs into struct kvm_regs, you could reuse ARM_pc
>> > here etc.
>> >
>>
>> I prefer not to, because we'd have those registers presumably for usr
>> mode and then we only define the others explicit. I think it's much
>> clearer to look at kvm_regs today.
>
> I disagree and think that you should reuse as much of the arch/arm/ code as
> possible. Not only does it make it easier to read by people who are familiar
> with that code (and in turn get you more reviewers) but it also means that
> we limit the amount of duplication that we have.
Reusing a struct just for the sake of reusing is not necessarily an
improvement. Some times it complicates things, and some times it's
misleading. To me, pt_regs carry the meaning that these are the
registers for a user space process that traps into the kernel - in KVM
we emulate a virtual CPU and that current definition is quite clear.
The argument that more people will review the code if the struct
contains a pt_regs field rather than a usr_regs field is completely
invalid, because I'm sure everyone that reviews virtualization code
will know that user mode is a mode on the cpu and it has some
registers and this is the state we store when we context switch a VM -
pt_regs could be read as the regs that we stored in the mode that the
VM happened to be in when we took an exception, which I would think is
crazy, and probably not what you suggest.
Writing the literal 15 for the PC register is not really a problem in
terms of duplication - it's nothing that requires separate
maintenance.
At this point the priority should really be correctness, readability,
and performance, imho.
>
> I think Marc (CC'd) had a go at this with some success.
>
great, if this improves the code, then I suggest someone rebases an
appropriate patch and sends it to the kvmarm mailing list so we can
have a look at it, but there are users out there looking to try
kvm/arm and we should try to give it to them.
>> >> +#ifndef __ARM_KVM_HOST_H__
>> >> +#define __ARM_KVM_HOST_H__
>> >> +
>> >> +#include <asm/kvm.h>
>> >> +
>> >> +#define KVM_MAX_VCPUS 4
>> >
>> > NR_CPUS?
>> >
>>
>> well this is defined by KVM generic code, and is common for other
>> architecture.
>
> I mean #define KVM_MAX_CPUS NR_CPUS. The 4 seems arbitrary.
>
>> >> +int __attribute_const__ kvm_target_cpu(void)
>> >> +{
>> >> + unsigned int midr;
>> >> +
>> >> + midr = read_cpuid_id();
>> >> + switch ((midr >> 4) & 0xfff) {
>> >> + case KVM_ARM_TARGET_CORTEX_A15:
>> >> + return KVM_ARM_TARGET_CORTEX_A15;
>> >
>> > I have this code already in perf_event.c. Can we move it somewhere common
>> > and share it? You should also check that the implementor field is 0x41.
>> >
>>
>> by all means, you can probably suggest a good place better than I can...
>
> cputype.h?
>
>> >> +#include <linux/module.h>
>> >> +
>> >> +EXPORT_SYMBOL_GPL(smp_send_reschedule);
>> >
>> > Erm...
>> >
>> > We already have arch/arm/kernel/armksyms.c for exports -- please use that.
>> > However, exporting such low-level operations sounds like a bad idea. How
>> > realistic is kvm-as-a-module on ARM anyway?
>> >
>>
>> at this point it's broken, so I'll just remove this and leave this for a
>> fun project for some poor soul at some point if anyone ever needs half
>> the code outside the kernel as a module (the other half needs to be
>> compiled in anyway)
>
> Ok, that suits me. If it's broken, let's not include it in the initial
> submission.
>
>> >> +int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>> >> +{
>> >> + return -EINVAL;
>> >> +}
>> >> +
>> >> +int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>> >> +{
>> >> + return -EINVAL;
>> >> +}
>> >
>> > Again, all looks like this should be implemented using regsets from what I
>> > can tell.
>> >
>>
>> this API has been discussed to death on the KVM lists, and we can of
>> course revive that if the regset makes it nicer - I'd prefer getting
>> this upstream the way that it is now though, where GET_REG / SET_REG
>> seems to be the way forward from a KVM perspective.
>
> I'm sure the API has been discussed, but I've not seen that conversation and
> I'm also not aware about whether or not regsets were considered as a
> possibility for this stuff. The advantages of using them are:
>
> 1. It's less code for the arch to implement (and most of what you
> need, you already have).
>
> 2. You can move the actual copying code into core KVM, like we have
> for ptrace.
>
> 3. New KVM ports (e.g. arm64) can reuse the core copying code
> easily.
>
> Furthermore, some registers (typically) floating point and GPRs will already
> have regsets for the ptrace code, so that can be reused if you share the
> datatypes.
>
> The big problem with getting things upstream and then changing it later is
> that you will break the ABI. I highly doubt that's feasible, so can we not
> just use regsets from the start for ARM?
>
>> >> +int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>> >> +{
>> >> + struct kvm_regs *cpu_reset;
>> >> +
>> >> + switch (vcpu->arch.target) {
>> >> + case KVM_ARM_TARGET_CORTEX_A15:
>> >> + if (vcpu->vcpu_id > a15_max_cpu_idx)
>> >> + return -EINVAL;
>> >> + cpu_reset = &a15_regs_reset;
>> >> + vcpu->arch.midr = read_cpuid_id();
>> >> + break;
>> >> + default:
>> >> + return -ENODEV;
>> >> + }
>> >> +
>> >> + /* Reset core registers */
>> >> + memcpy(&vcpu->arch.regs, cpu_reset, sizeof(vcpu->arch.regs));
>> >> +
>> >> + /* Reset CP15 registers */
>> >> + kvm_reset_coprocs(vcpu);
>> >> +
>> >> + return 0;
>> >> +}
>> >
>> > This is a nice way to plug in new CPUs but the way the rest of the code is
>> > currently written, all the ARMv7 and Cortex-A15 code is merged together. I
>> > *strongly* suggest you isolate this from the start, as it will help you see
>> > what is architected and what is implementation-specific.
>> >
>>
>> not entirely sure what you mean. You want a separate coproc.c file for
>> Cortex-A15 specific stuff like coproc_a15.c?
>
> Indeed. I think it will make adding new CPUs a lot clearer and separate the
> architecture from the implementation.
>
> Cheers,
>
> Will
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
