From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@qumranet.com>
Subject: Re: [PATCH 2/4][VTD] modifications to intel-iommu.c.
Date: Fri, 20 Jun 2008 21:13:03 +0300
Message-ID: <485BF32F.7010007@qumranet.com>
References: <1FE6DD409037234FAB833C420AA843EC018831CF@orsmsx424.amr.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org, Amit Shah <amit.shah@qumranet.com>,
	Muli Ben-Yehuda <MULI@il.ibm.com>,
	Ben-Ami Yassour <benami@il.ibm.com>,
	Anthony Liguori <anthony@codemonkey.ws>,
	Chris Wright <chrisw@redhat.com>,
	"Han, Weidong" <weidong.han@intel.com>
To: "Kay, Allen M" <allen.m.kay@intel.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from il.qumranet.com ([212.179.150.194]:23029 "EHLO il.qumranet.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750958AbYFTSNF (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 20 Jun 2008 14:13:05 -0400
In-Reply-To: <1FE6DD409037234FAB833C420AA843EC018831CF@orsmsx424.amr.corp.intel.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Kay, Allen M wrote:
> Modification to intel-iommu.c to support vt-d page table and context
> table mapping in kvm.  Mods to dmar.c and iova.c are due to header file
> moves to include/linux.
>   

> diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c
> index f941f60..a58a5b0 100644
> --- a/drivers/pci/dmar.c
> +++ b/drivers/pci/dmar.c
> @@ -26,8 +26,8 @@
>  
>  #include <linux/pci.h>
>  #include <linux/dmar.h>
> -#include "iova.h"
> -#include "intel-iommu.h"
> +#include <linux/iova.h>
> +#include <linux/intel-iommu.h>

This should have been done in the file movement patch to avoid breaking 
the build. 

>
>  
> +void kvm_intel_iommu_domain_exit(struct dmar_domain *domain)

This should be a generic API, not a kvm specific one.

> +{
> +    u64 end;
> +
> +    /* Domain 0 is reserved, so dont process it */
> +    if (!domain)
> +        return;

'domain' here is a pointer, not an identifier.

>
> +int kvm_intel_iommu_context_mapping(
> +    struct dmar_domain *domain, struct pci_dev *pdev)
> +{
> +    int rc;
> +    rc = domain_context_mapping(domain, pdev);
> +    return rc;
> +}
> +EXPORT_SYMBOL_GPL(kvm_intel_iommu_context_mapping);

What does the return value mean?

> +
> +int kvm_intel_iommu_page_mapping(
> +    struct dmar_domain *domain, dma_addr_t iova,
> +    u64 hpa, size_t size, int prot)
> +{
> +    int rc;
> +    rc = domain_page_mapping(domain, iova, hpa, size, prot);
> +    return rc;
> +}
> +EXPORT_SYMBOL_GPL(kvm_intel_iommu_page_mapping);

The function name makes it sound like it's retrieving information.  If 
it does something, put a verb in there.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.