Re: [PATCH v2] x86/msi: dynamically map pages for MSI-X tables

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.08.2023 18:15, Ruben Hakobyan wrote:
> Xen reserves a constant number of pages that can be used for mapping
> MSI-X tables. This limit is defined by FIX_MSIX_MAX_PAGES in fixmap.h.
> 
> Reserving a fixed number of pages could result in an -ENOMEM if a
> device requests a new page when the fixmap limit is exhausted and will
> necessitate manually adjusting the limit before compilation.
> 
> To avoid the issues with the current fixmap implementation, we modify
> the MSI-X page mapping logic to instead dynamically map new pages when
> they are needed by making use of ioremap().
> 
> Note that this approach is not suitable for 32-bit architectures, where
> the virtual address space is considerably smaller.

This addresses one of the issues raised on v1. There was also the concern
of the mapping now potentially involving memory allocation (if page tables
need populating). I wonder whether, alongside emitting a warning, we
couldn't fall back to using fixmap in such an event.

Furthermore there's the concern regarding VA space use: If the 512 entries
we presently permit don't suffice, we talk about more than 2M worth of
mappings, i.e. more than 4M worth of VA space (due to guard pages).
That's not entirely negligible, even if still only a fairly small share
out of the 64G that we set aside right now, and hence probably wants at
least mentioning.

> -static int msix_get_fixmap(struct arch_msix *msix, u64 table_paddr,
> +static void __iomem *msix_get_entry(struct arch_msix *msix, u64 table_paddr,
>                             u64 entry_paddr)
>  {
>      long nr_page;
> -    int idx;
> +    void __iomem *va = NULL;

Unnecessary initializer.

>      nr_page = (entry_paddr >> PAGE_SHIFT) - (table_paddr >> PAGE_SHIFT);
>  
>      if ( nr_page < 0 || nr_page >= MAX_MSIX_TABLE_PAGES )
> -        return -EINVAL;
> +        return NULL;

Please don't lose the error code; callers should not need to blindly (and
hence possibly wrongly) assume -ENOMEM when getting back NULL. See
xen/err.h.

>      spin_lock(&msix->table_lock);
>      if ( msix->table_refcnt[nr_page]++ == 0 )
>      {
> -        idx = msix_fixmap_alloc();
> -        if ( idx < 0 )
> +        va = ioremap(round_pgdown(entry_paddr), PAGE_SIZE);
> +        if ( va == NULL )
>          {
>              msix->table_refcnt[nr_page]--;
>              goto out;
>          }
> -        set_fixmap_nocache(idx, entry_paddr);
> -        msix->table_idx[nr_page] = idx;
> +        msix->table_va[nr_page] = va;
>      }
>      else
> -        idx = msix->table_idx[nr_page];
> +        va = msix->table_va[nr_page];
>  
>   out:
>      spin_unlock(&msix->table_lock);
> -    return idx;
> +    return va + (entry_paddr & ~PAGE_MASK);

This is wrong in the error case, i.e. when va is NULL.

>  }
>  
> -static void msix_put_fixmap(struct arch_msix *msix, int idx)
> +static void msix_put_entry(struct arch_msix *msix, const void __iomem 
> *entry_va)
>  {
>      int i;
> +    void __iomem *va = (void*)round_pgdown((unsigned long)entry_va);

Nit (style): Blank after * please.

>      spin_lock(&msix->table_lock);
>      for ( i = 0; i < MAX_MSIX_TABLE_PAGES; i++ )
>      {
> -        if ( msix->table_idx[i] == idx )
> +        if ( msix->table_va[i] == va )
>              break;
>      }
>      if ( i == MAX_MSIX_TABLE_PAGES )
> @@ -115,9 +84,8 @@ static void msix_put_fixmap(struct arch_msix *msix, int 
> idx)
>  
>      if ( --msix->table_refcnt[i] == 0 )
>      {
> -        clear_fixmap(idx);
> -        msix_fixmap_free(idx);
> -        msix->table_idx[i] = 0;
> +        iounmap(va);
> +        msix->table_va[i] = NULL;

While possibly benign here, please clear the field before unmapping,
such that there's no doubt about a transiently dangling pointer.

Jan