| Message ID | 1424343286-6792-4-git-send-email-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. M.
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? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Of course, with your new approach, this doesn't work anymore and we'd need to find another approach. M.
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)