Re: [PART1 RFC 5/9] svm: Add VMEXIT handlers for AVIC

From: "Radim Krčmář" <rkrcmar@redhat.com>
To: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	joro@8bytes.org, alex.williamson@redhat.com, gleb@kernel.org,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	wei@redhat.com, sherry.hurwitz@amd.com
Subject: Re: [PART1 RFC 5/9] svm: Add VMEXIT handlers for AVIC
Date: Thu, 18 Feb 2016 15:18:18 +0100	[thread overview]
Message-ID: <20160218141817.GA6289@potion.brq.redhat.com> (raw)
In-Reply-To: <56C52B80.5050104@amd.com>

2016-02-18 09:25+0700, Suravee Suthikulpanit:
> On 2/17/16 01:06, Radim Krčmář wrote:
>>2016-02-16 17:56+0100, Paolo Bonzini:
>>>>On 16/02/2016 15:13, Radim Krčmář wrote:
>>>>>>Yeah, I think atomic there means that it won't race with other writes to
>>>>>>the same byte in IRR.  We're fine as long as AVIC writes IRR before
>>>>>>checking IsRunning on every destination, which it seems to be.
>>>>
>>>>More precisely, if AVIC writes all IRRs (5.1) and ANDs all IsRunning
>>>>flags before checking the result of the AND (6).
>>>>
>>>>>>(It would, but I believe that AVIC designers made it sane and the spec
>>>>>>  doesn't let me read it in a way that supports your theories.)
>>>>
>>>>I hope so as well, and you've probably convinced me.  But I still think
>>>>the code is wrong in this patch.  Let's look at the spec that you pasted:
>>The code definitely is wrong.  I'll be more specific when disagreeing,
>>sorry.
>>
> 
> Would you please be a bit more specific on what you think I am not doing
> correctly to handle the #VMEXIT in the case of target not running below.
> 
> +    case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN:
> +        kvm_lapic_reg_write(apic, APIC_ICR2, icrh);
> +        kvm_lapic_reg_write(apic, APIC_ICR, icrl);
> 
> This is actually not just writing to the register. Please note that writing
> to APIC_ICR register would also be calling apic_send_ipi(), which results in
> injecting interrupts to the target core:

Exactly.  Injecting the interrupt in AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN
handler is causing the double-injection bug that Paolo described.

> Am I missing something?

Probably that AVIC already wrote to all IRRs (and sent appropriate
doorbells) before this VMEXIT, so KVM shouldn't repeat it.

KVM just has to make sure that targeted VCPUs notice the interrupt,
which means to kick (wake up) VCPUs that don't have IsRunning set.
There is no need to do anything with running VCPUs, because they
 - are in guest mode and noticed the doorbell
 - are in host mode, where they will
   1) VMRUN as fast as they can because the VCPU didn't want to halt
      (and IRR is handled on VMRUN)
   2) check IRR after unsetting IsRunning and goto (1) if there are
      pending interrupts.  (RFC doesn't do this, which is another bug)

It's still possible that we misunderstood the spec.  Does AVIC handle
IPIs differently?

Thanks.