Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1350575 > unrolled thread

[PART1 RFC v2 00/10] KVM: x86: Introduce SVM AVIC support

Back to article view | Back to linux.kernel

Contents

  [PART1 RFC v2 00/10] KVM: x86: Introduce SVM AVIC support Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 21:50 +0100
    [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 21:50 +0100
      Re: [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers Paolo Bonzini <pbonzini@redhat.com> - 2016-03-07 16:50 +0100
        Re: [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-14 08:50 +0100
          Re: [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers Paolo Bonzini <pbonzini@redhat.com> - 2016-03-14 13:30 +0100
            Re: [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-15 14:00 +0100
    [PART1 RFC v2 08/10] svm: Do not expose x2APIC when enable AVIC Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 21:50 +0100
    [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 21:50 +0100
      Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Paolo Bonzini <pbonzini@redhat.com> - 2016-03-07 17:50 +0100
        Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-15 18:20 +0100
          Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Paolo Bonzini <pbonzini@redhat.com> - 2016-03-15 18:30 +0100
            Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-16 07:30 +0100
              Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Paolo Bonzini <pbonzini@redhat.com> - 2016-03-16 08:30 +0100
                Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-16 09:30 +0100
                  Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Paolo Bonzini <pbonzini@redhat.com> - 2016-03-16 12:20 +0100
    [PART1 RFC v2 01/10] KVM: x86: Misc LAPIC changes to exposes helper functions Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 22:00 +0100
    [PART1 RFC v2 04/10] svm: clean up V_TPR, V_IRQ, V_INTR_PRIO, and V_INTR_MASKING Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 22:00 +0100
    [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 22:00 +0100
      Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Paolo Bonzini <pbonzini@redhat.com> - 2016-03-07 17:00 +0100
        Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Radim Krčmář <rkrcmar@redhat.com> - 2016-03-08 23:10 +0100
          Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Paolo Bonzini <pbonzini@redhat.com> - 2016-03-09 12:00 +0100
      Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Radim Krčmář <rkrcmar@redhat.com> - 2016-03-09 22:00 +0100
        Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Radim Krčmář <rkrcmar@redhat.com> - 2016-03-10 20:40 +0100
          Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Paolo Bonzini <pbonzini@redhat.com> - 2016-03-10 21:00 +0100
            Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Radim Krčmář <rkrcmar@redhat.com> - 2016-03-10 21:50 +0100
        Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-17 05:00 +0100
          Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Paolo Bonzini <pbonzini@redhat.com> - 2016-03-17 10:40 +0100
        [PATCH] KVM: split kvm_vcpu_wake_up from kvm_vcpu_kick Radim Krčmář <rkrcmar@redhat.com> - 2016-03-17 21:30 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1353987 — Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC

On 08/03/2016 23:05, Radim Krčmář wrote:
>>> >> +	case AVIC_INCMP_IPI_ERR_INV_TARGET:
>>> >> +		pr_err("%s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",
>>> >> +		       __func__, icrh, icrl, index);
>>> >> +		BUG();
>>> >> +		break;
>>> >> +	case AVIC_INCMP_IPI_ERR_INV_BK_PAGE:
>>> >> +		pr_err("%s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
>>> >> +		       __func__, icrh, icrl, index);
>>> >> +		BUG();
>>> >> +		break;
>> > 
>> > Please use WARN(1, "%s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
>> > __func__, icrh, icrl, index) (and likewise for invalid target) instead
>> > of BUG().
> I think that if we hit one of these, then WARNs would just flood the
> log.  I'd prefer WARN_ONCE on AVIC_INCMP_IPI_ERR_INV_BK_PAGE.
> (Btw. aren't icr and idx are pointless on this error?  and the function
>  name should be printed by WARN.)

Agreed.

> Invalid target is triggerable by the guest (by sending IPI to a
> non-existent LAPIC), so warning log level seems too severe.
> pr_info_ratelimited() or nothing would be better.

Definitely should be nothing.

Paolo

[toc] | [prev] | [next] | [standalone]

#1354445 — Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC

2016-03-04 14:46-0600, Suravee Suthikulpanit:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Introduce VMEXIT handlers, avic_incp_ipi_interception() and
> avic_noaccel_interception().
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> @@ -3690,6 +3690,264 @@ static int mwait_interception(struct vcpu_svm *svm)
> +	case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN: {
> +		kvm_for_each_vcpu(i, vcpu, kvm) {
> +			if (!kvm_apic_match_dest(vcpu, apic,
> +						 icrl & APIC_SHORT_MASK,
> +						 GET_APIC_DEST_FIELD(icrh),
> +						 icrl & APIC_DEST_MASK))
> +				continue;
> +
> +			kvm_vcpu_kick(vcpu);

KVM shouldn't kick VCPUs that are running.  (Imagine a broadcast when
most VCPUs are in guest mode.)

I think a new helper might be useful here: we only want to wake up from
wait queue, but never force VCPU out of guest mode ... kvm_vcpu_kick()
does both.

> +static int avic_noaccel_trap_write(struct vcpu_svm *svm)
> +{
> +	switch (offset) {
> +	case APIC_ID: {
> +	case APIC_LDR: {
> +	case APIC_DFR: {
> +	}

It's not enough to modify the AVIC map here.  Userspace can also change
the APIC page with kvm_vcpu_ioctl_set_lapic, so AVIC would better hook
into some common path.

I think that AVIC map should be connected to recalculate_apic_map() and
'struct kvm_apic_map' as we already have the mode and a coupling of
LAPICs and VCPUs there.

recalculate_apic_map() is currently quite wasteful as it recomputes the
whole map on every change, but its simplicity should be bearable.

> +static int avic_noaccel_interception(struct vcpu_svm *svm)
> +{
> +	int ret = 0;
> +	u32 offset = svm->vmcb->control.exit_info_1 & 0xFF0;
> +	u32 rw = (svm->vmcb->control.exit_info_1 >> 32) & 0x1;

Change "u32 rw" to "bool write"

> +	u32 vector = svm->vmcb->control.exit_info_2 & 0xFFFFFFFF;

and please #define those masks.

> +	pr_debug("%s: offset=%#x, rw=%#x, vector=%#x, vcpu_id=%#x, cpu=%#x\n",
> +		 __func__, offset, rw, vector, svm->vcpu.vcpu_id, svm->vcpu.cpu);
> +
> +	BUG_ON(offset >= 0x400);

These are valid faulting registers, so our implementation has to handle
them.  (And the rule is to never BUG if a recovery is simple.)

> +	switch (offset) {
> +	case APIC_ID:
> +	case APIC_EOI:
> +	case APIC_RRR:
> +	case APIC_LDR:
> +	case APIC_DFR:
> +	case APIC_SPIV:
> +	case APIC_ESR:
> +	case APIC_ICR:
> +	case APIC_LVTT:
> +	case APIC_LVTTHMR:
> +	case APIC_LVTPC:
> +	case APIC_LVT0:
> +	case APIC_LVT1:
> +	case APIC_LVTERR:
> +	case APIC_TMICT:
> +	case APIC_TDCR: {

(Try a helper that returns true/false for trap/fault registers, the code
 might look nicer.)

> +		/* Handling Trap */
> +		if (!rw) /* Trap read should never happens */
> +			BUG();
> +		ret = avic_noaccel_trap_write(svm);
> +		break;
> +	}
> +	default: {
> +		/* Handling Fault */
> +		if (rw)
> +			ret = avic_noaccel_fault_write(svm);
> +		else
> +			ret = avic_noaccel_fault_read(svm);
> +		skip_emulated_instruction(&svm->vcpu);

AVIC doesn't tell us what it wanted to write, so KVM has to emulate the
instruction.

[toc] | [prev] | [next] | [standalone]

#1355345 — Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC

2016-03-09 21:55+0100, Radim Krčmář:
> 2016-03-04 14:46-0600, Suravee Suthikulpanit:
>> +static int avic_noaccel_interception(struct vcpu_svm *svm)
>> +{
>> +	int ret = 0;
>> +	u32 offset = svm->vmcb->control.exit_info_1 & 0xFF0;
>> +	u32 rw = (svm->vmcb->control.exit_info_1 >> 32) & 0x1;
> 
> Change "u32 rw" to "bool write"
> 
>> +	u32 vector = svm->vmcb->control.exit_info_2 & 0xFFFFFFFF;
> 
> and please #define those masks.

I reconsidered.  Other users are being removed, so these masks will be
used only once and properly named variables are better then.

vector isn't used right now ... is the EOI vector you get by reading the
APIC page equal to it?

(Btw. edge EOI hacks for PIT and RTC won't work because AVIC doesn't
 exit when TMR=0, but I'd take it slow and tackle those after v3.)

[toc] | [prev] | [next] | [standalone]

#1355354 — Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC

On 10/03/2016 20:34, Radim Krčmář wrote:
> (Btw. edge EOI hacks for PIT and RTC won't work because AVIC doesn't
>  exit when TMR=0, but I'd take it slow and tackle those after v3.)

I'd just only enable AVIC for split irqchip...

Paolo

[toc] | [prev] | [next] | [standalone]

#1355367 — Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC

2016-03-10 20:54+0100, Paolo Bonzini:
> On 10/03/2016 20:34, Radim Krčmář wrote:
>> (Btw. edge EOI hacks for PIT and RTC won't work because AVIC doesn't
>>  exit when TMR=0, but I'd take it slow and tackle those after v3.)
> 
> I'd just only enable AVIC for split irqchip...

Great idea.

[toc] | [prev] | [next] | [standalone]

#1359566 — Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC

Hi Radim,

On 03/10/2016 03:55 AM, Radim Krčmář wrote:
>> >+	pr_debug("%s: offset=%#x, rw=%#x, vector=%#x, vcpu_id=%#x, cpu=%#x\n",
>> >+		 __func__, offset, rw, vector, svm->vcpu.vcpu_id, svm->vcpu.cpu);
>> >+
>> >+	BUG_ON(offset >= 0x400);
> These are valid faulting registers, so our implementation has to handle
> them.  (And the rule is to never BUG if a recovery is simple.)
>

Just want to clarify the part that you mentioned "to handle them". 
IIUC, offet 0x400 and above are for x2APIC stuff, which AVIC does not 
currently support. Also, since I have only advertised as xAPIC when 
enabling AVIC, if we run into the situation that the VM is trying to 
access these register, we should just ignore it (and not BUG). Do I 
understand that correctly?

Thanks,
Suravee

[toc] | [prev] | [next] | [standalone]

#1359667 — Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC

On 17/03/2016 04:58, Suravee Suthikulpanit wrote:
>>>
>>> >+    BUG_ON(offset >= 0x400);
>> These are valid faulting registers, so our implementation has to handle
>> them.  (And the rule is to never BUG if a recovery is simple.)
>>
> 
> Just want to clarify the part that you mentioned "to handle them". IIUC,
> offet 0x400 and above are for x2APIC stuff, which AVIC does not
> currently support. Also, since I have only advertised as xAPIC when
> enabling AVIC, if we run into the situation that the VM is trying to
> access these register, we should just ignore it (and not BUG). Do I
> understand that correctly?

Yes.  You can add a printk(KERN_DEBUG) though.

Paolo

[toc] | [prev] | [next] | [standalone]

#1360173 — [PATCH] KVM: split kvm_vcpu_wake_up from kvm_vcpu_kick

2016-03-18 02:44+0700, Suravee Suthikulpanit:
> On 3/10/16 03:55, Radim Krčmář wrote:
>>2016-03-04 14:46-0600, Suravee Suthikulpanit:
>>>>From: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
>>>>
>>>>Introduce VMEXIT handlers, avic_incp_ipi_interception() and
>>>>avic_noaccel_interception().
>>>>
>>>>Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
>>>>---
>>>>diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>>>@@ -3690,6 +3690,264 @@ static int mwait_interception(struct vcpu_svm *svm)
>>>>+	case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN: {
>>>>+		kvm_for_each_vcpu(i, vcpu, kvm) {
>>>>+			if (!kvm_apic_match_dest(vcpu, apic,
>>>>+						 icrl & APIC_SHORT_MASK,
>>>>+						 GET_APIC_DEST_FIELD(icrh),
>>>>+						 icrl & APIC_DEST_MASK))
>>>>+				continue;
>>>>+
>>>>+			kvm_vcpu_kick(vcpu);
>>KVM shouldn't kick VCPUs that are running.  (Imagine a broadcast when
>>most VCPUs are in guest mode.)
> 
> So, besides checking if the vcpu match the destination, I will add the check
> to see if the is_running bit is set before calling kvm_vcpu_kick()

That will do.

>>I think a new helper might be useful here: we only want to wake up from
>>wait queue, but never force VCPU out of guest mode ... kvm_vcpu_kick()
>>does both.
> 
> If I only kick non-running vcpu, do I still need this new helper function?

I would prefer it.  It's a minor performance optimization (non-running
VCPUs aren't in guest mode) and makes our intent clear.  Please use
include the following patch and use kvm_vcpu_wake_up instead.

---8<---
AVIC has a use for kvm_vcpu_wake_up.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 include/linux/kvm_host.h |  1 +
 virt/kvm/kvm_main.c      | 19 +++++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 861f690aa791..7b269626a3b3 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -650,6 +650,7 @@ void kvm_vcpu_mark_page_dirty(struct kvm_vcpu *vcpu, gfn_t gfn);
 void kvm_vcpu_block(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu);
+void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu);
 void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
 int kvm_vcpu_yield_to(struct kvm_vcpu *target);
 void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 1eae05236347..c39c54afdb74 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2054,13 +2054,8 @@ out:
 EXPORT_SYMBOL_GPL(kvm_vcpu_block);
 
 #ifndef CONFIG_S390
-/*
- * Kick a sleeping VCPU, or a guest VCPU in guest mode, into host kernel mode.
- */
-void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
+void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
 {
-	int me;
-	int cpu = vcpu->cpu;
 	wait_queue_head_t *wqp;
 
 	wqp = kvm_arch_vcpu_wq(vcpu);
@@ -2068,6 +2063,18 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 		wake_up_interruptible(wqp);
 		++vcpu->stat.halt_wakeup;
 	}
+}
+EXPORT_SYMBOL_GPL(kvm_vcpu_wake_up);
+
+/*
+ * Kick a sleeping VCPU, or a guest VCPU in guest mode, into host kernel mode.
+ */
+void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
+{
+	int me;
+	int cpu = vcpu->cpu;
+
+	kvm_vcpu_wake_up(vcpu);
 
 	me = get_cpu();
 	if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
-- 
2.7.2

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web