* [PATCH v1 00/10] "Non-shared" IOMMU support on ARM @ 2017-05-10 14:03 Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 01/10] xen/device-tree: Add dt_count_phandle_with_args helper Oleksandr Tyshchenko ` (9 more replies) 0 siblings, 10 replies; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-10 14:03 UTC (permalink / raw) To: xen-devel; +Cc: wei.liu2, julien.grall, sstabellini, ian.jackson, jbeulich Hi, all. The purpose of this patch series is to create a base for porting any "Non-shared" IOMMUs to Xen on ARM. Saying "Non-shared" IOMMU I mean the IOMMU that can't share the page table with the CPU. Primarily, we are interested in IPMMU-VMSA and I hope that it will be the first candidate. It is VMSA-compatible IOMMU that integrated in the newest Renesas R-Car Gen3 SoCs (ARM). I will push IPMMU-VMSA support in a while. With regard to the patch series, it was rebased on Xen 4.9.0-rc2 and based on RFC patch series I pushed some time ago[1]: - Patches 1 and 3 already have Julien's Rb. - Patches 2, 4, 9 were reworked. - Patch 5 was left untouched. - Patches 6-8 have light/cosmetic changes. - Patch 10 is new. Not really sure about x86-related changes (especially patch 2) since I had no possibility to check. Hope that I haven't broken anything for x86, but confirmation is needed. You can find patch series here. repo: https://github.com/otyshchenko1/xen.git branch: non_shared_iommu_v1 Thank you. [1] [RFC PATCH 0/9] "Non-shared" IOMMU support on ARM https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg01905.html Oleksandr Tyshchenko (10): xen/device-tree: Add dt_count_phandle_with_args helper iommu: Add extra order argument to the IOMMU APIs and platform callbacks xen/arm: p2m: Add helper to convert p2m type to IOMMU flags xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share iommu: Add extra use_iommu argument to iommu_domain_init() iommu/arm: Add alloc_page_table platform callback iommu: Split iommu_hwdom_init() into arch specific parts xen/arm: Add use_iommu flag to xen_arch_domainconfig xen/arm: domain_build: Don't expose the "iommus" property to the guest tools/libxl/libxl_arm.c | 10 +++++ xen/arch/arm/domain.c | 2 +- xen/arch/arm/domain_build.c | 4 ++ xen/arch/arm/p2m.c | 10 ++++- xen/arch/x86/domain.c | 2 +- xen/arch/x86/mm.c | 11 +++-- xen/arch/x86/mm/p2m-ept.c | 21 +--------- xen/arch/x86/mm/p2m-pt.c | 26 ++---------- xen/arch/x86/mm/p2m.c | 38 +++-------------- xen/arch/x86/x86_64/mm.c | 5 ++- xen/common/device_tree.c | 7 ++++ xen/common/grant_table.c | 10 ++--- xen/drivers/passthrough/amd/iommu_map.c | 49 ++++++++++++++++++++-- xen/drivers/passthrough/amd/pci_amd_iommu.c | 8 ++-- xen/drivers/passthrough/arm/iommu.c | 12 +++++- xen/drivers/passthrough/arm/smmu.c | 44 +++++++++++++++++++- xen/drivers/passthrough/iommu.c | 60 +++++++++------------------ xen/drivers/passthrough/vtd/iommu.c | 48 ++++++++++++++++++++- xen/drivers/passthrough/vtd/x86/vtd.c | 4 +- xen/drivers/passthrough/x86/iommu.c | 42 +++++++++++++++++-- xen/include/asm-arm/iommu.h | 7 +++- xen/include/asm-arm/p2m.h | 34 +++++++++++++++ xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 8 ++-- xen/include/public/arch-arm.h | 5 +++ xen/include/xen/device_tree.h | 19 +++++++++ xen/include/xen/iommu.h | 26 ++++++++---- 26 files changed, 353 insertions(+), 159 deletions(-) -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v1 01/10] xen/device-tree: Add dt_count_phandle_with_args helper 2017-05-10 14:03 [PATCH v1 00/10] "Non-shared" IOMMU support on ARM Oleksandr Tyshchenko @ 2017-05-10 14:03 ` Oleksandr Tyshchenko 2017-05-10 14:50 ` Jan Beulich 2017-05-10 14:03 ` [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks Oleksandr Tyshchenko ` (8 subsequent siblings) 9 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-10 14:03 UTC (permalink / raw) To: xen-devel; +Cc: wei.liu2, julien.grall, sstabellini, ian.jackson, jbeulich From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Port Linux helper of_count_phandle_with_args for counting number of phandles in a property. Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Reviewed-by: Julien Grall <julien.grall@arm.com> --- Changes in v1: - Add Julien's reviewed-by --- xen/common/device_tree.c | 7 +++++++ xen/include/xen/device_tree.h | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 7b009ea..60b0095 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -1663,6 +1663,13 @@ int dt_parse_phandle_with_args(const struct dt_device_node *np, index, out_args); } +int dt_count_phandle_with_args(const struct dt_device_node *np, + const char *list_name, + const char *cells_name) +{ + return __dt_parse_phandle_with_args(np, list_name, cells_name, 0, -1, NULL); +} + /** * unflatten_dt_node - Alloc and populate a device_node from the flat tree * @fdt: The parent device tree blob diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index 0aecbe0..738f1b6 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -764,6 +764,25 @@ int dt_parse_phandle_with_args(const struct dt_device_node *np, const char *cells_name, int index, struct dt_phandle_args *out_args); +/** + * dt_count_phandle_with_args() - Find the number of phandles references in a property + * @np: pointer to a device tree node containing a list + * @list_name: property name that contains a list + * @cells_name: property name that specifies phandles' arguments count + * + * Returns the number of phandle + argument tuples within a property. It + * is a typical pattern to encode a list of phandle and variable + * arguments into a single property. The number of arguments is encoded + * by a property in the phandle-target node. For example, a gpios + * property would contain a list of GPIO specifies consisting of a + * phandle and 1 or more arguments. The number of arguments are + * determined by the #gpio-cells property in the node pointed to by the + * phandle. + */ +int dt_count_phandle_with_args(const struct dt_device_node *np, + const char *list_name, + const char *cells_name); + #ifdef CONFIG_DEVICE_TREE_DEBUG #define dt_dprintk(fmt, args...) \ printk(XENLOG_DEBUG fmt, ## args) -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v1 01/10] xen/device-tree: Add dt_count_phandle_with_args helper 2017-05-10 14:03 ` [PATCH v1 01/10] xen/device-tree: Add dt_count_phandle_with_args helper Oleksandr Tyshchenko @ 2017-05-10 14:50 ` Jan Beulich 2017-05-10 15:06 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-10 14:50 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: ian.jackson, julien.grall, sstabellini, wei.liu2, xen-devel >>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Port Linux helper of_count_phandle_with_args for counting > number of phandles in a property. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Reviewed-by: Julien Grall <julien.grall@arm.com> > > --- > Changes in v1: > - Add Julien's reviewed-by > --- > xen/common/device_tree.c | 7 +++++++ > xen/include/xen/device_tree.h | 19 +++++++++++++++++++ > 2 files changed, 26 insertions(+) I'm sure I did point this out to you before: With this diffstat, I don't see why I've been copied on this patch. Likely the same for a few more in this series. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 01/10] xen/device-tree: Add dt_count_phandle_with_args helper 2017-05-10 14:50 ` Jan Beulich @ 2017-05-10 15:06 ` Oleksandr Tyshchenko 0 siblings, 0 replies; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-10 15:06 UTC (permalink / raw) To: Jan Beulich Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel Hi, Jan, all. On Wed, May 10, 2017 at 5:50 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Port Linux helper of_count_phandle_with_args for counting >> number of phandles in a property. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> Reviewed-by: Julien Grall <julien.grall@arm.com> >> >> --- >> Changes in v1: >> - Add Julien's reviewed-by >> --- >> xen/common/device_tree.c | 7 +++++++ >> xen/include/xen/device_tree.h | 19 +++++++++++++++++++ >> 2 files changed, 26 insertions(+) > > I'm sure I did point this out to you before: With this diffstat, I don't > see why I've been copied on this patch. Likely the same for a few > more in this series. My apologies for that to everybody who I have mistakenly CCed. > > Jan > -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-10 14:03 [PATCH v1 00/10] "Non-shared" IOMMU support on ARM Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 01/10] xen/device-tree: Add dt_count_phandle_with_args helper Oleksandr Tyshchenko @ 2017-05-10 14:03 ` Oleksandr Tyshchenko 2017-05-12 14:23 ` Jan Beulich 2017-05-10 14:03 ` [PATCH v1 03/10] xen/arm: p2m: Add helper to convert p2m type to IOMMU flags Oleksandr Tyshchenko ` (7 subsequent siblings) 9 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-10 14:03 UTC (permalink / raw) To: xen-devel; +Cc: wei.liu2, julien.grall, sstabellini, ian.jackson, jbeulich From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Replace existing single-page stuff (IOMMU APIs and platform callbacks) with the multi-page one followed by modifications of all related parts. These new map_pages/unmap_pages APIs do almost the same thing as old map_page/unmap_page ones except the formers have extra order argument and as the result can handle the number of pages. So have new platform callbacks. Although the current behavior was retained in all places (I hope), it should be noted that the rollback logic was moved from the common code to the IOMMU drivers. Now the IOMMU drivers are responsible for unmapping already mapped pages if something went wrong during mapping the number of pages (order > 0). Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> CC: Jan Beulich <jbeulich@suse.com> CC: Julien Grall <julien.grall@arm.com> --- Changes in v1: - Replace existing single-page IOMMU APIs/platform callbacks with multi-page ones instead of just keeping both variants of them. - Use order argument instead of page_count. - Clarify patch subject/description. --- xen/arch/x86/mm.c | 11 +++--- xen/arch/x86/mm/p2m-ept.c | 21 ++---------- xen/arch/x86/mm/p2m-pt.c | 26 +++----------- xen/arch/x86/mm/p2m.c | 38 ++++----------------- xen/arch/x86/x86_64/mm.c | 5 +-- xen/common/grant_table.c | 10 +++--- xen/drivers/passthrough/amd/iommu_map.c | 49 +++++++++++++++++++++++++-- xen/drivers/passthrough/amd/pci_amd_iommu.c | 8 ++--- xen/drivers/passthrough/arm/smmu.c | 41 ++++++++++++++++++++-- xen/drivers/passthrough/iommu.c | 21 ++++++------ xen/drivers/passthrough/vtd/iommu.c | 48 ++++++++++++++++++++++++-- xen/drivers/passthrough/vtd/x86/vtd.c | 4 +-- xen/drivers/passthrough/x86/iommu.c | 6 ++-- xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 8 +++-- xen/include/xen/iommu.h | 20 ++++++----- 15 files changed, 195 insertions(+), 121 deletions(-) diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 96bc280..c5bc3a5 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2623,11 +2623,14 @@ static int __get_page_type(struct page_info *page, unsigned long type, if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) ) { if ( (x & PGT_type_mask) == PGT_writable_page ) - iommu_ret = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page))); + iommu_ret = iommu_unmap_pages(d, + mfn_to_gmfn(d, page_to_mfn(page)), + 0); else if ( type == PGT_writable_page ) - iommu_ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)), - page_to_mfn(page), - IOMMUF_readable|IOMMUF_writable); + iommu_ret = iommu_map_pages(d, + mfn_to_gmfn(d, page_to_mfn(page)), + page_to_mfn(page), 0, + IOMMUF_readable|IOMMUF_writable); } } diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index f37a1f2..3a4b6b8 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -869,26 +869,9 @@ out: else { if ( iommu_flags ) - for ( i = 0; i < (1 << order); i++ ) - { - rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags); - if ( unlikely(rc) ) - { - while ( i-- ) - /* If statement to satisfy __must_check. */ - if ( iommu_unmap_page(p2m->domain, gfn + i) ) - continue; - - break; - } - } + rc = iommu_map_pages(d, gfn, mfn_x(mfn), order, iommu_flags); else - for ( i = 0; i < (1 << order); i++ ) - { - ret = iommu_unmap_page(d, gfn + i); - if ( !rc ) - rc = ret; - } + rc = iommu_unmap_pages(d, gfn, order); } } diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index 5079b59..51f3e10 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -507,7 +507,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, { /* XXX -- this might be able to be faster iff current->domain == d */ void *table; - unsigned long i, gfn_remainder = gfn; + unsigned long gfn_remainder = gfn; l1_pgentry_t *p2m_entry, entry_content; /* Intermediate table to free if we're replacing it with a superpage. */ l1_pgentry_t intermediate_entry = l1e_empty(); @@ -718,28 +718,10 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, amd_iommu_flush_pages(p2m->domain, gfn, page_order); } else if ( iommu_pte_flags ) - for ( i = 0; i < (1UL << page_order); i++ ) - { - rc = iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i, - iommu_pte_flags); - if ( unlikely(rc) ) - { - while ( i-- ) - /* If statement to satisfy __must_check. */ - if ( iommu_unmap_page(p2m->domain, gfn + i) ) - continue; - - break; - } - } + rc = iommu_map_pages(p2m->domain, gfn, mfn_x(mfn), page_order, + iommu_pte_flags); else - for ( i = 0; i < (1UL << page_order); i++ ) - { - int ret = iommu_unmap_page(p2m->domain, gfn + i); - - if ( !rc ) - rc = ret; - } + rc = iommu_unmap_pages(p2m->domain, gfn, page_order); } /* diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 1d57e5c..15ba71d 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -705,20 +705,9 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn, if ( !paging_mode_translate(p2m->domain) ) { - int rc = 0; - if ( need_iommu(p2m->domain) ) - { - for ( i = 0; i < (1 << page_order); i++ ) - { - int ret = iommu_unmap_page(p2m->domain, mfn + i); - - if ( !rc ) - rc = ret; - } - } - - return rc; + return iommu_unmap_pages(p2m->domain, mfn, page_order); + return 0; } ASSERT(gfn_locked_by_me(p2m, gfn)); @@ -765,23 +754,8 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn, if ( !paging_mode_translate(d) ) { if ( need_iommu(d) && t == p2m_ram_rw ) - { - for ( i = 0; i < (1 << page_order); i++ ) - { - rc = iommu_map_page(d, mfn_x(mfn_add(mfn, i)), - mfn_x(mfn_add(mfn, i)), - IOMMUF_readable|IOMMUF_writable); - if ( rc != 0 ) - { - while ( i-- > 0 ) - /* If statement to satisfy __must_check. */ - if ( iommu_unmap_page(d, mfn_x(mfn_add(mfn, i))) ) - continue; - - return rc; - } - } - } + return iommu_map_pages(d, mfn_x(mfn), mfn_x(mfn), page_order, + IOMMUF_readable|IOMMUF_writable); return 0; } @@ -1134,7 +1108,7 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn, { if ( !need_iommu(d) ) return 0; - return iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable); + return iommu_map_pages(d, gfn, gfn, 0, IOMMUF_readable|IOMMUF_writable); } gfn_lock(p2m, gfn, 0); @@ -1222,7 +1196,7 @@ int clear_identity_p2m_entry(struct domain *d, unsigned long gfn) { if ( !need_iommu(d) ) return 0; - return iommu_unmap_page(d, gfn); + return iommu_unmap_pages(d, gfn, 0); } gfn_lock(p2m, gfn, 0); diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c index 34f3250..24aaf88 100644 --- a/xen/arch/x86/x86_64/mm.c +++ b/xen/arch/x86/x86_64/mm.c @@ -1443,13 +1443,14 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm) if ( iommu_enabled && !iommu_passthrough && !need_iommu(hardware_domain) ) { for ( i = spfn; i < epfn; i++ ) - if ( iommu_map_page(hardware_domain, i, i, IOMMUF_readable|IOMMUF_writable) ) + if ( iommu_map_pages(hardware_domain, i, i, 0, + IOMMUF_readable|IOMMUF_writable) ) break; if ( i != epfn ) { while (i-- > old_max) /* If statement to satisfy __must_check. */ - if ( iommu_unmap_page(hardware_domain, i) ) + if ( iommu_unmap_pages(hardware_domain, i, 0) ) continue; goto destroy_m2p; diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 4fe9544..ecf8f82 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -964,13 +964,13 @@ __gnttab_map_grant_ref( !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) ) { if ( !(kind & MAPKIND_WRITE) ) - err = iommu_map_page(ld, frame, frame, - IOMMUF_readable|IOMMUF_writable); + err = iommu_map_pages(ld, frame, frame, 0, + IOMMUF_readable|IOMMUF_writable); } else if ( act_pin && !old_pin ) { if ( !kind ) - err = iommu_map_page(ld, frame, frame, IOMMUF_readable); + err = iommu_map_pages(ld, frame, frame, 0, IOMMUF_readable); } if ( err ) { @@ -1190,9 +1190,9 @@ __gnttab_unmap_common( kind = mapkind(lgt, rd, op->frame); if ( !kind ) - err = iommu_unmap_page(ld, op->frame); + err = iommu_unmap_pages(ld, op->frame, 0); else if ( !(kind & MAPKIND_WRITE) ) - err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable); + err = iommu_map_pages(ld, op->frame, op->frame, 0, IOMMUF_readable); double_gt_unlock(lgt, rgt); diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c index fd2327d..87ab99c 100644 --- a/xen/drivers/passthrough/amd/iommu_map.c +++ b/xen/drivers/passthrough/amd/iommu_map.c @@ -631,8 +631,9 @@ static int update_paging_mode(struct domain *d, unsigned long gfn) return 0; } -int amd_iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, - unsigned int flags) +static int __must_check amd_iommu_map_page(struct domain *d, unsigned long gfn, + unsigned long mfn, + unsigned int flags) { bool_t need_flush = 0; struct domain_iommu *hd = dom_iommu(d); @@ -720,7 +721,8 @@ out: return 0; } -int amd_iommu_unmap_page(struct domain *d, unsigned long gfn) +static int __must_check amd_iommu_unmap_page(struct domain *d, + unsigned long gfn) { unsigned long pt_mfn[7]; struct domain_iommu *hd = dom_iommu(d); @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn) return 0; } +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ +int __must_check amd_iommu_map_pages(struct domain *d, unsigned long gfn, + unsigned long mfn, unsigned int order, + unsigned int flags) +{ + unsigned long i; + int rc = 0; + + for ( i = 0; i < (1UL << order); i++ ) + { + rc = amd_iommu_map_page(d, gfn + i, mfn + i, flags); + if ( unlikely(rc) ) + { + while ( i-- ) + /* If statement to satisfy __must_check. */ + if ( amd_iommu_unmap_page(d, gfn + i) ) + continue; + + break; + } + } + + return rc; +} + +int __must_check amd_iommu_unmap_pages(struct domain *d, unsigned long gfn, + unsigned int order) +{ + unsigned long i; + int rc = 0; + + for ( i = 0; i < (1UL << order); i++ ) + { + int ret = amd_iommu_unmap_page(d, gfn + i); + if ( !rc ) + rc = ret; + } + + return rc; +} + int amd_iommu_reserve_domain_unity_map(struct domain *domain, u64 phys_addr, unsigned long size, int iw, int ir) diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index 8c25110..fe744d2 100644 --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -296,8 +296,8 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain *d) */ if ( mfn_valid(_mfn(pfn)) ) { - int ret = amd_iommu_map_page(d, pfn, pfn, - IOMMUF_readable|IOMMUF_writable); + int ret = amd_iommu_map_pages(d, pfn, pfn, 0, + IOMMUF_readable|IOMMUF_writable); if ( !rc ) rc = ret; @@ -620,8 +620,8 @@ const struct iommu_ops amd_iommu_ops = { .remove_device = amd_iommu_remove_device, .assign_device = amd_iommu_assign_device, .teardown = amd_iommu_domain_destroy, - .map_page = amd_iommu_map_page, - .unmap_page = amd_iommu_unmap_page, + .map_pages = amd_iommu_map_pages, + .unmap_pages = amd_iommu_unmap_pages, .free_page_table = deallocate_page_table, .reassign_device = reassign_device, .get_device_group_id = amd_iommu_group_id, diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index 1082fcf..527a592 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -2780,6 +2780,43 @@ static int __must_check arm_smmu_unmap_page(struct domain *d, unsigned long gfn) return 0; } +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ +static int __must_check arm_smmu_map_pages(struct domain *d, unsigned long gfn, + unsigned long mfn, unsigned int order, unsigned int flags) +{ + unsigned long i; + int rc = 0; + + for (i = 0; i < (1UL << order); i++) { + rc = arm_smmu_map_page(d, gfn + i, mfn + i, flags); + if (unlikely(rc)) { + while (i--) + /* If statement to satisfy __must_check. */ + if (arm_smmu_unmap_page(d, gfn + i)) + continue; + + break; + } + } + + return rc; +} + +static int __must_check arm_smmu_unmap_pages(struct domain *d, + unsigned long gfn, unsigned int order) +{ + unsigned long i; + int rc = 0; + + for (i = 0; i < (1UL << order); i++) { + int ret = arm_smmu_unmap_page(d, gfn + i); + if (!rc) + rc = ret; + } + + return rc; +} + static const struct iommu_ops arm_smmu_iommu_ops = { .init = arm_smmu_iommu_domain_init, .hwdom_init = arm_smmu_iommu_hwdom_init, @@ -2788,8 +2825,8 @@ static const struct iommu_ops arm_smmu_iommu_ops = { .iotlb_flush_all = arm_smmu_iotlb_flush_all, .assign_device = arm_smmu_assign_dev, .reassign_device = arm_smmu_reassign_dev, - .map_page = arm_smmu_map_page, - .unmap_page = arm_smmu_unmap_page, + .map_pages = arm_smmu_map_pages, + .unmap_pages = arm_smmu_unmap_pages, }; static __init const struct arm_smmu_device *find_smmu(const struct device *dev) diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 5e81813..3e9e4c3 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -188,7 +188,7 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) == PGT_writable_page) ) mapping |= IOMMUF_writable; - ret = hd->platform_ops->map_page(d, gfn, mfn, mapping); + ret = hd->platform_ops->map_pages(d, gfn, mfn, 0, mapping); if ( !rc ) rc = ret; @@ -249,8 +249,8 @@ void iommu_domain_destroy(struct domain *d) arch_iommu_domain_destroy(d); } -int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, - unsigned int flags) +int iommu_map_pages(struct domain *d, unsigned long gfn, unsigned long mfn, + unsigned int order, unsigned int flags) { const struct domain_iommu *hd = dom_iommu(d); int rc; @@ -258,13 +258,13 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, if ( !iommu_enabled || !hd->platform_ops ) return 0; - rc = hd->platform_ops->map_page(d, gfn, mfn, flags); + rc = hd->platform_ops->map_pages(d, gfn, mfn, order, flags); if ( unlikely(rc) ) { if ( !d->is_shutting_down && printk_ratelimit() ) printk(XENLOG_ERR - "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed: %d\n", - d->domain_id, gfn, mfn, rc); + "d%d: IOMMU mapping gfn %#lx to mfn %#lx order %u failed: %d\n", + d->domain_id, gfn, mfn, order, rc); if ( !is_hardware_domain(d) ) domain_crash(d); @@ -273,7 +273,8 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, return rc; } -int iommu_unmap_page(struct domain *d, unsigned long gfn) +int iommu_unmap_pages(struct domain *d, unsigned long gfn, + unsigned int order) { const struct domain_iommu *hd = dom_iommu(d); int rc; @@ -281,13 +282,13 @@ int iommu_unmap_page(struct domain *d, unsigned long gfn) if ( !iommu_enabled || !hd->platform_ops ) return 0; - rc = hd->platform_ops->unmap_page(d, gfn); + rc = hd->platform_ops->unmap_pages(d, gfn, order); if ( unlikely(rc) ) { if ( !d->is_shutting_down && printk_ratelimit() ) printk(XENLOG_ERR - "d%d: IOMMU unmapping gfn %#lx failed: %d\n", - d->domain_id, gfn, rc); + "d%d: IOMMU unmapping gfn %#lx order %u failed: %d\n", + d->domain_id, gfn, order, rc); if ( !is_hardware_domain(d) ) domain_crash(d); diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index a5c61c6..6c7f4c6 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1816,6 +1816,50 @@ static int __must_check intel_iommu_unmap_page(struct domain *d, return dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); } +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ +static int __must_check intel_iommu_map_pages(struct domain *d, + unsigned long gfn, + unsigned long mfn, + unsigned int order, + unsigned int flags) +{ + unsigned long i; + int rc = 0; + + for ( i = 0; i < (1UL << order); i++ ) + { + rc = intel_iommu_map_page(d, gfn + i, mfn + i, flags); + if ( unlikely(rc) ) + { + while ( i-- ) + /* If statement to satisfy __must_check. */ + if ( intel_iommu_unmap_page(d, gfn + i) ) + continue; + + break; + } + } + + return rc; +} + +static int __must_check intel_iommu_unmap_pages(struct domain *d, + unsigned long gfn, + unsigned int order) +{ + unsigned long i; + int rc = 0; + + for ( i = 0; i < (1UL << order); i++ ) + { + int ret = intel_iommu_unmap_page(d, gfn + i); + if ( !rc ) + rc = ret; + } + + return rc; +} + int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, int present) { @@ -2639,8 +2683,8 @@ const struct iommu_ops intel_iommu_ops = { .remove_device = intel_iommu_remove_device, .assign_device = intel_iommu_assign_device, .teardown = iommu_domain_teardown, - .map_page = intel_iommu_map_page, - .unmap_page = intel_iommu_unmap_page, + .map_pages = intel_iommu_map_pages, + .unmap_pages = intel_iommu_unmap_pages, .free_page_table = iommu_free_page_table, .reassign_device = reassign_device_ownership, .get_device_group_id = intel_iommu_group_id, diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c b/xen/drivers/passthrough/vtd/x86/vtd.c index 88a60b3..62a6ee6 100644 --- a/xen/drivers/passthrough/vtd/x86/vtd.c +++ b/xen/drivers/passthrough/vtd/x86/vtd.c @@ -143,8 +143,8 @@ void __hwdom_init vtd_set_hwdom_mapping(struct domain *d) tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K); for ( j = 0; j < tmp; j++ ) { - int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j, - IOMMUF_readable|IOMMUF_writable); + int ret = iommu_map_pages(d, pfn * tmp + j, pfn * tmp + j, 0, + IOMMUF_readable|IOMMUF_writable); if ( !rc ) rc = ret; diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index 0253823..973b72f 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -65,9 +65,9 @@ int arch_iommu_populate_page_table(struct domain *d) { ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH)); BUG_ON(SHARED_M2P(gfn)); - rc = hd->platform_ops->map_page(d, gfn, mfn, - IOMMUF_readable | - IOMMUF_writable); + rc = hd->platform_ops->map_pages(d, gfn, mfn, 0, + IOMMUF_readable | + IOMMUF_writable); } if ( rc ) { diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h index 99bc21c..8f44489 100644 --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h @@ -52,9 +52,11 @@ int amd_iommu_init(void); int amd_iommu_update_ivrs_mapping_acpi(void); /* mapping functions */ -int __must_check amd_iommu_map_page(struct domain *d, unsigned long gfn, - unsigned long mfn, unsigned int flags); -int __must_check amd_iommu_unmap_page(struct domain *d, unsigned long gfn); +int __must_check amd_iommu_map_pages(struct domain *d, unsigned long gfn, + unsigned long mfn, unsigned int order, + unsigned int flags); +int __must_check amd_iommu_unmap_pages(struct domain *d, unsigned long gfn, + unsigned int order); u64 amd_iommu_get_next_table_from_pte(u32 *entry); int __must_check amd_iommu_alloc_root(struct domain_iommu *hd); int amd_iommu_reserve_domain_unity_map(struct domain *domain, diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 5803e3f..3297998 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -71,14 +71,16 @@ int iommu_construct(struct domain *d); /* Function used internally, use iommu_domain_destroy */ void iommu_teardown(struct domain *d); -/* iommu_map_page() takes flags to direct the mapping operation. */ +/* iommu_map_pages() takes flags to direct the mapping operation. */ #define _IOMMUF_readable 0 #define IOMMUF_readable (1u<<_IOMMUF_readable) #define _IOMMUF_writable 1 #define IOMMUF_writable (1u<<_IOMMUF_writable) -int __must_check iommu_map_page(struct domain *d, unsigned long gfn, - unsigned long mfn, unsigned int flags); -int __must_check iommu_unmap_page(struct domain *d, unsigned long gfn); +int __must_check iommu_map_pages(struct domain *d, unsigned long gfn, + unsigned long mfn, unsigned int order, + unsigned int flags); +int __must_check iommu_unmap_pages(struct domain *d, unsigned long gfn, + unsigned int order); enum iommu_feature { @@ -168,9 +170,11 @@ struct iommu_ops { #endif /* HAS_PCI */ void (*teardown)(struct domain *d); - int __must_check (*map_page)(struct domain *d, unsigned long gfn, - unsigned long mfn, unsigned int flags); - int __must_check (*unmap_page)(struct domain *d, unsigned long gfn); + int __must_check (*map_pages)(struct domain *d, unsigned long gfn, + unsigned long mfn, unsigned int order, + unsigned int flags); + int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn, + unsigned int order); void (*free_page_table)(struct page_info *); #ifdef CONFIG_X86 void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value); @@ -213,7 +217,7 @@ void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev); * The purpose of the iommu_dont_flush_iotlb optional cpu flag is to * avoid unecessary iotlb_flush in the low level IOMMU code. * - * iommu_map_page/iommu_unmap_page must flush the iotlb but somethimes + * iommu_map_pages/iommu_unmap_pages must flush the iotlb but somethimes * this operation can be really expensive. This flag will be set by the * caller to notify the low level IOMMU code to avoid the iotlb flushes. * iommu_iotlb_flush/iommu_iotlb_flush_all will be explicitly called by -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-10 14:03 ` [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks Oleksandr Tyshchenko @ 2017-05-12 14:23 ` Jan Beulich 2017-05-12 15:50 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-12 14:23 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: ian.jackson, julien.grall, sstabellini, wei.liu2, xen-devel >>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: > @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn) > return 0; > } > > +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ Looking over the titles of the rest of this series it doesn't look like you're eliminating this TODO later. While I appreciate this not being done in the already large patch, I don't think such a TODO should be left around. If need be (e.g. because you can't test the change), get in touch with the maintainer(s). > +int __must_check amd_iommu_unmap_pages(struct domain *d, unsigned long gfn, > + unsigned int order) > +{ > + unsigned long i; > + int rc = 0; > + > + for ( i = 0; i < (1UL << order); i++ ) > + { > + int ret = amd_iommu_unmap_page(d, gfn + i); > + if ( !rc ) Blank line between declaration(s) and statement(s) please. x86 and generic iommu parts (and _only_ those, so please don't convert this into a blanket R-b) Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-12 14:23 ` Jan Beulich @ 2017-05-12 15:50 ` Oleksandr Tyshchenko 2017-05-12 16:17 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-12 15:50 UTC (permalink / raw) To: Jan Beulich Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel Hi Jan. On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn) >> return 0; >> } >> >> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ > > Looking over the titles of the rest of this series it doesn't look like > you're eliminating this TODO later. While I appreciate this not > being done in the already large patch, I don't think such a TODO > should be left around. If need be (e.g. because you can't test > the change), get in touch with the maintainer(s). I will drop this TODO everywhere. > >> +int __must_check amd_iommu_unmap_pages(struct domain *d, unsigned long gfn, >> + unsigned int order) >> +{ >> + unsigned long i; >> + int rc = 0; >> + >> + for ( i = 0; i < (1UL << order); i++ ) >> + { >> + int ret = amd_iommu_unmap_page(d, gfn + i); >> + if ( !rc ) > > Blank line between declaration(s) and statement(s) please. ok > > x86 and generic iommu parts (and _only_ those, so please don't > convert this into a blanket R-b) > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thank you. Sure. I won't put your R-b until I get R-b from ARM folks. > > Jan > -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-12 15:50 ` Oleksandr Tyshchenko @ 2017-05-12 16:17 ` Jan Beulich 2017-05-12 16:25 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-12 16:17 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel >>> On 12.05.17 at 17:50, <olekstysh@gmail.com> wrote: > On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn) >>> return 0; >>> } >>> >>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ >> >> Looking over the titles of the rest of this series it doesn't look like >> you're eliminating this TODO later. While I appreciate this not >> being done in the already large patch, I don't think such a TODO >> should be left around. If need be (e.g. because you can't test >> the change), get in touch with the maintainer(s). > I will drop this TODO everywhere. By "drop" you mean "address" or really just "drop"? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-12 16:17 ` Jan Beulich @ 2017-05-12 16:25 ` Oleksandr Tyshchenko 2017-05-15 7:22 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-12 16:25 UTC (permalink / raw) To: Jan Beulich Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel On Fri, May 12, 2017 at 7:17 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 12.05.17 at 17:50, <olekstysh@gmail.com> wrote: >> On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn) >>>> return 0; >>>> } >>>> >>>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ >>> >>> Looking over the titles of the rest of this series it doesn't look like >>> you're eliminating this TODO later. While I appreciate this not >>> being done in the already large patch, I don't think such a TODO >>> should be left around. If need be (e.g. because you can't test >>> the change), get in touch with the maintainer(s). >> I will drop this TODO everywhere. > > By "drop" you mean "address" or really just "drop"? I meant just drop. > > Jan > -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-12 16:25 ` Oleksandr Tyshchenko @ 2017-05-15 7:22 ` Jan Beulich 2017-05-15 10:43 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-15 7:22 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel >>> On 12.05.17 at 18:25, <olekstysh@gmail.com> wrote: > On Fri, May 12, 2017 at 7:17 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 12.05.17 at 17:50, <olekstysh@gmail.com> wrote: >>> On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>>>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn) >>>>> return 0; >>>>> } >>>>> >>>>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ >>>> >>>> Looking over the titles of the rest of this series it doesn't look like >>>> you're eliminating this TODO later. While I appreciate this not >>>> being done in the already large patch, I don't think such a TODO >>>> should be left around. If need be (e.g. because you can't test >>>> the change), get in touch with the maintainer(s). >>> I will drop this TODO everywhere. >> >> By "drop" you mean "address" or really just "drop"? > I meant just drop. Then I'm sorry, but no, this is not a way to address the comment I've made. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-15 7:22 ` Jan Beulich @ 2017-05-15 10:43 ` Oleksandr Tyshchenko 2017-05-15 12:33 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-15 10:43 UTC (permalink / raw) To: Jan Beulich Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel Hi, Jan On Mon, May 15, 2017 at 10:22 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 12.05.17 at 18:25, <olekstysh@gmail.com> wrote: >> On Fri, May 12, 2017 at 7:17 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 12.05.17 at 17:50, <olekstysh@gmail.com> wrote: >>>> On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>>>>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ >>>>> >>>>> Looking over the titles of the rest of this series it doesn't look like >>>>> you're eliminating this TODO later. While I appreciate this not >>>>> being done in the already large patch, I don't think such a TODO >>>>> should be left around. If need be (e.g. because you can't test >>>>> the change), get in touch with the maintainer(s). >>>> I will drop this TODO everywhere. >>> >>> By "drop" you mean "address" or really just "drop"? >> I meant just drop. > > Then I'm sorry, but no, this is not a way to address the comment I've > made. Indeed, there was some misunderstanding from my side on this. Let me elaborate a bit more on this: 1. Yes, this TODO shouldn't be just dropped, but needs to be addressed, so at least I will have them back in the patch 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so it makes me lots of work to do this change properly, so this is not only the question of testing the code, but rather having it written. 3. Please correct me if I'm wrong, but these are all *optimizations* which I am mentioning in that TODO, not something that breaks x86 or affects it in any way. That being said, can we postpone implementation of the *optimizations* in question and have those as a separate activity? Or if these *optimizations* must be present in the current patch series, could you, please, provide me with some hints how these TODO should be properly implemented? > > Jan > -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-15 10:43 ` Oleksandr Tyshchenko @ 2017-05-15 12:33 ` Jan Beulich 2017-05-16 12:48 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-15 12:33 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel >>> On 15.05.17 at 12:43, <olekstysh@gmail.com> wrote: > On Mon, May 15, 2017 at 10:22 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 12.05.17 at 18:25, <olekstysh@gmail.com> wrote: >>> On Fri, May 12, 2017 at 7:17 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 12.05.17 at 17:50, <olekstysh@gmail.com> wrote: >>>>> On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>>>>>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn) >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ >>>>>> >>>>>> Looking over the titles of the rest of this series it doesn't look like >>>>>> you're eliminating this TODO later. While I appreciate this not >>>>>> being done in the already large patch, I don't think such a TODO >>>>>> should be left around. If need be (e.g. because you can't test >>>>>> the change), get in touch with the maintainer(s). >>>>> I will drop this TODO everywhere. >>>> >>>> By "drop" you mean "address" or really just "drop"? >>> I meant just drop. >> >> Then I'm sorry, but no, this is not a way to address the comment I've >> made. > > Indeed, there was some misunderstanding from my side on this. > Let me elaborate a bit more on this: > 1. Yes, this TODO shouldn't be just dropped, but needs to be > addressed, so at least I will have them back in the patch > 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so > it makes me lots of work to do this change > properly, so this is not only the question of testing the code, but rather > having it written. > 3. Please correct me if I'm wrong, but these are all *optimizations* which > I am mentioning in that TODO, not something that breaks x86 or affects it > in any way. > > That being said, can we postpone implementation of the *optimizations* > in question > and have those as a separate activity? > Or if these *optimizations* must be present in the current patch > series, could you, please, provide me with some hints how > these TODO should be properly implemented? I'm puzzled. When I first commented on these TODOs I did say "While I appreciate this not being done in the already large patch, I don't think such a TODO should be left around. If need be (e.g. because you can't test the change), get in touch with the maintainer(s)." Of course the "e.g." extends to the actual implementation. IOW I'm not saying you need to do this work immediately and all by yourself, but there should be a clear plan on getting these items addressed. We shouldn't ship several releases with them still present. I'm sorry this hits you, but we've had too bad experience in the past with people leaving todo or fixme notes in the code, perhaps even promising to address them without ever doing so. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-15 12:33 ` Jan Beulich @ 2017-05-16 12:48 ` Oleksandr Tyshchenko 2017-05-16 13:11 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-16 12:48 UTC (permalink / raw) To: Jan Beulich Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel Hi, Jan On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 15.05.17 at 12:43, <olekstysh@gmail.com> wrote: >> On Mon, May 15, 2017 at 10:22 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 12.05.17 at 18:25, <olekstysh@gmail.com> wrote: >>>> On Fri, May 12, 2017 at 7:17 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>> On 12.05.17 at 17:50, <olekstysh@gmail.com> wrote: >>>>>> On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>>>>>>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn) >>>>>>>> return 0; >>>>>>>> } >>>>>>>> >>>>>>>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ >>>>>>> >>>>>>> Looking over the titles of the rest of this series it doesn't look like >>>>>>> you're eliminating this TODO later. While I appreciate this not >>>>>>> being done in the already large patch, I don't think such a TODO >>>>>>> should be left around. If need be (e.g. because you can't test >>>>>>> the change), get in touch with the maintainer(s). >>>>>> I will drop this TODO everywhere. >>>>> >>>>> By "drop" you mean "address" or really just "drop"? >>>> I meant just drop. >>> >>> Then I'm sorry, but no, this is not a way to address the comment I've >>> made. >> >> Indeed, there was some misunderstanding from my side on this. >> Let me elaborate a bit more on this: >> 1. Yes, this TODO shouldn't be just dropped, but needs to be >> addressed, so at least I will have them back in the patch >> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so >> it makes me lots of work to do this change >> properly, so this is not only the question of testing the code, but rather >> having it written. >> 3. Please correct me if I'm wrong, but these are all *optimizations* which >> I am mentioning in that TODO, not something that breaks x86 or affects it >> in any way. >> >> That being said, can we postpone implementation of the *optimizations* >> in question >> and have those as a separate activity? >> Or if these *optimizations* must be present in the current patch >> series, could you, please, provide me with some hints how >> these TODO should be properly implemented? > > I'm puzzled. When I first commented on these TODOs I did say > "While I appreciate this not being done in the already large patch, > I don't think such a TODO should be left around. If need be (e.g. > because you can't test the change), get in touch with the > maintainer(s)." Of course the "e.g." extends to the actual > implementation. IOW I'm not saying you need to do this work > immediately and all by yourself, but there should be a clear plan > on getting these items addressed. We shouldn't ship several > releases with them still present. I'm sorry this hits you, but we've > had too bad experience in the past with people leaving todo or > fixme notes in the code, perhaps even promising to address them > without ever doing so. I see. You are right about leaving TODO) Don't mind to get these items addressed *within current patch series* as separate patch or patches. So, we have to address for three IOMMUs: Intel/AMD and SMMU. I will leave SMMU for myself. Could you, please, provide me with some hints how these TODO should be properly implemented? Or I was thinking I can even just squash *pages with *page and send you a draft as we need to start from somewhere. What do you think? > > Jan > -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-16 12:48 ` Oleksandr Tyshchenko @ 2017-05-16 13:11 ` Jan Beulich 2017-05-17 15:28 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-16 13:11 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel >>> On 16.05.17 at 14:48, <olekstysh@gmail.com> wrote: > On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 15.05.17 at 12:43, <olekstysh@gmail.com> wrote: >>> Indeed, there was some misunderstanding from my side on this. >>> Let me elaborate a bit more on this: >>> 1. Yes, this TODO shouldn't be just dropped, but needs to be >>> addressed, so at least I will have them back in the patch >>> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so >>> it makes me lots of work to do this change >>> properly, so this is not only the question of testing the code, but rather >>> having it written. >>> 3. Please correct me if I'm wrong, but these are all *optimizations* which >>> I am mentioning in that TODO, not something that breaks x86 or affects it >>> in any way. >>> >>> That being said, can we postpone implementation of the *optimizations* >>> in question >>> and have those as a separate activity? >>> Or if these *optimizations* must be present in the current patch >>> series, could you, please, provide me with some hints how >>> these TODO should be properly implemented? >> >> I'm puzzled. When I first commented on these TODOs I did say >> "While I appreciate this not being done in the already large patch, >> I don't think such a TODO should be left around. If need be (e.g. >> because you can't test the change), get in touch with the >> maintainer(s)." Of course the "e.g." extends to the actual >> implementation. IOW I'm not saying you need to do this work >> immediately and all by yourself, but there should be a clear plan >> on getting these items addressed. We shouldn't ship several >> releases with them still present. I'm sorry this hits you, but we've >> had too bad experience in the past with people leaving todo or >> fixme notes in the code, perhaps even promising to address them >> without ever doing so. > I see. You are right about leaving TODO) > Don't mind to get these items addressed *within current patch series* > as separate patch or patches. > So, we have to address for three IOMMUs: Intel/AMD and SMMU. I will > leave SMMU for myself. > > Could you, please, provide me with some hints how these TODO should be > properly implemented? I have to admit that I don't really understand the request. Quite clearly we want to use large pages in the case that hardware supports them. > I was thinking I can even just squash *pages with *page and send you a > draft as we need to start from somewhere. I'm afraid I've lost too much of the context to see what you mean here. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-16 13:11 ` Jan Beulich @ 2017-05-17 15:28 ` Oleksandr Tyshchenko 2017-05-17 15:39 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-17 15:28 UTC (permalink / raw) To: Jan Beulich Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel Hi, Jan. On Tue, May 16, 2017 at 4:11 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 16.05.17 at 14:48, <olekstysh@gmail.com> wrote: >> On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 15.05.17 at 12:43, <olekstysh@gmail.com> wrote: >>>> Indeed, there was some misunderstanding from my side on this. >>>> Let me elaborate a bit more on this: >>>> 1. Yes, this TODO shouldn't be just dropped, but needs to be >>>> addressed, so at least I will have them back in the patch >>>> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so >>>> it makes me lots of work to do this change >>>> properly, so this is not only the question of testing the code, but rather >>>> having it written. >>>> 3. Please correct me if I'm wrong, but these are all *optimizations* which >>>> I am mentioning in that TODO, not something that breaks x86 or affects it >>>> in any way. >>>> >>>> That being said, can we postpone implementation of the *optimizations* >>>> in question >>>> and have those as a separate activity? >>>> Or if these *optimizations* must be present in the current patch >>>> series, could you, please, provide me with some hints how >>>> these TODO should be properly implemented? >>> >>> I'm puzzled. When I first commented on these TODOs I did say >>> "While I appreciate this not being done in the already large patch, >>> I don't think such a TODO should be left around. If need be (e.g. >>> because you can't test the change), get in touch with the >>> maintainer(s)." Of course the "e.g." extends to the actual >>> implementation. IOW I'm not saying you need to do this work >>> immediately and all by yourself, but there should be a clear plan >>> on getting these items addressed. We shouldn't ship several >>> releases with them still present. I'm sorry this hits you, but we've >>> had too bad experience in the past with people leaving todo or >>> fixme notes in the code, perhaps even promising to address them >>> without ever doing so. >> I see. You are right about leaving TODO) >> Don't mind to get these items addressed *within current patch series* >> as separate patch or patches. >> So, we have to address for three IOMMUs: Intel/AMD and SMMU. I will >> leave SMMU for myself. >> >> Could you, please, provide me with some hints how these TODO should be >> properly implemented? > > I have to admit that I don't really understand the request. Quite > clearly we want to use large pages in the case that hardware > supports them. > >> I was thinking I can even just squash *pages with *page and send you a >> draft as we need to start from somewhere. > > I'm afraid I've lost too much of the context to see what you mean > here. Sorry if I was unclear. At the moment patch contains three TODOs in the following files: 1. a/xen/drivers/passthrough/vtd/iommu.c 2. a/xen/drivers/passthrough/amd/iommu_map.c 3. a/xen/drivers/passthrough/arm/smmu.c And the *optimization* which I mentioned in that TODO is same for all three files. +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ I think that I could try to address this TODO by myself as I imagine it should be addressed and send you a draft or post RFC patch. As the result of this RFC patch we would have map_pages/unmap_pages callbacks only, but still iterate 4K pages. We need to start from somewhere and this patch would be a base point for continue optimizing. What do you think? Or you have another opinion? > > Jan > -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-17 15:28 ` Oleksandr Tyshchenko @ 2017-05-17 15:39 ` Jan Beulich 2017-05-17 18:49 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-17 15:39 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel >>> On 17.05.17 at 17:28, <olekstysh@gmail.com> wrote: > Hi, Jan. > > On Tue, May 16, 2017 at 4:11 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 16.05.17 at 14:48, <olekstysh@gmail.com> wrote: >>> On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 15.05.17 at 12:43, <olekstysh@gmail.com> wrote: >>>>> Indeed, there was some misunderstanding from my side on this. >>>>> Let me elaborate a bit more on this: >>>>> 1. Yes, this TODO shouldn't be just dropped, but needs to be >>>>> addressed, so at least I will have them back in the patch >>>>> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so >>>>> it makes me lots of work to do this change >>>>> properly, so this is not only the question of testing the code, but rather >>>>> having it written. >>>>> 3. Please correct me if I'm wrong, but these are all *optimizations* which >>>>> I am mentioning in that TODO, not something that breaks x86 or affects it >>>>> in any way. >>>>> >>>>> That being said, can we postpone implementation of the *optimizations* >>>>> in question >>>>> and have those as a separate activity? >>>>> Or if these *optimizations* must be present in the current patch >>>>> series, could you, please, provide me with some hints how >>>>> these TODO should be properly implemented? >>>> >>>> I'm puzzled. When I first commented on these TODOs I did say >>>> "While I appreciate this not being done in the already large patch, >>>> I don't think such a TODO should be left around. If need be (e.g. >>>> because you can't test the change), get in touch with the >>>> maintainer(s)." Of course the "e.g." extends to the actual >>>> implementation. IOW I'm not saying you need to do this work >>>> immediately and all by yourself, but there should be a clear plan >>>> on getting these items addressed. We shouldn't ship several >>>> releases with them still present. I'm sorry this hits you, but we've >>>> had too bad experience in the past with people leaving todo or >>>> fixme notes in the code, perhaps even promising to address them >>>> without ever doing so. >>> I see. You are right about leaving TODO) >>> Don't mind to get these items addressed *within current patch series* >>> as separate patch or patches. >>> So, we have to address for three IOMMUs: Intel/AMD and SMMU. I will >>> leave SMMU for myself. >>> >>> Could you, please, provide me with some hints how these TODO should be >>> properly implemented? >> >> I have to admit that I don't really understand the request. Quite >> clearly we want to use large pages in the case that hardware >> supports them. >> >>> I was thinking I can even just squash *pages with *page and send you a >>> draft as we need to start from somewhere. >> >> I'm afraid I've lost too much of the context to see what you mean >> here. > Sorry if I was unclear. > > At the moment patch contains three TODOs in the following files: > 1. a/xen/drivers/passthrough/vtd/iommu.c > 2. a/xen/drivers/passthrough/amd/iommu_map.c > 3. a/xen/drivers/passthrough/arm/smmu.c > > And the *optimization* which I mentioned in that TODO is same for all > three files. > +/* TODO: Optimize by squashing map_pages/unmap_pages with > map_page/unmap_page */ > > I think that I could try to address this TODO by myself as I imagine > it should be addressed and send you a draft or post RFC patch. > As the result of this RFC patch we would have map_pages/unmap_pages > callbacks only, but still iterate 4K pages. > > We need to start from somewhere and this patch would be a base point > for continue optimizing. > What do you think? Or you have another opinion? Well, yes, this would reduce the scope of the TODO, but no, it wouldn't eliminate it. After all the primary goal (from my perspective) of adding the order parameter is to make use of large pages whenever possible. And as said before - it's not like it has to be you who does the work, but I'd sort of expect that whoever introduces TODOs at least tries to arrange for them being taken care of (unless e.g. they affect exotic situations only), for example by pulling in the respective maintainers. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks 2017-05-17 15:39 ` Jan Beulich @ 2017-05-17 18:49 ` Oleksandr Tyshchenko 0 siblings, 0 replies; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-17 18:49 UTC (permalink / raw) To: Jan Beulich Cc: kevin.tian, Stefano Stabellini, Wei Liu, Ian Jackson, Julien Grall, suravee.suthikulpanit, xen-devel Hi, all. On Wed, May 17, 2017 at 6:39 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 17.05.17 at 17:28, <olekstysh@gmail.com> wrote: >> Hi, Jan. >> >> On Tue, May 16, 2017 at 4:11 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 16.05.17 at 14:48, <olekstysh@gmail.com> wrote: >>>> On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>> On 15.05.17 at 12:43, <olekstysh@gmail.com> wrote: >>>>>> Indeed, there was some misunderstanding from my side on this. >>>>>> Let me elaborate a bit more on this: >>>>>> 1. Yes, this TODO shouldn't be just dropped, but needs to be >>>>>> addressed, so at least I will have them back in the patch >>>>>> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so >>>>>> it makes me lots of work to do this change >>>>>> properly, so this is not only the question of testing the code, but rather >>>>>> having it written. >>>>>> 3. Please correct me if I'm wrong, but these are all *optimizations* which >>>>>> I am mentioning in that TODO, not something that breaks x86 or affects it >>>>>> in any way. >>>>>> >>>>>> That being said, can we postpone implementation of the *optimizations* >>>>>> in question >>>>>> and have those as a separate activity? >>>>>> Or if these *optimizations* must be present in the current patch >>>>>> series, could you, please, provide me with some hints how >>>>>> these TODO should be properly implemented? >>>>> >>>>> I'm puzzled. When I first commented on these TODOs I did say >>>>> "While I appreciate this not being done in the already large patch, >>>>> I don't think such a TODO should be left around. If need be (e.g. >>>>> because you can't test the change), get in touch with the >>>>> maintainer(s)." Of course the "e.g." extends to the actual >>>>> implementation. IOW I'm not saying you need to do this work >>>>> immediately and all by yourself, but there should be a clear plan >>>>> on getting these items addressed. We shouldn't ship several >>>>> releases with them still present. I'm sorry this hits you, but we've >>>>> had too bad experience in the past with people leaving todo or >>>>> fixme notes in the code, perhaps even promising to address them >>>>> without ever doing so. >>>> I see. You are right about leaving TODO) >>>> Don't mind to get these items addressed *within current patch series* >>>> as separate patch or patches. >>>> So, we have to address for three IOMMUs: Intel/AMD and SMMU. I will >>>> leave SMMU for myself. >>>> >>>> Could you, please, provide me with some hints how these TODO should be >>>> properly implemented? >>> >>> I have to admit that I don't really understand the request. Quite >>> clearly we want to use large pages in the case that hardware >>> supports them. >>> >>>> I was thinking I can even just squash *pages with *page and send you a >>>> draft as we need to start from somewhere. >>> >>> I'm afraid I've lost too much of the context to see what you mean >>> here. >> Sorry if I was unclear. >> >> At the moment patch contains three TODOs in the following files: >> 1. a/xen/drivers/passthrough/vtd/iommu.c >> 2. a/xen/drivers/passthrough/amd/iommu_map.c >> 3. a/xen/drivers/passthrough/arm/smmu.c >> >> And the *optimization* which I mentioned in that TODO is same for all >> three files. >> +/* TODO: Optimize by squashing map_pages/unmap_pages with >> map_page/unmap_page */ >> >> I think that I could try to address this TODO by myself as I imagine >> it should be addressed and send you a draft or post RFC patch. >> As the result of this RFC patch we would have map_pages/unmap_pages >> callbacks only, but still iterate 4K pages. >> >> We need to start from somewhere and this patch would be a base point >> for continue optimizing. >> What do you think? Or you have another opinion? > > Well, yes, this would reduce the scope of the TODO, but no, it > wouldn't eliminate it. After all the primary goal (from my > perspective) of adding the order parameter is to make use of > large pages whenever possible. And as said before - it's not like > it has to be you who does the work, but I'd sort of expect that > whoever introduces TODOs at least tries to arrange for them > being taken care of (unless e.g. they affect exotic situations > only), for example by pulling in the respective maintainers. Jan, I understand what you are taking about. CCed respective maintainers. Kevin, Suravee, First of all my apologies for the fact that I haven't CCed your earlier. I added changes to files you are the maintainers of. My bad. A bit of context. This patch touches Intel/AMD IOMMUs as well as other IOMMUs since it adds extra order argument. The purpose of adding extra order argument is to make use of super pages whenever possible. Being honest I am interested in adding IPMMU support on ARM and this kind of IOMMU does support super pages. And as we don't want to keep separate single-page and multi-page stuff together it was decided to rename existing APIs/callbacks and add order argument. In order not to brake existing x86-specific drivers (to retain current behavior) I had to introduce additional helpers inside them and leave some TODO which describe that some optimization is needed. I can try to reduce the scope of these TODO (to have map_pages/unmap_pages callbacks only, but still iterate 4K pages even if hardware supports large pages), but I am sure that I won't be able to eliminate them completely (to use large pages in the case that hardware supports them) due to the several reasons. I am neither familiar with x86 nor even have x86 boards, excuse me, but I don't even know support these hardware super pages or not. I want this patch to be accepted, so some common ground should be found on getting these items addressed. Maybe you already have some plan regarding adding such support? > > Jan -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v1 03/10] xen/arm: p2m: Add helper to convert p2m type to IOMMU flags 2017-05-10 14:03 [PATCH v1 00/10] "Non-shared" IOMMU support on ARM Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 01/10] xen/device-tree: Add dt_count_phandle_with_args helper Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks Oleksandr Tyshchenko @ 2017-05-10 14:03 ` Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 04/10] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared Oleksandr Tyshchenko ` (6 subsequent siblings) 9 siblings, 0 replies; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-10 14:03 UTC (permalink / raw) To: xen-devel; +Cc: wei.liu2, julien.grall, sstabellini, ian.jackson, jbeulich From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> The helper has the same purpose as existing for x86 one. It is used for choosing IOMMU mapping attribute according to the memory type. Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Reviewed-by: Julien Grall <julien.grall@arm.com> --- Changes in v1: - Add Julien's reviewed-by --- xen/include/asm-arm/p2m.h | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 18c57f9..9082ba0 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -5,6 +5,7 @@ #include <xen/radix-tree.h> #include <xen/rwlock.h> #include <xen/mem_access.h> +#include <xen/iommu.h> #include <public/vm_event.h> /* for vm_event_response_t */ #include <public/memory.h> #include <xen/p2m-common.h> @@ -357,6 +358,39 @@ static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order) return gfn_add(gfn, 1UL << order); } +/* + * p2m type to IOMMU flags + */ +static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt) +{ + unsigned int flags; + + switch( p2mt ) + { + case p2m_ram_rw: + case p2m_iommu_map_rw: + case p2m_map_foreign: + case p2m_grant_map_rw: + case p2m_mmio_direct_dev: + case p2m_mmio_direct_nc: + case p2m_mmio_direct_c: + flags = IOMMUF_readable | IOMMUF_writable; + break; + case p2m_ram_ro: + case p2m_iommu_map_ro: + case p2m_grant_map_ro: + flags = IOMMUF_readable; + break; + default: + flags = 0; + break; + } + + /* TODO Do we need to handle access permissions here? */ + + return flags; +} + #endif /* _XEN_P2M_H */ /* -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v1 04/10] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared 2017-05-10 14:03 [PATCH v1 00/10] "Non-shared" IOMMU support on ARM Oleksandr Tyshchenko ` (2 preceding siblings ...) 2017-05-10 14:03 ` [PATCH v1 03/10] xen/arm: p2m: Add helper to convert p2m type to IOMMU flags Oleksandr Tyshchenko @ 2017-05-10 14:03 ` Oleksandr Tyshchenko 2017-05-11 11:24 ` Julien Grall 2017-05-10 14:03 ` [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share Oleksandr Tyshchenko ` (5 subsequent siblings) 9 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-10 14:03 UTC (permalink / raw) To: xen-devel; +Cc: wei.liu2, julien.grall, sstabellini, ian.jackson, jbeulich From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Update IOMMU mapping if the IOMMU doesn't share page table with the CPU. The best place to do so on ARM is __p2m_set_entry(). Use mfn as an indicator of the required action. If mfn is valid call iommu_map_pages(), otherwise - iommu_unmap_pages(). Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> CC: Julien Grall <julien.grall@arm.com> --- Changes in v1: - Update IOMMU mapping in __p2m_set_entry() instead of p2m_set_entry(). - Pass order argument to IOMMU APIs instead of page_count. --- xen/arch/arm/p2m.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 34d5776..9ca491b 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -984,7 +984,15 @@ static int __p2m_set_entry(struct p2m_domain *p2m, p2m_free_entry(p2m, orig_pte, level); if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || p2m_valid(*entry)) ) - rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order); + { + if ( iommu_use_hap_pt(p2m->domain) ) + rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order); + else if ( !mfn_eq(smfn, INVALID_MFN) ) + rc = iommu_map_pages(p2m->domain, gfn_x(sgfn), mfn_x(smfn), + page_order, p2m_get_iommu_flags(t)); + else + rc = iommu_unmap_pages(p2m->domain, gfn_x(sgfn), page_order); + } else rc = 0; -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v1 04/10] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared 2017-05-10 14:03 ` [PATCH v1 04/10] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared Oleksandr Tyshchenko @ 2017-05-11 11:24 ` Julien Grall 2017-05-11 14:19 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Julien Grall @ 2017-05-11 11:24 UTC (permalink / raw) To: Oleksandr Tyshchenko, xen-devel Cc: wei.liu2, sstabellini, ian.jackson, jbeulich Hi Oleksandr, On 10/05/17 15:03, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Update IOMMU mapping if the IOMMU doesn't share page table with the CPU. > The best place to do so on ARM is __p2m_set_entry(). Use mfn as an indicator > of the required action. If mfn is valid call iommu_map_pages(), > otherwise - iommu_unmap_pages(). > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > CC: Julien Grall <julien.grall@arm.com> Acked-by: Julien Grall <julien.grall@arm.com> > > --- > Changes in v1: > - Update IOMMU mapping in __p2m_set_entry() instead of p2m_set_entry(). > - Pass order argument to IOMMU APIs instead of page_count. > --- > xen/arch/arm/p2m.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 34d5776..9ca491b 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -984,7 +984,15 @@ static int __p2m_set_entry(struct p2m_domain *p2m, > p2m_free_entry(p2m, orig_pte, level); > > if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || p2m_valid(*entry)) ) > - rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order); > + { > + if ( iommu_use_hap_pt(p2m->domain) ) > + rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order); > + else if ( !mfn_eq(smfn, INVALID_MFN) ) > + rc = iommu_map_pages(p2m->domain, gfn_x(sgfn), mfn_x(smfn), > + page_order, p2m_get_iommu_flags(t)); > + else > + rc = iommu_unmap_pages(p2m->domain, gfn_x(sgfn), page_order); > + } > else > rc = 0; > > Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 04/10] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared 2017-05-11 11:24 ` Julien Grall @ 2017-05-11 14:19 ` Oleksandr Tyshchenko 0 siblings, 0 replies; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-11 14:19 UTC (permalink / raw) To: Julien Grall Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu, Jan Beulich On Thu, May 11, 2017 at 2:24 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi Julien > > On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Update IOMMU mapping if the IOMMU doesn't share page table with the CPU. >> The best place to do so on ARM is __p2m_set_entry(). Use mfn as an >> indicator >> of the required action. If mfn is valid call iommu_map_pages(), >> otherwise - iommu_unmap_pages(). >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> CC: Julien Grall <julien.grall@arm.com> > > > Acked-by: Julien Grall <julien.grall@arm.com> Great. Thank you. > > >> >> --- >> Changes in v1: >> - Update IOMMU mapping in __p2m_set_entry() instead of >> p2m_set_entry(). >> - Pass order argument to IOMMU APIs instead of page_count. >> --- >> xen/arch/arm/p2m.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 34d5776..9ca491b 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -984,7 +984,15 @@ static int __p2m_set_entry(struct p2m_domain *p2m, >> p2m_free_entry(p2m, orig_pte, level); >> >> if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || >> p2m_valid(*entry)) ) >> - rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << >> page_order); >> + { >> + if ( iommu_use_hap_pt(p2m->domain) ) >> + rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << >> page_order); >> + else if ( !mfn_eq(smfn, INVALID_MFN) ) >> + rc = iommu_map_pages(p2m->domain, gfn_x(sgfn), mfn_x(smfn), >> + page_order, p2m_get_iommu_flags(t)); >> + else >> + rc = iommu_unmap_pages(p2m->domain, gfn_x(sgfn), page_order); >> + } >> else >> rc = 0; >> >> > > Cheers, > > -- > Julien Grall -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share 2017-05-10 14:03 [PATCH v1 00/10] "Non-shared" IOMMU support on ARM Oleksandr Tyshchenko ` (3 preceding siblings ...) 2017-05-10 14:03 ` [PATCH v1 04/10] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared Oleksandr Tyshchenko @ 2017-05-10 14:03 ` Oleksandr Tyshchenko 2017-05-11 11:28 ` Julien Grall 2017-05-10 14:03 ` [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() Oleksandr Tyshchenko ` (4 subsequent siblings) 9 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-10 14:03 UTC (permalink / raw) To: xen-devel; +Cc: wei.liu2, julien.grall, sstabellini, ian.jackson, jbeulich From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Not every integrated into ARM SoCs IOMMU can share page tables with the CPU and as result the iommu_use_hap_pt(d) is not always true. Reuse x86's iommu_hap_pt_share flag to indicate whether the IOMMU page table is shared or not. Now all IOMMU drivers on ARM are able to change this flag according to their possibilities like x86-variants do. Therefore set iommu_hap_pt_share flag for SMMU because it always shares page table with the CPU. Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> --- xen/drivers/passthrough/arm/smmu.c | 3 +++ xen/include/asm-arm/iommu.h | 7 +++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index 527a592..86ee12a 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -2870,6 +2870,9 @@ static __init int arm_smmu_dt_init(struct dt_device_node *dev, platform_features &= smmu->features; + /* Always share P2M table between the CPU and the SMMU */ + iommu_hap_pt_share = true; + return 0; } diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h index 57d9b1e..10a6f23 100644 --- a/xen/include/asm-arm/iommu.h +++ b/xen/include/asm-arm/iommu.h @@ -20,8 +20,11 @@ struct arch_iommu void *priv; }; -/* Always share P2M Table between the CPU and the IOMMU */ -#define iommu_use_hap_pt(d) (1) +/* + * The ARM domain always has a P2M table, but not every integrated into + * ARM SoCs IOMMU can use it as page table. + */ +#define iommu_use_hap_pt(d) (iommu_hap_pt_share) const struct iommu_ops *iommu_get_ops(void); void __init iommu_set_ops(const struct iommu_ops *ops); -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share 2017-05-10 14:03 ` [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share Oleksandr Tyshchenko @ 2017-05-11 11:28 ` Julien Grall 2017-05-11 14:38 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Julien Grall @ 2017-05-11 11:28 UTC (permalink / raw) To: Oleksandr Tyshchenko, xen-devel Cc: wei.liu2, sstabellini, ian.jackson, jbeulich Hi Oleksandr, On 10/05/17 15:03, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Not every integrated into ARM SoCs IOMMU can share page tables > with the CPU and as result the iommu_use_hap_pt(d) is not always true. > Reuse x86's iommu_hap_pt_share flag to indicate whether the IOMMU > page table is shared or not. > > Now all IOMMU drivers on ARM are able to change this flag > according to their possibilities like x86-variants do. > Therefore set iommu_hap_pt_share flag for SMMU because it always shares > page table with the CPU. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > xen/drivers/passthrough/arm/smmu.c | 3 +++ > xen/include/asm-arm/iommu.h | 7 +++++-- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c > index 527a592..86ee12a 100644 > --- a/xen/drivers/passthrough/arm/smmu.c > +++ b/xen/drivers/passthrough/arm/smmu.c > @@ -2870,6 +2870,9 @@ static __init int arm_smmu_dt_init(struct dt_device_node *dev, > > platform_features &= smmu->features; > > + /* Always share P2M table between the CPU and the SMMU */ > + iommu_hap_pt_share = true; > + I would prefer to bail-out if someone try to unshare the page-table rather than overriding. This would help us to know if someone are try to do that. So I would do: if ( !iommu_hap_pt_share ) { printk(....) return -EINVAL; } > return 0; > } > > diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h > index 57d9b1e..10a6f23 100644 > --- a/xen/include/asm-arm/iommu.h > +++ b/xen/include/asm-arm/iommu.h > @@ -20,8 +20,11 @@ struct arch_iommu > void *priv; > }; > > -/* Always share P2M Table between the CPU and the IOMMU */ > -#define iommu_use_hap_pt(d) (1) > +/* > + * The ARM domain always has a P2M table, but not every integrated into > + * ARM SoCs IOMMU can use it as page table. The first part: "ARM domain has a P2M table" is pretty obvious. I would instead say: "Not every ARM SoCs IOMMU use the same page-table format as the processor.". > + */ > +#define iommu_use_hap_pt(d) (iommu_hap_pt_share) > > const struct iommu_ops *iommu_get_ops(void); > void __init iommu_set_ops(const struct iommu_ops *ops); > Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share 2017-05-11 11:28 ` Julien Grall @ 2017-05-11 14:38 ` Oleksandr Tyshchenko 2017-05-11 17:58 ` Julien Grall 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-11 14:38 UTC (permalink / raw) To: Julien Grall Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu, Jan Beulich On Thu, May 11, 2017 at 2:28 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi Julien > > On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Not every integrated into ARM SoCs IOMMU can share page tables >> with the CPU and as result the iommu_use_hap_pt(d) is not always true. >> Reuse x86's iommu_hap_pt_share flag to indicate whether the IOMMU >> page table is shared or not. >> >> Now all IOMMU drivers on ARM are able to change this flag >> according to their possibilities like x86-variants do. >> Therefore set iommu_hap_pt_share flag for SMMU because it always shares >> page table with the CPU. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> xen/drivers/passthrough/arm/smmu.c | 3 +++ >> xen/include/asm-arm/iommu.h | 7 +++++-- >> 2 files changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/xen/drivers/passthrough/arm/smmu.c >> b/xen/drivers/passthrough/arm/smmu.c >> index 527a592..86ee12a 100644 >> --- a/xen/drivers/passthrough/arm/smmu.c >> +++ b/xen/drivers/passthrough/arm/smmu.c >> @@ -2870,6 +2870,9 @@ static __init int arm_smmu_dt_init(struct >> dt_device_node *dev, >> >> platform_features &= smmu->features; >> >> + /* Always share P2M table between the CPU and the SMMU */ >> + iommu_hap_pt_share = true; >> + > > > I would prefer to bail-out if someone try to unshare the page-table rather > than overriding. This would help us to know if someone are try to do that. > > So I would do: > > if ( !iommu_hap_pt_share ) > { > printk(....) > return -EINVAL; > } I got it for SMMU. But, for IPMMU we will override since iommu_hap_pt_share is true by default. Right? /* * The IPMMU can't reuse P2M table since it only supports * stage-1 page tables. */ iommu_hap_pt_share = false; > >> return 0; >> } >> >> diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h >> index 57d9b1e..10a6f23 100644 >> --- a/xen/include/asm-arm/iommu.h >> +++ b/xen/include/asm-arm/iommu.h >> @@ -20,8 +20,11 @@ struct arch_iommu >> void *priv; >> }; >> >> -/* Always share P2M Table between the CPU and the IOMMU */ >> -#define iommu_use_hap_pt(d) (1) >> +/* >> + * The ARM domain always has a P2M table, but not every integrated into >> + * ARM SoCs IOMMU can use it as page table. > > > The first part: "ARM domain has a P2M table" is pretty obvious. I would > instead say: "Not every ARM SoCs IOMMU use the same page-table format as the > processor.". Agree. > >> + */ >> +#define iommu_use_hap_pt(d) (iommu_hap_pt_share) >> >> const struct iommu_ops *iommu_get_ops(void); >> void __init iommu_set_ops(const struct iommu_ops *ops); >> > > Cheers, > > -- > Julien Grall -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share 2017-05-11 14:38 ` Oleksandr Tyshchenko @ 2017-05-11 17:58 ` Julien Grall 2017-05-11 18:21 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Julien Grall @ 2017-05-11 17:58 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu, Jan Beulich On 11/05/17 15:38, Oleksandr Tyshchenko wrote: > On Thu, May 11, 2017 at 2:28 PM, Julien Grall <julien.grall@arm.com> wrote: >> Hi Oleksandr, > Hi Julien Hi Oleksandr, >> >> On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >>> >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> Not every integrated into ARM SoCs IOMMU can share page tables >>> with the CPU and as result the iommu_use_hap_pt(d) is not always true. >>> Reuse x86's iommu_hap_pt_share flag to indicate whether the IOMMU >>> page table is shared or not. >>> >>> Now all IOMMU drivers on ARM are able to change this flag >>> according to their possibilities like x86-variants do. >>> Therefore set iommu_hap_pt_share flag for SMMU because it always shares >>> page table with the CPU. >>> >>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> --- >>> xen/drivers/passthrough/arm/smmu.c | 3 +++ >>> xen/include/asm-arm/iommu.h | 7 +++++-- >>> 2 files changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/drivers/passthrough/arm/smmu.c >>> b/xen/drivers/passthrough/arm/smmu.c >>> index 527a592..86ee12a 100644 >>> --- a/xen/drivers/passthrough/arm/smmu.c >>> +++ b/xen/drivers/passthrough/arm/smmu.c >>> @@ -2870,6 +2870,9 @@ static __init int arm_smmu_dt_init(struct >>> dt_device_node *dev, >>> >>> platform_features &= smmu->features; >>> >>> + /* Always share P2M table between the CPU and the SMMU */ >>> + iommu_hap_pt_share = true; >>> + >> >> >> I would prefer to bail-out if someone try to unshare the page-table rather >> than overriding. This would help us to know if someone are try to do that. >> >> So I would do: >> >> if ( !iommu_hap_pt_share ) >> { >> printk(....) >> return -EINVAL; >> } > I got it for SMMU. > > But, for IPMMU we will override since iommu_hap_pt_share is true by > default. Right? > > /* > * The IPMMU can't reuse P2M table since it only supports > * stage-1 page tables. > */ > iommu_hap_pt_share = false; Good point. Looking at the other driver, they print a message to notify the override. (See drivers/passthrough/amd/iommu_init.c for instance). Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share 2017-05-11 17:58 ` Julien Grall @ 2017-05-11 18:21 ` Oleksandr Tyshchenko 0 siblings, 0 replies; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-11 18:21 UTC (permalink / raw) To: Julien Grall Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu, Jan Beulich On Thu, May 11, 2017 at 8:58 PM, Julien Grall <julien.grall@arm.com> wrote: > > > On 11/05/17 15:38, Oleksandr Tyshchenko wrote: >> >> On Thu, May 11, 2017 at 2:28 PM, Julien Grall <julien.grall@arm.com> >> wrote: >>> >>> Hi Oleksandr, >> >> Hi Julien > > > Hi Oleksandr, > > >>> >>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >>>> >>>> >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> Not every integrated into ARM SoCs IOMMU can share page tables >>>> with the CPU and as result the iommu_use_hap_pt(d) is not always true. >>>> Reuse x86's iommu_hap_pt_share flag to indicate whether the IOMMU >>>> page table is shared or not. >>>> >>>> Now all IOMMU drivers on ARM are able to change this flag >>>> according to their possibilities like x86-variants do. >>>> Therefore set iommu_hap_pt_share flag for SMMU because it always shares >>>> page table with the CPU. >>>> >>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> --- >>>> xen/drivers/passthrough/arm/smmu.c | 3 +++ >>>> xen/include/asm-arm/iommu.h | 7 +++++-- >>>> 2 files changed, 8 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/xen/drivers/passthrough/arm/smmu.c >>>> b/xen/drivers/passthrough/arm/smmu.c >>>> index 527a592..86ee12a 100644 >>>> --- a/xen/drivers/passthrough/arm/smmu.c >>>> +++ b/xen/drivers/passthrough/arm/smmu.c >>>> @@ -2870,6 +2870,9 @@ static __init int arm_smmu_dt_init(struct >>>> dt_device_node *dev, >>>> >>>> platform_features &= smmu->features; >>>> >>>> + /* Always share P2M table between the CPU and the SMMU */ >>>> + iommu_hap_pt_share = true; >>>> + >>> >>> >>> >>> I would prefer to bail-out if someone try to unshare the page-table >>> rather >>> than overriding. This would help us to know if someone are try to do >>> that. >>> >>> So I would do: >>> >>> if ( !iommu_hap_pt_share ) >>> { >>> printk(....) >>> return -EINVAL; >>> } >> >> I got it for SMMU. >> >> But, for IPMMU we will override since iommu_hap_pt_share is true by >> default. Right? >> >> /* >> * The IPMMU can't reuse P2M table since it only supports >> * stage-1 page tables. >> */ >> iommu_hap_pt_share = false; > > > Good point. Looking at the other driver, they print a message to notify the > override. (See drivers/passthrough/amd/iommu_init.c for instance). I will print a message too. > > Cheers, > > -- > Julien Grall -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() 2017-05-10 14:03 [PATCH v1 00/10] "Non-shared" IOMMU support on ARM Oleksandr Tyshchenko ` (4 preceding siblings ...) 2017-05-10 14:03 ` [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share Oleksandr Tyshchenko @ 2017-05-10 14:03 ` Oleksandr Tyshchenko 2017-05-12 14:31 ` Jan Beulich 2017-05-10 14:03 ` [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback Oleksandr Tyshchenko ` (3 subsequent siblings) 9 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-10 14:03 UTC (permalink / raw) To: xen-devel; +Cc: wei.liu2, julien.grall, sstabellini, ian.jackson, jbeulich From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> The presence of this flag lets us know that the guest has devices which will most likely be used for passthrough and as the result the use of IOMMU is expected for this domain. In that case we have to call iommu_construct(), actually what the real assign_device call usually does. As iommu_domain_init() is called with use_iommu flag being forced to false for now, no functional change is intended for both ARM and x86. Basically, this patch is needed for non-shared IOMMUs on ARM only since the non-shared IOMMUs on x86 are ok if iommu_construct() is called later. But, in order to be more generic and for possible future optimization make this change appliable for both platforms. Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> CC: Jan Beulich <jbeulich@suse.com> --- Changes in v1: - Clarify patch subject/description. - s/bool_t/bool/ --- xen/arch/arm/domain.c | 2 +- xen/arch/x86/domain.c | 2 +- xen/drivers/passthrough/iommu.c | 11 +++++++++-- xen/include/xen/iommu.h | 2 +- 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 76310ed..ec19310 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -569,7 +569,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, ASSERT(config != NULL); /* p2m_init relies on some value initialized by the IOMMU subsystem */ - if ( (rc = iommu_domain_init(d)) != 0 ) + if ( (rc = iommu_domain_init(d, false)) != 0 ) goto fail; if ( (rc = p2m_init(d)) != 0 ) diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 90e2b1f..54037af 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -641,7 +641,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, if ( (rc = init_domain_irq_mapping(d)) != 0 ) goto fail; - if ( (rc = iommu_domain_init(d)) != 0 ) + if ( (rc = iommu_domain_init(d, false)) != 0 ) goto fail; } spin_lock_init(&d->arch.e820_lock); diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 3e9e4c3..c85f7b4 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -129,7 +129,7 @@ static void __init parse_iommu_param(char *s) } while ( ss ); } -int iommu_domain_init(struct domain *d) +int iommu_domain_init(struct domain *d, bool use_iommu) { struct domain_iommu *hd = dom_iommu(d); int ret = 0; @@ -142,7 +142,14 @@ int iommu_domain_init(struct domain *d) return 0; hd->platform_ops = iommu_get_ops(); - return hd->platform_ops->init(d); + ret = hd->platform_ops->init(d); + if ( ret ) + return ret; + + if ( use_iommu && !is_hardware_domain(d) ) + ret = iommu_construct(d); + + return ret; } static void __hwdom_init check_hwdom_reqs(struct domain *d) diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 3297998..3afbc3b 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -56,7 +56,7 @@ int iommu_setup(void); int iommu_add_device(struct pci_dev *pdev); int iommu_enable_device(struct pci_dev *pdev); int iommu_remove_device(struct pci_dev *pdev); -int iommu_domain_init(struct domain *d); +int iommu_domain_init(struct domain *d, bool use_iommu); void iommu_hwdom_init(struct domain *d); void iommu_domain_destroy(struct domain *d); int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn); -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() 2017-05-10 14:03 ` [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() Oleksandr Tyshchenko @ 2017-05-12 14:31 ` Jan Beulich 2017-05-12 17:00 ` Oleksandr Tyshchenko 2017-05-17 19:52 ` Julien Grall 0 siblings, 2 replies; 66+ messages in thread From: Jan Beulich @ 2017-05-12 14:31 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: ian.jackson, julien.grall, sstabellini, wei.liu2, xen-devel >>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > The presence of this flag lets us know that the guest > has devices which will most likely be used for passthrough > and as the result the use of IOMMU is expected for this domain. > In that case we have to call iommu_construct(), actually > what the real assign_device call usually does. > > As iommu_domain_init() is called with use_iommu flag being forced > to false for now, no functional change is intended for both ARM and x86. > > Basically, this patch is needed for non-shared IOMMUs on ARM only > since the non-shared IOMMUs on x86 are ok if iommu_construct() is called > later. But, in order to be more generic and for possible future optimization > make this change appliable for both platforms. I continue to be unconvinced that this is wanted / needed, as I continue to not see why shared vs unshared really matters here. After all we have both modes working on x86 without this flag. > @@ -142,7 +142,14 @@ int iommu_domain_init(struct domain *d) > return 0; > > hd->platform_ops = iommu_get_ops(); > - return hd->platform_ops->init(d); > + ret = hd->platform_ops->init(d); > + if ( ret ) > + return ret; > + > + if ( use_iommu && !is_hardware_domain(d) ) > + ret = iommu_construct(d); You don't handle the -ERESTART you may (and likely will) get here or in the caller. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() 2017-05-12 14:31 ` Jan Beulich @ 2017-05-12 17:00 ` Oleksandr Tyshchenko 2017-05-15 7:27 ` Jan Beulich 2017-05-17 19:52 ` Julien Grall 1 sibling, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-12 17:00 UTC (permalink / raw) To: Jan Beulich Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel Hi, Jan. On Fri, May 12, 2017 at 5:31 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> The presence of this flag lets us know that the guest >> has devices which will most likely be used for passthrough >> and as the result the use of IOMMU is expected for this domain. >> In that case we have to call iommu_construct(), actually >> what the real assign_device call usually does. >> >> As iommu_domain_init() is called with use_iommu flag being forced >> to false for now, no functional change is intended for both ARM and x86. >> >> Basically, this patch is needed for non-shared IOMMUs on ARM only >> since the non-shared IOMMUs on x86 are ok if iommu_construct() is called >> later. But, in order to be more generic and for possible future optimization >> make this change appliable for both platforms. > > I continue to be unconvinced that this is wanted / needed, as I > continue to not see why shared vs unshared really matters here. > After all we have both modes working on x86 without this flag. I know. But, for ARM we need this hint. We can't reuse the "retrieving mapping" code I moved to x86-specific part in patch #8 (due to lack of M2P on ARM) . > >> @@ -142,7 +142,14 @@ int iommu_domain_init(struct domain *d) >> return 0; >> >> hd->platform_ops = iommu_get_ops(); >> - return hd->platform_ops->init(d); >> + ret = hd->platform_ops->init(d); >> + if ( ret ) >> + return ret; >> + >> + if ( use_iommu && !is_hardware_domain(d) ) >> + ret = iommu_construct(d); > > You don't handle the -ERESTART you may (and likely will) get here > or in the caller. Indeed. I forgot about it. I most likely rework this patch not to call iommu_construct at all. But, there are an open questions here and in patch #7 I would like to clarify the first. 1. Are you against extra arguments at all? 2. If this question still makes sense, Shall I add extra need_iommu argument to "init" callback too and just pass thought incoming flag to IOMMU drivers? This change wouldn't affect x86 at all since we set this flag for ARM only (see patch #9). For x86 this flag would be always treated as unused. > > Jan > -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() 2017-05-12 17:00 ` Oleksandr Tyshchenko @ 2017-05-15 7:27 ` Jan Beulich 0 siblings, 0 replies; 66+ messages in thread From: Jan Beulich @ 2017-05-15 7:27 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel >>> On 12.05.17 at 19:00, <olekstysh@gmail.com> wrote: > On Fri, May 12, 2017 at 5:31 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> The presence of this flag lets us know that the guest >>> has devices which will most likely be used for passthrough >>> and as the result the use of IOMMU is expected for this domain. >>> In that case we have to call iommu_construct(), actually >>> what the real assign_device call usually does. >>> >>> As iommu_domain_init() is called with use_iommu flag being forced >>> to false for now, no functional change is intended for both ARM and x86. >>> >>> Basically, this patch is needed for non-shared IOMMUs on ARM only >>> since the non-shared IOMMUs on x86 are ok if iommu_construct() is called >>> later. But, in order to be more generic and for possible future optimization >>> make this change appliable for both platforms. >> >> I continue to be unconvinced that this is wanted / needed, as I >> continue to not see why shared vs unshared really matters here. >> After all we have both modes working on x86 without this flag. > I know. But, for ARM we need this hint. We can't reuse the "retrieving > mapping" code I moved to x86-specific part in patch #8 (due to lack of > M2P on ARM) . Well, see the reply to 08/10 I've sent a minute ago. >>> @@ -142,7 +142,14 @@ int iommu_domain_init(struct domain *d) >>> return 0; >>> >>> hd->platform_ops = iommu_get_ops(); >>> - return hd->platform_ops->init(d); >>> + ret = hd->platform_ops->init(d); >>> + if ( ret ) >>> + return ret; >>> + >>> + if ( use_iommu && !is_hardware_domain(d) ) >>> + ret = iommu_construct(d); >> >> You don't handle the -ERESTART you may (and likely will) get here >> or in the caller. > Indeed. I forgot about it. > > I most likely rework this patch not to call iommu_construct at all. > But, there are an open questions here and in patch #7 I would like to > clarify the first. > 1. Are you against extra arguments at all? No, extra arguments aren't the point. The kind of information passed is the questionable thing. > 2. If this question still makes sense, Shall I add extra need_iommu > argument to "init" callback too and just pass thought > incoming flag to IOMMU drivers? This change wouldn't affect x86 at > all since we set this flag for ARM only (see patch #9). > For x86 this flag would be always treated as unused. I won't give a firm answer here considering the wider question on M2P, but I'd like to say that always-unused flags on one architecture are often (but not always, namely not when there's a hardware feature one architecture has but others don't) a sign of a design/ abstraction problem. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() 2017-05-12 14:31 ` Jan Beulich 2017-05-12 17:00 ` Oleksandr Tyshchenko @ 2017-05-17 19:52 ` Julien Grall 2017-05-18 8:38 ` Jan Beulich 1 sibling, 1 reply; 66+ messages in thread From: Julien Grall @ 2017-05-17 19:52 UTC (permalink / raw) To: Jan Beulich, Oleksandr Tyshchenko Cc: ian.jackson, sstabellini, wei.liu2, xen-devel Hi Jan, On 05/12/2017 03:31 PM, Jan Beulich wrote: >>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> The presence of this flag lets us know that the guest >> has devices which will most likely be used for passthrough >> and as the result the use of IOMMU is expected for this domain. >> In that case we have to call iommu_construct(), actually >> what the real assign_device call usually does. >> >> As iommu_domain_init() is called with use_iommu flag being forced >> to false for now, no functional change is intended for both ARM and x86. >> >> Basically, this patch is needed for non-shared IOMMUs on ARM only >> since the non-shared IOMMUs on x86 are ok if iommu_construct() is called >> later. But, in order to be more generic and for possible future optimization >> make this change appliable for both platforms. > > I continue to be unconvinced that this is wanted / needed, as I > continue to not see why shared vs unshared really matters here. > After all we have both modes working on x86 without this flag. Well on x86 you allocate the page table on the fly in the unsharing case. This is only useful if you don't know whether a domain will have device assigned (e.g hotplug case). When you know that the domain will have device pass-throughed, you can populate the IOMMU page tables before hand avoiding to have to go through the list of page at the first assigned device. In embedded platform hotplug is likely to be inexistent. For servers, I don't know but likely page tables are going to be shared (or as I mentioned earlier partially shared). So I don't see any benefit of the current code over populating the IOMMU page tables from the beginning. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() 2017-05-17 19:52 ` Julien Grall @ 2017-05-18 8:38 ` Jan Beulich 2017-05-18 17:41 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-18 8:38 UTC (permalink / raw) To: Julien Grall, Oleksandr Tyshchenko Cc: ian.jackson, sstabellini, wei.liu2, xen-devel >>> On 17.05.17 at 21:52, <julien.grall@arm.com> wrote: > On 05/12/2017 03:31 PM, Jan Beulich wrote: >>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> The presence of this flag lets us know that the guest >>> has devices which will most likely be used for passthrough >>> and as the result the use of IOMMU is expected for this domain. >>> In that case we have to call iommu_construct(), actually >>> what the real assign_device call usually does. >>> >>> As iommu_domain_init() is called with use_iommu flag being forced >>> to false for now, no functional change is intended for both ARM and x86. >>> >>> Basically, this patch is needed for non-shared IOMMUs on ARM only >>> since the non-shared IOMMUs on x86 are ok if iommu_construct() is called >>> later. But, in order to be more generic and for possible future optimization >>> make this change appliable for both platforms. >> >> I continue to be unconvinced that this is wanted / needed, as I >> continue to not see why shared vs unshared really matters here. >> After all we have both modes working on x86 without this flag. > > Well on x86 you allocate the page table on the fly in the unsharing > case. This is only useful if you don't know whether a domain will have > device assigned (e.g hotplug case). > > When you know that the domain will have device pass-throughed, you can > populate the IOMMU page tables before hand avoiding to have to go > through the list of page at the first assigned device. > > In embedded platform hotplug is likely to be inexistent. For servers, I > don't know but likely page tables are going to be shared (or as I > mentioned earlier partially shared). > > So I don't see any benefit of the current code over populating the IOMMU > page tables from the beginning. Interesting. To me, the primary benefit is that we wouldn't need to introduce new code to handle yet another case specially. Anyway, the changes in this patch are simple enough, so I don't mean to block it despite being unconvinced of the basic idea. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() 2017-05-18 8:38 ` Jan Beulich @ 2017-05-18 17:41 ` Oleksandr Tyshchenko 2017-05-19 6:30 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-18 17:41 UTC (permalink / raw) To: Jan Beulich Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel Hi, all. On Thu, May 18, 2017 at 11:38 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 17.05.17 at 21:52, <julien.grall@arm.com> wrote: >> On 05/12/2017 03:31 PM, Jan Beulich wrote: >>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> The presence of this flag lets us know that the guest >>>> has devices which will most likely be used for passthrough >>>> and as the result the use of IOMMU is expected for this domain. >>>> In that case we have to call iommu_construct(), actually >>>> what the real assign_device call usually does. >>>> >>>> As iommu_domain_init() is called with use_iommu flag being forced >>>> to false for now, no functional change is intended for both ARM and x86. >>>> >>>> Basically, this patch is needed for non-shared IOMMUs on ARM only >>>> since the non-shared IOMMUs on x86 are ok if iommu_construct() is called >>>> later. But, in order to be more generic and for possible future optimization >>>> make this change appliable for both platforms. >>> >>> I continue to be unconvinced that this is wanted / needed, as I >>> continue to not see why shared vs unshared really matters here. >>> After all we have both modes working on x86 without this flag. >> >> Well on x86 you allocate the page table on the fly in the unsharing >> case. This is only useful if you don't know whether a domain will have >> device assigned (e.g hotplug case). >> >> When you know that the domain will have device pass-throughed, you can >> populate the IOMMU page tables before hand avoiding to have to go >> through the list of page at the first assigned device. >> >> In embedded platform hotplug is likely to be inexistent. For servers, I >> don't know but likely page tables are going to be shared (or as I >> mentioned earlier partially shared). >> >> So I don't see any benefit of the current code over populating the IOMMU >> page tables from the beginning. > > Interesting. To me, the primary benefit is that we wouldn't need to > introduce new code to handle yet another case specially. Anyway, > the changes in this patch are simple enough, so I don't mean to > block it despite being unconvinced of the basic idea. Thank you for your comments. I would like to say that I share Julien's opinion, but understand Jan's points too. I think some mutually agreeable solution should be worked out. It is not completely clear to me what I have to do with patches #6-#8. So, I will try to summarize thoughts regarding them. Please, correct me if I am wrong. patch #6: As for the current patch, likely the "init" platform callback should be extended with extra "use_iommu" argument as well as the "iommu_domain_init" API. In that case we would just pass thought incoming flag to the IOMMU drivers followed by updating "need_iommu" domain flag. Something like this: ... int iommu_domain_init(struct domain *d, bool use_iommu) { struct domain_iommu *hd = dom_iommu(d); int ret = 0; ret = arch_iommu_domain_init(d); if ( ret ) return ret; if ( !iommu_enabled ) return 0; hd->platform_ops = iommu_get_ops(); ret = hd->platform_ops->init(d, use_iommu); if ( ret ) return ret; d->need_iommu = !!use_iommu; return 0; } ... patch #7: This patch should be just dropped. patch #8: As we always allocate the page table for hardware domain, this patch should be reworked. The use_iommu flag should be set for both archs in case of hardware domain. Having d->need_iommu set at the early stage we won't skip IOMMU mapping updates anymore. And as the result the existing in iommu_hwdom_init() code that goes through the list of page and tries to retrieve mapping could be just dropped instead of moving it to the arch-specific part. So, what we would have as the final result: 1. In case of hardware domain "use_iommu" flag is always set for both ARM and x86. 2. For other domains the "use_iommu" flag is always unset for x86 only, but the real value is passed for ARM according to the libxl expectation about IOMMU usage. This would allow us to allocate the page table in advance on ARM and retain the current behavior for x86 (allocating the page table on-demand). What do you think? > > Jan > -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() 2017-05-18 17:41 ` Oleksandr Tyshchenko @ 2017-05-19 6:30 ` Jan Beulich 2017-05-19 8:56 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-19 6:30 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel >>> On 18.05.17 at 19:41, <olekstysh@gmail.com> wrote: > patch #6: As for the current patch, likely the "init" platform > callback should be extended with > extra "use_iommu" argument as well as the "iommu_domain_init" API. In > that case we > would just pass thought incoming flag to the IOMMU drivers followed by > updating "need_iommu" domain flag. > > Something like this: > ... > int iommu_domain_init(struct domain *d, bool use_iommu) > { > struct domain_iommu *hd = dom_iommu(d); > int ret = 0; > > ret = arch_iommu_domain_init(d); > if ( ret ) > return ret; > > if ( !iommu_enabled ) > return 0; > > hd->platform_ops = iommu_get_ops(); > ret = hd->platform_ops->init(d, use_iommu); > if ( ret ) > return ret; > > d->need_iommu = !!use_iommu; > > return 0; > } > ... The final shape of this primarily depends on ARM side needs. However, you need to be careful to make sure the final setting of d->need_iommu then is no different than it is today on at least x86. I'm mentioning this in particular because of e.g. d->need_iommu = !!iommu_dom0_strict; in iommu_hwdom_init(). Also as a minor remark note that in your new code the !! would not be needed. > patch #7: This patch should be just dropped. > > patch #8: As we always allocate the page table for hardware domain, > this patch should be reworked. > The use_iommu flag should be set for both archs in case of hardware > domain. Having d->need_iommu set at the early stage we won't skip > IOMMU > mapping updates anymore. And as the result the existing in > iommu_hwdom_init() code that goes through the list of page and tries > to retrieve mapping could be just dropped > instead of moving it to the arch-specific part. And again, careful here: There are three command line options influencing which pages do actually get mapped, and in which way (iommu=dom0-passthrough, iommu=dom0-strict, and VT-d's iommu_inclusive_mapping). The behavior after your change must not differ from current behavior regardless of which of these options may be used. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() 2017-05-19 6:30 ` Jan Beulich @ 2017-05-19 8:56 ` Oleksandr Tyshchenko 0 siblings, 0 replies; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-19 8:56 UTC (permalink / raw) To: Jan Beulich Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel Hi, Jan On Fri, May 19, 2017 at 9:30 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 18.05.17 at 19:41, <olekstysh@gmail.com> wrote: >> patch #6: As for the current patch, likely the "init" platform >> callback should be extended with >> extra "use_iommu" argument as well as the "iommu_domain_init" API. In >> that case we >> would just pass thought incoming flag to the IOMMU drivers followed by >> updating "need_iommu" domain flag. >> >> Something like this: >> ... >> int iommu_domain_init(struct domain *d, bool use_iommu) >> { >> struct domain_iommu *hd = dom_iommu(d); >> int ret = 0; >> >> ret = arch_iommu_domain_init(d); >> if ( ret ) >> return ret; >> >> if ( !iommu_enabled ) >> return 0; >> >> hd->platform_ops = iommu_get_ops(); >> ret = hd->platform_ops->init(d, use_iommu); >> if ( ret ) >> return ret; >> >> d->need_iommu = !!use_iommu; >> >> return 0; >> } >> ... > > The final shape of this primarily depends on ARM side needs. > However, you need to be careful to make sure the final setting > of d->need_iommu then is no different than it is today on at > least x86. I'm mentioning this in particular because of e.g. > > d->need_iommu = !!iommu_dom0_strict; > > in iommu_hwdom_init(). Yes, sure, I will take it into the account. > > Also as a minor remark note that in your new code the !! would > not be needed. > >> patch #7: This patch should be just dropped. >> >> patch #8: As we always allocate the page table for hardware domain, >> this patch should be reworked. >> The use_iommu flag should be set for both archs in case of hardware >> domain. Having d->need_iommu set at the early stage we won't skip >> IOMMU >> mapping updates anymore. And as the result the existing in >> iommu_hwdom_init() code that goes through the list of page and tries >> to retrieve mapping could be just dropped >> instead of moving it to the arch-specific part. > > And again, careful here: There are three command line options > influencing which pages do actually get mapped, and in which way > (iommu=dom0-passthrough, iommu=dom0-strict, and VT-d's > iommu_inclusive_mapping). The behavior after your change must > not differ from current behavior regardless of which of these > options may be used. Yes, sure. This is my target not to brake things. > > Jan > -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback 2017-05-10 14:03 [PATCH v1 00/10] "Non-shared" IOMMU support on ARM Oleksandr Tyshchenko ` (5 preceding siblings ...) 2017-05-10 14:03 ` [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() Oleksandr Tyshchenko @ 2017-05-10 14:03 ` Oleksandr Tyshchenko 2017-05-11 11:38 ` Julien Grall 2017-05-10 14:03 ` [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts Oleksandr Tyshchenko ` (2 subsequent siblings) 9 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-10 14:03 UTC (permalink / raw) To: xen-devel; +Cc: wei.liu2, julien.grall, sstabellini, ian.jackson, jbeulich From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> The alloc_page_table callback is a mandatory thing for the IOMMUs that don't share page table with the CPU on ARM. The non-shared IOMMUs have to perform all required actions here to be ready to handle IOMMU mapping updates right after completing it. The arch_iommu_populate_page_table() seems an appropriate place to call newly created callback. Since we will only be here for the non-shared IOMMUs always return error if the callback wasn't implemented. Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> CC: Jan Beulich <jbeulich@suse.com> --- Changes in V1: - Wrap callback in #ifdef CONFIG_ARM. --- xen/drivers/passthrough/arm/iommu.c | 5 +++-- xen/include/xen/iommu.h | 3 +++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c index 95b1abb..f132032 100644 --- a/xen/drivers/passthrough/arm/iommu.c +++ b/xen/drivers/passthrough/arm/iommu.c @@ -70,6 +70,7 @@ void arch_iommu_domain_destroy(struct domain *d) int arch_iommu_populate_page_table(struct domain *d) { - /* The IOMMU shares the p2m with the CPU */ - return -ENOSYS; + const struct iommu_ops *ops = iommu_get_ops(); + + return ops->alloc_page_table ? ops->alloc_page_table(d) : -ENOSYS; } diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 3afbc3b..f5914db 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -175,6 +175,9 @@ struct iommu_ops { unsigned int flags); int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn, unsigned int order); +#ifdef CONFIG_ARM + int (*alloc_page_table)(struct domain *d); +#endif /* CONFIG_ARM */ void (*free_page_table)(struct page_info *); #ifdef CONFIG_X86 void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value); -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback 2017-05-10 14:03 ` [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback Oleksandr Tyshchenko @ 2017-05-11 11:38 ` Julien Grall 2017-05-11 14:00 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Julien Grall @ 2017-05-11 11:38 UTC (permalink / raw) To: Oleksandr Tyshchenko, xen-devel Cc: wei.liu2, sstabellini, ian.jackson, jbeulich Hi Oleksandr, On 10/05/17 15:03, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > The alloc_page_table callback is a mandatory thing > for the IOMMUs that don't share page table with the CPU on ARM. > The non-shared IOMMUs have to perform all required actions here > to be ready to handle IOMMU mapping updates right after completing it. > > The arch_iommu_populate_page_table() seems an appropriate place > to call newly created callback. > Since we will only be here for the non-shared IOMMUs always > return error if the callback wasn't implemented. Why do you need a specific callback and not doing it directly in iommu_domain_init? My take here is in the unshare case, we may want to have multiple set of page tables (e.g one per device). So this should be left at the discretion of the IOMMU itself. Am I missing something? > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > CC: Jan Beulich <jbeulich@suse.com> > > --- > Changes in V1: > - Wrap callback in #ifdef CONFIG_ARM. > --- > xen/drivers/passthrough/arm/iommu.c | 5 +++-- > xen/include/xen/iommu.h | 3 +++ > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c > index 95b1abb..f132032 100644 > --- a/xen/drivers/passthrough/arm/iommu.c > +++ b/xen/drivers/passthrough/arm/iommu.c > @@ -70,6 +70,7 @@ void arch_iommu_domain_destroy(struct domain *d) > > int arch_iommu_populate_page_table(struct domain *d) > { > - /* The IOMMU shares the p2m with the CPU */ > - return -ENOSYS; > + const struct iommu_ops *ops = iommu_get_ops(); > + > + return ops->alloc_page_table ? ops->alloc_page_table(d) : -ENOSYS; > } > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > index 3afbc3b..f5914db 100644 > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -175,6 +175,9 @@ struct iommu_ops { > unsigned int flags); > int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn, > unsigned int order); > +#ifdef CONFIG_ARM > + int (*alloc_page_table)(struct domain *d); > +#endif /* CONFIG_ARM */ > void (*free_page_table)(struct page_info *); > #ifdef CONFIG_X86 > void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value); > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback 2017-05-11 11:38 ` Julien Grall @ 2017-05-11 14:00 ` Oleksandr Tyshchenko 2017-05-11 18:06 ` Julien Grall 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-11 14:00 UTC (permalink / raw) To: Julien Grall Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu, Jan Beulich On Thu, May 11, 2017 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi, Julien > > On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> The alloc_page_table callback is a mandatory thing >> for the IOMMUs that don't share page table with the CPU on ARM. >> The non-shared IOMMUs have to perform all required actions here >> to be ready to handle IOMMU mapping updates right after completing it. >> >> The arch_iommu_populate_page_table() seems an appropriate place >> to call newly created callback. >> Since we will only be here for the non-shared IOMMUs always >> return error if the callback wasn't implemented. > > > Why do you need a specific callback and not doing it directly in > iommu_domain_init? > > My take here is in the unshare case, we may want to have multiple set of > page tables (e.g one per device). So this should be left at the discretion > of the IOMMU itself. > > Am I missing something? I was thinking about extra need_iommu argument for init platform callback as I had done for iommu_domain_init API. But I had doubts regarding hw_domain. During iommu_domain_init execution we haven't known yet is the IOMMU expected for domain 0 or not. Taking into account that I needed to: - populate page table followed by setting need_iommu flag. - implement arch_iommu_populate_page_table() on ARM because of !iommu_use_hap_pt(d). - find a solution for hw_domain. I decided to use iommu_construct() and implement alloc_page_table callback to be called for populating page table. I thought that it would allow us to keep all required actions in a single place rather than spreading. > > >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> CC: Jan Beulich <jbeulich@suse.com> >> >> --- >> Changes in V1: >> - Wrap callback in #ifdef CONFIG_ARM. >> --- >> xen/drivers/passthrough/arm/iommu.c | 5 +++-- >> xen/include/xen/iommu.h | 3 +++ >> 2 files changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/xen/drivers/passthrough/arm/iommu.c >> b/xen/drivers/passthrough/arm/iommu.c >> index 95b1abb..f132032 100644 >> --- a/xen/drivers/passthrough/arm/iommu.c >> +++ b/xen/drivers/passthrough/arm/iommu.c >> @@ -70,6 +70,7 @@ void arch_iommu_domain_destroy(struct domain *d) >> >> int arch_iommu_populate_page_table(struct domain *d) >> { >> - /* The IOMMU shares the p2m with the CPU */ >> - return -ENOSYS; >> + const struct iommu_ops *ops = iommu_get_ops(); >> + >> + return ops->alloc_page_table ? ops->alloc_page_table(d) : -ENOSYS; >> } >> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >> index 3afbc3b..f5914db 100644 >> --- a/xen/include/xen/iommu.h >> +++ b/xen/include/xen/iommu.h >> @@ -175,6 +175,9 @@ struct iommu_ops { >> unsigned int flags); >> int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn, >> unsigned int order); >> +#ifdef CONFIG_ARM >> + int (*alloc_page_table)(struct domain *d); >> +#endif /* CONFIG_ARM */ >> void (*free_page_table)(struct page_info *); >> #ifdef CONFIG_X86 >> void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, >> unsigned int value); >> > > -- > Julien Grall -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback 2017-05-11 14:00 ` Oleksandr Tyshchenko @ 2017-05-11 18:06 ` Julien Grall 2017-05-11 18:43 ` Oleksandr Tyshchenko 2017-05-12 14:36 ` Jan Beulich 0 siblings, 2 replies; 66+ messages in thread From: Julien Grall @ 2017-05-11 18:06 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu, Jan Beulich Hi Oleksandr, On 11/05/17 15:00, Oleksandr Tyshchenko wrote: > On Thu, May 11, 2017 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote: >> Hi Oleksandr, > Hi, Julien > >> >> On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >>> >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> The alloc_page_table callback is a mandatory thing >>> for the IOMMUs that don't share page table with the CPU on ARM. >>> The non-shared IOMMUs have to perform all required actions here >>> to be ready to handle IOMMU mapping updates right after completing it. >>> >>> The arch_iommu_populate_page_table() seems an appropriate place >>> to call newly created callback. >>> Since we will only be here for the non-shared IOMMUs always >>> return error if the callback wasn't implemented. >> >> >> Why do you need a specific callback and not doing it directly in >> iommu_domain_init? >> >> My take here is in the unshare case, we may want to have multiple set of >> page tables (e.g one per device). So this should be left at the discretion >> of the IOMMU itself. >> >> Am I missing something? > I was thinking about extra need_iommu argument for init platform > callback as I had done for iommu_domain_init API. > But I had doubts regarding hw_domain. During iommu_domain_init > execution we haven't known yet is the IOMMU expected for domain 0 > or not. > > Taking into account that I needed to: > - populate page table followed by setting need_iommu flag. > - implement arch_iommu_populate_page_table() on ARM because of > !iommu_use_hap_pt(d). > - find a solution for hw_domain. > > I decided to use iommu_construct() and implement alloc_page_table > callback to be called for populating page table. > I thought that it would allow us to keep all required actions in a > single place rather than spreading. Looking at your patch #8, you always allocate the page table for hardware domain, so this is very similar to set iommu_enable in xen_arch_domain_config (see config. in start_xen). So this does not hold to me. Maybe Jan (IOMMU maintainer) has a different view on it. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback 2017-05-11 18:06 ` Julien Grall @ 2017-05-11 18:43 ` Oleksandr Tyshchenko 2017-05-12 14:36 ` Jan Beulich 1 sibling, 0 replies; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-11 18:43 UTC (permalink / raw) To: Julien Grall Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu, Jan Beulich On Thu, May 11, 2017 at 9:06 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi Julien > > > On 11/05/17 15:00, Oleksandr Tyshchenko wrote: >> >> On Thu, May 11, 2017 at 2:38 PM, Julien Grall <julien.grall@arm.com> >> wrote: >>> >>> Hi Oleksandr, >> >> Hi, Julien >> >>> >>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >>>> >>>> >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> The alloc_page_table callback is a mandatory thing >>>> for the IOMMUs that don't share page table with the CPU on ARM. >>>> The non-shared IOMMUs have to perform all required actions here >>>> to be ready to handle IOMMU mapping updates right after completing it. >>>> >>>> The arch_iommu_populate_page_table() seems an appropriate place >>>> to call newly created callback. >>>> Since we will only be here for the non-shared IOMMUs always >>>> return error if the callback wasn't implemented. >>> >>> >>> >>> Why do you need a specific callback and not doing it directly in >>> iommu_domain_init? >>> >>> My take here is in the unshare case, we may want to have multiple set of >>> page tables (e.g one per device). So this should be left at the >>> discretion >>> of the IOMMU itself. >>> >>> Am I missing something? >> >> I was thinking about extra need_iommu argument for init platform >> callback as I had done for iommu_domain_init API. >> But I had doubts regarding hw_domain. During iommu_domain_init >> execution we haven't known yet is the IOMMU expected for domain 0 >> or not. >> >> Taking into account that I needed to: >> - populate page table followed by setting need_iommu flag. >> - implement arch_iommu_populate_page_table() on ARM because of >> !iommu_use_hap_pt(d). >> - find a solution for hw_domain. >> >> I decided to use iommu_construct() and implement alloc_page_table >> callback to be called for populating page table. >> I thought that it would allow us to keep all required actions in a >> single place rather than spreading. > > > Looking at your patch #8, you always allocate the page table for hardware > domain, so this is very similar to set iommu_enable in > xen_arch_domain_config (see config. in start_xen). Indeed. I allocate page table for hw_domain only if need_iommu flag is set. The need_iommu flag depends on iommu_dom0_strict. We force iommu_dom0_strict to true. This all means that we always use iommu for hw_domain) So, you proposed to avoid new callback and allocate page table directly in "init" callback? This also means we have to add extra need_iommu argument to "init" callback. > > So this does not hold to me. Maybe Jan (IOMMU maintainer) has a different > view on it. > > Cheers, > > -- > Julien Grall -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback 2017-05-11 18:06 ` Julien Grall 2017-05-11 18:43 ` Oleksandr Tyshchenko @ 2017-05-12 14:36 ` Jan Beulich 1 sibling, 0 replies; 66+ messages in thread From: Jan Beulich @ 2017-05-12 14:36 UTC (permalink / raw) To: Julien Grall, Oleksandr Tyshchenko Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel >>> On 11.05.17 at 20:06, <julien.grall@arm.com> wrote: > Hi Oleksandr, > > On 11/05/17 15:00, Oleksandr Tyshchenko wrote: >> On Thu, May 11, 2017 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote: >>> Hi Oleksandr, >> Hi, Julien >> >>> >>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >>>> >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> The alloc_page_table callback is a mandatory thing >>>> for the IOMMUs that don't share page table with the CPU on ARM. >>>> The non-shared IOMMUs have to perform all required actions here >>>> to be ready to handle IOMMU mapping updates right after completing it. >>>> >>>> The arch_iommu_populate_page_table() seems an appropriate place >>>> to call newly created callback. >>>> Since we will only be here for the non-shared IOMMUs always >>>> return error if the callback wasn't implemented. >>> >>> >>> Why do you need a specific callback and not doing it directly in >>> iommu_domain_init? >>> >>> My take here is in the unshare case, we may want to have multiple set of >>> page tables (e.g one per device). So this should be left at the discretion >>> of the IOMMU itself. >>> >>> Am I missing something? >> I was thinking about extra need_iommu argument for init platform >> callback as I had done for iommu_domain_init API. >> But I had doubts regarding hw_domain. During iommu_domain_init >> execution we haven't known yet is the IOMMU expected for domain 0 >> or not. >> >> Taking into account that I needed to: >> - populate page table followed by setting need_iommu flag. >> - implement arch_iommu_populate_page_table() on ARM because of >> !iommu_use_hap_pt(d). >> - find a solution for hw_domain. >> >> I decided to use iommu_construct() and implement alloc_page_table >> callback to be called for populating page table. >> I thought that it would allow us to keep all required actions in a >> single place rather than spreading. > > Looking at your patch #8, you always allocate the page table for > hardware domain, so this is very similar to set iommu_enable in > xen_arch_domain_config (see config. in start_xen). > > So this does not hold to me. Maybe Jan (IOMMU maintainer) has a > different view on it. Well, I have to admit that I don't really understand the need for this new callback. But it looks like in a later reply Oleksandr moves to agreeing with you to drop this new hook. As it's ARM-specific, I'll leave this to you for the moment. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-10 14:03 [PATCH v1 00/10] "Non-shared" IOMMU support on ARM Oleksandr Tyshchenko ` (6 preceding siblings ...) 2017-05-10 14:03 ` [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback Oleksandr Tyshchenko @ 2017-05-10 14:03 ` Oleksandr Tyshchenko 2017-05-12 14:41 ` Jan Beulich 2017-05-10 14:03 ` [PATCH v1 09/10] xen/arm: Add use_iommu flag to xen_arch_domainconfig Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest Oleksandr Tyshchenko 9 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-10 14:03 UTC (permalink / raw) To: xen-devel; +Cc: wei.liu2, julien.grall, sstabellini, ian.jackson, jbeulich From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> The "retrieving mapping" code has never executed since iommu_use_hap_pt(d) always returned true on ARM so far. But, with introducing the non-shared IOMMU patch series we can no longer keep this code as is due to the lack of M2P support. In order to retain the current behavior for x86 this code was completely moved to x86 specific part. For ARM we just need to populate IOMMU page table if need_iommu flag is already set and the IOMMU is non-shared. So, the logic on ARM was changed a bit, but no functional change for x86. Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> CC: Jan Beulich <jbeulich@suse.com> CC: Julien Grall <julien.grall@arm.com> --- Changes in V1: - Clarify patch description. --- xen/drivers/passthrough/arm/iommu.c | 7 +++++++ xen/drivers/passthrough/iommu.c | 30 +----------------------------- xen/drivers/passthrough/x86/iommu.c | 36 ++++++++++++++++++++++++++++++++++++ xen/include/xen/iommu.h | 1 + 4 files changed, 45 insertions(+), 29 deletions(-) diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c index f132032..2198723 100644 --- a/xen/drivers/passthrough/arm/iommu.c +++ b/xen/drivers/passthrough/arm/iommu.c @@ -19,6 +19,7 @@ #include <xen/iommu.h> #include <xen/device_tree.h> #include <asm/device.h> +#include <xen/sched.h> static const struct iommu_ops *iommu_ops; @@ -59,6 +60,12 @@ void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d) return; } +void __hwdom_init arch_iommu_hwdom_init(struct domain *d) +{ + if ( need_iommu(d) && !iommu_use_hap_pt(d) ) + arch_iommu_populate_page_table(d); +} + int arch_iommu_domain_init(struct domain *d) { return iommu_dt_domain_init(d); diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index c85f7b4..e66eefb 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -177,36 +177,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m table", 0); d->need_iommu = !!iommu_dom0_strict; - if ( need_iommu(d) && !iommu_use_hap_pt(d) ) - { - struct page_info *page; - unsigned int i = 0; - int rc = 0; - - page_list_for_each ( page, &d->page_list ) - { - unsigned long mfn = page_to_mfn(page); - unsigned long gfn = mfn_to_gmfn(d, mfn); - unsigned int mapping = IOMMUF_readable; - int ret; - - if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) || - ((page->u.inuse.type_info & PGT_type_mask) - == PGT_writable_page) ) - mapping |= IOMMUF_writable; - - ret = hd->platform_ops->map_pages(d, gfn, mfn, 0, mapping); - if ( !rc ) - rc = ret; - - if ( !(i++ & 0xfffff) ) - process_pending_softirqs(); - } - if ( rc ) - printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", - d->domain_id, rc); - } + arch_iommu_hwdom_init(d); return hd->platform_ops->hwdom_init(d); } diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index 973b72f..904736b 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -118,6 +118,42 @@ void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d) panic("Presently, iommu must be enabled for PVH hardware domain\n"); } +void __hwdom_init arch_iommu_hwdom_init(struct domain *d) +{ + const struct domain_iommu *hd = dom_iommu(d); + + if ( need_iommu(d) && !iommu_use_hap_pt(d) ) + { + struct page_info *page; + unsigned int i = 0; + int rc = 0; + + page_list_for_each ( page, &d->page_list ) + { + unsigned long mfn = page_to_mfn(page); + unsigned long gfn = mfn_to_gmfn(d, mfn); + unsigned int mapping = IOMMUF_readable; + int ret; + + if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) || + ((page->u.inuse.type_info & PGT_type_mask) + == PGT_writable_page) ) + mapping |= IOMMUF_writable; + + ret = hd->platform_ops->map_pages(d, gfn, mfn, 0, mapping); + if ( !rc ) + rc = ret; + + if ( !(i++ & 0xfffff) ) + process_pending_softirqs(); + } + + if ( rc ) + printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", + d->domain_id, rc); + } +} + int arch_iommu_domain_init(struct domain *d) { struct domain_iommu *hd = dom_iommu(d); diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index f5914db..be43b28 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -65,6 +65,7 @@ void arch_iommu_domain_destroy(struct domain *d); int arch_iommu_domain_init(struct domain *d); int arch_iommu_populate_page_table(struct domain *d); void arch_iommu_check_autotranslated_hwdom(struct domain *d); +void arch_iommu_hwdom_init(struct domain *d); int iommu_construct(struct domain *d); -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-10 14:03 ` [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts Oleksandr Tyshchenko @ 2017-05-12 14:41 ` Jan Beulich 2017-05-12 15:25 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-12 14:41 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: ian.jackson, julien.grall, sstabellini, wei.liu2, xen-devel >>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: > The "retrieving mapping" code has never executed since > iommu_use_hap_pt(d) always returned true on ARM so far. But, with > introducing the non-shared IOMMU patch series we can no longer keep > this code as is due to the lack of M2P support. > > In order to retain the current behavior for x86 this code was completely > moved to x86 specific part. > For ARM we just need to populate IOMMU page table if need_iommu flag > is already set and the IOMMU is non-shared. > > So, the logic on ARM was changed a bit, but no functional change for x86. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Julien Grall <julien.grall@arm.com> > > --- > Changes in V1: > - Clarify patch description. My prior objection stands: You don't make clear why architecture- agnostic code needs to be moved into an architecture-specific file. Of course once you give a proper reason in the description, the change itself looks mostly fine - just one remark: > +void __hwdom_init arch_iommu_hwdom_init(struct domain *d) > +{ > + const struct domain_iommu *hd = dom_iommu(d); This isn't used outside of ... > + if ( need_iommu(d) && !iommu_use_hap_pt(d) ) > + { ... this if(), so should be moved here. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-12 14:41 ` Jan Beulich @ 2017-05-12 15:25 ` Oleksandr Tyshchenko 2017-05-12 15:34 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-12 15:25 UTC (permalink / raw) To: Jan Beulich Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel Hi, Jan. On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >> The "retrieving mapping" code has never executed since >> iommu_use_hap_pt(d) always returned true on ARM so far. But, with >> introducing the non-shared IOMMU patch series we can no longer keep >> this code as is due to the lack of M2P support. >> >> In order to retain the current behavior for x86 this code was completely >> moved to x86 specific part. >> For ARM we just need to populate IOMMU page table if need_iommu flag >> is already set and the IOMMU is non-shared. >> >> So, the logic on ARM was changed a bit, but no functional change for x86. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Julien Grall <julien.grall@arm.com> >> >> --- >> Changes in V1: >> - Clarify patch description. > > My prior objection stands: You don't make clear why architecture- > agnostic code needs to be moved into an architecture-specific file. Is the following description more cleaner? Although this code looks like architecture-agnostic code, there is only one thing that prevents it to be *completely* architecture-agnostic -> mfn_to_gmfn helper. As we don't have an M2P on ARM, it always returns incoming mfn. And if domain is not direct mapped we will get into the trouble using that. We didn't care about this code before because it has never been executed (iommu_use_hap_pt(d) always returned true on ARM so far). But, with introducing the non-shared IOMMU patch series we can no longer keep this code as is since it will be executed for non-shared IOMMU. So, move this code to x86-specific file in order not to expose a possible bug. > Of course once you give a proper reason in the description, the > change itself looks mostly fine - just one remark: > >> +void __hwdom_init arch_iommu_hwdom_init(struct domain *d) >> +{ >> + const struct domain_iommu *hd = dom_iommu(d); > > This isn't used outside of ... > >> + if ( need_iommu(d) && !iommu_use_hap_pt(d) ) >> + { > > ... this if(), so should be moved here. Agree. Will move. > > Jan > -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-12 15:25 ` Oleksandr Tyshchenko @ 2017-05-12 15:34 ` Jan Beulich 2017-05-15 7:20 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-12 15:34 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel >>> On 12.05.17 at 17:25, <olekstysh@gmail.com> wrote: > On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>> The "retrieving mapping" code has never executed since >>> iommu_use_hap_pt(d) always returned true on ARM so far. But, with >>> introducing the non-shared IOMMU patch series we can no longer keep >>> this code as is due to the lack of M2P support. >>> >>> In order to retain the current behavior for x86 this code was completely >>> moved to x86 specific part. >>> For ARM we just need to populate IOMMU page table if need_iommu flag >>> is already set and the IOMMU is non-shared. >>> >>> So, the logic on ARM was changed a bit, but no functional change for x86. >>> >>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> CC: Jan Beulich <jbeulich@suse.com> >>> CC: Julien Grall <julien.grall@arm.com> >>> >>> --- >>> Changes in V1: >>> - Clarify patch description. >> >> My prior objection stands: You don't make clear why architecture- >> agnostic code needs to be moved into an architecture-specific file. > > Is the following description more cleaner? > > Although this code looks like architecture-agnostic code, there is > only one thing that prevents it > to be *completely* architecture-agnostic -> mfn_to_gmfn helper. > As we don't have an M2P on ARM, it always returns incoming mfn. > And if domain is not direct mapped we will get into the trouble using that. > > We didn't care about this code before because it has never been executed > (iommu_use_hap_pt(d) always returned true on ARM so far). > But, with introducing the non-shared IOMMU patch series we can no longer keep > this code as is since it will be executed for non-shared IOMMU. > So, move this code to x86-specific file in order not to expose a possible bug. Yes, this helps. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-12 15:34 ` Jan Beulich @ 2017-05-15 7:20 ` Jan Beulich 2017-05-15 7:42 ` Julien Grall 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-15 7:20 UTC (permalink / raw) To: Julien Grall, Oleksandr Tyshchenko, Stefano Stabellini Cc: Ian Jackson, Wei Liu, xen-devel >>> On 12.05.17 at 17:34, <JBeulich@suse.com> wrote: >>>> On 12.05.17 at 17:25, <olekstysh@gmail.com> wrote: >> On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>>> The "retrieving mapping" code has never executed since >>>> iommu_use_hap_pt(d) always returned true on ARM so far. But, with >>>> introducing the non-shared IOMMU patch series we can no longer keep >>>> this code as is due to the lack of M2P support. >>>> >>>> In order to retain the current behavior for x86 this code was completely >>>> moved to x86 specific part. >>>> For ARM we just need to populate IOMMU page table if need_iommu flag >>>> is already set and the IOMMU is non-shared. >>>> >>>> So, the logic on ARM was changed a bit, but no functional change for x86. >>>> >>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> CC: Jan Beulich <jbeulich@suse.com> >>>> CC: Julien Grall <julien.grall@arm.com> >>>> >>>> --- >>>> Changes in V1: >>>> - Clarify patch description. >>> >>> My prior objection stands: You don't make clear why architecture- >>> agnostic code needs to be moved into an architecture-specific file. >> >> Is the following description more cleaner? >> >> Although this code looks like architecture-agnostic code, there is >> only one thing that prevents it >> to be *completely* architecture-agnostic -> mfn_to_gmfn helper. >> As we don't have an M2P on ARM, it always returns incoming mfn. >> And if domain is not direct mapped we will get into the trouble using that. >> >> We didn't care about this code before because it has never been executed >> (iommu_use_hap_pt(d) always returned true on ARM so far). >> But, with introducing the non-shared IOMMU patch series we can no longer keep >> this code as is since it will be executed for non-shared IOMMU. >> So, move this code to x86-specific file in order not to expose a possible bug. > > Yes, this helps. Having thought about this some more, what's still missing is a clear explanation why this new need of a non-stub mfn_to_gmfn() isn't finally enough of a reason to introduce an M2P on ARM. We currently have several uses already which ARM fakes one way or another: - gnttab_shared_gmfn() - gnttab_status_gmfn() - memory_exchange() - getdomaininfo() With this I think there's quite a bit of justification needed to keep going without M2P on ARM. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-15 7:20 ` Jan Beulich @ 2017-05-15 7:42 ` Julien Grall 2017-05-15 8:19 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Julien Grall @ 2017-05-15 7:42 UTC (permalink / raw) To: Jan Beulich, Oleksandr Tyshchenko, Stefano Stabellini Cc: Ian Jackson, nd, Wei Liu, xen-devel Hi Jan, On 15/05/2017 08:20, Jan Beulich wrote: >>>> On 12.05.17 at 17:34, <JBeulich@suse.com> wrote: >>>>> On 12.05.17 at 17:25, <olekstysh@gmail.com> wrote: >>> On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>>>> The "retrieving mapping" code has never executed since >>>>> iommu_use_hap_pt(d) always returned true on ARM so far. But, with >>>>> introducing the non-shared IOMMU patch series we can no longer keep >>>>> this code as is due to the lack of M2P support. >>>>> >>>>> In order to retain the current behavior for x86 this code was completely >>>>> moved to x86 specific part. >>>>> For ARM we just need to populate IOMMU page table if need_iommu flag >>>>> is already set and the IOMMU is non-shared. >>>>> >>>>> So, the logic on ARM was changed a bit, but no functional change for x86. >>>>> >>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>>> CC: Jan Beulich <jbeulich@suse.com> >>>>> CC: Julien Grall <julien.grall@arm.com> >>>>> >>>>> --- >>>>> Changes in V1: >>>>> - Clarify patch description. >>>> >>>> My prior objection stands: You don't make clear why architecture- >>>> agnostic code needs to be moved into an architecture-specific file. >>> >>> Is the following description more cleaner? >>> >>> Although this code looks like architecture-agnostic code, there is >>> only one thing that prevents it >>> to be *completely* architecture-agnostic -> mfn_to_gmfn helper. >>> As we don't have an M2P on ARM, it always returns incoming mfn. >>> And if domain is not direct mapped we will get into the trouble using that. >>> >>> We didn't care about this code before because it has never been executed >>> (iommu_use_hap_pt(d) always returned true on ARM so far). >>> But, with introducing the non-shared IOMMU patch series we can no longer keep >>> this code as is since it will be executed for non-shared IOMMU. >>> So, move this code to x86-specific file in order not to expose a possible bug. >> >> Yes, this helps. > > Having thought about this some more, what's still missing is a > clear explanation why this new need of a non-stub mfn_to_gmfn() > isn't finally enough of a reason to introduce an M2P on ARM. We > currently have several uses already which ARM fakes one way or > another: > - gnttab_shared_gmfn() This does not use mfn_to_gmfn on ARM. > - gnttab_status_gmfn() gnttab_status_gmfn() returns 0 so far. I have to look at this one. > - memory_exchange() Memory exchange does not work on ARM today and will require more work than that. When I looked at the code a couple of years ago, it was possible to drop the call to mfn_to_gmfn(). > - getdomaininfo() We could rework to store the gmfn in arch_domain. > With this I think there's quite a bit of justification needed to keep > going without M2P on ARM. As said in a previous thread, I made a quick calculation, ARM32 supports up 40-bit PA and IPA (e.g guest address), which means 28-bits of MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so we would need 2^28 * 4 = 1GiB of virtual address space and potentially physical memory. We don't have 1GB of VA space free on 32-bit right now. ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for alignment, so we would need 2^36 * 8 = 512GiB of virtual address space and potentially physical memory. While virtual address space is not a problem, the memory is a problem for embedded platform. We want Xen to be as lean as possible. So the M2P is not a solution on ARM. A better approach is to drop those calls from common code and replace by something different (as we did for gnttab_shared_mfn). Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-15 7:42 ` Julien Grall @ 2017-05-15 8:19 ` Jan Beulich 2017-05-15 11:45 ` Julien Grall 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-15 8:19 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Ian Jackson, Oleksandr Tyshchenko, xen-devel, nd >>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: > On 15/05/2017 08:20, Jan Beulich wrote: >> Having thought about this some more, what's still missing is a >> clear explanation why this new need of a non-stub mfn_to_gmfn() >> isn't finally enough of a reason to introduce an M2P on ARM. We >> currently have several uses already which ARM fakes one way or >> another: >> - gnttab_shared_gmfn() > > This does not use mfn_to_gmfn on ARM. Right, at the price of maintaining some other helper data. >> - gnttab_status_gmfn() > > gnttab_status_gmfn() returns 0 so far. I have to look at this one. > >> - memory_exchange() > > Memory exchange does not work on ARM today and will require more work > than that. When I looked at the code a couple of years ago, it was > possible to drop the call to mfn_to_gmfn(). > >> - getdomaininfo() > > We could rework to store the gmfn in arch_domain. Which again would mean you maintain extra data in order to avoid the more general M2P. >> With this I think there's quite a bit of justification needed to keep >> going without M2P on ARM. > > As said in a previous thread, I made a quick calculation, ARM32 supports > up 40-bit PA and IPA (e.g guest address), which means 28-bits of > MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so > we would need 2^28 * 4 = 1GiB of virtual address space and potentially > physical memory. We don't have 1GB of VA space free on 32-bit right now. How come? You don't share address spaces with guests. > ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means > 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for > alignment, so we would need 2^36 * 8 = 512GiB of virtual address space > and potentially physical memory. While virtual address space is not a > problem, the memory is a problem for embedded platform. We want Xen to > be as lean as possible. You don't need to allocate all 512Gb, the table can be as sparse as present memory permits. > So the M2P is not a solution on ARM. A better approach is to drop those > calls from common code and replace by something different (as we did for > gnttab_shared_mfn). I'm of the exact opposite opinion. Or at the very least, have a mode (read: config or command line option) where ARM maintains M2P and make features like the IOMMU one here depend on being in that mode. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-15 8:19 ` Jan Beulich @ 2017-05-15 11:45 ` Julien Grall 2017-05-15 12:43 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Julien Grall @ 2017-05-15 11:45 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Ian Jackson, Oleksandr Tyshchenko, xen-devel, nd Hi Jan, On 05/15/2017 09:19 AM, Jan Beulich wrote: >>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: >> On 15/05/2017 08:20, Jan Beulich wrote: >>> Having thought about this some more, what's still missing is a >>> clear explanation why this new need of a non-stub mfn_to_gmfn() >>> isn't finally enough of a reason to introduce an M2P on ARM. We >>> currently have several uses already which ARM fakes one way or >>> another: >>> - gnttab_shared_gmfn() >> >> This does not use mfn_to_gmfn on ARM. > > Right, at the price of maintaining some other helper data. saving few MB of memory in small board and hundreds in server if we use an M2P. The choice is very easy here. > >>> - gnttab_status_gmfn() >> >> gnttab_status_gmfn() returns 0 so far. I have to look at this one. >> >>> - memory_exchange() >> >> Memory exchange does not work on ARM today and will require more work >> than that. When I looked at the code a couple of years ago, it was >> possible to drop the call to mfn_to_gmfn(). >> >>> - getdomaininfo() >> >> We could rework to store the gmfn in arch_domain. > > Which again would mean you maintain extra data in order to avoid > the more general M2P. Yes saving MBs as above. > >>> With this I think there's quite a bit of justification needed to keep >>> going without M2P on ARM. >> >> As said in a previous thread, I made a quick calculation, ARM32 supports >> up 40-bit PA and IPA (e.g guest address), which means 28-bits of >> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so >> we would need 2^28 * 4 = 1GiB of virtual address space and potentially >> physical memory. We don't have 1GB of VA space free on 32-bit right now. > > How come? You don't share address spaces with guests. Below the layout for ARM32: * 0 - 12M <COMMON> * * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address * space * * 1G - 2G Xenheap: always-mapped memory * 2G - 4G Domheap: on-demand-mapped * >> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means >> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for >> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space >> and potentially physical memory. While virtual address space is not a >> problem, the memory is a problem for embedded platform. We want Xen to >> be as lean as possible. > > You don't need to allocate all 512Gb, the table can be as sparse as > present memory permits. I am aware about that... The main point is reducing the footprint of Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P. Likely this will increase the footprint of Xen ARM. For what benefits? Avoiding to store few byte in a non-generic way when we need it... I will comment about the IOMMU below. > >> So the M2P is not a solution on ARM. A better approach is to drop those >> calls from common code and replace by something different (as we did for >> gnttab_shared_mfn). > > I'm of the exact opposite opinion. Or at the very least, have a mode > (read: config or command line option) where ARM maintains M2P and > make features like the IOMMU one here depend on being in that mode. Well, in embedded platform you will know in advance that you will passthrough devices to a guest. So there is no point of deferring the creation of the page-tables until a device is been assigned. In server side, I would expect page-table to be shared most of the time. We might have to unshare some part of the page-tables but not everything as it is currently done on x86. So far, you didn't convince me the M2P is the right solution for ARM. We use more memory for benefits of, AFAICT, of device hotpluging (?) and been "generic". Anyway, I will let Stefano give his opinion on it. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-15 11:45 ` Julien Grall @ 2017-05-15 12:43 ` Jan Beulich 2017-05-17 15:45 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-15 12:43 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Ian Jackson, Oleksandr Tyshchenko, xen-devel, nd >>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote: > On 05/15/2017 09:19 AM, Jan Beulich wrote: >>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: >>> On 15/05/2017 08:20, Jan Beulich wrote: >>>> With this I think there's quite a bit of justification needed to keep >>>> going without M2P on ARM. >>> >>> As said in a previous thread, I made a quick calculation, ARM32 supports >>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of >>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so >>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially >>> physical memory. We don't have 1GB of VA space free on 32-bit right now. >> >> How come? You don't share address spaces with guests. > > Below the layout for ARM32: > > > * 0 - 12M <COMMON> > * > * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM > * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address > * space > * > * 1G - 2G Xenheap: always-mapped memory > * 2G - 4G Domheap: on-demand-mapped Since Domheap hardly covers all memory, the obvious thing would seem to be to take part of that region, just like on x86 we also had to reduce the direct mapping area in order to support systems with more than 5Tb. >>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means >>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for >>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space >>> and potentially physical memory. While virtual address space is not a >>> problem, the memory is a problem for embedded platform. We want Xen to >>> be as lean as possible. >> >> You don't need to allocate all 512Gb, the table can be as sparse as >> present memory permits. > > I am aware about that... The main point is reducing the footprint of > Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P. > > Likely this will increase the footprint of Xen ARM. For what benefits? > Avoiding to store few byte in a non-generic way when we need it... But that's the point: Generic code becomes more and more clumsy if non-generic mechanisms need to be introduced. Quite a few we've had the discussion of saving a few Mb here or there, and I've almost always been the one to ask for not wasting memory even if we have plenty, so I'm all with you on that aspect. Nevertheless there is a point where the trade-off between memory overhead and generic (i.e. easier to maintain) code crosses a boundary, and I'm simply wondering whether we aren't at that point. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-15 12:43 ` Jan Beulich @ 2017-05-17 15:45 ` Oleksandr Tyshchenko 2017-05-17 16:01 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-17 15:45 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Ian Jackson, Julien Grall, xen-devel, nd Hi, Jan. On Mon, May 15, 2017 at 3:43 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote: >> On 05/15/2017 09:19 AM, Jan Beulich wrote: >>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: >>>> On 15/05/2017 08:20, Jan Beulich wrote: >>>>> With this I think there's quite a bit of justification needed to keep >>>>> going without M2P on ARM. >>>> >>>> As said in a previous thread, I made a quick calculation, ARM32 supports >>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of >>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so >>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially >>>> physical memory. We don't have 1GB of VA space free on 32-bit right now. >>> >>> How come? You don't share address spaces with guests. >> >> Below the layout for ARM32: >> >> >> * 0 - 12M <COMMON> >> * >> * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM >> * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address >> * space >> * >> * 1G - 2G Xenheap: always-mapped memory >> * 2G - 4G Domheap: on-demand-mapped > > Since Domheap hardly covers all memory, the obvious thing would > seem to be to take part of that region, just like on x86 we also > had to reduce the direct mapping area in order to support systems > with more than 5Tb. > >>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means >>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for >>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space >>>> and potentially physical memory. While virtual address space is not a >>>> problem, the memory is a problem for embedded platform. We want Xen to >>>> be as lean as possible. >>> >>> You don't need to allocate all 512Gb, the table can be as sparse as >>> present memory permits. >> >> I am aware about that... The main point is reducing the footprint of >> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P. >> >> Likely this will increase the footprint of Xen ARM. For what benefits? >> Avoiding to store few byte in a non-generic way when we need it... > > But that's the point: Generic code becomes more and more clumsy > if non-generic mechanisms need to be introduced. Quite a few we've > had the discussion of saving a few Mb here or there, and I've almost > always been the one to ask for not wasting memory even if we have > plenty, so I'm all with you on that aspect. Nevertheless there is a > point where the trade-off between memory overhead and generic > (i.e. easier to maintain) code crosses a boundary, and I'm simply > wondering whether we aren't at that point. Is the lack of M2P support on ARM a blocker for this patch to be accepted? This patch I think is only prevents us from possible bugs in a future. Please correct me if I am wrong. > > Jan > -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-17 15:45 ` Oleksandr Tyshchenko @ 2017-05-17 16:01 ` Jan Beulich 2017-05-17 18:51 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-17 16:01 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: Stefano Stabellini, Wei Liu, Ian Jackson, Julien Grall, xen-devel, nd >>> On 17.05.17 at 17:45, <olekstysh@gmail.com> wrote: > On Mon, May 15, 2017 at 3:43 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote: >>> On 05/15/2017 09:19 AM, Jan Beulich wrote: >>>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: >>>>> On 15/05/2017 08:20, Jan Beulich wrote: >>>>>> With this I think there's quite a bit of justification needed to keep >>>>>> going without M2P on ARM. >>>>> >>>>> As said in a previous thread, I made a quick calculation, ARM32 supports >>>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of >>>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so >>>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially >>>>> physical memory. We don't have 1GB of VA space free on 32-bit right now. >>>> >>>> How come? You don't share address spaces with guests. >>> >>> Below the layout for ARM32: >>> >>> >>> * 0 - 12M <COMMON> >>> * >>> * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM >>> * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address >>> * space >>> * >>> * 1G - 2G Xenheap: always-mapped memory >>> * 2G - 4G Domheap: on-demand-mapped >> >> Since Domheap hardly covers all memory, the obvious thing would >> seem to be to take part of that region, just like on x86 we also >> had to reduce the direct mapping area in order to support systems >> with more than 5Tb. >> >>>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means >>>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for >>>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space >>>>> and potentially physical memory. While virtual address space is not a >>>>> problem, the memory is a problem for embedded platform. We want Xen to >>>>> be as lean as possible. >>>> >>>> You don't need to allocate all 512Gb, the table can be as sparse as >>>> present memory permits. >>> >>> I am aware about that... The main point is reducing the footprint of >>> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P. >>> >>> Likely this will increase the footprint of Xen ARM. For what benefits? >>> Avoiding to store few byte in a non-generic way when we need it... >> >> But that's the point: Generic code becomes more and more clumsy >> if non-generic mechanisms need to be introduced. Quite a few we've >> had the discussion of saving a few Mb here or there, and I've almost >> always been the one to ask for not wasting memory even if we have >> plenty, so I'm all with you on that aspect. Nevertheless there is a >> point where the trade-off between memory overhead and generic >> (i.e. easier to maintain) code crosses a boundary, and I'm simply >> wondering whether we aren't at that point. > > Is the lack of M2P support on ARM a blocker for this patch to be accepted? Well, if the ARM maintainers insist on baking their own thing every time we'd use the M2P if it was there, I think I can't reasonably block this patch. Otoh I'd prefer to not see the non-x86-specific code move to x86/, so perhaps the whole patch wants re-structuring using either a manifest definition in the ARM headers or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't). > This patch I think is only prevents us from possible bugs in a future. > Please correct me if I am wrong. Avoiding possible bugs in the future I didn't connect to this patch so far. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-17 16:01 ` Jan Beulich @ 2017-05-17 18:51 ` Oleksandr Tyshchenko 2017-05-17 20:30 ` Julien Grall 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-17 18:51 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Ian Jackson, Jan Beulich, xen-devel, nd Hi, all. On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 17.05.17 at 17:45, <olekstysh@gmail.com> wrote: >> On Mon, May 15, 2017 at 3:43 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote: >>>> On 05/15/2017 09:19 AM, Jan Beulich wrote: >>>>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: >>>>>> On 15/05/2017 08:20, Jan Beulich wrote: >>>>>>> With this I think there's quite a bit of justification needed to keep >>>>>>> going without M2P on ARM. >>>>>> >>>>>> As said in a previous thread, I made a quick calculation, ARM32 supports >>>>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of >>>>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so >>>>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially >>>>>> physical memory. We don't have 1GB of VA space free on 32-bit right now. >>>>> >>>>> How come? You don't share address spaces with guests. >>>> >>>> Below the layout for ARM32: >>>> >>>> >>>> * 0 - 12M <COMMON> >>>> * >>>> * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM >>>> * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address >>>> * space >>>> * >>>> * 1G - 2G Xenheap: always-mapped memory >>>> * 2G - 4G Domheap: on-demand-mapped >>> >>> Since Domheap hardly covers all memory, the obvious thing would >>> seem to be to take part of that region, just like on x86 we also >>> had to reduce the direct mapping area in order to support systems >>> with more than 5Tb. >>> >>>>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means >>>>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for >>>>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space >>>>>> and potentially physical memory. While virtual address space is not a >>>>>> problem, the memory is a problem for embedded platform. We want Xen to >>>>>> be as lean as possible. >>>>> >>>>> You don't need to allocate all 512Gb, the table can be as sparse as >>>>> present memory permits. >>>> >>>> I am aware about that... The main point is reducing the footprint of >>>> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P. >>>> >>>> Likely this will increase the footprint of Xen ARM. For what benefits? >>>> Avoiding to store few byte in a non-generic way when we need it... >>> >>> But that's the point: Generic code becomes more and more clumsy >>> if non-generic mechanisms need to be introduced. Quite a few we've >>> had the discussion of saving a few Mb here or there, and I've almost >>> always been the one to ask for not wasting memory even if we have >>> plenty, so I'm all with you on that aspect. Nevertheless there is a >>> point where the trade-off between memory overhead and generic >>> (i.e. easier to maintain) code crosses a boundary, and I'm simply >>> wondering whether we aren't at that point. >> >> Is the lack of M2P support on ARM a blocker for this patch to be accepted? > > Well, if the ARM maintainers insist on baking their own thing every > time we'd use the M2P if it was there, I think I can't reasonably > block this patch. Otoh I'd prefer to not see the non-x86-specific > code move to x86/, so perhaps the whole patch wants > re-structuring using either a manifest definition in the ARM headers > or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't). Jan, I am afraid but I didn't get what you meant here: "manifest definition in the ARM headers" Julien, Stefano what do you think in general? > >> This patch I think is only prevents us from possible bugs in a future. >> Please correct me if I am wrong. > > Avoiding possible bugs in the future I didn't connect to this patch so > far. > > Jan > -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-17 18:51 ` Oleksandr Tyshchenko @ 2017-05-17 20:30 ` Julien Grall 2017-05-18 8:53 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Julien Grall @ 2017-05-17 20:30 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: Stefano Stabellini, Wei Liu, Ian Jackson, Jan Beulich, xen-devel, nd On 05/17/2017 07:51 PM, Oleksandr Tyshchenko wrote: > Hi, all. Hi Oleksandr, > On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 17.05.17 at 17:45, <olekstysh@gmail.com> wrote: >>> On Mon, May 15, 2017 at 3:43 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote: >>>>> On 05/15/2017 09:19 AM, Jan Beulich wrote: >>>>>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: >>>>>>> On 15/05/2017 08:20, Jan Beulich wrote: >>>>>>>> With this I think there's quite a bit of justification needed to keep >>>>>>>> going without M2P on ARM. >>>>>>> >>>>>>> As said in a previous thread, I made a quick calculation, ARM32 supports >>>>>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of >>>>>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so >>>>>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially >>>>>>> physical memory. We don't have 1GB of VA space free on 32-bit right now. >>>>>> >>>>>> How come? You don't share address spaces with guests. >>>>> >>>>> Below the layout for ARM32: >>>>> >>>>> >>>>> * 0 - 12M <COMMON> >>>>> * >>>>> * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM >>>>> * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address >>>>> * space >>>>> * >>>>> * 1G - 2G Xenheap: always-mapped memory >>>>> * 2G - 4G Domheap: on-demand-mapped >>>> >>>> Since Domheap hardly covers all memory, the obvious thing would >>>> seem to be to take part of that region, just like on x86 we also >>>> had to reduce the direct mapping area in order to support systems >>>> with more than 5Tb. >>>> >>>>>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means >>>>>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for >>>>>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space >>>>>>> and potentially physical memory. While virtual address space is not a >>>>>>> problem, the memory is a problem for embedded platform. We want Xen to >>>>>>> be as lean as possible. >>>>>> >>>>>> You don't need to allocate all 512Gb, the table can be as sparse as >>>>>> present memory permits. >>>>> >>>>> I am aware about that... The main point is reducing the footprint of >>>>> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P. >>>>> >>>>> Likely this will increase the footprint of Xen ARM. For what benefits? >>>>> Avoiding to store few byte in a non-generic way when we need it... >>>> >>>> But that's the point: Generic code becomes more and more clumsy >>>> if non-generic mechanisms need to be introduced. Quite a few we've >>>> had the discussion of saving a few Mb here or there, and I've almost >>>> always been the one to ask for not wasting memory even if we have >>>> plenty, so I'm all with you on that aspect. Nevertheless there is a >>>> point where the trade-off between memory overhead and generic >>>> (i.e. easier to maintain) code crosses a boundary, and I'm simply >>>> wondering whether we aren't at that point. >>> >>> Is the lack of M2P support on ARM a blocker for this patch to be accepted? >> >> Well, if the ARM maintainers insist on baking their own thing every >> time we'd use the M2P if it was there, I think I can't reasonably >> block this patch. Otoh I'd prefer to not see the non-x86-specific >> code move to x86/, so perhaps the whole patch wants >> re-structuring using either a manifest definition in the ARM headers >> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't). > Jan, I am afraid but I didn't get what you meant here: > "manifest definition in the ARM headers" I think he meant to have either a define in the header mentioning the absence/presence of M2P. But my preference would be using the Kconfig here. > > Julien, Stefano what do you think in general? My point stands here. The M2P sounds a real waste of memory for a limited benefit. There are only a few place in common code that care about that and the only one where would potentially really need it (e.g iommu_hwdom_init()) can be replaced by allocating page table in advance (for justification see my answer on patch #6). I will take an action to replace the few mfn_to_gmfn by wrapper and proper implementation for ARM. So Jan suggestion for CONFIG_M2P (or maybe CONFIG_HAS_M2P) would be a good solution. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-17 20:30 ` Julien Grall @ 2017-05-18 8:53 ` Jan Beulich 2017-05-18 18:06 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Jan Beulich @ 2017-05-18 8:53 UTC (permalink / raw) To: Julien Grall, Oleksandr Tyshchenko Cc: Ian Jackson, nd, Wei Liu, Stefano Stabellini, xen-devel >>> On 17.05.17 at 22:30, <julien.grall@arm.com> wrote: > On 05/17/2017 07:51 PM, Oleksandr Tyshchenko wrote: >> On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote: >>> Well, if the ARM maintainers insist on baking their own thing every >>> time we'd use the M2P if it was there, I think I can't reasonably >>> block this patch. Otoh I'd prefer to not see the non-x86-specific >>> code move to x86/, so perhaps the whole patch wants >>> re-structuring using either a manifest definition in the ARM headers >>> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't). >> Jan, I am afraid but I didn't get what you meant here: >> "manifest definition in the ARM headers" > > I think he meant to have either a define in the header mentioning the > absence/presence of M2P. Yes, at least in a way. > But my preference would be using the Kconfig here. Depends on the symbol used: If such a symbol solely _indicates_ the presence, Kconfig would be better indeed. If, however, the symbol is e.g. a macro resolving to a per-arch implementation, with common code providing a default definition when the arch doesn't provide any, then the non-Kconfig variant may be preferable. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-18 8:53 ` Jan Beulich @ 2017-05-18 18:06 ` Oleksandr Tyshchenko 2017-05-19 6:33 ` Jan Beulich 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-18 18:06 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Ian Jackson, Julien Grall, xen-devel, nd Hi, all. On Thu, May 18, 2017 at 11:53 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 17.05.17 at 22:30, <julien.grall@arm.com> wrote: >> On 05/17/2017 07:51 PM, Oleksandr Tyshchenko wrote: >>> On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> Well, if the ARM maintainers insist on baking their own thing every >>>> time we'd use the M2P if it was there, I think I can't reasonably >>>> block this patch. Otoh I'd prefer to not see the non-x86-specific >>>> code move to x86/, so perhaps the whole patch wants >>>> re-structuring using either a manifest definition in the ARM headers >>>> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't). >>> Jan, I am afraid but I didn't get what you meant here: >>> "manifest definition in the ARM headers" >> >> I think he meant to have either a define in the header mentioning the >> absence/presence of M2P. > > Yes, at least in a way. > >> But my preference would be using the Kconfig here. > > Depends on the symbol used: If such a symbol solely _indicates_ > the presence, Kconfig would be better indeed. If, however, the > symbol is e.g. a macro resolving to a per-arch implementation, > with common code providing a default definition when the arch > doesn't provide any, then the non-Kconfig variant may be > preferable. > > Jan > Thank you for your comments. I have already posted a common comment regarding several patches in the current series as they are interrelated (please see patch #6), but I duplicate here only related to this patch part. ... patch #8: As we always allocate the page table for hardware domain, this patch should be reworked. The use_iommu flag should be set for both archs in case of hardware domain. Having d->need_iommu set at the early stage we won't skip IOMMU mapping updates anymore. And as the result the existing in iommu_hwdom_init() code that goes through the list of page and tries to retrieve mapping could be just dropped instead of moving it to the arch-specific part. ... Does the described above make sense? -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts 2017-05-18 18:06 ` Oleksandr Tyshchenko @ 2017-05-19 6:33 ` Jan Beulich 0 siblings, 0 replies; 66+ messages in thread From: Jan Beulich @ 2017-05-19 6:33 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: Stefano Stabellini, Wei Liu, Ian Jackson, Julien Grall, xen-devel, nd >>> On 18.05.17 at 20:06, <olekstysh@gmail.com> wrote: > On Thu, May 18, 2017 at 11:53 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 17.05.17 at 22:30, <julien.grall@arm.com> wrote: >>> On 05/17/2017 07:51 PM, Oleksandr Tyshchenko wrote: >>>> On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> Well, if the ARM maintainers insist on baking their own thing every >>>>> time we'd use the M2P if it was there, I think I can't reasonably >>>>> block this patch. Otoh I'd prefer to not see the non-x86-specific >>>>> code move to x86/, so perhaps the whole patch wants >>>>> re-structuring using either a manifest definition in the ARM headers >>>>> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't). >>>> Jan, I am afraid but I didn't get what you meant here: >>>> "manifest definition in the ARM headers" >>> >>> I think he meant to have either a define in the header mentioning the >>> absence/presence of M2P. >> >> Yes, at least in a way. >> >>> But my preference would be using the Kconfig here. >> >> Depends on the symbol used: If such a symbol solely _indicates_ >> the presence, Kconfig would be better indeed. If, however, the >> symbol is e.g. a macro resolving to a per-arch implementation, >> with common code providing a default definition when the arch >> doesn't provide any, then the non-Kconfig variant may be >> preferable. > > Thank you for your comments. > I have already posted a common comment regarding several patches in > the current series > as they are interrelated (please see patch #6), but I duplicate here > only related to this patch part. > > ... > patch #8: As we always allocate the page table for hardware domain, > this patch should be reworked. > The use_iommu flag should be set for both archs in case of hardware > domain. Having d->need_iommu set at the early stage we won't skip > IOMMU mapping updates anymore. And as the result the existing in > iommu_hwdom_init() code that goes through the list of page and tries > to retrieve mapping could be just dropped > instead of moving it to the arch-specific part. > ... > > Does the described above make sense? As just said in the other reply - only if there weren't all these extra overrides (one of which is even on by default, albeit we've been discussing recently whether that's actually [still] appropriate). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v1 09/10] xen/arm: Add use_iommu flag to xen_arch_domainconfig 2017-05-10 14:03 [PATCH v1 00/10] "Non-shared" IOMMU support on ARM Oleksandr Tyshchenko ` (7 preceding siblings ...) 2017-05-10 14:03 ` [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts Oleksandr Tyshchenko @ 2017-05-10 14:03 ` Oleksandr Tyshchenko 2017-05-11 11:42 ` Julien Grall 2017-05-10 14:03 ` [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest Oleksandr Tyshchenko 9 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-10 14:03 UTC (permalink / raw) To: xen-devel; +Cc: wei.liu2, julien.grall, sstabellini, ian.jackson, jbeulich From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> This flag is intended to let Xen know that the guest has devices which will most likely be used for passthrough and as the result the use of IOMMU is expected for this domain. The primary aim of this knowledge is to help the IOMMUs that don't share page tables with the CPU on ARM be ready before P2M code starts updating IOMMU mapping. So, if this flag is set the non-shared IOMMUs will populate their page tables at the domain creation time and thereby will be able to handle IOMMU mapping updates from *the very beginning*. In order to retain the current behavior for x86 still call iommu_domain_init() with use_iommu flag being forced to false. Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> CC: Jan Beulich <jbeulich@suse.com> CC: Julien Grall <julien.grall@arm.com> CC: Ian Jackson <ian.jackson@eu.citrix.com> CC: Wei Liu <wei.liu2@citrix.com> --- Changes in V1: - Treat use_iommu flag as the ARM decision only. Don't use common domain creation flag for it, use ARM config instead. - Clarify patch subject/description. --- tools/libxl/libxl_arm.c | 10 ++++++++++ xen/arch/arm/domain.c | 2 +- xen/include/public/arch-arm.h | 5 +++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index d842d88..9c4705e 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -78,6 +78,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, return ERROR_FAIL; } + /* TODO Are these assumptions enough to make decision about using IOMMU? */ + if ((d_config->num_dtdevs && d_config->dtdevs) || + (d_config->num_pcidevs && d_config->pcidevs)) + xc_config->use_iommu = 1; + else + xc_config->use_iommu = 0; + + LOG(DEBUG, "The use of IOMMU %s expected for this domain", + xc_config->use_iommu ? "is" : "isn't"); + return 0; } diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index ec19310..81c4b90 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -569,7 +569,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, ASSERT(config != NULL); /* p2m_init relies on some value initialized by the IOMMU subsystem */ - if ( (rc = iommu_domain_init(d, false)) != 0 ) + if ( (rc = iommu_domain_init(d, config->use_iommu ? true : false)) != 0 ) goto fail; if ( (rc = p2m_init(d)) != 0 ) diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index bd974fb..cb33f75 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -322,6 +322,11 @@ struct xen_arch_domainconfig { * */ uint32_t clock_frequency; + /* + * IN + * Inform the hypervisor that the use of IOMMU is expected for this domain. + */ + uint8_t use_iommu; }; #endif /* __XEN__ || __XEN_TOOLS__ */ -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v1 09/10] xen/arm: Add use_iommu flag to xen_arch_domainconfig 2017-05-10 14:03 ` [PATCH v1 09/10] xen/arm: Add use_iommu flag to xen_arch_domainconfig Oleksandr Tyshchenko @ 2017-05-11 11:42 ` Julien Grall 2017-05-11 14:04 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Julien Grall @ 2017-05-11 11:42 UTC (permalink / raw) To: Oleksandr Tyshchenko, xen-devel Cc: wei.liu2, sstabellini, ian.jackson, jbeulich Hi Oleksandr, On 10/05/17 15:03, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > This flag is intended to let Xen know that the guest has devices > which will most likely be used for passthrough and as the result > the use of IOMMU is expected for this domain. > The primary aim of this knowledge is to help the IOMMUs that don't > share page tables with the CPU on ARM be ready before P2M code starts > updating IOMMU mapping. > So, if this flag is set the non-shared IOMMUs will populate > their page tables at the domain creation time and thereby will be able > to handle IOMMU mapping updates from *the very beginning*. > > In order to retain the current behavior for x86 still call > iommu_domain_init() with use_iommu flag being forced to false. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Julien Grall <julien.grall@arm.com> > CC: Ian Jackson <ian.jackson@eu.citrix.com> > CC: Wei Liu <wei.liu2@citrix.com> > > --- > Changes in V1: > - Treat use_iommu flag as the ARM decision only. Don't use > common domain creation flag for it, use ARM config instead. > - Clarify patch subject/description. > --- > tools/libxl/libxl_arm.c | 10 ++++++++++ > xen/arch/arm/domain.c | 2 +- > xen/include/public/arch-arm.h | 5 +++++ > 3 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > index d842d88..9c4705e 100644 > --- a/tools/libxl/libxl_arm.c > +++ b/tools/libxl/libxl_arm.c > @@ -78,6 +78,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > return ERROR_FAIL; > } > > + /* TODO Are these assumptions enough to make decision about using IOMMU? */ > + if ((d_config->num_dtdevs && d_config->dtdevs) || > + (d_config->num_pcidevs && d_config->pcidevs)) Checking num_dtdevs and num_pcidevs is enough. It would be a bug if dtdevs and pcidevs are not null. > + xc_config->use_iommu = 1; > + else > + xc_config->use_iommu = 0; > + > + LOG(DEBUG, "The use of IOMMU %s expected for this domain", > + xc_config->use_iommu ? "is" : "isn't"); > + > return 0; > } > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index ec19310..81c4b90 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -569,7 +569,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, > ASSERT(config != NULL); > > /* p2m_init relies on some value initialized by the IOMMU subsystem */ > - if ( (rc = iommu_domain_init(d, false)) != 0 ) > + if ( (rc = iommu_domain_init(d, config->use_iommu ? true : false)) != 0 ) !!config->use_iommu is enough. > goto fail; > > if ( (rc = p2m_init(d)) != 0 ) > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index bd974fb..cb33f75 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -322,6 +322,11 @@ struct xen_arch_domainconfig { > * > */ > uint32_t clock_frequency; > + /* > + * IN > + * Inform the hypervisor that the use of IOMMU is expected for this domain. I would simplify to : "IOMMU is expected to be used for this domain". > + */ > + uint8_t use_iommu; > }; > #endif /* __XEN__ || __XEN_TOOLS__ */ > > Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 09/10] xen/arm: Add use_iommu flag to xen_arch_domainconfig 2017-05-11 11:42 ` Julien Grall @ 2017-05-11 14:04 ` Oleksandr Tyshchenko 0 siblings, 0 replies; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-11 14:04 UTC (permalink / raw) To: Julien Grall Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu, Jan Beulich On Thu, May 11, 2017 at 2:42 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi Julien > > > On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> This flag is intended to let Xen know that the guest has devices >> which will most likely be used for passthrough and as the result >> the use of IOMMU is expected for this domain. >> The primary aim of this knowledge is to help the IOMMUs that don't >> share page tables with the CPU on ARM be ready before P2M code starts >> updating IOMMU mapping. >> So, if this flag is set the non-shared IOMMUs will populate >> their page tables at the domain creation time and thereby will be able >> to handle IOMMU mapping updates from *the very beginning*. >> >> In order to retain the current behavior for x86 still call >> iommu_domain_init() with use_iommu flag being forced to false. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Julien Grall <julien.grall@arm.com> >> CC: Ian Jackson <ian.jackson@eu.citrix.com> >> CC: Wei Liu <wei.liu2@citrix.com> >> >> --- >> Changes in V1: >> - Treat use_iommu flag as the ARM decision only. Don't use >> common domain creation flag for it, use ARM config instead. >> - Clarify patch subject/description. >> --- >> tools/libxl/libxl_arm.c | 10 ++++++++++ >> xen/arch/arm/domain.c | 2 +- >> xen/include/public/arch-arm.h | 5 +++++ >> 3 files changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c >> index d842d88..9c4705e 100644 >> --- a/tools/libxl/libxl_arm.c >> +++ b/tools/libxl/libxl_arm.c >> @@ -78,6 +78,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, >> return ERROR_FAIL; >> } >> >> + /* TODO Are these assumptions enough to make decision about using >> IOMMU? */ >> + if ((d_config->num_dtdevs && d_config->dtdevs) || >> + (d_config->num_pcidevs && d_config->pcidevs)) > > > Checking num_dtdevs and num_pcidevs is enough. It would be a bug if dtdevs > and pcidevs are not null. ok > >> + xc_config->use_iommu = 1; >> + else >> + xc_config->use_iommu = 0; >> + >> + LOG(DEBUG, "The use of IOMMU %s expected for this domain", >> + xc_config->use_iommu ? "is" : "isn't"); >> + >> return 0; >> } >> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index ec19310..81c4b90 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -569,7 +569,7 @@ int arch_domain_create(struct domain *d, unsigned int >> domcr_flags, >> ASSERT(config != NULL); >> >> /* p2m_init relies on some value initialized by the IOMMU subsystem >> */ >> - if ( (rc = iommu_domain_init(d, false)) != 0 ) >> + if ( (rc = iommu_domain_init(d, config->use_iommu ? true : false)) != >> 0 ) > > > !!config->use_iommu is enough. ok > >> goto fail; >> >> if ( (rc = p2m_init(d)) != 0 ) >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h >> index bd974fb..cb33f75 100644 >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -322,6 +322,11 @@ struct xen_arch_domainconfig { >> * >> */ >> uint32_t clock_frequency; >> + /* >> + * IN >> + * Inform the hypervisor that the use of IOMMU is expected for this >> domain. > > > I would simplify to : "IOMMU is expected to be used for this domain". ok > >> + */ >> + uint8_t use_iommu; >> }; >> #endif /* __XEN__ || __XEN_TOOLS__ */ >> >> > > Cheers, > > -- > Julien Grall -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest 2017-05-10 14:03 [PATCH v1 00/10] "Non-shared" IOMMU support on ARM Oleksandr Tyshchenko ` (8 preceding siblings ...) 2017-05-10 14:03 ` [PATCH v1 09/10] xen/arm: Add use_iommu flag to xen_arch_domainconfig Oleksandr Tyshchenko @ 2017-05-10 14:03 ` Oleksandr Tyshchenko 2017-05-11 11:58 ` Julien Grall 9 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-10 14:03 UTC (permalink / raw) To: xen-devel; +Cc: wei.liu2, julien.grall, sstabellini, ian.jackson, jbeulich From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> We don't passthrough IOMMU device to DOM0 even if it is not used by Xen. Therefore exposing the property that describes the IOMMU master interfaces of the device does not make any sense. Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> --- xen/arch/arm/domain_build.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 3abacc0..2defb60 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -432,6 +432,10 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo, continue; } + /* Don't expose the property "iommus" to the guest */ + if ( dt_property_name_is_equal(prop, "iommus") ) + continue; + res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len); if ( res ) -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest 2017-05-10 14:03 ` [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest Oleksandr Tyshchenko @ 2017-05-11 11:58 ` Julien Grall 2017-05-11 14:15 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Julien Grall @ 2017-05-11 11:58 UTC (permalink / raw) To: Oleksandr Tyshchenko, xen-devel Cc: wei.liu2, sstabellini, ian.jackson, jbeulich Hi Oleksandr, On 10/05/17 15:03, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > We don't passthrough IOMMU device to DOM0 even if it is not used by > Xen. Therefore exposing the property that describes the IOMMU > master interfaces of the device does not make any sense. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > xen/arch/arm/domain_build.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 3abacc0..2defb60 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -432,6 +432,10 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo, > continue; > } > > + /* Don't expose the property "iommus" to the guest */ > + if ( dt_property_name_is_equal(prop, "iommus") ) > + continue; It would be useful to have a link to the bindings associated (Documentation/devicetree/bindings/iommu/iommu.txt). Also, whilst you are at it, you likely want to remove all the other iommu properties such as iommu-map. > + > res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len); > > if ( res ) > Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest 2017-05-11 11:58 ` Julien Grall @ 2017-05-11 14:15 ` Oleksandr Tyshchenko 2017-05-11 18:07 ` Julien Grall 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-11 14:15 UTC (permalink / raw) To: Julien Grall Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu, Jan Beulich On Thu, May 11, 2017 at 2:58 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi Julien > > On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> We don't passthrough IOMMU device to DOM0 even if it is not used by >> Xen. Therefore exposing the property that describes the IOMMU >> master interfaces of the device does not make any sense. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> xen/arch/arm/domain_build.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 3abacc0..2defb60 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -432,6 +432,10 @@ static int write_properties(struct domain *d, struct >> kernel_info *kinfo, >> continue; >> } >> >> + /* Don't expose the property "iommus" to the guest */ >> + if ( dt_property_name_is_equal(prop, "iommus") ) >> + continue; > > > It would be useful to have a link to the bindings associated > (Documentation/devicetree/bindings/iommu/iommu.txt). Agree. I will mention it in commit description. > > Also, whilst you are at it, you likely want to remove all the other iommu > properties such as iommu-map. Excuse me, I have never heard about it. Is it a required property? > >> + >> res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len); >> >> if ( res ) >> > > Cheers, > > -- > Julien Grall -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest 2017-05-11 14:15 ` Oleksandr Tyshchenko @ 2017-05-11 18:07 ` Julien Grall 2017-05-11 18:19 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 66+ messages in thread From: Julien Grall @ 2017-05-11 18:07 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu, Jan Beulich On 11/05/17 15:15, Oleksandr Tyshchenko wrote: > On Thu, May 11, 2017 at 2:58 PM, Julien Grall <julien.grall@arm.com> wrote: >> Hi Oleksandr, > Hi Julien Hi, >> >> On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >>> >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> We don't passthrough IOMMU device to DOM0 even if it is not used by >>> Xen. Therefore exposing the property that describes the IOMMU >>> master interfaces of the device does not make any sense. >>> >>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> --- >>> xen/arch/arm/domain_build.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 3abacc0..2defb60 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -432,6 +432,10 @@ static int write_properties(struct domain *d, struct >>> kernel_info *kinfo, >>> continue; >>> } >>> >>> + /* Don't expose the property "iommus" to the guest */ >>> + if ( dt_property_name_is_equal(prop, "iommus") ) >>> + continue; >> >> >> It would be useful to have a link to the bindings associated >> (Documentation/devicetree/bindings/iommu/iommu.txt). > Agree. I will mention it in commit description. > >> >> Also, whilst you are at it, you likely want to remove all the other iommu >> properties such as iommu-map. > Excuse me, I have never heard about it. Is it a required property? This property is used to translate an RID into a Master ID (see the documentation in bindings/pci/pci-iommu.txt). Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest 2017-05-11 18:07 ` Julien Grall @ 2017-05-11 18:19 ` Oleksandr Tyshchenko 2017-05-11 18:19 ` Julien Grall 0 siblings, 1 reply; 66+ messages in thread From: Oleksandr Tyshchenko @ 2017-05-11 18:19 UTC (permalink / raw) To: Julien Grall Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu, Jan Beulich On Thu, May 11, 2017 at 9:07 PM, Julien Grall <julien.grall@arm.com> wrote: > > > On 11/05/17 15:15, Oleksandr Tyshchenko wrote: >> >> On Thu, May 11, 2017 at 2:58 PM, Julien Grall <julien.grall@arm.com> >> wrote: >>> >>> Hi Oleksandr, >> >> Hi Julien > > > Hi, > > >>> >>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >>>> >>>> >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> We don't passthrough IOMMU device to DOM0 even if it is not used by >>>> Xen. Therefore exposing the property that describes the IOMMU >>>> master interfaces of the device does not make any sense. >>>> >>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> --- >>>> xen/arch/arm/domain_build.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>> index 3abacc0..2defb60 100644 >>>> --- a/xen/arch/arm/domain_build.c >>>> +++ b/xen/arch/arm/domain_build.c >>>> @@ -432,6 +432,10 @@ static int write_properties(struct domain *d, >>>> struct >>>> kernel_info *kinfo, >>>> continue; >>>> } >>>> >>>> + /* Don't expose the property "iommus" to the guest */ >>>> + if ( dt_property_name_is_equal(prop, "iommus") ) >>>> + continue; >>> >>> >>> >>> It would be useful to have a link to the bindings associated >>> (Documentation/devicetree/bindings/iommu/iommu.txt). >> >> Agree. I will mention it in commit description. >> >>> >>> Also, whilst you are at it, you likely want to remove all the other iommu >>> properties such as iommu-map. >> >> Excuse me, I have never heard about it. Is it a required property? > > > This property is used to translate an RID into a Master ID (see the > documentation in bindings/pci/pci-iommu.txt). So, these two should be skipped too: - iommu-map - iommu-map-mask Right? > > Cheers, > > -- > Julien Grall -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest 2017-05-11 18:19 ` Oleksandr Tyshchenko @ 2017-05-11 18:19 ` Julien Grall 0 siblings, 0 replies; 66+ messages in thread From: Julien Grall @ 2017-05-11 18:19 UTC (permalink / raw) To: Oleksandr Tyshchenko Cc: xen-devel, Stefano Stabellini, Ian Jackson, Wei Liu, Jan Beulich On 11/05/17 19:19, Oleksandr Tyshchenko wrote: > On Thu, May 11, 2017 at 9:07 PM, Julien Grall <julien.grall@arm.com> wrote: >> This property is used to translate an RID into a Master ID (see the >> documentation in bindings/pci/pci-iommu.txt). > So, these two should be skipped too: > - iommu-map > - iommu-map-mask > > Right? Yep. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 66+ messages in thread
end of thread, other threads:[~2017-05-19 8:56 UTC | newest] Thread overview: 66+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-10 14:03 [PATCH v1 00/10] "Non-shared" IOMMU support on ARM Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 01/10] xen/device-tree: Add dt_count_phandle_with_args helper Oleksandr Tyshchenko 2017-05-10 14:50 ` Jan Beulich 2017-05-10 15:06 ` Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks Oleksandr Tyshchenko 2017-05-12 14:23 ` Jan Beulich 2017-05-12 15:50 ` Oleksandr Tyshchenko 2017-05-12 16:17 ` Jan Beulich 2017-05-12 16:25 ` Oleksandr Tyshchenko 2017-05-15 7:22 ` Jan Beulich 2017-05-15 10:43 ` Oleksandr Tyshchenko 2017-05-15 12:33 ` Jan Beulich 2017-05-16 12:48 ` Oleksandr Tyshchenko 2017-05-16 13:11 ` Jan Beulich 2017-05-17 15:28 ` Oleksandr Tyshchenko 2017-05-17 15:39 ` Jan Beulich 2017-05-17 18:49 ` Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 03/10] xen/arm: p2m: Add helper to convert p2m type to IOMMU flags Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 04/10] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared Oleksandr Tyshchenko 2017-05-11 11:24 ` Julien Grall 2017-05-11 14:19 ` Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share Oleksandr Tyshchenko 2017-05-11 11:28 ` Julien Grall 2017-05-11 14:38 ` Oleksandr Tyshchenko 2017-05-11 17:58 ` Julien Grall 2017-05-11 18:21 ` Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init() Oleksandr Tyshchenko 2017-05-12 14:31 ` Jan Beulich 2017-05-12 17:00 ` Oleksandr Tyshchenko 2017-05-15 7:27 ` Jan Beulich 2017-05-17 19:52 ` Julien Grall 2017-05-18 8:38 ` Jan Beulich 2017-05-18 17:41 ` Oleksandr Tyshchenko 2017-05-19 6:30 ` Jan Beulich 2017-05-19 8:56 ` Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback Oleksandr Tyshchenko 2017-05-11 11:38 ` Julien Grall 2017-05-11 14:00 ` Oleksandr Tyshchenko 2017-05-11 18:06 ` Julien Grall 2017-05-11 18:43 ` Oleksandr Tyshchenko 2017-05-12 14:36 ` Jan Beulich 2017-05-10 14:03 ` [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts Oleksandr Tyshchenko 2017-05-12 14:41 ` Jan Beulich 2017-05-12 15:25 ` Oleksandr Tyshchenko 2017-05-12 15:34 ` Jan Beulich 2017-05-15 7:20 ` Jan Beulich 2017-05-15 7:42 ` Julien Grall 2017-05-15 8:19 ` Jan Beulich 2017-05-15 11:45 ` Julien Grall 2017-05-15 12:43 ` Jan Beulich 2017-05-17 15:45 ` Oleksandr Tyshchenko 2017-05-17 16:01 ` Jan Beulich 2017-05-17 18:51 ` Oleksandr Tyshchenko 2017-05-17 20:30 ` Julien Grall 2017-05-18 8:53 ` Jan Beulich 2017-05-18 18:06 ` Oleksandr Tyshchenko 2017-05-19 6:33 ` Jan Beulich 2017-05-10 14:03 ` [PATCH v1 09/10] xen/arm: Add use_iommu flag to xen_arch_domainconfig Oleksandr Tyshchenko 2017-05-11 11:42 ` Julien Grall 2017-05-11 14:04 ` Oleksandr Tyshchenko 2017-05-10 14:03 ` [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest Oleksandr Tyshchenko 2017-05-11 11:58 ` Julien Grall 2017-05-11 14:15 ` Oleksandr Tyshchenko 2017-05-11 18:07 ` Julien Grall 2017-05-11 18:19 ` Oleksandr Tyshchenko 2017-05-11 18:19 ` Julien Grall
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.