Re: [PATCH 4/4][VTD] vt-d specific files in KVM

[Anthony Liguori <anthony@codemonkey.ws>]

Muli Ben-Yehuda wrote:
> On Mon, Jun 09, 2008 at 05:43:15PM -0700, Kay, Allen M wrote:
>   
>> vt-d specific files in KVM for contructing vt-d page tables and
>> programming vt-d context entries.
>>     
>
> Hi Allen,
>
> Some comments below, patches will follow up.
>  
>   
>> Signed-off-by: Allen M. Kay <allen.m.kay@intel.com>
>>     
>
>   
>> diff --git a/arch/x86/kvm/vtd.c b/arch/x86/kvm/vtd.c
>> new file mode 100644
>> index 0000000..634802c
>> --- /dev/null
>> +++ b/arch/x86/kvm/vtd.c
>> @@ -0,0 +1,197 @@
>> +/*
>> + * Copyright (c) 2006, Intel Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
>> + * Place - Suite 330, Boston, MA 02111-1307 USA.
>> + *
>> + * Copyright (C) 2006-2008 Intel Corporation
>> + * Author: Allen M. Kay <allen.m.kay@intel.com>
>> + * Author: Weidong Han <weidong.han@intel.com>
>> + */
>> +
>> +#include <linux/list.h>
>> +#include <linux/kvm_host.h>
>> +#include <linux/pci.h>
>> +#include <linux/dmar.h>
>> +#include <linux/intel-iommu.h>
>> +#include "vtd.h"
>> +
>> +int kvm_iommu_map_pages(struct kvm *kvm,
>> +	gfn_t base_gfn, unsigned long npages)
>> +{
>> +	gfn_t gfn = base_gfn;
>> +	pfn_t pfn;
>> +	struct page *page;
>> +	int i, rc;
>> +
>> +	if (!kvm->arch.domain)
>> +		return -EFAULT;
>> +
>> +	printk(KERN_DEBUG "kvm_iommu_map_page: gpa = %lx\n",
>> +		gfn << PAGE_SHIFT);
>> +	printk(KERN_DEBUG "kvm_iommu_map_page: hpa = %lx\n",
>> +		gfn_to_pfn(kvm, base_gfn) << PAGE_SHIFT);
>> +	printk(KERN_DEBUG "kvm_iommu_map_page: size = %lx\n",
>> +		npages*PAGE_SIZE);
>> +
>> +	for (i = 0; i < npages; i++) {
>> +		pfn = gfn_to_pfn(kvm, gfn);
>> +		if (pfn_valid(pfn)) {
>>     
>
> Checking against pfn_valid() isn't enough to differentiate between RAM
> and MMIO areas. I think the consensus was that we also need to check
> PageReserved(), i.e.,
>
> if (pfn_valid(pfn) && !PageReserved(pfn_to_page(pfn))) ...
>   

When checking the error return of gfn_to_pfn(), you should use 
is_error_pfn().  There's no need to differentiate mmio/ram pages in the 
code, the goal is just error checking.

>> +			rc = kvm_intel_iommu_page_mapping(kvm->arch.domain,
>> +				gfn << PAGE_SHIFT, pfn << PAGE_SHIFT,
>> +				PAGE_SIZE, DMA_PTE_READ | DMA_PTE_WRITE);
>> +			if (rc) {
>> +				page = gfn_to_page(kvm, gfn);
>> +				put_page(page);
>>     

kvm_release_pfn_clean() should be used here.

> If we fail to map some of the domain's memory, shouldn't we bail out
> of giving it pass-through access at all?
>
>   
>> +			}
>> +		} else {
>> +			printk(KERN_DEBUG "kvm_iommu_map_page:"
>> +				"invalid pfn=%lx\n", pfn);
>> +			return 0;
>>     
>
> I think we should BUG_ON() (or at least WARN_ON()) if we hit a slot
> that has both RAM and an MMIO region. 
>
>   
>> +		}
>> +		gfn++;
>> +	}
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_iommu_map_pages);
>> +
>> +static int kvm_iommu_map_memslots(struct kvm *kvm)
>> +{
>> +	int i, rc;
>> +	for (i = 0; i < kvm->nmemslots; i++) {
>> +		rc = kvm_iommu_map_pages(kvm, kvm->memslots[i].base_gfn,
>> +				kvm->memslots[i].npages);
>> +		if (rc)
>> +			return rc;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int kvm_iommu_unmap_memslots(struct kvm *kvm);
>> +int kvm_iommu_map_guest(struct kvm *kvm,
>> +	struct kvm_pci_passthrough_dev *pci_pt_dev)
>> +{
>> +	struct pci_dev *pdev = NULL;
>> +
>> +	printk(KERN_DEBUG "kvm_iommu_map_guest: host bdf = %x:%x:%x\n",
>> +		pci_pt_dev->host.busnr,
>> +		PCI_SLOT(pci_pt_dev->host.devfn),
>> +		PCI_FUNC(pci_pt_dev->host.devfn));
>> +
>> +	for_each_pci_dev(pdev) {
>> +		if ((pdev->bus->number == pci_pt_dev->host.busnr) &&
>> +			(pdev->devfn == pci_pt_dev->host.devfn))
>> +			goto found;
>>     
>
> We can stick the `found' stanza in a seperate function and call it
> here, which gets rid of one goto.
>
>   
>> +	}
>> +	if (kvm->arch.domain) {
>> +		kvm_intel_iommu_domain_exit(kvm->arch.domain);
>> +		kvm->arch.domain = NULL;
>> +	}
>> +	return -ENODEV;
>> +found:
>> +	kvm->arch.domain = kvm_intel_iommu_domain_alloc(pdev);
>> +	if (kvm->arch.domain == NULL)
>> +		printk(KERN_WARN "kvm_iommu_map_guest: domain == NULL\n");
>> +	else
>> +		printk(KERN_INFO "kvm_iommu_map_guest: domain = %p\n",
>> +			kvm->arch.domain);
>> +	if (kvm_iommu_map_memslots(kvm)) {
>>     
>
> We shouldn't call map_memslots if domain == NULL.
>
>   
>> +		kvm_iommu_unmap_memslots(kvm);
>> +		return -EFAULT;
>> +	}
>> +	kvm_intel_iommu_context_mapping(kvm->arch.domain, pdev);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_iommu_map_guest);
>> +
>> +static int kvm_iommu_put_pages(struct kvm *kvm,
>> +	gfn_t base_gfn, unsigned long npages)
>> +{
>> +	gfn_t gfn = base_gfn;
>> +	struct page *page;
>> +	int i;
>> +
>> +	if (!kvm->arch.domain)
>> +		return -EFAULT;
>> +
>> +	printk(KERN_DEBUG "kvm_iommu_put_pages: gpa = %lx\n",
>> +		gfn << PAGE_SHIFT);
>> +	printk(KERN_DEBUG "kvm_iommu_put_pages: hpa = %lx\n",
>> +		gfn_to_pfn(kvm, gfn) << PAGE_SHIFT);
>> +	printk(KERN_DEBUG "kvm_iommu_put_pages: size = %lx\n",
>> +		npages*PAGE_SIZE);
>> +
>> +	for (i = 0; i < npages; i++) {
>> +		page = gfn_to_page(kvm, gfn);
>> +		put_page(page);
>> +		gfn++;
>>     

Likewise, you should use kvm_release_pfn_dirty() here.

Note, this patch series isn't bisect friendly.  In the third patch, you 
introduce a makefile change for vtd.o but don't introduce the file until 
the fourth patch.  This will break bisection.

Regards,

Anthony Liguori