From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= Subject: Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests Date: Mon, 7 Apr 2014 16:40:04 +0200 Message-ID: <5342B8C4.5030800@citrix.com> References: <1396622254-13069-1-git-send-email-roger.pau@citrix.com> <1396622254-13069-2-git-send-email-roger.pau@citrix.com> <533EED620200007800005C88@nat28.tlf.novell.com> <5342915E.1090900@citrix.com> <5342B073020000780000624B@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 1WXAiu-0008A8-DE for xen-devel@lists.xenproject.org; Mon, 07 Apr 2014 14:40:32 +0000 In-Reply-To: <5342B073020000780000624B@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: xen-devel@lists.xenproject.org, Keir Fraser List-Id: xen-devel@lists.xenproject.org On 07/04/14 14:04, Jan Beulich wrote: >>>> On 07.04.14 at 13:51, wrote: >> On 04/04/14 17:35, Jan Beulich wrote: >>>>>> On 04.04.14 at 16:37, wrote: >>>> --- a/xen/common/grant_table.c >>>> +++ b/xen/common/grant_table.c >>>> @@ -749,6 +749,22 @@ __gnttab_map_grant_ref( >>>> goto undo_out; >>>> } >>>> } >>>> + else if ( need_iommu(ld) ) >>>> + { >>>> + int err; >>>> + >>>> + BUG_ON(!paging_mode_translate(ld)); >>>> + err = iommu_map_page(ld, op->host_addr >> PAGE_SHIFT, frame, >>>> + op->flags & GNTMAP_readonly ? >>>> + IOMMUF_readable : >>>> + IOMMUF_readable|IOMMUF_writable); >>>> + if ( err ) >>>> + { >>>> + double_gt_unlock(lgt, rgt); >>>> + rc = GNTST_general_error; >>>> + goto undo_out; >>>> + } >>>> + } >>> >>> As much of this as possible should be folded with the if() branch. >>> And looking at the PV code, I think it makes no sense for the >>> conditions whether to map the page r/o or r/w to be different >>> between PV and non-PV. >>> >>> Plus - wouldn't it be better to have this taken care of via >>> create_grant_host_mapping(), by not blindly calling >>> iommu_unmap_page() on anything other than p2m_ram_rw in >>> ept_set_entry() and p2m_set_entry(), the more that this should >>> already be taken care of in the iommu_hap_pt_share case. >> >> Thanks for the comment, it indeed makes much more sense to fix this in >> ept_set_entry/p2m_set_entry, the following patch adds IOMMU support for >> all page types except p2m_mmio_direct (when the hap memory map is not >> shared with IOMMU): > > This seems to be going a little too far - I'm not sure we want to include > all types here: p2m_mmio_dm, p2m_ram_paging_*, p2m_ram_paged, > and p2m_ram_broken all might require different treatment. Please do > this via some sort of switch() setting the permissions instead, calling > iommu_unmap_page() when the permissions remain zero. Right, I was looking at the wrong p2m.h header (the ARM one), which has a more limited set of p2m types. I'm not sure if the type p2m_ram_shared should be given an IOMMU read-only entry, that's what I've done in the patch below. Also, there's a comment on the top of p2m.h that worries me: /* * AMD IOMMU: When we share p2m table with iommu, bit 52 -bit 58 in pte * cannot be non-zero, otherwise, hardware generates io page faults when * device access those pages. Therefore, p2m_ram_rw has to be defined as 0. */ Does this mean that on AMD if the map is shared between HAP and IOMMU, the only page type accessible from the IOMMU would be p2m_ram_rw? If so, that should be fixed also (probably by setting iommu_hap_pt_share = 0 on AMD hardware). --- diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index beb7288..993d11a 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -475,18 +475,37 @@ out: iommu_pte_flush(d, gfn, (u64*)ept_entry, order, vtd_pte_present); else { - if ( p2mt == p2m_ram_rw ) + unsigned int flags; + + switch( p2mt ) + { + case p2m_ram_rw: + case p2m_grant_map_rw: + case p2m_map_foreign: + flags = IOMMUF_readable | IOMMUF_writable; + break; + case p2m_ram_ro: + case p2m_grant_map_ro: + case p2m_ram_shared: + flags = IOMMUF_readable; + break; + default: + flags = 0; + break; + } + + if ( flags != 0 ) { if ( order > 0 ) { for ( i = 0; i < (1 << order); i++ ) iommu_map_page( p2m->domain, gfn - offset + i, mfn_x(mfn) - offset + i, - IOMMUF_readable | IOMMUF_writable); + flags); } else if ( !order ) iommu_map_page( - p2m->domain, gfn, mfn_x(mfn), IOMMUF_readable | IOMMUF_writable); + p2m->domain, gfn, mfn_x(mfn), flags); } else { diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index 766fd67..33addf9 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -450,10 +450,28 @@ p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, } else { - if ( p2mt == p2m_ram_rw ) + unsigned int flags; + + switch( p2mt ) + { + case p2m_ram_rw: + case p2m_grant_map_rw: + case p2m_map_foreign: + flags = IOMMUF_readable | IOMMUF_writable; + break; + case p2m_ram_ro: + case p2m_grant_map_ro: + case p2m_ram_shared: + flags = IOMMUF_readable; + break; + default: + flags = 0; + break; + } + + if ( flags != 0 ) for ( i = 0; i < (1UL << page_order); i++ ) - iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, - IOMMUF_readable|IOMMUF_writable); + iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, flags); else for ( int i = 0; i < (1UL << page_order); i++ ) iommu_unmap_page(p2m->domain, gfn+i);