Re: [Xen-devel] [RFC PATCH 8/9] iommu: Split iommu_hwdom_init() into arch specific parts

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On Thu, Mar 23, 2017 at 11:08 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 22.03.17 at 19:40, <olekstysh@xxxxxxxxx> wrote:
>> On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>> On 15.03.17 at 21:05, <olekstysh@xxxxxxxxx> 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
>



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel