From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anthony Liguori Subject: Re: [PATCH 4/4][VTD] vt-d specific files in KVM Date: Tue, 10 Jun 2008 10:02:45 -0500 Message-ID: <484E9795.4010409@codemonkey.ws> References: <1FE6DD409037234FAB833C420AA843EC018831D5@orsmsx424.amr.corp.intel.com> <20080610102759.GG7307@il.ibm.com> <484E8EFC.10007@codemonkey.ws> <20080610145638.GF6702@il.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: "Kay, Allen M" , kvm@vger.kernel.org, Amit Shah , Ben-Ami Yassour1 , Avi Kivity , Chris Wright , "Han, Weidong" To: Muli Ben-Yehuda Return-path: Received: from yx-out-2324.google.com ([74.125.44.30]:28800 "EHLO yx-out-2324.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753875AbYFJPDC (ORCPT ); Tue, 10 Jun 2008 11:03:02 -0400 Received: by yx-out-2324.google.com with SMTP id 31so251682yxl.1 for ; Tue, 10 Jun 2008 08:03:01 -0700 (PDT) In-Reply-To: <20080610145638.GF6702@il.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: Muli Ben-Yehuda wrote: > On Tue, Jun 10, 2008 at 09:26:04AM -0500, Anthony Liguori wrote: > > >>> Checking against pfn_valid() isn't enough to differentiate between >>> RAM and MMIO areas. I think the consensus was that we also need to >>> check PageReserved(), i.e., >>> >>> if (pfn_valid(pfn) && !PageReserved(pfn_to_page(pfn))) ... >>> >>> >> When checking the error return of gfn_to_pfn(), you should use >> is_error_pfn(). There's no need to differentiate mmio/ram pages in >> the code, the goal is just error checking. >> > > I'd have to check the exact semantics of is_error_pfn() to see if it > fits, since strictly speaking what we are doing is not checking > pfn_to_page() for errors. We need to differentiate between gfns which > represent RAM (which needs to be mapped into the VT-d page tables) and > gfns which don't (e.g, slots which represent an MMIO region), which > should not be mapped in the VT-d page tables. > Why? Wouldn't MMIO pages have to be mapped in the VT-d page table in order to support pass-through? It certainly can't hurt, can it? At any rate, looking at the code again, the else clause is: > + printk(KERN_DEBUG "kvm_iommu_map_page:" > + "invalid pfn=%lx\n", pfn); > + return 0; Which looks like error handling to me. I don't think it's at all safe to assume that a slot is either entirely MMIO or entirely RAM. You could very easily construct a slot that's a mix of both so if this is an attempt to skip MMIO slots, it's broken. Regards, Anthony Liguori > Cheers, > Muli > >