* [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
@ 2019-01-18 16:03 Paul Durrant
2019-01-18 16:06 ` Julien Grall
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Paul Durrant @ 2019-01-18 16:03 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Jun Nakajima,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Sander Eikelenboom, Julien Grall, Paul Durrant,
Jan Beulich, Roger Pau Monné,
Chao Gao
...and remove alignment assertions.
Testing shows that certain callers of iommu_legacy_map/unmap() specify
order > 0 ranges that are not order aligned thus causing one of the
IS_ALIGNED() assertions to fire.
This patch removes those assertions and modifies iommu_map/unmap() and
iommu_legacy_map/unmap() to take a page_count argument rather than a
page_order. Using a count actually makes more sense because the valid
set of mapping orders is specific to the IOMMU implementation and to it
should be up to the implementation specific code to translate a mapping
count into an optimal set of mapping orders (when the code is finally
modified to support orders > 0).
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Reported-by: Chao Gao <chao.gao@intel.com>
Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
xen/arch/x86/mm.c | 6 ++----
xen/arch/x86/mm/p2m-ept.c | 5 +++--
xen/arch/x86/mm/p2m-pt.c | 4 ++--
xen/arch/x86/mm/p2m.c | 9 +++++----
xen/arch/x86/x86_64/mm.c | 6 ++----
xen/common/grant_table.c | 8 ++++----
xen/drivers/passthrough/iommu.c | 29 +++++++++++------------------
xen/drivers/passthrough/x86/iommu.c | 4 ++--
xen/include/xen/iommu.h | 8 ++++----
9 files changed, 35 insertions(+), 44 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 7ec5954b03..caccfe3f79 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2801,11 +2801,9 @@ static int _get_page_type(struct page_info *page, unsigned long type,
mfn_t mfn = page_to_mfn(page);
if ( (x & PGT_type_mask) == PGT_writable_page )
- iommu_ret = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)),
- PAGE_ORDER_4K);
+ iommu_ret = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)), 1);
else if ( type == PGT_writable_page )
- iommu_ret = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn,
- PAGE_ORDER_4K,
+ iommu_ret = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn, 1,
IOMMUF_readable |
IOMMUF_writable);
}
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 2b2bf31aad..56341ca678 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -885,8 +885,9 @@ out:
rc = iommu_pte_flush(d, gfn, &ept_entry->epte, order, vtd_pte_present);
else if ( need_iommu_pt_sync(d) )
rc = iommu_flags ?
- iommu_legacy_map(d, _dfn(gfn), mfn, order, iommu_flags) :
- iommu_legacy_unmap(d, _dfn(gfn), order);
+ iommu_legacy_map(d, _dfn(gfn), mfn, 1u << order,
+ iommu_flags) :
+ iommu_legacy_unmap(d, _dfn(gfn), 1u << order);
}
unmap_domain_page(table);
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 12f92cf1f0..ac86a895a0 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -694,9 +694,9 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
if ( need_iommu_pt_sync(p2m->domain) )
rc = iommu_pte_flags ?
- iommu_legacy_map(d, _dfn(gfn), mfn, page_order,
+ iommu_legacy_map(d, _dfn(gfn), mfn, 1u << page_order,
iommu_pte_flags) :
- iommu_legacy_unmap(d, _dfn(gfn), page_order);
+ iommu_legacy_unmap(d, _dfn(gfn), 1u << page_order);
else if ( iommu_use_hap_pt(d) && iommu_old_flags )
amd_iommu_flush_pages(p2m->domain, gfn, page_order);
}
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index d14ce57dd5..ae3d2acd36 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -780,7 +780,8 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn_l, unsigned long mfn,
if ( !paging_mode_translate(p2m->domain) )
return need_iommu_pt_sync(p2m->domain) ?
- iommu_legacy_unmap(p2m->domain, _dfn(mfn), page_order) : 0;
+ iommu_legacy_unmap(p2m->domain, _dfn(mfn), 1u << page_order) :
+ 0;
ASSERT(gfn_locked_by_me(p2m, gfn));
P2M_DEBUG("removing gfn=%#lx mfn=%#lx\n", gfn_l, mfn);
@@ -827,7 +828,7 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
if ( !paging_mode_translate(d) )
return (need_iommu_pt_sync(d) && t == p2m_ram_rw) ?
- iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn, page_order,
+ iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn, 1u << page_order,
IOMMUF_readable | IOMMUF_writable) : 0;
/* foreign pages are added thru p2m_add_foreign */
@@ -1308,7 +1309,7 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l,
{
if ( !need_iommu_pt_sync(d) )
return 0;
- return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l), PAGE_ORDER_4K,
+ return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l), 1,
IOMMUF_readable | IOMMUF_writable);
}
@@ -1399,7 +1400,7 @@ int clear_identity_p2m_entry(struct domain *d, unsigned long gfn_l)
{
if ( !need_iommu_pt_sync(d) )
return 0;
- return iommu_legacy_unmap(d, _dfn(gfn_l), PAGE_ORDER_4K);
+ return iommu_legacy_unmap(d, _dfn(gfn_l), 1);
}
gfn_lock(p2m, gfn, 0);
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index d8f558bc3a..a5afab402f 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1436,16 +1436,14 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
!need_iommu_pt_sync(hardware_domain) )
{
for ( i = spfn; i < epfn; i++ )
- if ( iommu_legacy_map(hardware_domain, _dfn(i), _mfn(i),
- PAGE_ORDER_4K,
+ if ( iommu_legacy_map(hardware_domain, _dfn(i), _mfn(i), 1,
IOMMUF_readable | IOMMUF_writable) )
break;
if ( i != epfn )
{
while (i-- > old_max)
/* If statement to satisfy __must_check. */
- if ( iommu_legacy_unmap(hardware_domain, _dfn(i),
- PAGE_ORDER_4K) )
+ if ( iommu_legacy_unmap(hardware_domain, _dfn(i), 1) )
continue;
goto destroy_m2p;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index fd099a8f25..4bd0b46166 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1134,13 +1134,13 @@ map_grant_ref(
!(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
{
if ( !(kind & MAPKIND_WRITE) )
- err = iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0,
+ err = iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 1,
IOMMUF_readable | IOMMUF_writable);
}
else if ( act_pin && !old_pin )
{
if ( !kind )
- err = iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0,
+ err = iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 1,
IOMMUF_readable);
}
if ( err )
@@ -1389,9 +1389,9 @@ unmap_common(
kind = mapkind(lgt, rd, op->mfn);
if ( !kind )
- err = iommu_legacy_unmap(ld, _dfn(mfn_x(op->mfn)), 0);
+ err = iommu_legacy_unmap(ld, _dfn(mfn_x(op->mfn)), 1);
else if ( !(kind & MAPKIND_WRITE) )
- err = iommu_legacy_map(ld, _dfn(mfn_x(op->mfn)), op->mfn, 0,
+ err = iommu_legacy_map(ld, _dfn(mfn_x(op->mfn)), op->mfn, 1,
IOMMUF_readable);
double_gt_unlock(lgt, rgt);
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index bd1af35a13..b7a08d105d 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -226,7 +226,7 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
== PGT_writable_page) )
mapping |= IOMMUF_writable;
- ret = iommu_map(d, _dfn(dfn), _mfn(mfn), mapping, 0,
+ ret = iommu_map(d, _dfn(dfn), _mfn(mfn), mapping, 1,
&flush_flags);
if ( !rc )
@@ -311,7 +311,7 @@ void iommu_domain_destroy(struct domain *d)
}
int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
- unsigned int page_order, unsigned int flags,
+ unsigned int page_count, unsigned int flags,
unsigned int *flush_flags)
{
const struct domain_iommu *hd = dom_iommu(d);
@@ -321,10 +321,7 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
if ( !iommu_enabled || !hd->platform_ops )
return 0;
- ASSERT(IS_ALIGNED(dfn_x(dfn), (1ul << page_order)));
- ASSERT(IS_ALIGNED(mfn_x(mfn), (1ul << page_order)));
-
- for ( i = 0; i < (1ul << page_order); i++ )
+ for ( i = 0; i < page_count; i++ )
{
rc = hd->platform_ops->map_page(d, dfn_add(dfn, i), mfn_add(mfn, i),
flags, flush_flags);
@@ -354,15 +351,14 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
}
int iommu_legacy_map(struct domain *d, dfn_t dfn, mfn_t mfn,
- unsigned int page_order, unsigned int flags)
+ unsigned int page_count, unsigned int flags)
{
unsigned int flush_flags = 0;
- int rc = iommu_map(d, dfn, mfn, page_order, flags, &flush_flags);
+ int rc = iommu_map(d, dfn, mfn, page_count, flags, &flush_flags);
if ( !this_cpu(iommu_dont_flush_iotlb) )
{
- int err = iommu_iotlb_flush(d, dfn, (1u << page_order),
- flush_flags);
+ int err = iommu_iotlb_flush(d, dfn, page_count, flush_flags);
if ( !rc )
rc = err;
@@ -371,7 +367,7 @@ int iommu_legacy_map(struct domain *d, dfn_t dfn, mfn_t mfn,
return rc;
}
-int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
+int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_count,
unsigned int *flush_flags)
{
const struct domain_iommu *hd = dom_iommu(d);
@@ -381,9 +377,7 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
if ( !iommu_enabled || !hd->platform_ops )
return 0;
- ASSERT(IS_ALIGNED(dfn_x(dfn), (1ul << page_order)));
-
- for ( i = 0; i < (1ul << page_order); i++ )
+ for ( i = 0; i < page_count; i++ )
{
int err = hd->platform_ops->unmap_page(d, dfn_add(dfn, i),
flush_flags);
@@ -409,15 +403,14 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
return rc;
}
-int iommu_legacy_unmap(struct domain *d, dfn_t dfn, unsigned int page_order)
+int iommu_legacy_unmap(struct domain *d, dfn_t dfn, unsigned int page_count)
{
unsigned int flush_flags = 0;
- int rc = iommu_unmap(d, dfn, page_order, &flush_flags);
+ int rc = iommu_unmap(d, dfn, page_count, &flush_flags);
if ( !this_cpu(iommu_dont_flush_iotlb) )
{
- int err = iommu_iotlb_flush(d, dfn, (1u << page_order),
- flush_flags);
+ int err = iommu_iotlb_flush(d, dfn, page_count, flush_flags);
if ( !rc )
rc = err;
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index e40d7a7d7b..53d4dbc60c 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -67,7 +67,7 @@ int arch_iommu_populate_page_table(struct domain *d)
{
ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
BUG_ON(SHARED_M2P(gfn));
- rc = iommu_map(d, _dfn(gfn), _mfn(mfn), PAGE_ORDER_4K,
+ rc = iommu_map(d, _dfn(gfn), _mfn(mfn), 1,
IOMMUF_readable | IOMMUF_writable,
&flush_flags);
}
@@ -245,7 +245,7 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
if ( paging_mode_translate(d) )
rc = set_identity_p2m_entry(d, pfn, p2m_access_rw, 0);
else
- rc = iommu_map(d, _dfn(pfn), _mfn(pfn), PAGE_ORDER_4K,
+ rc = iommu_map(d, _dfn(pfn), _mfn(pfn), 1,
IOMMUF_readable | IOMMUF_writable, &flush_flags);
if ( rc )
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index cdc8021cbd..82fb86c7ff 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -111,17 +111,17 @@ enum
#define IOMMU_FLUSHF_modified (1u << _IOMMU_FLUSHF_modified)
int __must_check iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
- unsigned int page_order, unsigned int flags,
+ unsigned int page_count, unsigned int flags,
unsigned int *flush_flags);
int __must_check iommu_unmap(struct domain *d, dfn_t dfn,
- unsigned int page_order,
+ unsigned int page_count,
unsigned int *flush_flags);
int __must_check iommu_legacy_map(struct domain *d, dfn_t dfn, mfn_t mfn,
- unsigned int page_order,
+ unsigned int page_count,
unsigned int flags);
int __must_check iommu_legacy_unmap(struct domain *d, dfn_t dfn,
- unsigned int page_order);
+ unsigned int page_count);
int __must_check iommu_lookup_page(struct domain *d, dfn_t dfn, mfn_t *mfn,
unsigned int *flags);
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-18 16:03 [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap() Paul Durrant
@ 2019-01-18 16:06 ` Julien Grall
2019-01-18 16:09 ` Paul Durrant
2019-01-18 17:40 ` Andrew Cooper
2019-01-21 11:27 ` Jan Beulich
2 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2019-01-18 16:06 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Jun Nakajima,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Sander Eikelenboom, Jan Beulich, Chao Gao,
Roger Pau Monné
On 18/01/2019 16:03, Paul Durrant wrote:
> ...and remove alignment assertions.
>
> Testing shows that certain callers of iommu_legacy_map/unmap() specify
> order > 0 ranges that are not order aligned thus causing one of the
> IS_ALIGNED() assertions to fire.
>
> This patch removes those assertions and modifies iommu_map/unmap() and
> iommu_legacy_map/unmap() to take a page_count argument rather than a
> page_order. Using a count actually makes more sense because the valid
> set of mapping orders is specific to the IOMMU implementation and to it
> should be up to the implementation specific code to translate a mapping
> count into an optimal set of mapping orders (when the code is finally
> modified to support orders > 0).
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Reported-by: Chao Gao <chao.gao@intel.com>
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
You put those tags after ---. Don't you want them in the final commit?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-18 16:06 ` Julien Grall
@ 2019-01-18 16:09 ` Paul Durrant
0 siblings, 0 replies; 15+ messages in thread
From: Paul Durrant @ 2019-01-18 16:09 UTC (permalink / raw)
To: 'Julien Grall', xen-devel
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Jun Nakajima,
Konrad Rzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Sander Eikelenboom, Jan Beulich, Ian Jackson,
Chao Gao, Roger Pau Monne
> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 18 January 2019 16:07
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Chao Gao <chao.gao@intel.com>; Sander Eikelenboom
> <linux@eikelenboom.it>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau
> Monne <roger.pau@citrix.com>; George Dunlap <George.Dunlap@citrix.com>;
> Ian Jackson <Ian.Jackson@citrix.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim
> (Xen.org) <tim@xen.org>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian
> <kevin.tian@intel.com>
> Subject: Re: [PATCH] iommu: specify page_count rather than page_order to
> iommu_map/unmap()...
>
>
>
> On 18/01/2019 16:03, Paul Durrant wrote:
> > ...and remove alignment assertions.
> >
> > Testing shows that certain callers of iommu_legacy_map/unmap() specify
> > order > 0 ranges that are not order aligned thus causing one of the
> > IS_ALIGNED() assertions to fire.
> >
> > This patch removes those assertions and modifies iommu_map/unmap() and
> > iommu_legacy_map/unmap() to take a page_count argument rather than a
> > page_order. Using a count actually makes more sense because the valid
> > set of mapping orders is specific to the IOMMU implementation and to it
> > should be up to the implementation specific code to translate a mapping
> > count into an optimal set of mapping orders (when the code is finally
> > modified to support orders > 0).
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > Reported-by: Chao Gao <chao.gao@intel.com>
> > Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> You put those tags after ---. Don't you want them in the final commit?
>
Good point. If there is a v2 then I'll move them, otherwise I hope that can be fixed up on commit.
Paul
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-18 16:03 [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap() Paul Durrant
2019-01-18 16:06 ` Julien Grall
@ 2019-01-18 17:40 ` Andrew Cooper
2019-01-21 9:19 ` Paul Durrant
2019-01-21 11:27 ` Jan Beulich
2 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2019-01-18 17:40 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Jun Nakajima,
Konrad Rzeszutek Wilk, George Dunlap, Tim Deegan, Ian Jackson,
Sander Eikelenboom, Julien Grall, Jan Beulich, Chao Gao,
Roger Pau Monné
On 18/01/2019 16:03, Paul Durrant wrote:
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index cdc8021cbd..82fb86c7ff 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -111,17 +111,17 @@ enum
> #define IOMMU_FLUSHF_modified (1u << _IOMMU_FLUSHF_modified)
>
> int __must_check iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
> - unsigned int page_order, unsigned int flags,
> + unsigned int page_count, unsigned int flags,
> unsigned int *flush_flags);
I'd take the opportunity to make page_count an unsigned long, as we can
now sensibly issue a single call for an entire BAR, and some graphics
card BARs are getting to be a ludicrous size.
Otherwise, LGTM and can also be fixed on commit.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-18 17:40 ` Andrew Cooper
@ 2019-01-21 9:19 ` Paul Durrant
0 siblings, 0 replies; 15+ messages in thread
From: Paul Durrant @ 2019-01-21 9:19 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Jun Nakajima,
Konrad Rzeszutek Wilk, Tim (Xen.org),
George Dunlap, Sander Eikelenboom, Julien Grall, Jan Beulich,
Ian Jackson, Chao Gao, Roger Pau Monne
> -----Original Message-----
> From: Andrew Cooper
> Sent: 18 January 2019 17:41
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Chao Gao <chao.gao@intel.com>; Sander Eikelenboom
> <linux@eikelenboom.it>; Jan Beulich <jbeulich@suse.com>; Wei Liu
> <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; George
> Dunlap <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> Julien Grall <julien.grall@arm.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim
> (Xen.org) <tim@xen.org>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian
> <kevin.tian@intel.com>
> Subject: Re: [PATCH] iommu: specify page_count rather than page_order to
> iommu_map/unmap()...
>
> On 18/01/2019 16:03, Paul Durrant wrote:
> > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > index cdc8021cbd..82fb86c7ff 100644
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -111,17 +111,17 @@ enum
> > #define IOMMU_FLUSHF_modified (1u << _IOMMU_FLUSHF_modified)
> >
> > int __must_check iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
> > - unsigned int page_order, unsigned int flags,
> > + unsigned int page_count, unsigned int flags,
> > unsigned int *flush_flags);
>
> I'd take the opportunity to make page_count an unsigned long, as we can
> now sensibly issue a single call for an entire BAR, and some graphics
> card BARs are getting to be a ludicrous size.
The 1G order is still vetoed by mmio_order (http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/mm/p2m.c#l2228) so the BAR limit for a single mapping operation is still going to 2M. So I think we're a way off needing a 64-bit count.
>
> Otherwise, LGTM and can also be fixed on commit.
>
Bear in mind that, if you want to move to a 64-bit count, then the flush operations should be adjusted accordingly; which is a non-trivial amount of code churn.
Paul
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-18 16:03 [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap() Paul Durrant
2019-01-18 16:06 ` Julien Grall
2019-01-18 17:40 ` Andrew Cooper
@ 2019-01-21 11:27 ` Jan Beulich
2019-01-21 11:56 ` Paul Durrant
2019-01-22 17:02 ` Roger Pau Monné
2 siblings, 2 replies; 15+ messages in thread
From: Jan Beulich @ 2019-01-21 11:27 UTC (permalink / raw)
To: Paul Durrant
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Sander Eikelenboom, Julien Grall, Jun Nakajima, xen-devel,
Chao Gao, Roger Pau Monne
>>> On 18.01.19 at 17:03, <paul.durrant@citrix.com> wrote:
> ...and remove alignment assertions.
>
> Testing shows that certain callers of iommu_legacy_map/unmap() specify
> order > 0 ranges that are not order aligned thus causing one of the
> IS_ALIGNED() assertions to fire.
As said before - without a much better explanation of why the current
order-based model is unsuitable (so far I've been provided only vague
pointers into "somewhere in PVH Dom0 boot code" iirc) to understand
why it's undesirable to simply make those call sites obey to the current
requirements, I'm not happy to see us go this route.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-21 11:27 ` Jan Beulich
@ 2019-01-21 11:56 ` Paul Durrant
2019-01-21 12:04 ` Jan Beulich
2019-01-22 17:02 ` Roger Pau Monné
1 sibling, 1 reply; 15+ messages in thread
From: Paul Durrant @ 2019-01-21 11:56 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Sander Eikelenboom, Julien Grall, Jun Nakajima,
xen-devel, Ian Jackson, Chao Gao, Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 21 January 2019 11:28
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei
> Liu <wei.liu2@citrix.com>; Sander Eikelenboom <linux@eikelenboom.it>;
> George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Chao Gao <chao.gao@intel.com>; Jun Nakajima
> <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel <xen-
> devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH] iommu: specify page_count rather than page_order to
> iommu_map/unmap()...
>
> >>> On 18.01.19 at 17:03, <paul.durrant@citrix.com> wrote:
> > ...and remove alignment assertions.
> >
> > Testing shows that certain callers of iommu_legacy_map/unmap() specify
> > order > 0 ranges that are not order aligned thus causing one of the
> > IS_ALIGNED() assertions to fire.
>
> As said before - without a much better explanation of why the current
> order-based model is unsuitable (so far I've been provided only vague
> pointers into "somewhere in PVH Dom0 boot code" iirc) to understand
> why it's undesirable to simply make those call sites obey to the current
> requirements, I'm not happy to see us go this route.
I thought...
"Using a count actually makes more sense because the valid
set of mapping orders is specific to the IOMMU implementation and to it
should be up to the implementation specific code to translate a mapping
count into an optimal set of mapping orders (when the code is finally
modified to support orders > 0)."
...was reasonably clear. Is that not a reasonable justification? What else could I say?
Paul
>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-21 11:56 ` Paul Durrant
@ 2019-01-21 12:04 ` Jan Beulich
2019-01-21 13:22 ` Paul Durrant
0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2019-01-21 12:04 UTC (permalink / raw)
To: Paul Durrant
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim Deegan, george.dunlap, Sander Eikelenboom,
Julien Grall, Jun Nakajima, xen-devel, Ian Jackson, Chao Gao,
Roger Pau Monne
>>> On 21.01.19 at 12:56, <Paul.Durrant@citrix.com> wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 21 January 2019 11:28
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei
>> Liu <wei.liu2@citrix.com>; Sander Eikelenboom <linux@eikelenboom.it>;
>> George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
>> <Ian.Jackson@citrix.com>; Chao Gao <chao.gao@intel.com>; Jun Nakajima
>> <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; xen-devel <xen-
>> devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
>> Subject: Re: [PATCH] iommu: specify page_count rather than page_order to
>> iommu_map/unmap()...
>>
>> >>> On 18.01.19 at 17:03, <paul.durrant@citrix.com> wrote:
>> > ...and remove alignment assertions.
>> >
>> > Testing shows that certain callers of iommu_legacy_map/unmap() specify
>> > order > 0 ranges that are not order aligned thus causing one of the
>> > IS_ALIGNED() assertions to fire.
>>
>> As said before - without a much better explanation of why the current
>> order-based model is unsuitable (so far I've been provided only vague
>> pointers into "somewhere in PVH Dom0 boot code" iirc) to understand
>> why it's undesirable to simply make those call sites obey to the current
>> requirements, I'm not happy to see us go this route.
>
> I thought...
>
> "Using a count actually makes more sense because the valid
> set of mapping orders is specific to the IOMMU implementation and to it
> should be up to the implementation specific code to translate a mapping
> count into an optimal set of mapping orders (when the code is finally
> modified to support orders > 0)."
>
> ...was reasonably clear. Is that not a reasonable justification? What else
> could I say?
Well, I was hoping to be pointed at the (apparently multiple) call sites
where making them match the current function pattern is more involved
and/or less desirable than changing the functions here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-21 12:04 ` Jan Beulich
@ 2019-01-21 13:22 ` Paul Durrant
2019-01-22 10:46 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Paul Durrant @ 2019-01-21 13:22 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Sander Eikelenboom, Julien Grall, Jun Nakajima,
xen-devel, Ian Jackson, Chao Gao, Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 21 January 2019 12:05
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> Wei Liu <wei.liu2@citrix.com>; Sander Eikelenboom <linux@eikelenboom.it>;
> Chao Gao <chao.gao@intel.com>; Jun Nakajima <jun.nakajima@intel.com>;
> Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org)
> <tim@xen.org>
> Subject: RE: [PATCH] iommu: specify page_count rather than page_order to
> iommu_map/unmap()...
>
> >>> On 21.01.19 at 12:56, <Paul.Durrant@citrix.com> wrote:
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 21 January 2019 11:28
> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> >> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> Wei
> >> Liu <wei.liu2@citrix.com>; Sander Eikelenboom <linux@eikelenboom.it>;
> >> George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
> >> <Ian.Jackson@citrix.com>; Chao Gao <chao.gao@intel.com>; Jun Nakajima
> >> <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>; Stefano
> >> Stabellini <sstabellini@kernel.org>; xen-devel <xen-
> >> devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> >> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> >> Subject: Re: [PATCH] iommu: specify page_count rather than page_order
> to
> >> iommu_map/unmap()...
> >>
> >> >>> On 18.01.19 at 17:03, <paul.durrant@citrix.com> wrote:
> >> > ...and remove alignment assertions.
> >> >
> >> > Testing shows that certain callers of iommu_legacy_map/unmap()
> specify
> >> > order > 0 ranges that are not order aligned thus causing one of the
> >> > IS_ALIGNED() assertions to fire.
> >>
> >> As said before - without a much better explanation of why the current
> >> order-based model is unsuitable (so far I've been provided only vague
> >> pointers into "somewhere in PVH Dom0 boot code" iirc) to understand
> >> why it's undesirable to simply make those call sites obey to the
> current
> >> requirements, I'm not happy to see us go this route.
> >
> > I thought...
> >
> > "Using a count actually makes more sense because the valid
> > set of mapping orders is specific to the IOMMU implementation and to it
> > should be up to the implementation specific code to translate a mapping
> > count into an optimal set of mapping orders (when the code is finally
> > modified to support orders > 0)."
> >
> > ...was reasonably clear. Is that not a reasonable justification? What
> else
> > could I say?
>
> Well, I was hoping to be pointed at the (apparently multiple) call sites
> where making them match the current function pattern is more involved
> and/or less desirable than changing the functions here.
AFAICT, one of them is memory.c:populate_physmap() where the extent order comes from the memop_args and the memory comes from alloc_domheap_pages(), which I don't believe aligns memory on the specified order. Regardless of the alignment though, the fact that order comes from a hypercall argument and may not match any of the orders supported by the IOMMU implementation makes me think that using a page count is better.
Paul
>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-21 13:22 ` Paul Durrant
@ 2019-01-22 10:46 ` Jan Beulich
2019-01-22 16:36 ` Paul Durrant
2019-01-22 18:23 ` Andrew Cooper
0 siblings, 2 replies; 15+ messages in thread
From: Jan Beulich @ 2019-01-22 10:46 UTC (permalink / raw)
To: Paul Durrant
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim Deegan, george.dunlap, Sander Eikelenboom,
Julien Grall, Jun Nakajima, xen-devel, IanJackson, Chao Gao,
Roger Pau Monne
>>> On 21.01.19 at 14:22, <Paul.Durrant@citrix.com> wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 21 January 2019 12:05
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
>> Jackson <Ian.Jackson@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
>> Wei Liu <wei.liu2@citrix.com>; Sander Eikelenboom <linux@eikelenboom.it>;
>> Chao Gao <chao.gao@intel.com>; Jun Nakajima <jun.nakajima@intel.com>;
>> Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>;
>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org)
>> <tim@xen.org>
>> Subject: RE: [PATCH] iommu: specify page_count rather than page_order to
>> iommu_map/unmap()...
>>
>> >>> On 21.01.19 at 12:56, <Paul.Durrant@citrix.com> wrote:
>> >> -----Original Message-----
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: 21 January 2019 11:28
>> >> To: Paul Durrant <Paul.Durrant@citrix.com>
>> >> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>> >> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
>> Wei
>> >> Liu <wei.liu2@citrix.com>; Sander Eikelenboom <linux@eikelenboom.it>;
>> >> George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
>> >> <Ian.Jackson@citrix.com>; Chao Gao <chao.gao@intel.com>; Jun Nakajima
>> >> <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>; Stefano
>> >> Stabellini <sstabellini@kernel.org>; xen-devel <xen-
>> >> devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
>> >> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
>> >> Subject: Re: [PATCH] iommu: specify page_count rather than page_order
>> to
>> >> iommu_map/unmap()...
>> >>
>> >> >>> On 18.01.19 at 17:03, <paul.durrant@citrix.com> wrote:
>> >> > ...and remove alignment assertions.
>> >> >
>> >> > Testing shows that certain callers of iommu_legacy_map/unmap()
>> specify
>> >> > order > 0 ranges that are not order aligned thus causing one of the
>> >> > IS_ALIGNED() assertions to fire.
>> >>
>> >> As said before - without a much better explanation of why the current
>> >> order-based model is unsuitable (so far I've been provided only vague
>> >> pointers into "somewhere in PVH Dom0 boot code" iirc) to understand
>> >> why it's undesirable to simply make those call sites obey to the
>> current
>> >> requirements, I'm not happy to see us go this route.
>> >
>> > I thought...
>> >
>> > "Using a count actually makes more sense because the valid
>> > set of mapping orders is specific to the IOMMU implementation and to it
>> > should be up to the implementation specific code to translate a mapping
>> > count into an optimal set of mapping orders (when the code is finally
>> > modified to support orders > 0)."
>> >
>> > ...was reasonably clear. Is that not a reasonable justification? What
>> else
>> > could I say?
>>
>> Well, I was hoping to be pointed at the (apparently multiple) call sites
>> where making them match the current function pattern is more involved
>> and/or less desirable than changing the functions here.
>
> AFAICT, one of them is memory.c:populate_physmap() where the extent order
> comes from the memop_args and the memory comes from alloc_domheap_pages(),
> which I don't believe aligns memory on the specified order.
Of course it does (in MFN space). What I notice is that the gpfn passed
in is not validated to be suitably aligned for the specified order. With
guest_physmap_add_entry() processing each 4k page separately this
doesn't currently have any bad effects, but I think it's a bug
nevertheless. After all the comment in struct xen_memory_reservation's
declaration says "size/alignment of each". The issue with fixing flaws
like this is that there's always the risk of causing regressions with
existing guests.
> Regardless of the
> alignment though, the fact that order comes from a hypercall argument and may
> not match any of the orders supported by the IOMMU implementation makes me
> think that using a page count is better.
Splitting up guest requests is orthogonal to whether a count or an
order is more suitable as a parameter.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-22 10:46 ` Jan Beulich
@ 2019-01-22 16:36 ` Paul Durrant
2019-01-22 18:23 ` Andrew Cooper
1 sibling, 0 replies; 15+ messages in thread
From: Paul Durrant @ 2019-01-22 16:36 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Sander Eikelenboom, Julien Grall, Jun Nakajima,
xen-devel, Ian Jackson, Chao Gao, Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 22 January 2019 10:47
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> Wei Liu <wei.liu2@citrix.com>; Sander Eikelenboom <linux@eikelenboom.it>;
> Chao Gao <chao.gao@intel.com>; Jun Nakajima <jun.nakajima@intel.com>;
> Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org)
> <tim@xen.org>
> Subject: RE: [PATCH] iommu: specify page_count rather than page_order to
> iommu_map/unmap()...
>
> >>> On 21.01.19 at 14:22, <Paul.Durrant@citrix.com> wrote:
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 21 January 2019 12:05
> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> >> <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>;
> Ian
> >> Jackson <Ian.Jackson@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>;
> >> Wei Liu <wei.liu2@citrix.com>; Sander Eikelenboom
> <linux@eikelenboom.it>;
> >> Chao Gao <chao.gao@intel.com>; Jun Nakajima <jun.nakajima@intel.com>;
> >> Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>;
> >> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org)
> >> <tim@xen.org>
> >> Subject: RE: [PATCH] iommu: specify page_count rather than page_order
> to
> >> iommu_map/unmap()...
> >>
> >> >>> On 21.01.19 at 12:56, <Paul.Durrant@citrix.com> wrote:
> >> >> -----Original Message-----
> >> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> Sent: 21 January 2019 11:28
> >> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> >> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> >> >> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> >> Wei
> >> >> Liu <wei.liu2@citrix.com>; Sander Eikelenboom
> <linux@eikelenboom.it>;
> >> >> George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
> >> >> <Ian.Jackson@citrix.com>; Chao Gao <chao.gao@intel.com>; Jun
> Nakajima
> >> >> <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>; Stefano
> >> >> Stabellini <sstabellini@kernel.org>; xen-devel <xen-
> >> >> devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> >> >> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> >> >> Subject: Re: [PATCH] iommu: specify page_count rather than
> page_order
> >> to
> >> >> iommu_map/unmap()...
> >> >>
> >> >> >>> On 18.01.19 at 17:03, <paul.durrant@citrix.com> wrote:
> >> >> > ...and remove alignment assertions.
> >> >> >
> >> >> > Testing shows that certain callers of iommu_legacy_map/unmap()
> >> specify
> >> >> > order > 0 ranges that are not order aligned thus causing one of
> the
> >> >> > IS_ALIGNED() assertions to fire.
> >> >>
> >> >> As said before - without a much better explanation of why the
> current
> >> >> order-based model is unsuitable (so far I've been provided only
> vague
> >> >> pointers into "somewhere in PVH Dom0 boot code" iirc) to understand
> >> >> why it's undesirable to simply make those call sites obey to the
> >> current
> >> >> requirements, I'm not happy to see us go this route.
> >> >
> >> > I thought...
> >> >
> >> > "Using a count actually makes more sense because the valid
> >> > set of mapping orders is specific to the IOMMU implementation and to
> it
> >> > should be up to the implementation specific code to translate a
> mapping
> >> > count into an optimal set of mapping orders (when the code is finally
> >> > modified to support orders > 0)."
> >> >
> >> > ...was reasonably clear. Is that not a reasonable justification? What
> >> else
> >> > could I say?
> >>
> >> Well, I was hoping to be pointed at the (apparently multiple) call
> sites
> >> where making them match the current function pattern is more involved
> >> and/or less desirable than changing the functions here.
> >
> > AFAICT, one of them is memory.c:populate_physmap() where the extent
> order
> > comes from the memop_args and the memory comes from
> alloc_domheap_pages(),
> > which I don't believe aligns memory on the specified order.
>
> Of course it does (in MFN space). What I notice is that the gpfn passed
> in is not validated to be suitably aligned for the specified order. With
> guest_physmap_add_entry() processing each 4k page separately this
> doesn't currently have any bad effects, but I think it's a bug
> nevertheless. After all the comment in struct xen_memory_reservation's
> declaration says "size/alignment of each". The issue with fixing flaws
> like this is that there's always the risk of causing regressions with
> existing guests.
>
> > Regardless of the
> > alignment though, the fact that order comes from a hypercall argument
> and may
> > not match any of the orders supported by the IOMMU implementation makes
> me
> > think that using a page count is better.
>
> Splitting up guest requests is orthogonal to whether a count or an
> order is more suitable as a parameter.
Ok, I'm not prepared to argue the point any more. I withdraw the patch.
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-21 11:27 ` Jan Beulich
2019-01-21 11:56 ` Paul Durrant
@ 2019-01-22 17:02 ` Roger Pau Monné
2019-01-23 10:47 ` Jan Beulich
1 sibling, 1 reply; 15+ messages in thread
From: Roger Pau Monné @ 2019-01-22 17:02 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Sander Eikelenboom, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel, Chao Gao
On Mon, Jan 21, 2019 at 04:27:38AM -0700, Jan Beulich wrote:
> >>> On 18.01.19 at 17:03, <paul.durrant@citrix.com> wrote:
> > ...and remove alignment assertions.
> >
> > Testing shows that certain callers of iommu_legacy_map/unmap() specify
> > order > 0 ranges that are not order aligned thus causing one of the
> > IS_ALIGNED() assertions to fire.
>
> As said before - without a much better explanation of why the current
> order-based model is unsuitable (so far I've been provided only vague
> pointers into "somewhere in PVH Dom0 boot code" iirc) to understand
> why it's undesirable to simply make those call sites obey to the current
> requirements, I'm not happy to see us go this route.
The current PVH dom0 builder will try to always use the biggest
possible order to populate the physmap.
However, the memory map used by dom0 is not under our control, so it's
quite likely that a RAM region starts at a 4K only aligned address.
dom0 builder will then find the next 2M or 1G aligned address and
populate the space between the current address and the next aligned
address using an order as high as possible. In the above scenario,
it's perfectly fine to populate a 4K aligned entry using an order of 5
for example, in order to reach the next 2M or 1G aligned address.
Not removing the asserts would imply that in the above example the
dom0 builder has to iterate over all the 4K pages and make repeated
guest_physmap_add_page calls with order 0. This is sub-optimal,
creates a non-trivial overhead to the Dom0 builder, and also promotes
the open-coding of loops around guest_physmap_add_page.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-22 10:46 ` Jan Beulich
2019-01-22 16:36 ` Paul Durrant
@ 2019-01-22 18:23 ` Andrew Cooper
2019-01-23 10:42 ` Jan Beulich
1 sibling, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2019-01-22 18:23 UTC (permalink / raw)
To: Jan Beulich, Paul Durrant
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Tim Deegan, george.dunlap, Sander Eikelenboom, Julien Grall,
Jun Nakajima, xen-devel, IanJackson, Chao Gao, Roger Pau Monne
On 22/01/2019 10:46, Jan Beulich wrote:
>
>> Regardless of the
>> alignment though, the fact that order comes from a hypercall argument and may
>> not match any of the orders supported by the IOMMU implementation makes me
>> think that using a page count is better.
> Splitting up guest requests is orthogonal to whether a count or an
> order is more suitable as a parameter.
No - this is most certainly not true.
Any arbitrary mapping can be expressed with a single map call, given a
start/count. This is not true of a start/order pair, so start/count is
strictly more expressive.
Furthermore, I've already given the following concrete options as to why
start/count is better than start/order: Reduced caller looping, reduced
TLB flushing in the current implementation, and the fact we literally
have hypercalls using this mechanism who's API is stable.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-22 18:23 ` Andrew Cooper
@ 2019-01-23 10:42 ` Jan Beulich
0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2019-01-23 10:42 UTC (permalink / raw)
To: Andrew Cooper
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Tim Deegan, george.dunlap, Sander Eikelenboom, Julien Grall,
Paul Durrant, Jun Nakajima, xen-devel, IanJackson, Chao Gao,
Roger Pau Monne
>>> On 22.01.19 at 19:23, <andrew.cooper3@citrix.com> wrote:
> On 22/01/2019 10:46, Jan Beulich wrote:
>>
>>> Regardless of the
>>> alignment though, the fact that order comes from a hypercall argument and may
>>> not match any of the orders supported by the IOMMU implementation makes me
>>> think that using a page count is better.
>> Splitting up guest requests is orthogonal to whether a count or an
>> order is more suitable as a parameter.
>
> No - this is most certainly not true.
>
> Any arbitrary mapping can be expressed with a single map call, given a
> start/count. This is not true of a start/order pair, so start/count is
> strictly more expressive.
>
> Furthermore, I've already given the following concrete options as to why
> start/count is better than start/order: Reduced caller looping,
That depends on how many callers are actually affected. Hence my
request for concrete examples. I'm about to look into what Roger's
latest reply is meaning in this regard.
> reduced TLB flushing in the current implementation,
I'm afraid I lack the connection to the amount of TLB flushing done.
> and the fact we literally have hypercalls using this mechanism who's
> API is stable.
Hmm, would you mind helping me with this? All the memop-s are
using order values as input. XEN_DOMCTL_memory_mapping is
not a stable interface. What else do we have that I can't seem
to recall?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...
2019-01-22 17:02 ` Roger Pau Monné
@ 2019-01-23 10:47 ` Jan Beulich
0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2019-01-23 10:47 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Sander Eikelenboom, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel, Chao Gao
>>> On 22.01.19 at 18:02, <roger.pau@citrix.com> wrote:
> On Mon, Jan 21, 2019 at 04:27:38AM -0700, Jan Beulich wrote:
>> >>> On 18.01.19 at 17:03, <paul.durrant@citrix.com> wrote:
>> > ...and remove alignment assertions.
>> >
>> > Testing shows that certain callers of iommu_legacy_map/unmap() specify
>> > order > 0 ranges that are not order aligned thus causing one of the
>> > IS_ALIGNED() assertions to fire.
>>
>> As said before - without a much better explanation of why the current
>> order-based model is unsuitable (so far I've been provided only vague
>> pointers into "somewhere in PVH Dom0 boot code" iirc) to understand
>> why it's undesirable to simply make those call sites obey to the current
>> requirements, I'm not happy to see us go this route.
>
> The current PVH dom0 builder will try to always use the biggest
> possible order to populate the physmap.
>
> However, the memory map used by dom0 is not under our control, so it's
> quite likely that a RAM region starts at a 4K only aligned address.
> dom0 builder will then find the next 2M or 1G aligned address and
> populate the space between the current address and the next aligned
> address using an order as high as possible. In the above scenario,
> it's perfectly fine to populate a 4K aligned entry using an order of 5
> for example, in order to reach the next 2M or 1G aligned address.
When filling the gap between an entry aligned no better than 4k,
you unavoidably will need to allocate (and map) at least one
order-0 chunk. If you started with an order-5 one, in the next
iteration the alignment would still be no better than 4k (and hence
you'd never reach an alignment of 2M or 1G)..
> Not removing the asserts would imply that in the above example the
> dom0 builder has to iterate over all the 4K pages and make repeated
> guest_physmap_add_page calls with order 0. This is sub-optimal,
> creates a non-trivial overhead to the Dom0 builder, and also promotes
> the open-coding of loops around guest_physmap_add_page.
It would need to make one request with order-0, and then individual
requests with higher orders to incrementally increase alignment. I've
already indicated that I think the function should accept all orders as
inputs, irrespective of underlying hardware capabilities, if it doesn't
already. But that's still not the same as accepting arbitrary counts as
inputs.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-01-23 10:47 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-18 16:03 [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap() Paul Durrant
2019-01-18 16:06 ` Julien Grall
2019-01-18 16:09 ` Paul Durrant
2019-01-18 17:40 ` Andrew Cooper
2019-01-21 9:19 ` Paul Durrant
2019-01-21 11:27 ` Jan Beulich
2019-01-21 11:56 ` Paul Durrant
2019-01-21 12:04 ` Jan Beulich
2019-01-21 13:22 ` Paul Durrant
2019-01-22 10:46 ` Jan Beulich
2019-01-22 16:36 ` Paul Durrant
2019-01-22 18:23 ` Andrew Cooper
2019-01-23 10:42 ` Jan Beulich
2019-01-22 17:02 ` Roger Pau Monné
2019-01-23 10:47 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.