From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PART1 RFC v4 08/11] svm: Add VMEXIT handlers for AVIC Date: Fri, 29 Apr 2016 16:56:27 +0200 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-Transfer-Encoding: QUOTED-PRINTABLE 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 To: Suravee Suthikulanit Return-path: Content-Disposition: inline In-Reply-To: <572289D1.5040901@amd.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 2016-04-28 17:08-0500, Suravee Suthikulanit: > On 4/12/2016 11:22 AM, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: >> 2016-04-07 03:20-0500, Suravee Suthikulpanit: >> > From: Suravee Suthikulpanit >> >=20 >> > This patch introduces VMEXIT handlers, avic_incomplete_ipi_interce= ption() >> > and avic_unaccelerated_access_interception() along with two trace = points >> > (trace_kvm_avic_incomplete_ipi and trace_kvm_avic_unaccelerated_ac= cess). >> >=20 >> > 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 =3D ffs(dlid) - 1; >> > + ret =3D avic_handle_ldr_write(&svm->vcpu, svm->vcpu.vcpu_id, li= d); >> > + if (ret) >> > + return 0; >>=20 >> OS can actually change LDR, so the old one should be invalidated. >>=20 >> (No OS does, but that is not an important factor for the hypervisor.= ) >>=20 >=20 > 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 me= ans we > would need to cached the previous LDR value since the one in vAPIC ba= cking > 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? >> > [...] >>=20 >> > + if (vm_data->ldr_mode !=3D mod) { >> > + clear_page(page_address(vm_data->avic_logical_id_table_page)); >> > + vm_data->ldr_mode =3D mod; >> > + } >> > + break; >> > + } >>=20 >> 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 i= ts >> maps with them. Test with save/restore or migration. >=20 > 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 som= ething > like kvm_x86_ops->apic_post_state_restore(), which sets up the new ph= ysical > 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; >>=20 >> because we should not return, but continue to emulate the access. >=20 > Then, this would continue as if we are handling the normal fault acce= ss. Exactly, it is a normal access to an undefined register. >> > + } >> > + >> > + if (trap) { >> > + /* Handling Trap */ >> > + if (!write) /* Trap read should never happens */ >> > + BUG(); >>=20 >> (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.) >=20 > 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.