Re: [PATCH 1/2] VT-d: Support multiple device assignment for KVM

[Avi Kivity <avi@redhat.com>]

Han, Weidong wrote:
> In order to support multiple device assignment for KVM, this patch does following main changes:
>    - extend dmar_domain to own multiple devices from different iommus, use a bitmap of iommus to replace iommu pointer in dmar_domain.
>    - add a flag DOMAIN_FLAG_VIRTUAL_MACHINE to represent KVM VT-d usage. Many functions (e.g. intel_map_single() and intel_unmap_single()) won't be used by KVM VT-d. Let them return directly when this flag is set.
>   


This seems brittle.  An API that has some functions shorted out 
depending on some flag is hard to understand and use.

We should either implement the functions, or split the API into a basic 
version that talks only to one device, and an expanded versions that 
talks to multiple devices, and is implemented by the using the lower 
level API.  This may require more changes due to the need to share io 
pagetables.

>    - "SAGAW" capability may be different across iommus, that's to say the VT-d page table levels may be different among iommus. This patch uses a defaut agaw, and skip top levels of page tables for iommus which have smaller agaw than default.
>   

Neat trick.

>  void free_dmar_iommu(struct intel_iommu *iommu)
>  {
>         struct dmar_domain *domain;
> @@ -960,7 +1054,14 @@ void free_dmar_iommu(struct intel_iommu *iommu)
>         for (; i < cap_ndoms(iommu->cap); ) {
>                 domain = iommu->domains[i];
>                 clear_bit(i, iommu->domain_ids);
> -               domain_exit(domain);
> +
> +               if (domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) {
> +                       /* domain may be referenced by other iommus */
> +                       if (domain_in_other_iommus(domain, iommu) == 0)
> +                               domain_exit(domain);
> +               }
> +               else
> +                       domain_exit(domain);
>   

Things like this are best expressed using reference counts, which 
removes the need for the test as well.

> +
> +               /* Skip top levels of page tables for
> +                * iommu which has less agaw than default.
> +                */
> +               for (agaw = domain->agaw; agaw != iommu->agaw; agaw--) {
> +                       pgd = phys_to_virt(dma_pte_addr(*pgd));
> +                       if (!dma_pte_present(*pgd)) {
> +                               spin_unlock_irqrestore(&iommu->lock, flags);
> +                               return -ENOMEM;
> +                       }
> +               }
> +       }
>   

Need to check that the agaw is sufficient for mapped memory (and when 
adding a device or mapping more memory, need a similar check).


-- 
error compiling committee.c: too many arguments to function