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

[Han, Weidong]

Avi Kivity wrote:
> 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.

This flag just helps identify kvm VT-d usage, and let kvm VT-d APIs reuse native VT-d code, it needn't duplicate code for kvm VT-d APIs, and it won't impact existed native VT-d code. 

> 
> 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.

The expanded versions that supports multiple devices will need to change dmar_domain, this will cause lots of changes, almost duplicate the main functions.

> 
>>    - "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.

will add a reference count for it.

> 
>> +
>> +               /* 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).

I think we can check the smallest agaw across iommus for mapped memory when mapping memory.

Regards,
Weidong