| Message ID | 1424343286-6792-4-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 19 February 2015 at 13:40, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 19/02/15 10:54, Ard Biesheuvel wrote: >> --- >> arch/arm/kvm/mmu.c | 2 +- >> arch/arm64/include/asm/kvm_arm.h | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c >> index 136662547ca6..fa8ec55220ea 100644 >> --- a/arch/arm/kvm/mmu.c >> +++ b/arch/arm/kvm/mmu.c >> @@ -1530,7 +1530,7 @@ void kvm_toggle_cache(struct kvm_vcpu *vcpu, bool was_enabled) >> stage2_flush_vm(vcpu->kvm); >> >> /* Caches are now on, stop trapping VM ops (until a S/W op) */ >> - if (now_enabled) >> + if (0)//now_enabled) >> vcpu_set_hcr(vcpu, vcpu_get_hcr(vcpu) & ~HCR_TVM); >> >> trace_kvm_toggle_cache(*vcpu_pc(vcpu), was_enabled, now_enabled); >> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h >> index 8afb863f5a9e..437e1ec17539 100644 >> --- a/arch/arm64/include/asm/kvm_arm.h >> +++ b/arch/arm64/include/asm/kvm_arm.h >> @@ -75,7 +75,7 @@ >> * FMO: Override CPSR.F and enable signaling with VF >> * SWIO: Turn set/way invalidates into set/way clean+invalidate >> */ >> -#define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \ >> +#define HCR_GUEST_FLAGS (HCR_TSC | /* HCR_TSW | */ HCR_TWE | HCR_TWI | HCR_VM | \ > > Why do we stop to trap S/W ops here? We can't let the guest issue those > without doing anything, as this will break anything that expects the > data to make it to memory. Think of the 32bit kernel decompressor, for > example. > TBH patch #3 is just a q'n'd hack to ensure that the TVM bit remains set in HCR. I was assuming that cleaning the entire cache on mmu enable/disable would be sufficient to quantify the performance impact and check whether patch #2 works as advertised. I was wondering: isn't calling stage2_flush_vm() for each set of each way very costly?
On 19 February 2015 at 15:19, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 19/02/15 13:44, Ard Biesheuvel wrote: >> On 19 February 2015 at 13:40, Marc Zyngier <marc.zyngier@arm.com> wrote: >>> On 19/02/15 10:54, Ard Biesheuvel wrote: >>>> --- >>>> arch/arm/kvm/mmu.c | 2 +- >>>> arch/arm64/include/asm/kvm_arm.h | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c >>>> index 136662547ca6..fa8ec55220ea 100644 >>>> --- a/arch/arm/kvm/mmu.c >>>> +++ b/arch/arm/kvm/mmu.c >>>> @@ -1530,7 +1530,7 @@ void kvm_toggle_cache(struct kvm_vcpu *vcpu, bool was_enabled) >>>> stage2_flush_vm(vcpu->kvm); >>>> >>>> /* Caches are now on, stop trapping VM ops (until a S/W op) */ >>>> - if (now_enabled) >>>> + if (0)//now_enabled) >>>> vcpu_set_hcr(vcpu, vcpu_get_hcr(vcpu) & ~HCR_TVM); >>>> >>>> trace_kvm_toggle_cache(*vcpu_pc(vcpu), was_enabled, now_enabled); >>>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h >>>> index 8afb863f5a9e..437e1ec17539 100644 >>>> --- a/arch/arm64/include/asm/kvm_arm.h >>>> +++ b/arch/arm64/include/asm/kvm_arm.h >>>> @@ -75,7 +75,7 @@ >>>> * FMO: Override CPSR.F and enable signaling with VF >>>> * SWIO: Turn set/way invalidates into set/way clean+invalidate >>>> */ >>>> -#define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \ >>>> +#define HCR_GUEST_FLAGS (HCR_TSC | /* HCR_TSW | */ HCR_TWE | HCR_TWI | HCR_VM | \ >>> >>> Why do we stop to trap S/W ops here? We can't let the guest issue those >>> without doing anything, as this will break anything that expects the >>> data to make it to memory. Think of the 32bit kernel decompressor, for >>> example. >>> >> >> TBH patch #3 is just a q'n'd hack to ensure that the TVM bit remains >> set in HCR. I was assuming that cleaning the entire cache on mmu >> enable/disable would be sufficient to quantify the performance impact >> and check whether patch #2 works as advertised. > > OK. > >> I was wondering: isn't calling stage2_flush_vm() for each set of each >> way very costly? > > It's only called once, when TVM is not set. We then set TVM to make sure > that this doesn't happen anymore, until we stop trapping. > Ah, right, of course. > Of course, with your new approach, this doesn't work anymore and we'd > need to find another approach. > Well, *if* this approach is feasible in the first place, I'm sure we can find another bit to set to keep track of whether to perform the s/w operations. For now, I just ripped it out because I was afraid it might skew performance measurements done with the TVM kept set.
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c index 136662547ca6..fa8ec55220ea 100644 --- a/arch/arm/kvm/mmu.c +++ b/arch/arm/kvm/mmu.c @@ -1530,7 +1530,7 @@ void kvm_toggle_cache(struct kvm_vcpu *vcpu, bool was_enabled) stage2_flush_vm(vcpu->kvm); /* Caches are now on, stop trapping VM ops (until a S/W op) */ - if (now_enabled) + if (0)//now_enabled) vcpu_set_hcr(vcpu, vcpu_get_hcr(vcpu) & ~HCR_TVM); trace_kvm_toggle_cache(*vcpu_pc(vcpu), was_enabled, now_enabled); diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h index 8afb863f5a9e..437e1ec17539 100644 --- a/arch/arm64/include/asm/kvm_arm.h +++ b/arch/arm64/include/asm/kvm_arm.h @@ -75,7 +75,7 @@ * FMO: Override CPSR.F and enable signaling with VF * SWIO: Turn set/way invalidates into set/way clean+invalidate */ -#define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \ +#define HCR_GUEST_FLAGS (HCR_TSC | /* HCR_TSW | */ HCR_TWE | HCR_TWI | HCR_VM | \ HCR_TVM | HCR_BSU_IS | HCR_FB | HCR_TAC | \ HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW) #define HCR_VIRT_EXCP_MASK (HCR_VA | HCR_VI | HCR_VF)