Amit Shah wrote:
quoted text
> We introduce three hypercalls:
> 1. When the guest wants to check if a particular device is an assigned device
>    (this is done once per device by the guest to enable / disable hypercall-
>    based translation of addresses)
> 
> 2. map: to convert guest phyical addresses to host physical address to pass on
>    to the device for DMA. We also pin the pages thus requested so that they're
>    not swapped out.
> 
> 3. unmap: to unpin the pages and free any information we might have stored.
> 
> Signed-off-by: Amit Shah <amit.shah@qumranet.com>
> ---
>  arch/x86/kvm/x86.c         |  211 +++++++++++++++++++++++++++++++++++++++++++-
>  include/asm-x86/kvm_host.h |   15 +++
>  include/asm-x86/kvm_para.h |    8 ++
>  3 files changed, 233 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fb9b329..94ee4db 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -24,8 +24,11 @@
>  #include <linux/interrupt.h>
>  #include <linux/kvm.h>
>  #include <linux/fs.h>
> +#include <linux/list.h>
> +#include <linux/pci.h>
>  #include <linux/vmalloc.h>
>  #include <linux/module.h>
> +#include <linux/highmem.h>
>  #include <linux/mman.h>
>  #include <linux/highmem.h>
>  
> @@ -76,6 +79,9 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>  	{ "halt_exits", VCPU_STAT(halt_exits) },
>  	{ "halt_wakeup", VCPU_STAT(halt_wakeup) },
>  	{ "hypercalls", VCPU_STAT(hypercalls) },
> +	{ "hypercall_map", VCPU_STAT(hypercall_map) },
> +	{ "hypercall_unmap", VCPU_STAT(hypercall_unmap) },
> +	{ "hypercall_pv_dev", VCPU_STAT(hypercall_pv_dev) },
>  	{ "request_irq", VCPU_STAT(request_irq_exits) },
>  	{ "irq_exits", VCPU_STAT(irq_exits) },
>  	{ "host_state_reload", VCPU_STAT(host_state_reload) },
> @@ -95,9 +101,164 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>  	{ NULL }
>  };
>  
> +static struct kvm_pv_dma_map*
> +find_pci_pv_dmap(struct list_head *head, dma_addr_t dma)
> +{
might be better to prefix those functions with kvm? Even though they are 
static, it seems to be the current practice.

quoted text
> +static int pv_map_hypercall(struct kvm_vcpu *vcpu, int npages, gfn_t page_gfn)
> +{
> +	int i, r = 0;
> +	struct page *host_page;
> +	struct scatterlist *sg;
> +	struct kvm_pv_dma_map *dmap;
> +	unsigned long *shared_addr, *hcall_page;
> +
> +	/* We currently don't support dma mappings which have more than
> +	 * PAGE_SIZE/sizeof(unsigned long *) pages
> +	 */
> +	if (!npages || npages > MAX_PVDMA_PAGES) {
> +		printk(KERN_INFO "%s: Illegal number of pages: %d\n",
> +		       __func__, npages);
> +		goto out;
> +	}
> +
> +	host_page = gfn_to_page(vcpu->kvm, page_gfn);
you need mmap_sem held for read to use gfn_to_page.

quoted text
> +	if (is_error_page(host_page)) {
> +		printk(KERN_INFO "%s: Bad gfn %p\n", __func__,
> +		       (void *)page_gfn);
> +		goto out;
> +	}
> +	hcall_page = shared_addr = kmap(host_page);
> +
> +	/* scatterlist to map guest dma pages into host physical
> +	 * memory -- if they exceed the DMA map limit
> +	 */
> +	sg = kcalloc(npages, sizeof(struct scatterlist), GFP_KERNEL);
> +	if (sg == NULL) {
> +		printk(KERN_INFO "%s: Couldn't allocate memory (sg)\n",
> +		       __func__);
> +		goto out_unmap;
> +	}
> +
> +	/* List to store all guest pages mapped into host. This will
> +	 * be used later to free pages on the host. Think of this as a
> +	 * translation table from guest dma addresses into host dma
> +	 * addresses
> +	 */
> +	dmap = kzalloc(sizeof(*dmap), GFP_KERNEL);
> +	if (dmap == NULL) {
> +		printk(KERN_INFO "%s: Couldn't allocate memory\n",
> +		       __func__);
> +		goto out_unmap_sg;
> +	}
> +
> +	/* FIXME: consider the length of the last page. Guest should
> +	 * send this info.
> +	 */
> +	for (i = 0; i < npages; i++) {
> +		struct page *page;
> +		gpa_t gpa;
> +
> +		gpa = *shared_addr++;
> +		page = gfn_to_page(vcpu->kvm, gpa >> PAGE_SHIFT);
care for locking here too.

quoted text
> +		if (is_error_page(page)) {
> +			int j;
> +			printk(KERN_INFO "kvm %s: gpa %p not valid\n",
> +			       __func__, (void *)gpa);
> +
> +			for (j = 0; j < i; j++)
> +				put_page(sg_page(&sg[j]));
> +			goto out_unmap_sg_dmap;
> +		}
> +		prepare_sg_entry(&sg[i], page);
> +		get_page(sg_page(&sg[i]));
> +	}
> +
> +	/* Put this on the dmap_head list, so that we can find it
> +	 * later for the 'free' operation
> +	 */
> +	dmap->sg = sg;
> +	dmap->nents = npages;
> +	list_add(&dmap->list, &vcpu->kvm->arch.pci_pv_dmap_head);
> +
> +	/* FIXME: guest should send the direction */
> +	r = dma_ops->map_sg(NULL, sg, npages, PCI_DMA_BIDIRECTIONAL);
> +	if (r) {
> +		r = npages;
> +		*hcall_page = sg[0].dma_address | (*hcall_page & ~PAGE_MASK);
> +	}
> +
> + out_unmap:
> +	if (!r)
> +		*hcall_page = bad_dma_address;
> +	kunmap(host_page);
> + out:
> +	++vcpu->stat.hypercall_map;
> +	return r;
> + out_unmap_sg_dmap:
> +	kfree(dmap);
> + out_unmap_sg:
> +	kfree(sg);
> +	goto out_unmap;
those backwards goto are very clumsy. Might be better to give it a 
further attention in order to avoid id.

quoted text
> +}
> +
> +static int free_dmap(struct kvm_pv_dma_map *dmap, struct list_head *head)
> +{
> +	int i;
> +
> +	if (!dmap)
> +		return 1;
that's ugly.

it's better to keep the free function with free-like semantics: just a 
void function that plainly returns if !dmap, and check in the caller.

quoted text
> +static int
> +pv_mapped_pci_device_hypercall(struct kvm_vcpu *vcpu, gfn_t page_gfn)
> +{
> +	int r = 0;
> +	unsigned long *shared_addr;
> +	struct page *host_page;
> +	struct kvm_pci_pt_info pci_pt_info;
> +
> +	host_page = gfn_to_page(vcpu->kvm, page_gfn);
locking
quoted text
> +	if (is_error_page(host_page)) {
> +		printk(KERN_INFO "%s: gfn %p not valid\n",
> +		       __func__, (void *)page_gfn);
> +		r = -1;
r = -1 is not really informative. Better use some meaningful error.

We can return here, and avoid this goto if we always increment the 
hypercall counter in the beginning of the function. But this is nitpicking.

quoted text
> +		goto out;
> +	}
> +	shared_addr = kmap(host_page);
> +	memcpy(&pci_pt_info, shared_addr, sizeof(pci_pt_info));
> +
> +	if (find_pci_pt_dev(&vcpu->kvm->arch.pci_pt_dev_head,
> +			    &pci_pt_info, 0, KVM_PT_SOURCE_ASSIGN))
> +		r++; /* We have assigned the device */
> +
> +	kunmap(host_page);
better use atomic mappings here.

--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/