Re: [PATCH] KVM: arm/arm64: arch_timer: Mark physical interrupt active when a virtual interrupt is pending
Hi Marc,
On 08/02/2019 14:43, Marc Zyngier wrote:
> When a guest gets scheduled, KVM performs a "load" operation,
> which for the timer includes evaluating the virtual "active" state
> of the interrupt, and replicating it on the physical side. This
> ensures that the deactivation in the guest will also take place
> in the physical GIC distributor.
>
> If the interrupt is not yet active, we flag it as inactive on the
> physical side. This means that on restoring the timer registers,
> if the timer has expired, we'll immediately take an interrupt.
> That's absolutely fine, as the interrupt will then be flagged as
> active on the physical side. What this assumes though is that we'll
> enter the guest right after having taken the interrupt, and that
> the guest will quickly ACK the interrupt, making it active at on
Nit: "at on" -> pick one
> the virtual side.
>
> It turns out that quite often, this assumption doesn't really hold.
> The guest may be preempted on the back on this interrupt, either
on the back of*
> from kernel space or whilst running at EL1 when a host interrupt
> fires. When this happens, we repeat the whole sequence on the
> next load (interrupt marked as inactive, timer registers restored,
> interrupt fires). And if it takes a really long time for a guest
> to activate the interrupt (as it does with nested virt), we end-up
> with many such events in quick succession, leading to the guest only
> making very slow progress.
>
> This can also be seen with the number of virtual timer interrupt on the
> host being far greater than the same number in the guest.
>
> An easy way to fix this is to evaluate the timer state when performing
> the "load" operation, just like we do when the interrupt actually fires.
> If the timer has a pending virtual interrupt at this stage, then we
> can safely flag the physical interrupt as being active, which prevents
> spurious exits.
>
> Signed-off-by: Marc Zyngier
Otherwise, I think the change makes sense:
Reviewed-by: Julien Thierry
Cheers,
> ---
> virt/kvm/arm/arch_timer.c | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 7449651ae2e5..70c18479ccd5 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -487,12 +487,21 @@ static inline void set_timer_irq_phys_active(struct
> arch_timer_context *ctx, boo
> static void kvm_timer_vcpu_load_gic(struct arch_timer_context *ctx)
> {
> struct kvm_vcpu *vcpu = ctx->vcpu;
> - bool phys_active;
> + bool phys_active = false;
> +
> + /*
> + * Update the timer output so that it is likely to match the
> + * state we're about to restore. If the timer expires between
> + * this point and the register restoration, we'll take the
> + * interrupt anyway.
> + */
> + kvm_timer_update_irq(ctx->vcpu, kvm_timer_should_fire(ctx), ctx);
>
> if (irqchip_in_kernel(vcpu->kvm))
> phys_active = kvm_vgic_map_is_active(vcpu, ctx->irq.irq);
> - else
> - phys_active = ctx->irq.level;
> +
> + phys_active |= ctx->irq.level;
> +
> set_timer_irq_phys_active(ctx, phys_active);
> }
>
>
--
Julien Thierry
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm/arm64: Update MAINTAINERS entries
On 12/02/2019 12:11, Suzuki K Poulose wrote: > Hi Marc, > > On 12/02/2019 11:06, Marc Zyngier wrote: >> For historical reasons, KVM/arm and KVM/arm64 have had different >> entries in the MAINTAINER file. This makes little sense, as they are >> maintained together. >> >> On top of that, we have a bunch of talented people helping with >> the reviewing, and they deserve to be mentionned in the consolidated >> entry. > > Thanks for the recognition ! > >> >> Acked-by: Christoffer Dall >> Signed-off-by: Marc Zyngier >> --- >> MAINTAINERS | 18 +++--- >> 1 file changed, 7 insertions(+), 11 deletions(-) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 51029a425dbe..91ec0682443b 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -8297,29 +8297,25 @@ S: Maintained >> F: arch/x86/include/asm/svm.h >> F: arch/x86/kvm/svm.c >> >> -KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm) >> +KERNEL VIRTUAL MACHINE FOR ARM/ARM64 (KVM/arm, KVM/arm64) >> M: Christoffer Dall >> M: Marc Zyngier >> +R: James Morse >> +R: Julien Thierry > >> +R: Suzuki Pouloze > > Please note that it is: "Suzuki K Poulose" Arghhh! Now fixed. Sorry about that. > > With that: > > Acked-by: Suzuki K Poulose Thanks, M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 00/27] KVM: x86/mmu: Remove fast invalidate mechanism
On 05/02/19 21:54, Sean Christopherson wrote:
> ...and clean up the MMIO spte generation code.
>
> === Non-x86 Folks ===
> Patch 01/27 (which you should also be cc'd on) changes the prototype for
> a function that is defined by all architectures but only actually used
> by x86. Feel free to skip the rest of this cover letter.
>
> === x86 Folks ===
> Though not explicitly stated, for all intents and purposes the fast
> invalidate mechanism was added to speed up the scenario where removing
> a memslot, e.g. when accessing PCI ROM, caused KVM to flush all shadow
> entries[1].
>
> The other use cases of "flush everything" are VM teardown and handling
> MMIO generation overflow, neither of which is a performance critial
> path (see "Other Uses" below).
>
> For the memslot case, zapping all shadow entries is overkill, i.e. KVM
> only needs to zap the entries associated with the memslot, but KVM has
> historically used a big hammer because if all you have is a hammer,
> everything looks like a nail.
>
> Rather than zap all pages when removing a memslot, zap only the shadow
> entries associated with said memslot. I see a performance improvement
> of ~5% when zapping only the pages for the deleted memslot when using a
> slightly modified version of the original benchmark[2][3][4] (I don't
> have easy access to a system with hundreds of gigs of memory).
>
> $ cat shell.sh
> #!/bin/sh
>
> echo 3 > /proc/sys/vm/drop_caches
> ./mmtest -c 8 -m 2000 -e ./rom.sh
>
> I.e. running 8 threads and 2gb of memory per thread, time in milliseconds:
>
> Before: 89.117
> After: 84.768
>
>
> With the memslot use case gone, maintaining the fast invalidate path
> adds a moderate amount of complexity but provides little to no value.
> Furhtermore, its existence may give the impression that it's "ok" to zap
> all shadow pages. Remove the fast invalidate mechanism to simplify the
> code and to discourage future code from zapping all pages as using such
> a big hammer should be a last resort.
>
> Unfortunately, simply reverting the fast invalidate code re-introduces
> shortcomings in the previous code, notably that KVM may hold a spinlock
> and not schedule for an extended period of time. Releasing the lock to
> voluntarily reschedule is perfectly doable, and for the VM teardown case
> of "zap everything" it can be added with no additional changes since KVM
> doesn't need to provide any ordering guarantees about sptes in that case.
>
> For the MMIO case, KVM needs to prevent vCPUs from consuming stale sptes
> created with an old generation number. This should be a non-issue as
> KVM is supposed to prevent consuming stale MMIO sptes regardless of the
> zapping mechanism employed, releasing mmu_lock while zapping simply
> makes it much more likely to hit any bug leading to stale spte usage.
> As luck would have it, there are a number of corner cases in the MMIO
> spte invalidation flow that could theoretically result in reusing stale
> sptes. So before reworking memslot zapping and reverting to the slow
> invalidate mechanism, fix a number of bugs related to MMIO generations
> and opportunistically clean up the memslots/MMIO generation code.
>
>
> === History ===
> Flushing of shadow pages when removing a memslot was originally added
> by commit 34d4cb8fca1f ("KVM: MMU: nuke shadowed pgtable pages and ptes
> on memslot destruction"), and obviously emphasized functionality over
> performance. Commit 2df72e9bc4c5 ("KVM: split kvm_arch_flush_shadow")
> added a path to allow flushing only the removed slot's shadow pages,
> but x86 just redirected to the "zap all" flow.
>
> Eventually, it became evident that zapping everything is slow, and so
> x86 developed a more efficient hammer in the form of the fast invalidate
> mechanism.
>
> === Other Uses ===
> When a VM is being destroyed, either there are no active vcpus, i.e.
> there's no lock contention, or the VM has ungracefully terminated, in
> which case we want to reclaim its pages as quickly as possible, i.e.
> not release the MMU lock if there are still CPUs executing in the VM.
>
> The MMIO generation scenario is almost literally a one-in-a-million
> occurrence, i.e. is not a performance sensitive scenario.
>
> It's worth noting that prior to the "fast invalidate" series being
> applied, there was actually another use case of kvm_mmu_zap_all() in
> emulator_fix_hypercall() whose existence may have contributed to
> improving the performance of "zap all" instead of avoiding it altogether.
> But that usage was removed by the series itself in commit 758ccc89b83c
> ("KVM: x86: drop calling kvm_mmu_zap_all in emulator_fix_hypercall").
>
> [1]
> https://lkml.kernel.org/r/1369960590-14138-1-git-send-email-xiaoguangr...@linux.vnet.ibm.com
> [2]
> https://lkml.kernel.org/r/1368706673-8530-1-git-send-email-xiaoguangr...@linux.vnet.ibm.com
> [3] http://lkml.iu.edu/hypermail/linux/kernel/1305.2/00277.html
> [4]
Re: [PATCH] KVM: arm/arm64: Update MAINTAINERS entries
Hi Marc, On 12/02/2019 11:06, Marc Zyngier wrote: For historical reasons, KVM/arm and KVM/arm64 have had different entries in the MAINTAINER file. This makes little sense, as they are maintained together. On top of that, we have a bunch of talented people helping with the reviewing, and they deserve to be mentionned in the consolidated entry. Thanks for the recognition ! Acked-by: Christoffer Dall Signed-off-by: Marc Zyngier --- MAINTAINERS | 18 +++--- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 51029a425dbe..91ec0682443b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8297,29 +8297,25 @@ S: Maintained F:arch/x86/include/asm/svm.h F:arch/x86/kvm/svm.c -KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm) +KERNEL VIRTUAL MACHINE FOR ARM/ARM64 (KVM/arm, KVM/arm64) M:Christoffer Dall M:Marc Zyngier +R: James Morse +R: Julien Thierry +R: Suzuki Pouloze Please note that it is: "Suzuki K Poulose" With that: Acked-by: Suzuki K Poulose ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH] KVM: arm/arm64: Update MAINTAINERS entries
For historical reasons, KVM/arm and KVM/arm64 have had different entries in the MAINTAINER file. This makes little sense, as they are maintained together. On top of that, we have a bunch of talented people helping with the reviewing, and they deserve to be mentionned in the consolidated entry. Acked-by: Christoffer Dall Signed-off-by: Marc Zyngier --- MAINTAINERS | 18 +++--- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 51029a425dbe..91ec0682443b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8297,29 +8297,25 @@ S: Maintained F: arch/x86/include/asm/svm.h F: arch/x86/kvm/svm.c -KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm) +KERNEL VIRTUAL MACHINE FOR ARM/ARM64 (KVM/arm, KVM/arm64) M: Christoffer Dall M: Marc Zyngier +R: James Morse +R: Julien Thierry +R: Suzuki Pouloze L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers) L: kvmarm@lists.cs.columbia.edu W: http://systems.cs.columbia.edu/projects/kvm-arm T: git git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git -S: Supported +S: Maintained F: arch/arm/include/uapi/asm/kvm* F: arch/arm/include/asm/kvm* F: arch/arm/kvm/ -F: virt/kvm/arm/ -F: include/kvm/arm_* - -KERNEL VIRTUAL MACHINE FOR ARM64 (KVM/arm64) -M: Christoffer Dall -M: Marc Zyngier -L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers) -L: kvmarm@lists.cs.columbia.edu -S: Maintained F: arch/arm64/include/uapi/asm/kvm* F: arch/arm64/include/asm/kvm* F: arch/arm64/kvm/ +F: virt/kvm/arm/ +F: include/kvm/arm_* KERNEL VIRTUAL MACHINE FOR MIPS (KVM/mips) M: James Hogan -- 2.20.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] clocksource/arm_arch_timer: Store physical timer IRQ number for KVM on VHE
On 24/01/2019 14:46, Christoffer Dall wrote:
> From: Andre Przywara
>
> A host running in VHE mode gets the EL2 physical timer as its time
> source (accessed using the EL1 sysreg accessors, which get re-directed
> to the EL2 sysregs by VHE).
>
> The EL1 physical timer remains unused by the host kernel, allowing us to
> pass that on directly to a KVM guest and saves us from emulating this
> timer for the guest on VHE systems.
>
> Store the EL1 Physical Timer's IRQ number in
> struct arch_timer_kvm_info on VHE systems to allow KVM to use it.
>
> Signed-off-by: Andre Przywara
> Signed-off-by: Marc Zyngier
> Signed-off-by: Christoffer Dall
> ---
> Patches in preparation for nested virtualization on KVM/Arm depend on this
> change, so we would like to merge this via the kvmarm tree or have a stable
> branch including this patch.
>
> Please let us know your preference. Thanks.
Hi Christopher,
sorry for the delay. I'm fine if you want to take it through your tree.
Acked-by: Daniel Lezcano
> drivers/clocksource/arm_arch_timer.c | 11 +--
> include/clocksource/arm_arch_timer.h | 1 +
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c
> b/drivers/clocksource/arm_arch_timer.c
> index 9a7d4dc00b6e..b9243e2328b4 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -1206,6 +1206,13 @@ static enum arch_timer_ppi_nr __init
> arch_timer_select_ppi(void)
> return ARCH_TIMER_PHYS_SECURE_PPI;
> }
>
> +static void __init arch_timer_populate_kvm_info(void)
> +{
> + arch_timer_kvm_info.virtual_irq = arch_timer_ppi[ARCH_TIMER_VIRT_PPI];
> + if (is_kernel_in_hyp_mode())
> + arch_timer_kvm_info.physical_irq =
> arch_timer_ppi[ARCH_TIMER_PHYS_NONSECURE_PPI];
> +}
> +
> static int __init arch_timer_of_init(struct device_node *np)
> {
> int i, ret;
> @@ -1220,7 +1227,7 @@ static int __init arch_timer_of_init(struct device_node
> *np)
> for (i = ARCH_TIMER_PHYS_SECURE_PPI; i < ARCH_TIMER_MAX_TIMER_PPI; i++)
> arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
>
> - arch_timer_kvm_info.virtual_irq = arch_timer_ppi[ARCH_TIMER_VIRT_PPI];
> + arch_timer_populate_kvm_info();
>
> rate = arch_timer_get_cntfrq();
> arch_timer_of_configure_rate(rate, np);
> @@ -1550,7 +1557,7 @@ static int __init arch_timer_acpi_init(struct
> acpi_table_header *table)
> arch_timer_ppi[ARCH_TIMER_HYP_PPI] =
> acpi_gtdt_map_ppi(ARCH_TIMER_HYP_PPI);
>
> - arch_timer_kvm_info.virtual_irq = arch_timer_ppi[ARCH_TIMER_VIRT_PPI];
> + arch_timer_populate_kvm_info();
>
> /*
>* When probing via ACPI, we have no mechanism to override the sysreg
> diff --git a/include/clocksource/arm_arch_timer.h
> b/include/clocksource/arm_arch_timer.h
> index 349e5957c949..702967d996bb 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -74,6 +74,7 @@ enum arch_timer_spi_nr {
> struct arch_timer_kvm_info {
> struct timecounter timecounter;
> int virtual_irq;
> + int physical_irq;
> };
>
> struct arch_timer_mem_frame {
>
--
Facebook |
Twitter |
