Re: [PATCH 4/6] kvm: lapic: Add apicv activate/deactivate helper function

From: "Suthikulpanit, Suravee" <Suravee.Suthikulpanit@amd.com>
To: "Jan H. Schönherr" <jschoenh@amazon.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>
Cc: "joro@8bytes.org" <joro@8bytes.org>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"rkrcmar@redhat.com" <rkrcmar@redhat.com>
Subject: Re: [PATCH 4/6] kvm: lapic: Add apicv activate/deactivate helper function
Date: Mon, 15 Jul 2019 22:35:00 +0000	[thread overview]
Message-ID: <499a4947-bb65-a0ea-cf16-088a3a4b1ebd@amd.com> (raw)
In-Reply-To: <813c8c15-af74-6d77-4a30-2e0ee4b05006@amazon.de>

Jan,

On 5/8/2019 5:27 PM, Jan H. Schönherr wrote:
> On 22/03/2019 12.57, Suthikulpanit, Suravee wrote:
>> Introduce a helper function for setting lapic parameters when
>> activate/deactivate apicv.
>>
>> Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
>> ---
>>   arch/x86/kvm/lapic.c | 23 ++++++++++++++++++-----
>>   arch/x86/kvm/lapic.h |  1 +
>>   2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 95295cf81283..9c5cd1a98928 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -2126,6 +2126,22 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
>>
>>   }
>>
>> +void kvm_apic_update_apicv(struct kvm_vcpu *vcpu)
>> +{
>> +     struct kvm_lapic *apic = vcpu->arch.apic;
>> +     bool active = vcpu->arch.apicv_active;
>> +
>> +     if (active) {
>> +             /* irr_pending is always true when apicv is activated. */
>> +             apic->irr_pending = true;
>> +             apic->isr_count = 1;
>> +     } else {
>> +             apic->irr_pending = !!count_vectors(apic->regs + APIC_IRR);
> What about:
>                  apic->irr_pending = apic_search_irr(apic) != -1;
> instead? (more in line with the logic in apic_clear_irr())

Sure, that works also.

> Related to this, I wonder if we need to ensure to execute
> kvm_x86_ops->sync_pir_to_irr() just before apicv_active transitions
> from true to false, so that we don't miss an interrupt and irr_pending
> is set correctly in this function (on Intel at least).

I'm not sure about the PIR on Intel, but AMD since AMD IOMMU hardware
directly updates the vAPIC backing page, the driver should not need
to worry.

> Hmm... there seems to be other stuff as well, that depends on
> vcpu->arch.apicv_active, which is not updated on a transition. For
> example: posted interrupts, 

You are right. We need to also handle the posted-interrupt
activate/deactivate. I am also doing this in V2.

> CR8 intercept, 

When APICv is deactivated, the update_cr8_intercept() should handle
updating of CR8 interception.

> and a potential asymmetry via avic_vcpu_load()/avic_vcpu_put()
> because the bottom half of just one of the two functions may be
> skipped
Not sure if I understand the concern that you mentioned here.
However, once we add the IOMMU activation/deactivation code for
posted-interrupt, we should not need to worry about calling
avic_update_iommu_vcpu_affinity() at the end of the functions
here.

Thanks,
Suravee
> Regards
> Jan
> 