* [PATCH RFC 0/2] Fix grant map/unmap with auto-translated guests @ 2014-04-04 14:37 Roger Pau Monne 2014-04-04 14:37 ` [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on " Roger Pau Monne 2014-04-04 14:37 ` [PATCH RFC 2/2] xen: expose that grant table mappings update the IOMMU Roger Pau Monne 0 siblings, 2 replies; 9+ messages in thread From: Roger Pau Monne @ 2014-04-04 14:37 UTC (permalink / raw) To: xen-devel This series adds proper IOMMU entries when performing grant mapping/unmapping inside of auto-translated guests. In order for the guest to know if it's safe to use grant mapped pages for IO with hardware devices a new xen feature is introduced. [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on [PATCH RFC 2/2] xen: expose that grant table mappings update the ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests 2014-04-04 14:37 [PATCH RFC 0/2] Fix grant map/unmap with auto-translated guests Roger Pau Monne @ 2014-04-04 14:37 ` Roger Pau Monne 2014-04-04 15:35 ` Jan Beulich 2014-04-04 14:37 ` [PATCH RFC 2/2] xen: expose that grant table mappings update the IOMMU Roger Pau Monne 1 sibling, 1 reply; 9+ messages in thread From: Roger Pau Monne @ 2014-04-04 14:37 UTC (permalink / raw) To: xen-devel; +Cc: Keir Fraser, Jan Beulich, Roger Pau Monne When using grant mappings with auto-translated guests (PVH, HVM), Xen was not updating the IOMMU entries for the domain. This patch adds the necessary bits in order to properly update the IOMMU when doing grant maps/unmap on auto-translated guests. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Cc: Mukesh Rathor <mukesh.rathor@oracle.com> Cc: Keir Fraser <keir@xen.org> Cc: Jan Beulich <jbeulich@suse.com> --- xen/common/grant_table.c | 28 ++++++++++++++++++++++++++++ 1 files changed, 28 insertions(+), 0 deletions(-) diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 107b000..665f34b 100644 --- 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; + } + } TRACE_1D(TRC_MEM_PAGE_GRANT_MAP, op->dom); @@ -947,6 +963,18 @@ __gnttab_unmap_common( goto unmap_out; } } + else if ( need_iommu(ld) ) + { + int err; + + BUG_ON(!paging_mode_translate(ld)); + err = iommu_unmap_page(ld, op->host_addr >> PAGE_SHIFT); + if ( err ) + { + rc = GNTST_general_error; + goto unmap_out; + } + } /* If just unmapped a writable mapping, mark as dirtied */ if ( !(op->flags & GNTMAP_readonly) ) -- 1.7.7.5 (Apple Git-26) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests 2014-04-04 14:37 ` [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on " Roger Pau Monne @ 2014-04-04 15:35 ` Jan Beulich 2014-04-07 11:51 ` Roger Pau Monné 0 siblings, 1 reply; 9+ messages in thread From: Jan Beulich @ 2014-04-04 15:35 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel, Keir Fraser >>> On 04.04.14 at 16:37, <roger.pau@citrix.com> 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. > @@ -947,6 +963,18 @@ __gnttab_unmap_common( > goto unmap_out; > } > } > + else if ( need_iommu(ld) ) > + { > + int err; > + > + BUG_ON(!paging_mode_translate(ld)); > + err = iommu_unmap_page(ld, op->host_addr >> PAGE_SHIFT); > + if ( err ) > + { > + rc = GNTST_general_error; > + goto unmap_out; > + } > + } Same thing here obviously. Jan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests 2014-04-04 15:35 ` Jan Beulich @ 2014-04-07 11:51 ` Roger Pau Monné 2014-04-07 12:04 ` Jan Beulich 0 siblings, 1 reply; 9+ messages in thread From: Roger Pau Monné @ 2014-04-07 11:51 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Keir Fraser On 04/04/14 17:35, Jan Beulich wrote: >>>> On 04.04.14 at 16:37, <roger.pau@citrix.com> 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): --- diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index beb7288..611c8e2 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -475,18 +475,22 @@ out: iommu_pte_flush(d, gfn, (u64*)ept_entry, order, vtd_pte_present); else { - if ( p2mt == p2m_ram_rw ) + if ( (p2mt != p2m_invalid) && (p2mt != p2m_mmio_direct) ) { + unsigned int flags; + + flags = ( (p2mt == p2m_grant_map_ro) || (p2mt == p2m_ram_ro) ) ? + IOMMUF_readable : IOMMUF_readable | IOMMUF_writable; 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..c564cb6 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -450,11 +450,15 @@ p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, } else { - if ( p2mt == p2m_ram_rw ) + if ( (p2mt != p2m_invalid) && (p2mt != p2m_mmio_direct) ) + { + unsigned int flags; + + flags = ( (p2mt == p2m_grant_map_ro) || (p2mt == p2m_ram_ro) ) ? + IOMMUF_readable : IOMMUF_readable | IOMMUF_writable; for ( i = 0; i < (1UL << page_order); i++ ) - iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, - IOMMUF_readable|IOMMUF_writable); - else + 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); } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests 2014-04-07 11:51 ` Roger Pau Monné @ 2014-04-07 12:04 ` Jan Beulich 2014-04-07 14:40 ` Roger Pau Monné 0 siblings, 1 reply; 9+ messages in thread From: Jan Beulich @ 2014-04-07 12:04 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Keir Fraser >>> On 07.04.14 at 13:51, <roger.pau@citrix.com> wrote: > On 04/04/14 17:35, Jan Beulich wrote: >>>>> On 04.04.14 at 16:37, <roger.pau@citrix.com> 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. Jan > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -475,18 +475,22 @@ out: > iommu_pte_flush(d, gfn, (u64*)ept_entry, order, > vtd_pte_present); > else > { > - if ( p2mt == p2m_ram_rw ) > + if ( (p2mt != p2m_invalid) && (p2mt != p2m_mmio_direct) ) > { > + unsigned int flags; > + > + flags = ( (p2mt == p2m_grant_map_ro) || (p2mt == > p2m_ram_ro) ) ? > + IOMMUF_readable : IOMMUF_readable | > IOMMUF_writable; > 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..c564cb6 100644 > --- a/xen/arch/x86/mm/p2m-pt.c > +++ b/xen/arch/x86/mm/p2m-pt.c > @@ -450,11 +450,15 @@ p2m_set_entry(struct p2m_domain *p2m, unsigned long > gfn, mfn_t mfn, > } > else > { > - if ( p2mt == p2m_ram_rw ) > + if ( (p2mt != p2m_invalid) && (p2mt != p2m_mmio_direct) ) > + { > + unsigned int flags; > + > + flags = ( (p2mt == p2m_grant_map_ro) || (p2mt == > p2m_ram_ro) ) ? > + IOMMUF_readable : IOMMUF_readable | > IOMMUF_writable; > for ( i = 0; i < (1UL << page_order); i++ ) > - iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, > - IOMMUF_readable|IOMMUF_writable); > - else > + 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); > } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests 2014-04-07 12:04 ` Jan Beulich @ 2014-04-07 14:40 ` Roger Pau Monné 2014-04-07 15:18 ` Jan Beulich 0 siblings, 1 reply; 9+ messages in thread From: Roger Pau Monné @ 2014-04-07 14:40 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Keir Fraser On 07/04/14 14:04, Jan Beulich wrote: >>>> On 07.04.14 at 13:51, <roger.pau@citrix.com> wrote: >> On 04/04/14 17:35, Jan Beulich wrote: >>>>>> On 04.04.14 at 16:37, <roger.pau@citrix.com> 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); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests 2014-04-07 14:40 ` Roger Pau Monné @ 2014-04-07 15:18 ` Jan Beulich 2014-04-07 15:28 ` Andres Lagar-Cavilla 0 siblings, 1 reply; 9+ messages in thread From: Jan Beulich @ 2014-04-07 15:18 UTC (permalink / raw) To: Roger Pau Monné Cc: xen-devel, Keir Fraser, Andres Lagar-Cavilla, Tim Deegan >>> On 07.04.14 at 16:40, <roger.pau@citrix.com> wrote: > On 07/04/14 14:04, Jan Beulich wrote: >>>>> On 07.04.14 at 13:51, <roger.pau@citrix.com> wrote: >>> On 04/04/14 17:35, Jan Beulich wrote: >>>>>>> On 04.04.14 at 16:37, <roger.pau@citrix.com> 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. I'm not sure about the right behavior there too, hence copying Tim and Andres. However, I'm told page sharing doesn't work when an IOMMU is in use by a domain anyway, so not handling this case properly wouldn't have any bad consequences for now. > 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 Yes, that does mean exactly that. > so, that should be fixed also (probably by setting iommu_hap_pt_share = > 0 on AMD hardware). We're considering to drop the sharing altogether anyway, and are anxiously awaiting Intel to fix their VT-d code for this to not have an otherwise expected performance impact. Jan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests 2014-04-07 15:18 ` Jan Beulich @ 2014-04-07 15:28 ` Andres Lagar-Cavilla 0 siblings, 0 replies; 9+ messages in thread From: Andres Lagar-Cavilla @ 2014-04-07 15:28 UTC (permalink / raw) To: Jan Beulich Cc: Keir Fraser, Tim Deegan, Andres Lagar-Cavilla, xen-devel, "Roger Pau Monné" On Apr 7, 2014, at 11:18 AM, "Jan Beulich" <JBeulich@suse.com> wrote: >>>> On 07.04.14 at 16:40, <roger.pau@citrix.com> wrote: >> On 07/04/14 14:04, Jan Beulich wrote: >>>>>> On 07.04.14 at 13:51, <roger.pau@citrix.com> wrote: >>>> On 04/04/14 17:35, Jan Beulich wrote: >>>>>>>> On 04.04.14 at 16:37, <roger.pau@citrix.com> 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. > > I'm not sure about the right behavior there too, hence copying Tim > and Andres. However, I'm told page sharing doesn't work when an > IOMMU is in use by a domain anyway, so not handling this case > properly wouldn't have any bad consequences for now. IIRC we put a big interlock XORing sharing/paging/pod and IOMMU. Once the IOMMU thinks it can do DMA you can't mutate that p2m region willy-nilly. So easy path for now is to impose mutual exclusion between those features. Andres > >> 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 > > Yes, that does mean exactly that. > >> so, that should be fixed also (probably by setting iommu_hap_pt_share = >> 0 on AMD hardware). > > We're considering to drop the sharing altogether anyway, and are > anxiously awaiting Intel to fix their VT-d code for this to not have an > otherwise expected performance impact. > > Jan > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH RFC 2/2] xen: expose that grant table mappings update the IOMMU 2014-04-04 14:37 [PATCH RFC 0/2] Fix grant map/unmap with auto-translated guests Roger Pau Monne 2014-04-04 14:37 ` [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on " Roger Pau Monne @ 2014-04-04 14:37 ` Roger Pau Monne 1 sibling, 0 replies; 9+ messages in thread From: Roger Pau Monne @ 2014-04-04 14:37 UTC (permalink / raw) To: xen-devel; +Cc: Keir Fraser, Jan Beulich, Roger Pau Monne Add a new XENFEAT_hvm_gntmap_supports_iommu that is used to check whether the hypervisor properly updates the IOMMU on auto-translated guests when doing a grant table map/unmap operation. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Cc: Mukesh Rathor <mukesh.rathor@oracle.com> Cc: Keir Fraser <keir@xen.org> Cc: Jan Beulich <jbeulich@suse.com> --- xen/common/kernel.c | 2 ++ xen/include/public/features.h | 6 ++++++ 2 files changed, 8 insertions(+), 0 deletions(-) diff --git a/xen/common/kernel.c b/xen/common/kernel.c index b371f8f..7589dc1 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -316,11 +316,13 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) case guest_type_pvh: fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) | (1U << XENFEAT_supervisor_mode_kernel) | + (1U << XENFEAT_hvm_gntmap_supports_iommu) | (1U << XENFEAT_hvm_callback_vector); break; case guest_type_hvm: fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) | (1U << XENFEAT_hvm_callback_vector) | + (1U << XENFEAT_hvm_gntmap_supports_iommu) | (1U << XENFEAT_hvm_pirqs); break; } diff --git a/xen/include/public/features.h b/xen/include/public/features.h index a149aa6..eaa0187 100644 --- a/xen/include/public/features.h +++ b/xen/include/public/features.h @@ -94,6 +94,12 @@ /* operation as Dom0 is supported */ #define XENFEAT_dom0 11 +/* + * If set, GNTTABOP_map_grant_ref sets the proper IOMMU mappings so the + * resulting mapped page can be used for IO with hardware devices. + */ +#define XENFEAT_hvm_gntmap_supports_iommu 12 + #define XENFEAT_NR_SUBMAPS 1 #endif /* __XEN_PUBLIC_FEATURES_H__ */ -- 1.7.7.5 (Apple Git-26) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-04-07 15:28 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-04 14:37 [PATCH RFC 0/2] Fix grant map/unmap with auto-translated guests Roger Pau Monne 2014-04-04 14:37 ` [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on " Roger Pau Monne 2014-04-04 15:35 ` Jan Beulich 2014-04-07 11:51 ` Roger Pau Monné 2014-04-07 12:04 ` Jan Beulich 2014-04-07 14:40 ` Roger Pau Monné 2014-04-07 15:18 ` Jan Beulich 2014-04-07 15:28 ` Andres Lagar-Cavilla 2014-04-04 14:37 ` [PATCH RFC 2/2] xen: expose that grant table mappings update the IOMMU Roger Pau Monne
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).