From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [V9 PATCH 7/8] pvh dom0: check for vioapic null ptr in vioapic_range Date: Tue, 22 Apr 2014 17:11:59 -0700 Message-ID: <20140422171159.61230575@mantra.us.oracle.com> References: <1397607172-32065-1-git-send-email-mukesh.rathor@oracle.com> <1397607172-32065-8-git-send-email-mukesh.rathor@oracle.com> <534EC6850200007800009B08@nat28.tlf.novell.com> <20140416184448.25c62fb1@mantra.us.oracle.com> <534F96DF0200007800009D57@nat28.tlf.novell.com> <20140421175932.3f93d0ee@mantra.us.oracle.com> <53563769020000780000A8FD@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WcknN-0007tM-Ho for xen-devel@lists.xenproject.org; Wed, 23 Apr 2014 00:12:13 +0000 In-Reply-To: <53563769020000780000A8FD@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: George.Dunlap@eu.citrix.com, tim@xen.org, eddie.dong@intel.com, keir.xen@gmail.com, jun.nakajima@intel.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On Tue, 22 Apr 2014 08:33:29 +0100 "Jan Beulich" wrote: > >>> On 22.04.14 at 02:59, wrote: > > On Thu, 17 Apr 2014 07:54:55 +0100 > > "Jan Beulich" wrote: ....... > I think I'm getting the idea, but the code neither refernces > pvh_mmio_handlers[], nor is that array's initialization well done > (should be using . = style instead, omitting the > NULLs). oops, forgot to add the if pvh check. > >> That aside - why is this coming up only now? The emulation path > >> getting reached shouldn't really depend on Dom0 vs Domu? > > > > The io emulation is handled by handle_pvh_io; there shouldn't be > > path for pvh domu leading to this function with all the > > restrictions and limitations it has at present. > > In which case we're back to the initial question: Why is this patch > needed in the first place? If there is a separate emulation path > already, how do we manage to get to the point where you added the > extra check? As described in the patch description: ----- pvh doesn't use apic emulation, as a result vioapic_init is not called and vioapic ptr in struct hvm_domain is not initialized. One path that would access the ptr for pvh is : hvm_hap_nested_page_fault -> handle_mmio -> hvmemul_do_io -> hvm_mmio_intercept -> vioapic_range ----- The only caller of hvm_hap_nested_page_fault is ept_handle_violation. Now, 3 calls to handle_mmio in hvm_hap_nested_page_fault: 1st is for nested vcpu, so doesn't apply to PVH. 2nd has is_hvm check, So it must have been the third one that I had observed the vioapic_range crash in a while ago, and had made note of it. Looking at it: if ( (p2mt == p2m_mmio_dm) || (access_w && (p2mt == p2m_ram_ro)) ) { put_gfn(p2m->domain, gfn); if ( !handle_mmio() ) doesn't seem apply to domu. Unfortunately, I can't reproduce it now so maybe it was an ept violation due to some bug, and a crash in vioapic_range before printing the gfn/mfns etc by ept_handle_violation made me make a note to put a check in it. Hope that makes sense, and I'll assume you are ok with pvh_mmio_handlers[] change. Otherwise, please lmk. thanks mukesh