Re: [PATCH 3/3] x86/iommu: use a rangeset for hwdom setup

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.11.2023 10:47, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -364,9 +364,20 @@ static const struct hvm_mmcfg *vpci_mmcfg_find(const 
> struct domain *d,
>      return NULL;
>  }
>  
> -bool vpci_is_mmcfg_address(const struct domain *d, paddr_t addr)
> +int vpci_subtract_mmcfg(const struct domain *d, struct rangeset *r)

While there, also add __hwdom_init?

> @@ -447,62 +451,135 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain 
> *d)
>      if ( iommu_hwdom_passthrough )
>          return;
>  
> -    max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
> -    top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
> +    map = rangeset_new(NULL, NULL, 0);
> +    if ( !map )
> +        panic("IOMMU initialization failed unable to allocate rangeset\n");

This reads a little odd, and could probably do with shortening anyway
(e.g. "IOMMU init: unable to allocate rangeset\n").

> +    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;

It's > here, but ...

> -            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));
>              if ( rc )
> -                printk(XENLOG_WARNING
> -                       "%pd: identity mapping of %lx failed: %d\n",
> -                       d, pfn, rc);
> +                panic("IOMMU failed to remove unusable memory: %d\n",
> +                      rc);
> +            continue;
> +
> +        case E820_RESERVED:
> +            if ( !iommu_hwdom_inclusive && !iommu_hwdom_reserved )
> +                continue;
> +            break;
> +
> +        case E820_RAM:
> +            if ( iommu_hwdom_strict )
> +                continue;
> +            break;
> +
> +        default:
> +            if ( !iommu_hwdom_inclusive || PFN_DOWN(entry.addr) >= max_pfn )
> +                continue;

... >= here?

> +            entry.size = pfn_to_paddr(max_pfn) - 1 - entry.addr;

Why the subtraction of 1 when you're calculating size? Don't you actually
need to add 1 to max_pfn before converting to paddr?

While overall things look plausible elsewhere, I'm hoping that - as asked
for by Andrew - it'll be possible to split this some, to make it a little
more reasonable to actually look at the changes done.

Jan