Yu, Fenghua wrote:
> 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.  
> 
> I would agree with Avi. It's wired to merge the KVM api and native
> api together while they are logically separate. If we have two sets
> of interfaces for KVM and native iommu each, code is clean and robust
> and easy to extend in the future. If you well organize the code, we
> should have both good interface and shared code. While it makes sense
> to share code at beginning of KVM like this, it will get harder to
> maintain and evolve both KVM and native later, e.g. changing one part
> may break the other.

I will remove the flag judgement, and implement independent functions for KVM. 
       
> 
> Some comments on the code in this patch:
> 
> 1. You don't need to check DOMAIN_FLAG_VIRTUAL_MACHINE in the DMA
> functions. When the DMA functions are called, the domain should not
> have the DOMAIN_FLAG_VIRTUAL_MACHINE flag. Otherwise, the domain
> allocation code is wrong. 

Yes,  I will remove these meaningless checks.
  
> 
> 2. You need to initialize domain->flags=0 for DMA domain. It's random
> number after the domain is allocated by kmem_cache_alloc. 

Right. Thanks.

Regards,
Weidong

> 
> Thanks.
> 
> -Fenghua

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

Reply via email to