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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
