Re: [PATCH 3/5] KVM: PPC: Paravirtualize SPRG4-7, ESR, PIR, MASn

[Scott Wood <scottwood@freescale.com>]

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