Re: [PATCH v3 02/11] x86/iommu: add common page-table allocator

[Jan Beulich <jbeulich@suse.com>]

On 03.08.2020 14:29, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> Instead of having separate page table allocation functions in VT-d and AMD
> IOMMU code, we could use a common allocation function in the general x86 code.
> 
> This patch adds a new allocation function, iommu_alloc_pgtable(), for this
> purpose. The function adds the page table pages to a list. The pages in this
> list are then freed by iommu_free_pgtables(), which is called by
> domain_relinquish_resources() after PCI devices have been de-assigned.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> 
> v2:
>  - This is split out from a larger patch of the same name in v1
> ---
>  xen/arch/x86/domain.c               |  9 +++++-
>  xen/drivers/passthrough/x86/iommu.c | 50 +++++++++++++++++++++++++++++
>  xen/include/asm-x86/iommu.h         |  7 ++++
>  3 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index f8084dc9e3..d1ecc7b83b 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -2153,7 +2153,8 @@ int domain_relinquish_resources(struct domain *d)
>          d->arch.rel_priv = PROG_ ## x; /* Fallthrough */ case PROG_ ## x
>  
>          enum {
> -            PROG_paging = 1,
> +            PROG_iommu_pagetables = 1,
> +            PROG_paging,
>              PROG_vcpu_pagetables,
>              PROG_shared,
>              PROG_xen,

Is there a particular reason to make this new item the very first
one?

> @@ -257,6 +260,53 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>          return;
>  }
>  
> +int iommu_free_pgtables(struct domain *d)
> +{
> +    struct domain_iommu *hd = dom_iommu(d);
> +    struct page_info *pg;
> +
> +    while ( (pg = page_list_remove_head(&hd->arch.pgtables.list)) )
> +    {
> +        free_domheap_page(pg);
> +
> +        if ( general_preempt_check() )
> +            return -ERESTART;

Perhaps better only check once every so many pages?

> +struct page_info *iommu_alloc_pgtable(struct domain *d)
> +{
> +    struct domain_iommu *hd = dom_iommu(d);
> +    unsigned int memflags = 0;
> +    struct page_info *pg;
> +    void *p;
> +
> +#ifdef CONFIG_NUMA
> +    if (hd->node != NUMA_NO_NODE)

Missing blanks inside parentheses.

> @@ -131,6 +135,9 @@ int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
>          iommu_vcall(ops, sync_cache, addr, size);       \
>  })
>  
> +int __must_check iommu_free_pgtables(struct domain *d);
> +struct page_info * __must_check iommu_alloc_pgtable(struct domain *d);

Commonly we put a blank on the left side of *, but none to its right.

Jan