Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753882AbcD2O4e (ORCPT ); Fri, 29 Apr 2016 10:56:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34382 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753038AbcD2O4d (ORCPT ); Fri, 29 Apr 2016 10:56:33 -0400 Date: Fri, 29 Apr 2016 16:56:27 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Suravee Suthikulanit Cc: pbonzini@redhat.com, joro@8bytes.org, bp@alien8.de, gleb@kernel.org, alex.williamson@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, wei@redhat.com, sherry.hurwitz@amd.com Subject: Re: [PART1 RFC v4 08/11] svm: Add VMEXIT handlers for AVIC Message-ID: <20160429145627.GC15747@potion> References: <1460017232-17429-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1460017232-17429-9-git-send-email-Suravee.Suthikulpanit@amd.com> <20160412162245.GD6762@potion.brq.redhat.com> <572289D1.5040901@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <572289D1.5040901@amd.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3286 Lines: 85 2016-04-28 17:08-0500, Suravee Suthikulanit: > On 4/12/2016 11:22 AM, Radim Krčmář wrote: >> 2016-04-07 03:20-0500, Suravee Suthikulpanit: >> > From: Suravee Suthikulpanit >> > >> > This patch introduces VMEXIT handlers, avic_incomplete_ipi_interception() >> > and avic_unaccelerated_access_interception() along with two trace points >> > (trace_kvm_avic_incomplete_ipi and trace_kvm_avic_unaccelerated_access). >> > >> > Signed-off-by: Suravee Suthikulpanit >> > --- >> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> > @@ -3515,6 +3515,250 @@ static int mwait_interception(struct vcpu_svm *svm) >> > [...] >> > + lid = ffs(dlid) - 1; >> > + ret = avic_handle_ldr_write(&svm->vcpu, svm->vcpu.vcpu_id, lid); >> > + if (ret) >> > + return 0; >> >> OS can actually change LDR, so the old one should be invalidated. >> >> (No OS does, but that is not an important factor for the hypervisor.) >> > > By validating the old one, are you suggesting that we should disable the > logical APIC table entry previously used by this vcpu? If so, that means we > would need to cached the previous LDR value since the one in vAPIC backing > page would already be updated. Yes, the cache could be used to speed up recalculate_apic_map() too, so it might not be a total waste. Which reminds me that physical APIC_ID doesn't use correct cache. svm->vcpu.vcpu_id is only the initial ID, but APIC_ID could be changed more than once. It would be great to make APIC_ID read-only in all cases, because x86 spec allows us to do so, but KVM_SET_LAPIC can set APIC ID too and I think we don't retroactively modify userspace API ... Paolo? >> > [...] >> >> > + if (vm_data->ldr_mode != mod) { >> > + clear_page(page_address(vm_data->avic_logical_id_table_page)); >> > + vm_data->ldr_mode = mod; >> > + } >> > + break; >> > + } >> >> All these cases need to be called on KVM_SET_LAPIC -- the userspace can >> provide completely new set of APIC registers and AVIC should build its >> maps with them. Test with save/restore or migration. > > Hm.. This means we might need to introduce a new hook which is called from > the arch/x86/kvm/lapic.c: kvm_apic_post_state_restore(). Probably something > like kvm_x86_ops->apic_post_state_restore(), which sets up the new physical > and logical APIC id tables for AVIC. Sounds good. I imagine the callback would just call avic_unaccel_trap_write() for relevant registers. >> > + return ret; >> >> because we should not return, but continue to emulate the access. > > Then, this would continue as if we are handling the normal fault access. Exactly, it is a normal access to an undefined register. >> > + } >> > + >> > + if (trap) { >> > + /* Handling Trap */ >> > + if (!write) /* Trap read should never happens */ >> > + BUG(); >> >> (BUG_ON(!write) is shorter, though I would avoid BUG -- only guests are >> going to fail, so we don't need to kill the host.) > > Ok. What about just WARN_ONCE(!write, "svm: Handling trap read.\n"); Sure, it's a hardware bug and calling avic_unaccel_trap_write() on a read access shouldn't result in a bug. I am slightly inclined towards 'if (trap && write)' and optional 'WARN_ONCE(trap,' in the else branch. Thanks.