| Message ID | 20210525134321.303768132@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VMX: configure posted interrupt descriptor when assigning device (v5) | expand |
On Tue, May 25, 2021 at 10:41:17AM -0300, Marcelo Tosatti wrote: > KVM_REQ_UNBLOCK will be used to exit a vcpu from > its inner vcpu halt emulation loop. > > Rename KVM_REQ_PENDING_TIMER to KVM_REQ_UNBLOCK, switch > PowerPC to arch specific request bit. > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > Index: kvm/include/linux/kvm_host.h > =================================================================== > --- kvm.orig/include/linux/kvm_host.h > +++ kvm/include/linux/kvm_host.h > @@ -146,7 +146,7 @@ static inline bool is_error_page(struct > */ > #define KVM_REQ_TLB_FLUSH (0 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) > #define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) > -#define KVM_REQ_PENDING_TIMER 2 > +#define KVM_REQ_UNBLOCK 2 > #define KVM_REQ_UNHALT 3 > #define KVM_REQUEST_ARCH_BASE 8 > > Index: kvm/virt/kvm/kvm_main.c > =================================================================== > --- kvm.orig/virt/kvm/kvm_main.c > +++ kvm/virt/kvm/kvm_main.c > @@ -2794,6 +2794,8 @@ static int kvm_vcpu_check_block(struct k > goto out; > if (signal_pending(current)) > goto out; > + if (kvm_check_request(KVM_REQ_UNBLOCK, vcpu)) > + goto out; > > ret = 0; > out: > Index: kvm/Documentation/virt/kvm/vcpu-requests.rst > =================================================================== > --- kvm.orig/Documentation/virt/kvm/vcpu-requests.rst > +++ kvm/Documentation/virt/kvm/vcpu-requests.rst > @@ -118,10 +118,11 @@ KVM_REQ_MMU_RELOAD > necessary to inform each VCPU to completely refresh the tables. This > request is used for that. > > -KVM_REQ_PENDING_TIMER > +KVM_REQ_UNBLOCK > > This request may be made from a timer handler run on the host on behalf > - of a VCPU. It informs the VCPU thread to inject a timer interrupt. > + of a VCPU, or when device assignment is performed. It informs the VCPU to > + exit the vcpu halt inner loop. > > KVM_REQ_UNHALT > > Index: kvm/arch/powerpc/include/asm/kvm_host.h > =================================================================== > --- kvm.orig/arch/powerpc/include/asm/kvm_host.h > +++ kvm/arch/powerpc/include/asm/kvm_host.h > @@ -51,6 +51,7 @@ > /* PPC-specific vcpu->requests bit members */ > #define KVM_REQ_WATCHDOG KVM_ARCH_REQ(0) > #define KVM_REQ_EPR_EXIT KVM_ARCH_REQ(1) > +#define KVM_REQ_PENDING_TIMER KVM_ARCH_REQ(2) > > #include <linux/mmu_notifier.h> > > Index: kvm/arch/x86/kvm/lapic.c > =================================================================== > --- kvm.orig/arch/x86/kvm/lapic.c > +++ kvm/arch/x86/kvm/lapic.c > @@ -1657,7 +1657,7 @@ static void apic_timer_expired(struct kv > } > > atomic_inc(&apic->lapic_timer.pending); > - kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu); > + kvm_make_request(KVM_REQ_UNBLOCK, vcpu); > if (from_timer_fn) > kvm_vcpu_kick(vcpu); > } Pure question on the existing code: why do we need kvm_make_request() for timer? As I see kvm_vcpu_check_block() already checks explicitly for timers: static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu) { ... if (kvm_cpu_has_pending_timer(vcpu)) goto out; ... } for x86: int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) { if (lapic_in_kernel(vcpu)) return apic_has_pending_timer(vcpu); return 0; } So wondering why we can drop the two references to KVM_REQ_PENDING_TIMER in x86 directly.. > Index: kvm/arch/x86/kvm/x86.c > =================================================================== > --- kvm.orig/arch/x86/kvm/x86.c > +++ kvm/arch/x86/kvm/x86.c > @@ -9300,7 +9300,7 @@ static int vcpu_run(struct kvm_vcpu *vcp > if (r <= 0) > break; > > - kvm_clear_request(KVM_REQ_PENDING_TIMER, vcpu); > + kvm_clear_request(KVM_REQ_UNBLOCK, vcpu); > if (kvm_cpu_has_pending_timer(vcpu)) > kvm_inject_pending_timer_irqs(vcpu); > > >
On Tue, May 25, 2021 at 03:14:33PM -0400, Peter Xu wrote: > On Tue, May 25, 2021 at 10:41:17AM -0300, Marcelo Tosatti wrote: > > KVM_REQ_UNBLOCK will be used to exit a vcpu from > > its inner vcpu halt emulation loop. > > > > Rename KVM_REQ_PENDING_TIMER to KVM_REQ_UNBLOCK, switch > > PowerPC to arch specific request bit. > > > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > > > Index: kvm/include/linux/kvm_host.h > > =================================================================== > > --- kvm.orig/include/linux/kvm_host.h > > +++ kvm/include/linux/kvm_host.h > > @@ -146,7 +146,7 @@ static inline bool is_error_page(struct > > */ > > #define KVM_REQ_TLB_FLUSH (0 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) > > #define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) > > -#define KVM_REQ_PENDING_TIMER 2 > > +#define KVM_REQ_UNBLOCK 2 > > #define KVM_REQ_UNHALT 3 > > #define KVM_REQUEST_ARCH_BASE 8 > > > > Index: kvm/virt/kvm/kvm_main.c > > =================================================================== > > --- kvm.orig/virt/kvm/kvm_main.c > > +++ kvm/virt/kvm/kvm_main.c > > @@ -2794,6 +2794,8 @@ static int kvm_vcpu_check_block(struct k > > goto out; > > if (signal_pending(current)) > > goto out; > > + if (kvm_check_request(KVM_REQ_UNBLOCK, vcpu)) > > + goto out; > > > > ret = 0; > > out: > > Index: kvm/Documentation/virt/kvm/vcpu-requests.rst > > =================================================================== > > --- kvm.orig/Documentation/virt/kvm/vcpu-requests.rst > > +++ kvm/Documentation/virt/kvm/vcpu-requests.rst > > @@ -118,10 +118,11 @@ KVM_REQ_MMU_RELOAD > > necessary to inform each VCPU to completely refresh the tables. This > > request is used for that. > > > > -KVM_REQ_PENDING_TIMER > > +KVM_REQ_UNBLOCK > > > > This request may be made from a timer handler run on the host on behalf > > - of a VCPU. It informs the VCPU thread to inject a timer interrupt. > > + of a VCPU, or when device assignment is performed. It informs the VCPU to > > + exit the vcpu halt inner loop. > > > > KVM_REQ_UNHALT > > > > Index: kvm/arch/powerpc/include/asm/kvm_host.h > > =================================================================== > > --- kvm.orig/arch/powerpc/include/asm/kvm_host.h > > +++ kvm/arch/powerpc/include/asm/kvm_host.h > > @@ -51,6 +51,7 @@ > > /* PPC-specific vcpu->requests bit members */ > > #define KVM_REQ_WATCHDOG KVM_ARCH_REQ(0) > > #define KVM_REQ_EPR_EXIT KVM_ARCH_REQ(1) > > +#define KVM_REQ_PENDING_TIMER KVM_ARCH_REQ(2) > > > > #include <linux/mmu_notifier.h> > > > > Index: kvm/arch/x86/kvm/lapic.c > > =================================================================== > > --- kvm.orig/arch/x86/kvm/lapic.c > > +++ kvm/arch/x86/kvm/lapic.c > > @@ -1657,7 +1657,7 @@ static void apic_timer_expired(struct kv > > } > > > > atomic_inc(&apic->lapic_timer.pending); > > - kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu); > > + kvm_make_request(KVM_REQ_UNBLOCK, vcpu); > > if (from_timer_fn) > > kvm_vcpu_kick(vcpu); > > } > > Pure question on the existing code: why do we need kvm_make_request() for > timer? As I see kvm_vcpu_check_block() already checks explicitly for timers: > > static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu) > { > ... > if (kvm_cpu_has_pending_timer(vcpu)) > goto out; > ... > } > > for x86: > > int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) > { > if (lapic_in_kernel(vcpu)) > return apic_has_pending_timer(vcpu); > > return 0; > } > > So wondering why we can drop the two references to KVM_REQ_PENDING_TIMER in x86 > directly.. See commit 06e05645661211b9eaadaf6344c335d2e80f0ba2
On Tue, May 25, 2021 at 04:26:37PM -0300, Marcelo Tosatti wrote: > On Tue, May 25, 2021 at 03:14:33PM -0400, Peter Xu wrote: > > On Tue, May 25, 2021 at 10:41:17AM -0300, Marcelo Tosatti wrote: > > > KVM_REQ_UNBLOCK will be used to exit a vcpu from > > > its inner vcpu halt emulation loop. > > > > > > Rename KVM_REQ_PENDING_TIMER to KVM_REQ_UNBLOCK, switch > > > PowerPC to arch specific request bit. > > > > > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > > > > > Index: kvm/include/linux/kvm_host.h > > > =================================================================== > > > --- kvm.orig/include/linux/kvm_host.h > > > +++ kvm/include/linux/kvm_host.h > > > @@ -146,7 +146,7 @@ static inline bool is_error_page(struct > > > */ > > > #define KVM_REQ_TLB_FLUSH (0 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) > > > #define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) > > > -#define KVM_REQ_PENDING_TIMER 2 > > > +#define KVM_REQ_UNBLOCK 2 > > > #define KVM_REQ_UNHALT 3 > > > #define KVM_REQUEST_ARCH_BASE 8 > > > > > > Index: kvm/virt/kvm/kvm_main.c > > > =================================================================== > > > --- kvm.orig/virt/kvm/kvm_main.c > > > +++ kvm/virt/kvm/kvm_main.c > > > @@ -2794,6 +2794,8 @@ static int kvm_vcpu_check_block(struct k > > > goto out; > > > if (signal_pending(current)) > > > goto out; > > > + if (kvm_check_request(KVM_REQ_UNBLOCK, vcpu)) > > > + goto out; > > > > > > ret = 0; > > > out: > > > Index: kvm/Documentation/virt/kvm/vcpu-requests.rst > > > =================================================================== > > > --- kvm.orig/Documentation/virt/kvm/vcpu-requests.rst > > > +++ kvm/Documentation/virt/kvm/vcpu-requests.rst > > > @@ -118,10 +118,11 @@ KVM_REQ_MMU_RELOAD > > > necessary to inform each VCPU to completely refresh the tables. This > > > request is used for that. > > > > > > -KVM_REQ_PENDING_TIMER > > > +KVM_REQ_UNBLOCK > > > > > > This request may be made from a timer handler run on the host on behalf > > > - of a VCPU. It informs the VCPU thread to inject a timer interrupt. > > > + of a VCPU, or when device assignment is performed. It informs the VCPU to > > > + exit the vcpu halt inner loop. > > > > > > KVM_REQ_UNHALT > > > > > > Index: kvm/arch/powerpc/include/asm/kvm_host.h > > > =================================================================== > > > --- kvm.orig/arch/powerpc/include/asm/kvm_host.h > > > +++ kvm/arch/powerpc/include/asm/kvm_host.h > > > @@ -51,6 +51,7 @@ > > > /* PPC-specific vcpu->requests bit members */ > > > #define KVM_REQ_WATCHDOG KVM_ARCH_REQ(0) > > > #define KVM_REQ_EPR_EXIT KVM_ARCH_REQ(1) > > > +#define KVM_REQ_PENDING_TIMER KVM_ARCH_REQ(2) > > > > > > #include <linux/mmu_notifier.h> > > > > > > Index: kvm/arch/x86/kvm/lapic.c > > > =================================================================== > > > --- kvm.orig/arch/x86/kvm/lapic.c > > > +++ kvm/arch/x86/kvm/lapic.c > > > @@ -1657,7 +1657,7 @@ static void apic_timer_expired(struct kv > > > } > > > > > > atomic_inc(&apic->lapic_timer.pending); > > > - kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu); > > > + kvm_make_request(KVM_REQ_UNBLOCK, vcpu); > > > if (from_timer_fn) > > > kvm_vcpu_kick(vcpu); > > > } > > > > Pure question on the existing code: why do we need kvm_make_request() for > > timer? As I see kvm_vcpu_check_block() already checks explicitly for timers: > > > > static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu) > > { > > ... > > if (kvm_cpu_has_pending_timer(vcpu)) > > goto out; > > ... > > } > > > > for x86: > > > > int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) > > { > > if (lapic_in_kernel(vcpu)) > > return apic_has_pending_timer(vcpu); > > > > return 0; > > } > > > > So wondering why we can drop the two references to KVM_REQ_PENDING_TIMER in x86 > > directly.. > > See commit 06e05645661211b9eaadaf6344c335d2e80f0ba2 I see, thanks Marcelo. Then we might have checked twice on timer pending in kvm_vcpu_check_block() for x86, as we also checks KVM_REQ_UNBLOCK now. Didn't think further on how to make it better, e.g. simply dropping kvm_cpu_has_pending_timer() won't work since it seems to still be useful for non-x86.. Then this patch looks good to me: Reviewed-by: Peter Xu <peterx@redhat.com>
On 25/05/21 21:52, Peter Xu wrote: > Didn't think further on how to make it better, e.g. simply dropping > kvm_cpu_has_pending_timer() won't work since it seems to still be > useful for non-x86.. It should be possible to use KVM_REQ_UNBLOCK in other architectures, and drop kvm_cpu_has_pending_timer(). Paolo
On 25/05/21 15:41, Marcelo Tosatti wrote: > -KVM_REQ_PENDING_TIMER > +KVM_REQ_UNBLOCK > > This request may be made from a timer handler run on the host on behalf > - of a VCPU. It informs the VCPU thread to inject a timer interrupt. > + of a VCPU, or when device assignment is performed. It informs the VCPU to > + exit the vcpu halt inner loop. > A small change: This request informs the vCPU to exit kvm_vcpu_block. It is used for example from timer handlers that run on the host on behalf of a vCPU, or in order to update the interrupt routing and ensure that assigned devices will wake up the vCPU. Paolo
Index: kvm/include/linux/kvm_host.h =================================================================== --- kvm.orig/include/linux/kvm_host.h +++ kvm/include/linux/kvm_host.h @@ -146,7 +146,7 @@ static inline bool is_error_page(struct */ #define KVM_REQ_TLB_FLUSH (0 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) #define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) -#define KVM_REQ_PENDING_TIMER 2 +#define KVM_REQ_UNBLOCK 2 #define KVM_REQ_UNHALT 3 #define KVM_REQUEST_ARCH_BASE 8 Index: kvm/virt/kvm/kvm_main.c =================================================================== --- kvm.orig/virt/kvm/kvm_main.c +++ kvm/virt/kvm/kvm_main.c @@ -2794,6 +2794,8 @@ static int kvm_vcpu_check_block(struct k goto out; if (signal_pending(current)) goto out; + if (kvm_check_request(KVM_REQ_UNBLOCK, vcpu)) + goto out; ret = 0; out: Index: kvm/Documentation/virt/kvm/vcpu-requests.rst =================================================================== --- kvm.orig/Documentation/virt/kvm/vcpu-requests.rst +++ kvm/Documentation/virt/kvm/vcpu-requests.rst @@ -118,10 +118,11 @@ KVM_REQ_MMU_RELOAD necessary to inform each VCPU to completely refresh the tables. This request is used for that. -KVM_REQ_PENDING_TIMER +KVM_REQ_UNBLOCK This request may be made from a timer handler run on the host on behalf - of a VCPU. It informs the VCPU thread to inject a timer interrupt. + of a VCPU, or when device assignment is performed. It informs the VCPU to + exit the vcpu halt inner loop. KVM_REQ_UNHALT Index: kvm/arch/powerpc/include/asm/kvm_host.h =================================================================== --- kvm.orig/arch/powerpc/include/asm/kvm_host.h +++ kvm/arch/powerpc/include/asm/kvm_host.h @@ -51,6 +51,7 @@ /* PPC-specific vcpu->requests bit members */ #define KVM_REQ_WATCHDOG KVM_ARCH_REQ(0) #define KVM_REQ_EPR_EXIT KVM_ARCH_REQ(1) +#define KVM_REQ_PENDING_TIMER KVM_ARCH_REQ(2) #include <linux/mmu_notifier.h> Index: kvm/arch/x86/kvm/lapic.c =================================================================== --- kvm.orig/arch/x86/kvm/lapic.c +++ kvm/arch/x86/kvm/lapic.c @@ -1657,7 +1657,7 @@ static void apic_timer_expired(struct kv } atomic_inc(&apic->lapic_timer.pending); - kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu); + kvm_make_request(KVM_REQ_UNBLOCK, vcpu); if (from_timer_fn) kvm_vcpu_kick(vcpu); } Index: kvm/arch/x86/kvm/x86.c =================================================================== --- kvm.orig/arch/x86/kvm/x86.c +++ kvm/arch/x86/kvm/x86.c @@ -9300,7 +9300,7 @@ static int vcpu_run(struct kvm_vcpu *vcp if (r <= 0) break; - kvm_clear_request(KVM_REQ_PENDING_TIMER, vcpu); + kvm_clear_request(KVM_REQ_UNBLOCK, vcpu); if (kvm_cpu_has_pending_timer(vcpu)) kvm_inject_pending_timer_irqs(vcpu);
KVM_REQ_UNBLOCK will be used to exit a vcpu from its inner vcpu halt emulation loop. Rename KVM_REQ_PENDING_TIMER to KVM_REQ_UNBLOCK, switch PowerPC to arch specific request bit. Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>