From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Egger Subject: Re: [PATCH] nestedhvm: ASID emulation Date: Fri, 15 Apr 2011 11:08:54 +0200 Message-ID: <4DA80B26.7060700@amd.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 04/15/11 11:05, Keir Fraser wrote: > > > > On 15/04/2011 09:20, "Christoph Egger" wrote: > >> On 04/14/11 16:43, Keir Fraser wrote: >>> On 14/04/2011 15:01, "Christoph Egger" wrote: >>> >>>>> What if some other vcpu's nv_n1asid or nv_n2asid got assigned the same HW >>>>> asid in this generation as this vcpu's (now stale, as it's from a previous >>>>> generation's) nv_n2asid? This PCPU can be interleaving execution of other >>>>> HVM VCPUs after all. >>>> >>>> I am not sure if I got you right. You mean what if two vcpus run on one >>>> physical cpu? In this case svm_do_resume() calls hvm_asid_flush_vcpu() >>>> before so that asid_generation and core_asid_generation do not match and >>>> a new asid is always assigned. >>> >>> No, it only does that if a given VCPU gets scheduled onto a *different* PCPU >>> than last time it ran. >>> >>> I've attached a mostly rewritten version of your patch that is about half >>> the size and I believe has a fighting chance of being correct (however it is >>> only build tested). Give it a look and a spin. >> >> Yes, it is correct. I like the idea to maintain a generation per asid. >> Please apply it. Thanks for this work. > > You didn't notice my subtle error in switching n1 and n2 asid selection in > svm_asid_handle_vmrun()? ;-) Oh, now that you mention it... A l2 smp guest actually boots up. Maybe I need to use more vcpus than I have physical cpus to hit that bug. I think, this snippet should be like this: struct vcpu *curr = current; struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb; struct hvm_vcpu_asid *curr_asid; bool_t need_flush; bool_t vcpu_guestmode = 0; if (nestedhvm_enabled(curr->domain) && nestedhvm_vcpu_in_guestmode(curr)) vcpu_guestmode = 1; curr_asid = vcpu_guestmode ? &curr->arch.hvm_vcpu.n2_asid : &curr->arch.hvm_vcpu.n1_asid; need_flush = hvm_asid_handle_vmenter(curr_asid); Christoph > Actually I bet it would take a lot of testing to pick up on that error, > since the transposition doesn't much matter except > that we are then switched round relative to which ASID gets flushed on > guest-initiated INVLPGA, and also we'd flush the 'wrong' ASID when guest > requests a new L2 guest ASID. Anyway, glad I spotted it before I checked the > patch in! Stale TLB bugs are no fun. > > -- Keir > >> Christoph >> > > > -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632