| Message ID | 1489608329-7275-9-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: > --- 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_page(d, gfn, mfn, 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); The code being moved from here has survived the previous separation of arch-independent from arch-specific code, and looking at the code I also can't immediately see what's x86-specific here, so please make the description explain why the code as is can't be used. Jan
On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >> --- 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_page(d, gfn, mfn, 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); > > The code being moved from here has survived the previous separation > of arch-independent from arch-specific code, and looking at the code > I also can't immediately see what's x86-specific here, so please make > the description explain why the code as is can't be used. > > Jan > You are right, there is nothing x86-specific here at first sight. The reason why I moved this code to x86 folder is that we don't need to retrieve IOMMU mappings on ARM this way. With need_iommu being explicitly set at the hardware domain creation time we just need to ask unshared IOMMU driver to allocate its page table to be ready to receive and process IOMMU mappings from P2M code. Other points that had prevented me from using it as is. If the hardware domain isn't 1:1 mapped we won't know gfn. IIRC, we can find mfn by gfn for particular domain on ARM, but not vise versa. Also this iommu_hwdom_init() is being called before allocating domain memory on ARM. What is the point? So, the d->page_list is empty during it execution.
>>> On 22.03.17 at 19:40, <olekstysh@gmail.com> wrote: > On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >>> --- 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_page(d, gfn, mfn, 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); >> >> The code being moved from here has survived the previous separation >> of arch-independent from arch-specific code, and looking at the code >> I also can't immediately see what's x86-specific here, so please make >> the description explain why the code as is can't be used. > > You are right, there is nothing x86-specific here at first sight. > The reason why I moved this code to x86 folder is that we don't need > to retrieve IOMMU mappings on ARM this way. With need_iommu being > explicitly set at the hardware domain creation time > we just need to ask unshared IOMMU driver to allocate its page table > to be ready to receive and process IOMMU mappings from P2M code. > > Other points that had prevented me from using it as is. > If the hardware domain isn't 1:1 mapped we won't know gfn. IIRC, we > can find mfn by gfn for particular domain on ARM, but not vise versa. > Also this iommu_hwdom_init() is being called before allocating domain > memory on ARM. What is the point? > So, the d->page_list is empty during it execution. In which case the question even more so is - why move the code if it simply does nothing in your case? Jan
On Thu, Mar 23, 2017 at 11:08 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 22.03.17 at 19:40, <olekstysh@gmail.com> wrote: >> On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >>>> --- 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_page(d, gfn, mfn, 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); >>> >>> The code being moved from here has survived the previous separation >>> of arch-independent from arch-specific code, and looking at the code >>> I also can't immediately see what's x86-specific here, so please make >>> the description explain why the code as is can't be used. >> >> You are right, there is nothing x86-specific here at first sight. >> The reason why I moved this code to x86 folder is that we don't need >> to retrieve IOMMU mappings on ARM this way. With need_iommu being >> explicitly set at the hardware domain creation time >> we just need to ask unshared IOMMU driver to allocate its page table >> to be ready to receive and process IOMMU mappings from P2M code. >> >> Other points that had prevented me from using it as is. >> If the hardware domain isn't 1:1 mapped we won't know gfn. IIRC, we >> can find mfn by gfn for particular domain on ARM, but not vise versa. >> Also this iommu_hwdom_init() is being called before allocating domain >> memory on ARM. What is the point? >> So, the d->page_list is empty during it execution. > > In which case the question even more so is - why move the code > if it simply does nothing in your case? Reasonable question. Before answering you I would like to clarify the reason why the iommu_hwdom_init() is being called before allocating domain memory on ARM. Is it a bug or there is an explanation for doing this. I think that even if I don't move this code to x86 part I will still need to call arch_iommu_hwdom_init() after. In such case arch_iommu_hwdom_init() would do what it is supposed to do on ARM and would be a stub on x86. What do you think? > > Jan >
>>> On 23.03.17 at 13:40, <olekstysh@gmail.com> wrote: > On Thu, Mar 23, 2017 at 11:08 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 22.03.17 at 19:40, <olekstysh@gmail.com> wrote: >>> On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >>>>> --- 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_page(d, gfn, mfn, 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); >>>> >>>> The code being moved from here has survived the previous separation >>>> of arch-independent from arch-specific code, and looking at the code >>>> I also can't immediately see what's x86-specific here, so please make >>>> the description explain why the code as is can't be used. >>> >>> You are right, there is nothing x86-specific here at first sight. >>> The reason why I moved this code to x86 folder is that we don't need >>> to retrieve IOMMU mappings on ARM this way. With need_iommu being >>> explicitly set at the hardware domain creation time >>> we just need to ask unshared IOMMU driver to allocate its page table >>> to be ready to receive and process IOMMU mappings from P2M code. >>> >>> Other points that had prevented me from using it as is. >>> If the hardware domain isn't 1:1 mapped we won't know gfn. IIRC, we >>> can find mfn by gfn for particular domain on ARM, but not vise versa. >>> Also this iommu_hwdom_init() is being called before allocating domain >>> memory on ARM. What is the point? >>> So, the d->page_list is empty during it execution. >> >> In which case the question even more so is - why move the code >> if it simply does nothing in your case? > > Reasonable question. Before answering you I would like to clarify the reason > why the iommu_hwdom_init() is being called before allocating domain > memory on ARM. > Is it a bug or there is an explanation for doing this. Well, as that's different from x86 I'm afraid I'm the wrong one to ask - the ARM maintainers would likely be in a much better position to explain. The code sequence in each arch's construct_dom0() is pretty clearly very different in this regard (ARM calls the function almost first, while x86 calls it almost last. Jan
Hi, Sorry for the late answer. On 23/03/17 12:40, Oleksandr Tyshchenko wrote: > On Thu, Mar 23, 2017 at 11:08 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 22.03.17 at 19:40, <olekstysh@gmail.com> wrote: >>> On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >>>> The code being moved from here has survived the previous separation >>>> of arch-independent from arch-specific code, and looking at the code >>>> I also can't immediately see what's x86-specific here, so please make >>>> the description explain why the code as is can't be used. >>> >>> You are right, there is nothing x86-specific here at first sight. >>> The reason why I moved this code to x86 folder is that we don't need >>> to retrieve IOMMU mappings on ARM this way. With need_iommu being >>> explicitly set at the hardware domain creation time >>> we just need to ask unshared IOMMU driver to allocate its page table >>> to be ready to receive and process IOMMU mappings from P2M code. >>> >>> Other points that had prevented me from using it as is. >>> If the hardware domain isn't 1:1 mapped we won't know gfn. IIRC, we >>> can find mfn by gfn for particular domain on ARM, but not vise versa. >>> Also this iommu_hwdom_init() is being called before allocating domain >>> memory on ARM. What is the point? >>> So, the d->page_list is empty during it execution. >> >> In which case the question even more so is - why move the code >> if it simply does nothing in your case? I didn't move the code, because back then we were only planning to support shared page table (iommu_use_hap_pt(...) always returns true on ARM so far). With more perspective, this code cannot work on ARM because of mfn_to_gmfn (we don't have an M2P). If we keep the code like that after this series, this will at least expose a bug (the helper always assume a direct mapping). So I think moving the code is the right solution. > > Reasonable question. Before answering you I would like to clarify the reason > why the iommu_hwdom_init() is being called before allocating domain > memory on ARM. > Is it a bug or there is an explanation for doing this. No real reason, I didn't see a reason to call this function later on. I would be interested to know whether there is a latent bug. Anyway, I think this is a good idea to fully initialize the IOMMU early for DOM0 as the builder will take care of assigning the non-PCI device protected. Cheers,
On Wed, Apr 19, 2017 at 9:09 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi, Hi, Julien. > > Sorry for the late answer. > > On 23/03/17 12:40, Oleksandr Tyshchenko wrote: >> >> On Thu, Mar 23, 2017 at 11:08 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> >>>>>> On 22.03.17 at 19:40, <olekstysh@gmail.com> wrote: >>>> >>>> On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>> >>>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >>>>> >>>>> The code being moved from here has survived the previous separation >>>>> of arch-independent from arch-specific code, and looking at the code >>>>> I also can't immediately see what's x86-specific here, so please make >>>>> the description explain why the code as is can't be used. >>>> >>>> >>>> You are right, there is nothing x86-specific here at first sight. >>>> The reason why I moved this code to x86 folder is that we don't need >>>> to retrieve IOMMU mappings on ARM this way. With need_iommu being >>>> explicitly set at the hardware domain creation time >>>> we just need to ask unshared IOMMU driver to allocate its page table >>>> to be ready to receive and process IOMMU mappings from P2M code. >>>> >>>> Other points that had prevented me from using it as is. >>>> If the hardware domain isn't 1:1 mapped we won't know gfn. IIRC, we >>>> can find mfn by gfn for particular domain on ARM, but not vise versa. >>>> Also this iommu_hwdom_init() is being called before allocating domain >>>> memory on ARM. What is the point? >>>> So, the d->page_list is empty during it execution. >>> >>> >>> In which case the question even more so is - why move the code >>> if it simply does nothing in your case? > > > I didn't move the code, because back then we were only planning to support > shared page table (iommu_use_hap_pt(...) always returns true on ARM so far). > > With more perspective, this code cannot work on ARM because of mfn_to_gmfn > (we don't have an M2P). If we keep the code like that after this series, > this will at least expose a bug (the helper always assume a direct mapping). Agree. > > So I think moving the code is the right solution. > >> >> Reasonable question. Before answering you I would like to clarify the >> reason >> why the iommu_hwdom_init() is being called before allocating domain >> memory on ARM. >> Is it a bug or there is an explanation for doing this. > > > No real reason, I didn't see a reason to call this function later on. I > would be interested to know whether there is a latent bug. > > Anyway, I think this is a good idea to fully initialize the IOMMU early for > DOM0 as the builder will take care of assigning the non-PCI device > protected. Does this patch do right things or I have missed something? > > Cheers, > > -- > Julien Grall
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 6c17c59..cfe3bd1 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_page(d, gfn, mfn, 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 69cd6c5..b353449 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_page(d, gfn, mfn, 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 3150d7b..43cbb80 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);