Re: [Xen-devel] [PATCH v2 07/13] iommu: Make decision about needing IOMMU for hardware domains in advance

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi, Jan.

On Wed, Dec 6, 2017 at 7:01 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 25.07.17 at 19:26, <olekstysh@xxxxxxxxx> wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>
>> The hardware domains require IOMMU to be used in the most cases and
>> a decision to use it is made at hardware domain construction time.
>> But, it is not the best moment for the non-shared IOMMUs due to
>> the necessity of retrieving all mapping which could happen in a period
>> of time between IOMMU per-domain initialization and this moment.
>
> Which mappings are you talking about here? Just like with the earlier
> patch - the reason for the change needs to be clear to someone
> reading just this commit message.

I am talking about the IOMMU mappings (gfn <-> mfn) which were skipped and
as the result they didn't reach IOMMU pagetable.
P2M code didn't invoke iommu_map_pages() since a "need_iommu" flag wasn't set.
So, iterating through the list of the pages we had to re-create lost
IOMMU mapping page by page.

I will clarify description.

>
>> @@ -141,6 +141,15 @@ int iommu_domain_init(struct domain *d, bool use_iommu)
>>      if ( !iommu_enabled )
>>          return 0;
>>
>> +    if ( is_hardware_domain(d) )
>> +    {
>> +        if ( (paging_mode_translate(d) && !iommu_passthrough) ||
>> +              iommu_dom0_strict )
>> +            use_iommu = 1;
>> +        else
>> +            use_iommu = 0;
>
> I'd prefer if you used a simple assignment here, rather than if/else.
ok

>
>> @@ -175,37 +182,6 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>          return;
>>
>>      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);
>> -    }
>>
>>      return hd->platform_ops->hwdom_init(d);
>>  }
>
> Just to double check - this change was tested on x86 Dom0, at
> least PV (for PVH I'd at least expect that you've did some static
> code analysis to make sure this doesn't put in further roadblocks)?

I am afraid I didn't get the second part of this sentence.

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel