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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

Sorry for the delay in the reply.

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?

Thanks, Roger.

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