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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi, Roger

On Thu, Jan 18, 2018 at 2:09 PM, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> Sorry for the delay in the reply.
No problem.

>
> On Tue, Jul 25, 2017 at 08:26:49PM +0300, Oleksandr Tyshchenko 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.
>>
>> So, make a decision about needing IOMMU a bit earlier, in 
>> iommu_domain_init().
>> Having "d->need_iommu" flag set at the early stage we won't skip
>> any IOMMU mapping updates. And as the result the existing in 
>> iommu_hwdom_init()
>> code that goes through the list of the pages and tries to retrieve mapping
>> for non-shared IOMMUs won't be needed anymore and can be just dropped.
>
> If I understand this correctly the approach looks fine to me, and it's
> inline with what I'm doing for PVHv2 Dom0. Ie: the IOMMU is
> initialized _before_ populating the memory map, so that when pages are
> added to the p2m they are also added to the IOMMU page tables if
> required.
>
> This avoids having to iterate over the list of domain pages in
> iommu_hwdom_init, because it's empty at the point iommu_hwdom_init is
> called for a PVHv2 Dom0.
>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxx>
>>
>> ---
>> Changes in v1:
>>    -
>>
>> Changes in v2:
>>    - This is the result of reworking old patch:
>>      [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific 
>> parts
>> ---
>>  xen/drivers/passthrough/iommu.c | 44 
>> ++++++++++-------------------------------
>>  1 file changed, 10 insertions(+), 34 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/iommu.c 
>> b/xen/drivers/passthrough/iommu.c
>> index 19c87d1..f5e5b7e 100644
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -52,7 +52,7 @@ custom_param("iommu", parse_iommu_param);
>>  bool_t __initdata iommu_enable = 1;
>>  bool_t __read_mostly iommu_enabled;
>>  bool_t __read_mostly force_iommu;
>> -bool_t __hwdom_initdata iommu_dom0_strict;
>> +bool_t __read_mostly iommu_dom0_strict;
>>  bool_t __read_mostly iommu_verbose;
>>  bool_t __read_mostly iommu_workaround_bios_bug;
>>  bool_t __read_mostly iommu_igfx = 1;
>> @@ -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;
>> +    }
>> +
>>      hd->platform_ops = iommu_get_ops();
>>      ret = hd->platform_ops->init(d, use_iommu);
>>      if ( ret )
>> @@ -161,8 +170,6 @@ static void __hwdom_init check_hwdom_reqs(struct domain 
>> *d)
>>      if ( iommu_passthrough )
>>          panic("Dom0 uses paging translated mode, dom0-passthrough must not 
>> be "
>>                "enabled\n");
>> -
>> -    iommu_dom0_strict = 1;
>>  }
>>
>>  void __hwdom_init iommu_hwdom_init(struct domain *d)
>> @@ -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;
>
> Where is this set now? You seem to remove setting need_iommu here, but
> I don't see it being set anywhere else. Am I missing something?

d->need_iommu is set in iommu_domain_init(). This was done by previous
patch [1].
For your convenience you can see how the whole function looks like [2].

[1]
[PATCH v2 06/13] iommu: Add extra use_iommu argument to iommu_domain_init()
https://marc.info/?l=xen-devel&m=150100368126600&w=2

[2] 
https://github.com/otyshchenko1/xen/blob/non_shared_iommu_v2/xen/drivers/passthrough/iommu.c#L158

>
> Thanks, Roger.

-- 
Regards,

Oleksandr Tyshchenko

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