Re: [PATCH v2 5/6] x86/iommu: switch hwdom IOMMU to use a rangeset

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.12.2023 10:43, Roger Pau Monne wrote:
> @@ -476,58 +406,55 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain 
> *d)
>      if ( !map )
>          panic("IOMMU init: unable to allocate rangeset\n");
>  
> -    max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
> -    top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
> +    if ( iommu_hwdom_inclusive )
> +    {
> +        /* Add the whole range below 4GB, UNUSABLE regions will be removed. 
> */
> +        rc = rangeset_add_range(map, 0, max_pfn);
> +        if ( rc )
> +            panic("IOMMU inclusive mappings can't be added: %d\n",
> +                  rc);
> +    }
>  
> -    for ( i = 0, start = 0, count = 0; i < top; )
> +    for ( i = 0; i < e820.nr_map; i++ )
>      {
> -        unsigned long pfn = pdx_to_pfn(i);
> -        unsigned int perms = hwdom_iommu_map(d, pfn, max_pfn);
> +        struct e820entry entry = e820.map[i];
>  
> -        if ( !perms )
> -            /* nothing */;
> -        else if ( paging_mode_translate(d) )
> +        switch ( entry.type )
>          {
> -            int rc;
> +        case E820_UNUSABLE:
> +            if ( !iommu_hwdom_inclusive || PFN_DOWN(entry.addr) > max_pfn )
> +                continue;

The !iommu_hwdom_inclusive part isn't really needed here, is it? The ...

> -            rc = p2m_add_identity_entry(d, pfn,
> -                                        perms & IOMMUF_writable ? 
> p2m_access_rw
> -                                                                : 
> p2m_access_r,
> -                                        0);
> +            rc = rangeset_remove_range(map, PFN_DOWN(entry.addr),
> +                                       PFN_DOWN(entry.addr + entry.size - 
> 1));

... call here would then simply be a no-op, as it looks. And things would
overall look more safe if the removal was skipped for fewer reasons.

> @@ -605,7 +532,7 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>      rangeset_destroy(map);
>  
>      /* Use if to avoid compiler warning */
> -    if ( iommu_iotlb_flush_all(d, flush_flags) )
> +    if ( iommu_iotlb_flush_all(d, map_data.flush_flags) )
>          return;
>  }

Ah yes, here is said change. But I think for correctness this wants
moving to the earlier patch.

Jan