On 09/05/2011 05:28 PM, Alexander Graf wrote: >> + /* >> + * SPRG4-7 are user-readable, so we can't keep these >> + * consistent between the magic page and the real >> + * registers. We provide space in case the guest >> + * can deal with this. >> + * >> + * This also applies to SPRG3 on some chips. >> + */ >> + __u64 sprg4; >> + __u64 sprg5; >> + __u64 sprg6; >> + __u64 sprg7; > > Hrm. You're touching sprg4-7 but don't remove the fields from vcpu->arch. That sounds wrong. Also, the entry/exit code needs to use these now instead of the vcpu struct fields to restore the correct values. The original idea, as the comment states, was just to provide an area that the guest could use for this, as we can't keep it fully synced with the hardware registers since the hw regs don't trap on read, and the paravirt doesn't trap on write. However, I think it could work reasonably well to use this as the backing store instead of vcpu->arch.sprg4-7. The guest would still see inconsistency if it writes to paravirt and then reads from the hw reg without an intervening exit, so that restriction on use still applies, but qemu would see the right thing in sregs, and we wouldn't have the duplication. >> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c >> index 5f078bc..34da20d 100644 >> --- a/arch/powerpc/kernel/asm-offsets.c >> +++ b/arch/powerpc/kernel/asm-offsets.c >> @@ -431,6 +431,15 @@ int main(void) >> DEFINE(VCPU_SHARED_MSR, offsetof(struct kvm_vcpu_arch_shared, msr)); >> DEFINE(VCPU_SHADOW_MSR, offsetof(struct kvm_vcpu, arch.shadow_msr)); >> >> +#ifdef CONFIG_FSL_BOOKE >> + DEFINE(VCPU_SHARED_MAS0, offsetof(struct kvm_vcpu_arch_shared, mas0)); >> + DEFINE(VCPU_SHARED_MAS1, offsetof(struct kvm_vcpu_arch_shared, mas1)); >> + DEFINE(VCPU_SHARED_MAS2, offsetof(struct kvm_vcpu_arch_shared, mas2)); >> + DEFINE(VCPU_SHARED_MAS7_3, offsetof(struct kvm_vcpu_arch_shared, mas7_3)); >> + DEFINE(VCPU_SHARED_MAS4, offsetof(struct kvm_vcpu_arch_shared, mas4)); >> + DEFINE(VCPU_SHARED_MAS6, offsetof(struct kvm_vcpu_arch_shared, mas6)); >> +#endif > > While I agree that they only make sense on BookE, the fields in the ABI are not #ifdef'ed, so I don't see why the asm-offsets fields should be. OK. >> +#define SPR_FROM 0 >> +#define SPR_TO 0x100 >> + >> +#define KVM_INST_SPR(sprn, moveto) (0x7c0002a6 | \ >> + (((sprn) & 0x1f) << 16) | \ >> + (((sprn) & 0x3e0) << 6) | \ >> + (moveto)) > > #define KVM_INST_MFSPR(sprn) KVM_INST_MFSPR(sprn, SPR_FROM) > #define KVM_INST_MTSPR(sprn) KVM_INST_MFSPR(sprn, SPR_TO) > > makes it more readable really :) OK. >> @@ -618,6 +618,7 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) >> vcpu->arch.pc = 0; >> vcpu->arch.shared->msr = 0; >> vcpu->arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS; >> + vcpu->arch.shared->pir = vcpu->vcpu_id; > > That one rings a bell. Are you sure this patch set is on top of the other one that fixes PIR? Yes. Now that it's paravirted we need to store it somewhere other than just vcpu->vcpu_id. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
