On Thu, Oct 4, 2012 at 9:02 AM, Min-gyu Kim <mingyu84.kim@xxxxxxxxxxx> wrote: > > >> -----Original Message----- >> From: kvm-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] On >> Behalf Of Christoffer Dall >> Sent: Monday, October 01, 2012 4:22 AM >> To: Will Deacon >> Cc: kvm@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; >> kvmarm@xxxxxxxxxxxxxxxxxxxxx; rusty.russell@xxxxxxxxxx; avi@xxxxxxxxxx; >> marc.zyngier@xxxxxxx >> Subject: Re: [PATCH 06/15] KVM: ARM: Initial skeleton to compile KVM >> support >> >> 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, ...) >> > > I agree with Will. It seems to be overreacting to kill the entire system. > > From the code above, BUG_ON case clearly points out that there happened a > serious bug case. However, killing the corresponding VM may not cause any > further problem. > Then leave some logs for debugging and killing the VM seems to be enough. > > Let's assume KVM for ARM is distributed with a critical bug. > If the case is defended by BUG_ON, it will cause host to shutdown. > If the case is defended by killing VM, it will cause VM to shutdown. > In my opinion, latter case seems to be better. > > I looked for a guide on BUG_ON and found this: > http://yarchive.net/comp/linux/BUG.html > > I completely agree with all this, no further argument is needed. The point of a BUG_ON is to explicitly state the reason for a bug that will anyhow cause the host kernel to overall malfunction. The above bug_on statement is long gone (see new patches), and if you see other cases like this, the code should have been tested and we can remove the BUG_ON. >> >> > >> >> >> +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 > -- 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
